diff mbox

[v2,2/2] sheepdog: implement .bdrv_co_is_allocated()

Message ID 1366613950-10918-3-git-send-email-namei.unix@gmail.com
State New
Headers show

Commit Message

Liu Yuan April 22, 2013, 6:59 a.m. UTC 
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(+)

Comments

Stefan Hajnoczi April 22, 2013, noon UTC | #1 
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.
Liu Yuan April 22, 2013, 12:10 p.m. UTC | #2 
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
Stefan Hajnoczi April 22, 2013, 3:03 p.m. UTC | #3 
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.
Liu Yuan April 22, 2013, 3:18 p.m. UTC | #4 
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
Stefan Hajnoczi April 22, 2013, 8:46 p.m. UTC | #5 
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
Liu Yuan April 23, 2013, 5:55 a.m. UTC | #6 
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
diff mbox

Patch

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,