[Qemu-devel] [PATCH v2 0/2] implement .bdrv_co_is_allocated

[Qemu-devel] [PATCH v2 0/2] implement .bdrv_co_is_allocated

[Liu Yuan]

From: Liu Yuan <tailai.ly@taobao.com>

v2:
 - correct 'end' calculation
 - get the longest unallocated area
 - replace SECTOR_SIZE with BDRV_SECTOR_SIZE

PATCH 1/2 is ia prepare patch
PATCH 2/2 implement .bdrv_co_is_allocated

Liu Yuan (2):
  sheepdog: use BDRV_SECTOR_SIZE
  sheepdog: implement .bdrv_co_is_allocated()

 block/sheepdog.c |   46 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 40 insertions(+), 6 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH v2 1/2] sheepdog: use BDRV_SECTOR_SIZE

[Liu Yuan]

From: Liu Yuan <tailai.ly@taobao.com>

Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Liu Yuan <tailai.ly@taobao.com>
---
 block/sheepdog.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index c099117..0b700a3 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -89,7 +89,6 @@
 #define SD_NR_VDIS   (1U << 24)
 #define SD_DATA_OBJ_SIZE (UINT64_C(1) << 22)
 #define SD_MAX_VDI_SIZE (SD_DATA_OBJ_SIZE * MAX_DATA_OBJS)
-#define SECTOR_SIZE 512
 
 #define SD_INODE_SIZE (sizeof(SheepdogInode))
 #define CURRENT_VDI_ID 0
