Patchwork [v3,11/19] block: define get_block_status return value

login
register
mail settings
Submitter Paolo Bonzini
Date July 25, 2013, 2:23 p.m.
Message ID <1374762197-7261-12-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/261722/
State New
Headers show

Comments

Paolo Bonzini - July 25, 2013, 2:23 p.m.
Define the return value of get_block_status.  Bits 0, 1, 2 and 9-62
are valid; bit 63 (the sign bit) is reserved for errors.  Bits 3-8
are left for future extensions.

The return code is compatible with the old is_allocated API: if a driver
only returns 0 or 1 (aka BDRV_BLOCK_DATA) like is_allocated used to,
clients of is_allocated will not have any change in behavior.  Still,
we will return more precise information in the next patches and the
new definition of bdrv_is_allocated is already prepared for this.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c               | 10 ++++++++--
 include/block/block.h | 26 ++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 2 deletions(-)
Kevin Wolf - July 30, 2013, 2:14 p.m.
Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> Define the return value of get_block_status.  Bits 0, 1, 2 and 9-62
> are valid; bit 63 (the sign bit) is reserved for errors.  Bits 3-8
> are left for future extensions.
> 
> The return code is compatible with the old is_allocated API: if a driver
> only returns 0 or 1 (aka BDRV_BLOCK_DATA) like is_allocated used to,
> clients of is_allocated will not have any change in behavior.  Still,
> we will return more precise information in the next patches and the
> new definition of bdrv_is_allocated is already prepared for this.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block.c               | 10 ++++++++--
>  include/block/block.h | 26 ++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index f533c36..7cfbf71 100644
> --- a/block.c
> +++ b/block.c
> @@ -3004,7 +3004,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>  
>      if (!bs->drv->bdrv_co_get_block_status) {
>          *pnum = nb_sectors;
> -        return 1;
> +        return BDRV_BLOCK_DATA;
>      }
>  
>      return bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
> @@ -3054,7 +3054,13 @@ int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
>  int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num,
>                                     int nb_sectors, int *pnum)
>  {
> -    return bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
> +    int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    return
> +        (ret & BDRV_BLOCK_DATA) ||
> +        ((ret & BDRV_BLOCK_ZERO) && !bdrv_has_zero_init(bs));
>  }
>  
>  /*
> diff --git a/include/block/block.h b/include/block/block.h
> index e41854e..d044b31 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -81,6 +81,32 @@ typedef struct BlockDevOps {
>  #define BDRV_SECTOR_SIZE   (1ULL << BDRV_SECTOR_BITS)
>  #define BDRV_SECTOR_MASK   ~(BDRV_SECTOR_SIZE - 1)
>  
> +/* BDRV_BLOCK_DATA: data is read from bs->file or another file
> + * BDRV_BLOCK_ZERO: sectors read as zero
> + * BDRV_BLOCK_OFFSET_VALID: sector stored in bs->file as raw data
> + *
> + * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 represent the offset in
> + * bs->file where sector data can be read from as raw data.
> + *
> + * DATA == 0 && ZERO == 0 means that data is read from backing_hd if present.
> + *
> + * DATA ZERO OFFSET_VALID
> + *  t    t        t       sectors read as zero, bs->file is zero at offset
> + *  t    f        t       sectors read as valid from bs->file at offset
> + *  f    t        t       sectors preallocated, read as zero, bs->file not
> + *                        necessarily zero at offset
> + *  f    f        t       sectors preallocated but read from backing_hd,
> + *                        bs->file contains garbage at offset
> + *  t    t        f       sectors preallocated, read as zero, unknown offset
> + *  t    f        f       sectors read from unknown file or offset
> + *  f    t        f       not allocated or unknown offset, read as zero
> + *  f    f        f       not allocated or unknown offset, read from backing_hd
> + */
> +#define BDRV_BLOCK_DATA         1
> +#define BDRV_BLOCK_ZERO         2
> +#define BDRV_BLOCK_OFFSET_VALID 4
> +#define BDRV_BLOCK_OFFSET_MASK  BDRV_SECTOR_MASK

