[v4,12/20] qed: Switch to .bdrv_co_block_status()

Message ID 20171012185916.22776-13-eblake@redhat.com
State New
Headers show
Series
  • add byte-based block_status driver callbacks
Related show

Commit Message

Eric Blake Oct. 12, 2017, 6:59 p.m.
We are gradually moving away from sector-based interfaces, towards
byte-based.  Update the qed driver accordingly, taking the opportunity
to inline qed_is_allocated_cb() into its lone caller (the callback
used to be important, until we switched qed to coroutines).  There is
no intent to optimize based on the want_zero flag for this format.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v4: rebase to interface change, inline pointless callback
v3: no change
v2: rebase to mapping flag, fix mask in qed_is_allocated_cb
---
 block/qed.c | 84 +++++++++++++++++++++----------------------------------------
 1 file changed, 28 insertions(+), 56 deletions(-)

Comments

Vladimir Sementsov-Ogievskiy Nov. 30, 2017, 10:27 a.m. | #1
12.10.2017 21:59, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based.  Update the qed driver accordingly, taking the opportunity
> to inline qed_is_allocated_cb() into its lone caller (the callback
> used to be important, until we switched qed to coroutines).  There is
> no intent to optimize based on the want_zero flag for this format.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v4: rebase to interface change, inline pointless callback
> v3: no change
> v2: rebase to mapping flag, fix mask in qed_is_allocated_cb
> ---
>   block/qed.c | 84 +++++++++++++++++++++----------------------------------------
>   1 file changed, 28 insertions(+), 56 deletions(-)
>
> diff --git a/block/qed.c b/block/qed.c
> index 28e2ec89e8..cbab5b6f83 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -688,74 +688,46 @@ finish:
>       return ret;
>   }
>
> -typedef struct {
> -    BlockDriverState *bs;
> -    Coroutine *co;
> -    uint64_t pos;
> -    int64_t status;
> -    int *pnum;
> -    BlockDriverState **file;
> -} QEDIsAllocatedCB;
> -
> -/* Called with table_lock held.  */
> -static void qed_is_allocated_cb(void *opaque, int ret, uint64_t offset, size_t len)
> -{
> -    QEDIsAllocatedCB *cb = opaque;
> -    BDRVQEDState *s = cb->bs->opaque;
> -    *cb->pnum = len / BDRV_SECTOR_SIZE;
> -    switch (ret) {
> -    case QED_CLUSTER_FOUND:
> -        offset |= qed_offset_into_cluster(s, cb->pos);
> -        cb->status = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset;
> -        *cb->file = cb->bs->file->bs;
> -        break;
> -    case QED_CLUSTER_ZERO:
> -        cb->status = BDRV_BLOCK_ZERO;
> -        break;
> -    case QED_CLUSTER_L2:
> -    case QED_CLUSTER_L1:
> -        cb->status = 0;
> -        break;
> -    default:
> -        assert(ret < 0);
> -        cb->status = ret;
> -        break;
> -    }
> -
> -    if (cb->co) {
> -        aio_co_wake(cb->co);
> -    }
> -}
> -
> -static int64_t coroutine_fn bdrv_qed_co_get_block_status(BlockDriverState *bs,
> -                                                 int64_t sector_num,
> -                                                 int nb_sectors, int *pnum,
> +static int coroutine_fn bdrv_qed_co_block_status(BlockDriverState *bs,
> +                                                 bool want_zero,
> +                                                 int64_t pos, int64_t bytes,
> +                                                 int64_t *pnum, int64_t *map,
>                                                    BlockDriverState **file)
>   {
>       BDRVQEDState *s = bs->opaque;
> -    size_t len = (size_t)nb_sectors * BDRV_SECTOR_SIZE;
> -    QEDIsAllocatedCB cb = {
> -        .bs = bs,
> -        .pos = (uint64_t)sector_num * BDRV_SECTOR_SIZE,
> -        .status = BDRV_BLOCK_OFFSET_MASK,
> -        .pnum = pnum,
> -        .file = file,
> -    };
> +    size_t len;

size_t len = bytes;

with that:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> +    int status;
>       QEDRequest request = { .l2_table = NULL };
>       uint64_t offset;
>       int ret;
>
>       qemu_co_mutex_lock(&s->table_lock);
> -    ret = qed_find_cluster(s, &request, cb.pos, &len, &offset);
> -    qed_is_allocated_cb(&cb, ret, offset, len);
> +    ret = qed_find_cluster(s, &request, pos, &len, &offset);

len is in-out parameter, you can't use it uninitialized.

>
> -    /* The callback was invoked immediately */
> -    assert(cb.status != BDRV_BLOCK_OFFSET_MASK);
> +    *pnum = len;
> +    switch (ret) {
> +    case QED_CLUSTER_FOUND:
> +        *map = offset | qed_offset_into_cluster(s, pos);
> +        status = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
> +        *file = bs->file->bs;
> +        break;
> +    case QED_CLUSTER_ZERO:
> +        status = BDRV_BLOCK_ZERO;
> +        break;
> +    case QED_CLUSTER_L2:
> +    case QED_CLUSTER_L1:
> +        status = 0;
> +        break;
> +    default:
> +        assert(ret < 0);
> +        status = ret;
> +        break;
> +    }
>
>       qed_unref_l2_cache_entry(request.l2_table);
>       qemu_co_mutex_unlock(&s->table_lock);
>
> -    return cb.status;
> +    return status;
>   }
>
>   static BDRVQEDState *acb_to_s(QEDAIOCB *acb)
> @@ -1595,7 +1567,7 @@ static BlockDriver bdrv_qed = {
>       .bdrv_child_perm          = bdrv_format_default_perms,
>       .bdrv_create              = bdrv_qed_create,
>       .bdrv_has_zero_init       = bdrv_has_zero_init_1,
> -    .bdrv_co_get_block_status = bdrv_qed_co_get_block_status,
> +    .bdrv_co_block_status     = bdrv_qed_co_block_status,
>       .bdrv_co_readv            = bdrv_qed_co_readv,
>       .bdrv_co_writev           = bdrv_qed_co_writev,
>       .bdrv_co_pwrite_zeroes    = bdrv_qed_co_pwrite_zeroes,
Eric Blake Nov. 30, 2017, 11:17 p.m. | #2
On 11/30/2017 04:27 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.10.2017 21:59, Eric Blake wrote:
>> We are gradually moving away from sector-based interfaces, towards
>> byte-based.  Update the qed driver accordingly, taking the opportunity
>> to inline qed_is_allocated_cb() into its lone caller (the callback
>> used to be important, until we switched qed to coroutines).  There is
>> no intent to optimize based on the want_zero flag for this format.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>

>>   {
>>       BDRVQEDState *s = bs->opaque;
>> -    size_t len = (size_t)nb_sectors * BDRV_SECTOR_SIZE;
>> -    QEDIsAllocatedCB cb = {
>> -        .bs = bs,
>> -        .pos = (uint64_t)sector_num * BDRV_SECTOR_SIZE,
>> -        .status = BDRV_BLOCK_OFFSET_MASK,
>> -        .pnum = pnum,
>> -        .file = file,
>> -    };
>> +    size_t len;
> 
> size_t len = bytes;
> 

Or rather, size_t len = MIN(bytes, SIZE_MAX); thanks to 32-bit platforms.

> with that:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
>> +    int status;
>>       QEDRequest request = { .l2_table = NULL };
>>       uint64_t offset;
>>       int ret;
>>
>>       qemu_co_mutex_lock(&s->table_lock);
>> -    ret = qed_find_cluster(s, &request, cb.pos, &len, &offset);
>> -    qed_is_allocated_cb(&cb, ret, offset, len);
>> +    ret = qed_find_cluster(s, &request, pos, &len, &offset);
> 
> len is in-out parameter, you can't use it uninitialized.

Good catch.
Vladimir Sementsov-Ogievskiy Dec. 4, 2017, 8:59 a.m. | #3
01.12.2017 02:17, Eric Blake wrote:
> On 11/30/2017 04:27 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 12.10.2017 21:59, Eric Blake wrote:
>>> We are gradually moving away from sector-based interfaces, towards
>>> byte-based.  Update the qed driver accordingly, taking the opportunity
>>> to inline qed_is_allocated_cb() into its lone caller (the callback
>>> used to be important, until we switched qed to coroutines). There is
>>> no intent to optimize based on the want_zero flag for this format.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>>
>
>>>   {
>>>       BDRVQEDState *s = bs->opaque;
>>> -    size_t len = (size_t)nb_sectors * BDRV_SECTOR_SIZE;
>>> -    QEDIsAllocatedCB cb = {
>>> -        .bs = bs,
>>> -        .pos = (uint64_t)sector_num * BDRV_SECTOR_SIZE,
>>> -        .status = BDRV_BLOCK_OFFSET_MASK,
>>> -        .pnum = pnum,
>>> -        .file = file,
>>> -    };
>>> +    size_t len;
>>
>> size_t len = bytes;
>>
>
> Or rather, size_t len = MIN(bytes, SIZE_MAX); thanks to 32-bit platforms.

ok, this is even better

>
>> with that:
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>>> +    int status;
>>>       QEDRequest request = { .l2_table = NULL };
>>>       uint64_t offset;
>>>       int ret;
>>>
>>>       qemu_co_mutex_lock(&s->table_lock);
>>> -    ret = qed_find_cluster(s, &request, cb.pos, &len, &offset);
>>> -    qed_is_allocated_cb(&cb, ret, offset, len);
>>> +    ret = qed_find_cluster(s, &request, pos, &len, &offset);
>>
>> len is in-out parameter, you can't use it uninitialized.
>
> Good catch.
>

Patch

diff --git a/block/qed.c b/block/qed.c
index 28e2ec89e8..cbab5b6f83 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -688,74 +688,46 @@  finish:
     return ret;
 }

-typedef struct {
-    BlockDriverState *bs;
-    Coroutine *co;
-    uint64_t pos;
-    int64_t status;
-    int *pnum;
-    BlockDriverState **file;
-} QEDIsAllocatedCB;
-
-/* Called with table_lock held.  */
-static void qed_is_allocated_cb(void *opaque, int ret, uint64_t offset, size_t len)
-{
-    QEDIsAllocatedCB *cb = opaque;
-    BDRVQEDState *s = cb->bs->opaque;
-    *cb->pnum = len / BDRV_SECTOR_SIZE;
-    switch (ret) {
-    case QED_CLUSTER_FOUND:
-        offset |= qed_offset_into_cluster(s, cb->pos);
-        cb->status = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset;
-        *cb->file = cb->bs->file->bs;
-        break;
-    case QED_CLUSTER_ZERO:
-        cb->status = BDRV_BLOCK_ZERO;
-        break;
-    case QED_CLUSTER_L2:
-    case QED_CLUSTER_L1:
-        cb->status = 0;
-        break;
-    default:
-        assert(ret < 0);
-        cb->status = ret;
-        break;
-    }
-
-    if (cb->co) {
-        aio_co_wake(cb->co);
-    }
-}
-
-static int64_t coroutine_fn bdrv_qed_co_get_block_status(BlockDriverState *bs,
-                                                 int64_t sector_num,
-                                                 int nb_sectors, int *pnum,
+static int coroutine_fn bdrv_qed_co_block_status(BlockDriverState *bs,
+                                                 bool want_zero,
+                                                 int64_t pos, int64_t bytes,
+                                                 int64_t *pnum, int64_t *map,
                                                  BlockDriverState **file)
 {
     BDRVQEDState *s = bs->opaque;
-    size_t len = (size_t)nb_sectors * BDRV_SECTOR_SIZE;
-    QEDIsAllocatedCB cb = {
-        .bs = bs,
-        .pos = (uint64_t)sector_num * BDRV_SECTOR_SIZE,
-        .status = BDRV_BLOCK_OFFSET_MASK,
-        .pnum = pnum,
-        .file = file,
-    };
+    size_t len;
+    int status;
     QEDRequest request = { .l2_table = NULL };
     uint64_t offset;
     int ret;

     qemu_co_mutex_lock(&s->table_lock);
-    ret = qed_find_cluster(s, &request, cb.pos, &len, &offset);
-    qed_is_allocated_cb(&cb, ret, offset, len);
+    ret = qed_find_cluster(s, &request, pos, &len, &offset);

-    /* The callback was invoked immediately */
-    assert(cb.status != BDRV_BLOCK_OFFSET_MASK);
+    *pnum = len;
+    switch (ret) {
+    case QED_CLUSTER_FOUND:
+        *map = offset | qed_offset_into_cluster(s, pos);
+        status = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
+        *file = bs->file->bs;
+        break;
+    case QED_CLUSTER_ZERO:
+        status = BDRV_BLOCK_ZERO;
+        break;
+    case QED_CLUSTER_L2:
+    case QED_CLUSTER_L1:
+        status = 0;
+        break;
+    default:
+        assert(ret < 0);
+        status = ret;
+        break;
+    }

     qed_unref_l2_cache_entry(request.l2_table);
     qemu_co_mutex_unlock(&s->table_lock);

-    return cb.status;
+    return status;
 }

 static BDRVQEDState *acb_to_s(QEDAIOCB *acb)
@@ -1595,7 +1567,7 @@  static BlockDriver bdrv_qed = {
     .bdrv_child_perm          = bdrv_format_default_perms,
     .bdrv_create              = bdrv_qed_create,
     .bdrv_has_zero_init       = bdrv_has_zero_init_1,
-    .bdrv_co_get_block_status = bdrv_qed_co_get_block_status,
+    .bdrv_co_block_status     = bdrv_qed_co_block_status,
     .bdrv_co_readv            = bdrv_qed_co_readv,
     .bdrv_co_writev           = bdrv_qed_co_writev,
     .bdrv_co_pwrite_zeroes    = bdrv_qed_co_pwrite_zeroes,