@@ -1220,7 +1219,7 @@ static int sd_open(BlockDriverState *bs, const char *filename,
     s->min_dirty_data_idx = UINT32_MAX;
     s->max_dirty_data_idx = 0;
 
-    bs->total_sectors = s->inode.vdi_size / SECTOR_SIZE;
+    bs->total_sectors = s->inode.vdi_size / BDRV_SECTOR_SIZE;
     pstrcpy(s->name, sizeof(s->name), vdi);
     qemu_co_mutex_init(&s->lock);
     g_free(buf);
@@ -1605,10 +1604,10 @@ static int coroutine_fn sd_co_rw_vector(void *p)
 {
     SheepdogAIOCB *acb = p;
     int ret = 0;
-    unsigned long len, done = 0, total = acb->nb_sectors * SECTOR_SIZE;
-    unsigned long idx = acb->sector_num * SECTOR_SIZE / SD_DATA_OBJ_SIZE;
+    unsigned long len, done = 0, total = acb->nb_sectors * BDRV_SECTOR_SIZE;
+    unsigned long idx = acb->sector_num * BDRV_SECTOR_SIZE / SD_DATA_OBJ_SIZE;
     uint64_t oid;
-    uint64_t offset = (acb->sector_num * SECTOR_SIZE) % SD_DATA_OBJ_SIZE;
+    uint64_t offset = (acb->sector_num * BDRV_SECTOR_SIZE) % SD_DATA_OBJ_SIZE;
     BDRVSheepdogState *s = acb->common.bs->opaque;
     SheepdogInode *inode = &s->inode;
     AIOReq *aio_req;
@@ -1727,7 +1726,7 @@ static coroutine_fn int sd_co_writev(BlockDriverState *bs, int64_t sector_num,
     int ret;
 
     if (bs->growable && sector_num + nb_sectors > bs->total_sectors) {
-        ret = sd_truncate(bs, (sector_num + nb_sectors) * SECTOR_SIZE);
+        ret = sd_truncate(bs, (sector_num + nb_sectors) * BDRV_SECTOR_SIZE);
         if (ret < 0) {
             return ret;
         }
-- 
1.7.9.5

[Qemu-devel] [PATCH v2 2/2] sheepdog: implement .bdrv_co_is_allocated()

[Liu Yuan]

From: Liu Yuan <tailai.ly@taobao.com>

Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Liu Yuan <tailai.ly@taobao.com>
---
 block/sheepdog.c |   36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 0b700a3..0dbf039 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2137,6 +2137,39 @@ static coroutine_fn int sd_co_discard(BlockDriverState *bs, int64_t sector_num,
     return acb->ret;
 }
 
+static coroutine_fn int
+sd_co_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
+                   int *pnum)
+{
+    BDRVSheepdogState *s = bs->opaque;
+    SheepdogInode *inode = &s->inode;
+    unsigned long start = sector_num * BDRV_SECTOR_SIZE / SD_DATA_OBJ_SIZE, idx,
+                  end = DIV_ROUND_UP((sector_num + nb_sectors) *
+                                     BDRV_SECTOR_SIZE, SD_DATA_OBJ_SIZE);
+    int ret = 1;
+
+    for (idx = start; idx <= end; idx++) {
+        if (inode->data_vdi_id[idx] == 0) {
+            break;
+        }
+    }
+    if (idx == start) {
+        /* Get te longest length of unallocated sectors */
+        ret = 0;
+        for (idx = start + 1; idx <= end; idx++) {
+            if (inode->data_vdi_id[idx] != 0) {
+                break;
+            }
+        }
+    }
+
+    *pnum = (idx - start) * SD_DATA_OBJ_SIZE / BDRV_SECTOR_SIZE;
+    if (*pnum > nb_sectors) {
+        *pnum = nb_sectors;
+    }
+    return ret;
+}
+
 static QEMUOptionParameter sd_create_options[] = {
     {
         .name = BLOCK_OPT_SIZE,
@@ -2170,6 +2203,7 @@ static BlockDriver bdrv_sheepdog = {
     .bdrv_co_writev = sd_co_writev,
     .bdrv_co_flush_to_disk  = sd_co_flush_to_disk,
     .bdrv_co_discard = sd_co_discard,
+    .bdrv_co_is_allocated = sd_co_is_allocated,
 
     .bdrv_snapshot_create   = sd_snapshot_create,
     .bdrv_snapshot_goto     = sd_snapshot_goto,
@@ -2196,6 +2230,7 @@ static BlockDriver bdrv_sheepdog_tcp = {
     .bdrv_co_writev = sd_co_writev,
     .bdrv_co_flush_to_disk  = sd_co_flush_to_disk,
     .bdrv_co_discard = sd_co_discard,
+    .bdrv_co_is_allocated = sd_co_is_allocated,
 
     .bdrv_snapshot_create   = sd_snapshot_create,
     .bdrv_snapshot_goto     = sd_snapshot_goto,
@@ -2222,6 +2257,7 @@ static BlockDriver bdrv_sheepdog_unix = {
     .bdrv_co_writev = sd_co_writev,
     .bdrv_co_flush_to_disk  = sd_co_flush_to_disk,
     .bdrv_co_discard = sd_co_discard,
+    .bdrv_co_is_allocated = sd_co_is_allocated,
 
     .bdrv_snapshot_create   = sd_snapshot_create,
     .bdrv_snapshot_goto     = sd_snapshot_goto,
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH v2 2/2] sheepdog: implement .bdrv_co_is_allocated()

[Stefan Hajnoczi]

On Mon, Apr 22, 2013 at 02:59:10PM +0800, Liu Yuan wrote:
> +static coroutine_fn int
> +sd_co_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
> +                   int *pnum)
> +{
> +    BDRVSheepdogState *s = bs->opaque;
> +    SheepdogInode *inode = &s->inode;
> +    unsigned long start = sector_num * BDRV_SECTOR_SIZE / SD_DATA_OBJ_SIZE, idx,
> +                  end = DIV_ROUND_UP((sector_num + nb_sectors) *
> +                                     BDRV_SECTOR_SIZE, SD_DATA_OBJ_SIZE);

Please put the variable declarations on separate lines, it's very easy
to miss "idx".

> +    int ret = 1;
> +
> +    for (idx = start; idx <= end; idx++) {

Should this be idx < end?  Otherwise you are checking one beyond the
last SD_DATA_OBJ_SIZE.

> +        if (inode->data_vdi_id[idx] == 0) {
> +            break;
> +        }
> +    }
> +    if (idx == start) {
> +        /* Get te longest length of unallocated sectors */

s/te/the/

> +        ret = 0;
> +        for (idx = start + 1; idx <= end; idx++) {
> +            if (inode->data_vdi_id[idx] != 0) {
> +                break;
> +            }
> +        }
> +    }

Here is a more concise way of implementing these two loops:

int ret = !!inode->data_vdi_id[idx];
for (idx = start + 1; idx < end; idx++) {
    if (!!inode->data_vdi_id[idx] != ret) {
        break;
    }
}

I like this better because it avoids code duplication, but it's a
question of style.  Feel free to stick to your approach if you like.

Re: [Qemu-devel] [PATCH v2 2/2] sheepdog: implement .bdrv_co_is_allocated()

[Liu Yuan]

On 04/22/2013 08:00 PM, Stefan Hajnoczi wrote:
> On Mon, Apr 22, 2013 at 02:59:10PM +0800, Liu Yuan wrote:
>> +static coroutine_fn int
>> +sd_co_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
>> +                   int *pnum)
>> +{
>> +    BDRVSheepdogState *s = bs->opaque;
>> +    SheepdogInode *inode = &s->inode;
>> +    unsigned long start = sector_num * BDRV_SECTOR_SIZE / SD_DATA_OBJ_SIZE, idx,
>> +                  end = DIV_ROUND_UP((sector_num + nb_sectors) *
>> +                                     BDRV_SECTOR_SIZE, SD_DATA_OBJ_SIZE);
> 
> Please put the variable declarations on separate lines, it's very easy
> to miss "idx".

Okay.

> 
>> +    int ret = 1;
>> +
>> +    for (idx = start; idx <= end; idx++) {
> 
> Should this be idx < end?  Otherwise you are checking one beyond the
> last SD_DATA_OBJ_SIZE.

No. the end is index of last object, not index of last object + 1.

> 
>> +        if (inode->data_vdi_id[idx] == 0) {
>> +            break;
>> +        }
>> +    }
>> +    if (idx == start) {
>> +        /* Get te longest length of unallocated sectors */
> 
> s/te/the/
> 
>> +        ret = 0;
>> +        for (idx = start + 1; idx <= end; idx++) {
>> +            if (inode->data_vdi_id[idx] != 0) {
>> +                break;
>> +            }
>> +        }
>> +    }
> 
> Here is a more concise way of implementing these two loops:
> 
> int ret = !!inode->data_vdi_id[idx];
> for (idx = start + 1; idx < end; idx++) {
>     if (!!inode->data_vdi_id[idx] != ret) {
>         break;
>     }
> }
> 
> I like this better because it avoids code duplication, but it's a
> question of style.  Feel free to stick to your approach if you like.

The trick of your code looks fantastic to me and I like your idea to
reduce the duplicated code as much as possible but the sacrifice of code
readability for the resulted code is somewhat too high, so I think not
worth of it and I'll stick to my stupid but more clear version in V3.

Thanks,
Yuan

Re: [Qemu-devel] [PATCH v2 2/2] sheepdog: implement .bdrv_co_is_allocated()

[Stefan Hajnoczi]

On Mon, Apr 22, 2013 at 08:10:58PM +0800, Liu Yuan wrote:
> On 04/22/2013 08:00 PM, Stefan Hajnoczi wrote:
> > On Mon, Apr 22, 2013 at 02:59:10PM +0800, Liu Yuan wrote:
> >> +static coroutine_fn int
> >> +sd_co_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
> >> +                   int *pnum)
> >> +{
> >> +    BDRVSheepdogState *s = bs->opaque;
> >> +    SheepdogInode *inode = &s->inode;
> >> +    unsigned long start = sector_num * BDRV_SECTOR_SIZE / SD_DATA_OBJ_SIZE, idx,
> >> +                  end = DIV_ROUND_UP((sector_num + nb_sectors) *
> >> +                                     BDRV_SECTOR_SIZE, SD_DATA_OBJ_SIZE);
> > 
> > Please put the variable declarations on separate lines, it's very easy
> > to miss "idx".
> 
> Okay.
> 
> > 
> >> +    int ret = 1;
> >> +
> >> +    for (idx = start; idx <= end; idx++) {
> > 
> > Should this be idx < end?  Otherwise you are checking one beyond the
> > last SD_DATA_OBJ_SIZE.
> 
> No. the end is index of last object, not index of last object + 1.

Imagine sector_num = 0 and nb_sectors = SD_DATA_OBJ_SIZE / BDRV_SECTOR_SIZE.

start = 0
end = 1

You don't want object 1, only object 0.

Re: [Qemu-devel] [PATCH v2 2/2] sheepdog: implement .bdrv_co_is_allocated()

[Liu Yuan]

On 04/22/2013 11:03 PM, Stefan Hajnoczi wrote:
> Imagine sector_num = 0 and nb_sectors = SD_DATA_OBJ_SIZE / BDRV_SECTOR_SIZE.
> 
> start = 0
> end = 1
> 
> You don't want object 1, only object 0.

Hmm, math, ouch. So nb_sectors include sector_num? What I mean is

If [start, end] mean a range of sectors,so

1. nb_sectors = end - start + 1 (I assume this one)
2. nb_sectors = end - start (you meant this one?)

which one is correct for .co_is_allocated?

Thanks,
Yuan

Re: [Qemu-devel] [PATCH v2 2/2] sheepdog: implement .bdrv_co_is_allocated()

[Stefan Hajnoczi]

On Mon, Apr 22, 2013 at 5:18 PM, Liu Yuan <namei.unix@gmail.com> wrote:
> On 04/22/2013 11:03 PM, Stefan Hajnoczi wrote:
>> Imagine sector_num = 0 and nb_sectors = SD_DATA_OBJ_SIZE / BDRV_SECTOR_SIZE.
>>
>> start = 0
>> end = 1
>>
>> You don't want object 1, only object 0.
>
> Hmm, math, ouch. So nb_sectors include sector_num? What I mean is
>
> If [start, end] mean a range of sectors,so
>
> 1. nb_sectors = end - start + 1 (I assume this one)
> 2. nb_sectors = end - start (you meant this one?)
>
> which one is correct for .co_is_allocated?

The first sector is included in nb_sectors.  Mathematically the range
is defined as [sector_num, sector_num + nb_sectors).  Notice the
half-open interval, sector_num + nb_sectors is excluded.  The last
included sector is sector_num + nb_sectors - 1.

You can look at qcow2's implementation, especially
count_contiguous_clusters() to double-check.

Stefan

Re: [Qemu-devel] [PATCH v2 2/2] sheepdog: implement .bdrv_co_is_allocated()

[Liu Yuan]

On 04/23/2013 04:46 AM, Stefan Hajnoczi wrote:
> The first sector is included in nb_sectors.  Mathematically the range
> is defined as [sector_num, sector_num + nb_sectors).  Notice the
> half-open interval, sector_num + nb_sectors is excluded.  The last
> included sector is sector_num + nb_sectors - 1.
> 
> You can look at qcow2's implementation, especially
> count_contiguous_clusters() to double-check.

Okay, thanks for you explanation. I'll update it in v4.

Yuan