When are block driver supposed to set the BDRV_BLOCK_OFFSET_VALID flag?

For example, qcow2 could in theory set the flag, it has all of the
information already in memory. But with a fragmented image this might
mean that it returns only one cluster instead of a large area with one
bdrv_get_block_status() call.

Should the caller pass a flag that tells whether he is interested in the
offset or not?

Kevin
Paolo Bonzini - July 30, 2013, 2:19 p.m.
Il 30/07/2013 16:14, Kevin Wolf ha scritto:
> Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
>> Define the return value of get_block_status.  Bits 0, 1, 2 and 9-62
>> are valid; bit 63 (the sign bit) is reserved for errors.  Bits 3-8
>> are left for future extensions.
>>
>> The return code is compatible with the old is_allocated API: if a driver
>> only returns 0 or 1 (aka BDRV_BLOCK_DATA) like is_allocated used to,
>> clients of is_allocated will not have any change in behavior.  Still,
>> we will return more precise information in the next patches and the
>> new definition of bdrv_is_allocated is already prepared for this.
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  block.c               | 10 ++++++++--
>>  include/block/block.h | 26 ++++++++++++++++++++++++++
>>  2 files changed, 34 insertions(+), 2 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index f533c36..7cfbf71 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3004,7 +3004,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>>  
>>      if (!bs->drv->bdrv_co_get_block_status) {
>>          *pnum = nb_sectors;
>> -        return 1;
>> +        return BDRV_BLOCK_DATA;
>>      }
>>  
>>      return bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
>> @@ -3054,7 +3054,13 @@ int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
>>  int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num,
>>                                     int nb_sectors, int *pnum)
>>  {
>> -    return bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
>> +    int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +    return
>> +        (ret & BDRV_BLOCK_DATA) ||
>> +        ((ret & BDRV_BLOCK_ZERO) && !bdrv_has_zero_init(bs));
>>  }
>>  
>>  /*
>> diff --git a/include/block/block.h b/include/block/block.h
>> index e41854e..d044b31 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -81,6 +81,32 @@ typedef struct BlockDevOps {
>>  #define BDRV_SECTOR_SIZE   (1ULL << BDRV_SECTOR_BITS)
>>  #define BDRV_SECTOR_MASK   ~(BDRV_SECTOR_SIZE - 1)
>>  
>> +/* BDRV_BLOCK_DATA: data is read from bs->file or another file
>> + * BDRV_BLOCK_ZERO: sectors read as zero
>> + * BDRV_BLOCK_OFFSET_VALID: sector stored in bs->file as raw data
>> + *
>> + * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 represent the offset in
>> + * bs->file where sector data can be read from as raw data.
>> + *
>> + * DATA == 0 && ZERO == 0 means that data is read from backing_hd if present.
>> + *
>> + * DATA ZERO OFFSET_VALID
>> + *  t    t        t       sectors read as zero, bs->file is zero at offset
>> + *  t    f        t       sectors read as valid from bs->file at offset
>> + *  f    t        t       sectors preallocated, read as zero, bs->file not
>> + *                        necessarily zero at offset
>> + *  f    f        t       sectors preallocated but read from backing_hd,
>> + *                        bs->file contains garbage at offset
>> + *  t    t        f       sectors preallocated, read as zero, unknown offset
>> + *  t    f        f       sectors read from unknown file or offset
>> + *  f    t        f       not allocated or unknown offset, read as zero
>> + *  f    f        f       not allocated or unknown offset, read from backing_hd
>> + */
>> +#define BDRV_BLOCK_DATA         1
>> +#define BDRV_BLOCK_ZERO         2
>> +#define BDRV_BLOCK_OFFSET_VALID 4
>> +#define BDRV_BLOCK_OFFSET_MASK  BDRV_SECTOR_MASK
> 
> When are block driver supposed to set the BDRV_BLOCK_OFFSET_VALID flag?

Always if they can provide the information.

> For example, qcow2 could in theory set the flag, it has all of the
> information already in memory.

In fact it does later in the series.

> But with a fragmented image this might
> mean that it returns only one cluster instead of a large area with one
> bdrv_get_block_status() call.

This is just theoretical, right?  qcow2_get_cluster_offset only works on
areas that are contiguous in the raw image.

> Should the caller pass a flag that tells whether he is interested in the
> offset or not?

That would complicate the API (and the implementation too if you want to
honor it in the formats).  Since is_allocated works the same way and it
wasn't a problem so far, I decided not to have such a flag.

Paolo
Kevin Wolf - July 30, 2013, 2:26 p.m.
Am 30.07.2013 um 16:19 hat Paolo Bonzini geschrieben:
> Il 30/07/2013 16:14, Kevin Wolf ha scritto:
> > Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> >> Define the return value of get_block_status.  Bits 0, 1, 2 and 9-62
> >> are valid; bit 63 (the sign bit) is reserved for errors.  Bits 3-8
> >> are left for future extensions.
> >>
> >> The return code is compatible with the old is_allocated API: if a driver
> >> only returns 0 or 1 (aka BDRV_BLOCK_DATA) like is_allocated used to,
> >> clients of is_allocated will not have any change in behavior.  Still,
> >> we will return more precise information in the next patches and the
> >> new definition of bdrv_is_allocated is already prepared for this.
> >>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >>  block.c               | 10 ++++++++--
> >>  include/block/block.h | 26 ++++++++++++++++++++++++++
> >>  2 files changed, 34 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/block.c b/block.c
> >> index f533c36..7cfbf71 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -3004,7 +3004,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
> >>  
> >>      if (!bs->drv->bdrv_co_get_block_status) {
> >>          *pnum = nb_sectors;
> >> -        return 1;
> >> +        return BDRV_BLOCK_DATA;
> >>      }
> >>  
> >>      return bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
> >> @@ -3054,7 +3054,13 @@ int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
> >>  int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num,
> >>                                     int nb_sectors, int *pnum)
> >>  {
> >> -    return bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
> >> +    int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
> >> +    if (ret < 0) {
> >> +        return ret;
> >> +    }
> >> +    return
> >> +        (ret & BDRV_BLOCK_DATA) ||
> >> +        ((ret & BDRV_BLOCK_ZERO) && !bdrv_has_zero_init(bs));
> >>  }
> >>  
> >>  /*
> >> diff --git a/include/block/block.h b/include/block/block.h
> >> index e41854e..d044b31 100644
> >> --- a/include/block/block.h
> >> +++ b/include/block/block.h
> >> @@ -81,6 +81,32 @@ typedef struct BlockDevOps {
> >>  #define BDRV_SECTOR_SIZE   (1ULL << BDRV_SECTOR_BITS)
> >>  #define BDRV_SECTOR_MASK   ~(BDRV_SECTOR_SIZE - 1)
> >>  
> >> +/* BDRV_BLOCK_DATA: data is read from bs->file or another file
> >> + * BDRV_BLOCK_ZERO: sectors read as zero
> >> + * BDRV_BLOCK_OFFSET_VALID: sector stored in bs->file as raw data
> >> + *
> >> + * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 represent the offset in
> >> + * bs->file where sector data can be read from as raw data.
> >> + *
> >> + * DATA == 0 && ZERO == 0 means that data is read from backing_hd if present.
> >> + *
> >> + * DATA ZERO OFFSET_VALID
> >> + *  t    t        t       sectors read as zero, bs->file is zero at offset
> >> + *  t    f        t       sectors read as valid from bs->file at offset
> >> + *  f    t        t       sectors preallocated, read as zero, bs->file not
> >> + *                        necessarily zero at offset
> >> + *  f    f        t       sectors preallocated but read from backing_hd,
> >> + *                        bs->file contains garbage at offset
> >> + *  t    t        f       sectors preallocated, read as zero, unknown offset
> >> + *  t    f        f       sectors read from unknown file or offset
> >> + *  f    t        f       not allocated or unknown offset, read as zero
> >> + *  f    f        f       not allocated or unknown offset, read from backing_hd
> >> + */
> >> +#define BDRV_BLOCK_DATA         1
> >> +#define BDRV_BLOCK_ZERO         2
> >> +#define BDRV_BLOCK_OFFSET_VALID 4
> >> +#define BDRV_BLOCK_OFFSET_MASK  BDRV_SECTOR_MASK
> > 
> > When are block driver supposed to set the BDRV_BLOCK_OFFSET_VALID flag?
> 
> Always if they can provide the information.
> 
> > For example, qcow2 could in theory set the flag, it has all of the
> > information already in memory.
> 
> In fact it does later in the series.
> 
> > But with a fragmented image this might
> > mean that it returns only one cluster instead of a large area with one
> > bdrv_get_block_status() call.
> 
> This is just theoretical, right?  qcow2_get_cluster_offset only works on
> areas that are contiguous in the raw image.

Right, somehow I was thinking that .bdrv_is_allocated() used something
more clever, but it doesn't. So this isn't worse than what we have today.

Kevin

Patch

diff --git a/block.c b/block.c
index f533c36..7cfbf71 100644
--- a/block.c
+++ b/block.c
@@ -3004,7 +3004,7 @@  static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
 
     if (!bs->drv->bdrv_co_get_block_status) {
         *pnum = nb_sectors;
-        return 1;
+        return BDRV_BLOCK_DATA;
     }
 
     return bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
@@ -3054,7 +3054,13 @@  int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
 int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num,
                                    int nb_sectors, int *pnum)
 {
-    return bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
+    int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
+    if (ret < 0) {
+        return ret;
+    }
+    return
+        (ret & BDRV_BLOCK_DATA) ||
+        ((ret & BDRV_BLOCK_ZERO) && !bdrv_has_zero_init(bs));
 }
 
 /*
diff --git a/include/block/block.h b/include/block/block.h
index e41854e..d044b31 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -81,6 +81,32 @@  typedef struct BlockDevOps {
 #define BDRV_SECTOR_SIZE   (1ULL << BDRV_SECTOR_BITS)
 #define BDRV_SECTOR_MASK   ~(BDRV_SECTOR_SIZE - 1)
 
+/* BDRV_BLOCK_DATA: data is read from bs->file or another file
+ * BDRV_BLOCK_ZERO: sectors read as zero
+ * BDRV_BLOCK_OFFSET_VALID: sector stored in bs->file as raw data
+ *
+ * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 represent the offset in
+ * bs->file where sector data can be read from as raw data.
+ *
+ * DATA == 0 && ZERO == 0 means that data is read from backing_hd if present.
+ *
+ * DATA ZERO OFFSET_VALID
+ *  t    t        t       sectors read as zero, bs->file is zero at offset
+ *  t    f        t       sectors read as valid from bs->file at offset
+ *  f    t        t       sectors preallocated, read as zero, bs->file not
+ *                        necessarily zero at offset
+ *  f    f        t       sectors preallocated but read from backing_hd,
+ *                        bs->file contains garbage at offset
+ *  t    t        f       sectors preallocated, read as zero, unknown offset
+ *  t    f        f       sectors read from unknown file or offset
+ *  f    t        f       not allocated or unknown offset, read as zero
+ *  f    f        f       not allocated or unknown offset, read from backing_hd
+ */
+#define BDRV_BLOCK_DATA         1
+#define BDRV_BLOCK_ZERO         2
+#define BDRV_BLOCK_OFFSET_VALID 4
+#define BDRV_BLOCK_OFFSET_MASK  BDRV_SECTOR_MASK
+
 typedef enum {
     BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
 } BlockErrorAction;