Patchwork [PATCHv4] block/get_block_status: avoid redundant callouts on raw devices

login
register
mail settings
Submitter Peter Lieven
Date Oct. 2, 2013, 2:20 p.m.
Message ID <1380723636-18456-1-git-send-email-pl@kamp.de>
Download mbox | patch
Permalink /patch/279747/
State New
Headers show

Comments

Peter Lieven - Oct. 2, 2013, 2:20 p.m.
if a raw device like an iscsi target or host device is used
the current implementation makes a second call out to get
the block status of bs->file. however, the raw driver already
has called bdrv_get_block_status on bs->file.

v4: use a flag to detect the raw driver instead of the strncmp
    hack.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block.c               |    4 ++--
 block/raw_bsd.c       |    6 +++++-
 include/block/block.h |    3 +++
 3 files changed, 10 insertions(+), 3 deletions(-)
Stefan Hajnoczi - Oct. 2, 2013, 3:06 p.m.
On Wed, Oct 02, 2013 at 04:20:36PM +0200, Peter Lieven wrote:
> if a raw device like an iscsi target or host device is used
> the current implementation makes a second call out to get
> the block status of bs->file. however, the raw driver already
> has called bdrv_get_block_status on bs->file.
> 
> v4: use a flag to detect the raw driver instead of the strncmp
>     hack.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block.c               |    4 ++--
>  block/raw_bsd.c       |    6 +++++-
>  include/block/block.h |    3 +++
>  3 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 93e113a..7fa2e43 100644
> --- a/block.c
> +++ b/block.c
> @@ -3161,7 +3161,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>  
>      if (bs->file &&
>          (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
> -        (ret & BDRV_BLOCK_OFFSET_VALID)) {
> +        (ret & BDRV_BLOCK_OFFSET_VALID) && !(ret & BDRV_BLOCK_RAW)) {
>          ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
>                                          *pnum, pnum);
>          if (ret2 >= 0) {
> @@ -3172,7 +3172,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>          }
>      }
>  
> -    return ret;
> +    return ret & ~BDRV_BLOCK_RAW;
>  }
>  
>  /* Coroutine wrapper for bdrv_get_block_status() */
> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> index d4ace60..a9e0209 100644
> --- a/block/raw_bsd.c
> +++ b/block/raw_bsd.c
> @@ -62,7 +62,11 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
>                                              int64_t sector_num,
>                                              int nb_sectors, int *pnum)
>  {
> -    return bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum);
> +    int64_t ret = bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    return ret | BDRV_BLOCK_RAW;
>  }
>  
>  static int coroutine_fn raw_co_write_zeroes(BlockDriverState *bs,
> diff --git a/include/block/block.h b/include/block/block.h
> index f808550..cb7019b 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -84,6 +84,8 @@ typedef struct BlockDevOps {
>  /* 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
> + * BDRV_BLOCK_RAW: used internally to indicate that the request
> + *                 was piped through the raw driver

Sorry I didn't review this earlier but this flag looks hacky and I'm not
confident about merging the patch yet.

The patch makes me wonder if the raw_bsd driver should avoid calling
bs->file itself:

return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID |
       (sector_num << BDRV_SECTOR_BITS);

Let block.c:bdrv_co_get_block_status() call down into bs->file.

The problem is then the protocol cannot report unallocated sectors with
this approach.

I think we want to preserve bs' offset while taking the other flags from
bs->file (DATA, ZERO).

Peter, Paolo: What do you think of this approach?

Stefan
Eric Blake - Oct. 2, 2013, 3:08 p.m.
On 10/02/2013 08:20 AM, Peter Lieven wrote:
> if a raw device like an iscsi target or host device is used
> the current implementation makes a second call out to get
> the block status of bs->file. however, the raw driver already
> has called bdrv_get_block_status on bs->file.
> 
> v4: use a flag to detect the raw driver instead of the strncmp
>     hack.

The v4 designation...

> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---

...should appear after the --- separator (useful to reviewers, but
doesn't need to be part of the git history).

>  block.c               |    4 ++--
>  block/raw_bsd.c       |    6 +++++-
>  include/block/block.h |    3 +++
>  3 files changed, 10 insertions(+), 3 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>
Paolo Bonzini - Oct. 2, 2013, 3:13 p.m.
Il 02/10/2013 17:06, Stefan Hajnoczi ha scritto:
> Sorry I didn't review this earlier but this flag looks hacky and I'm not
> confident about merging the patch yet.
> 
> The patch makes me wonder if the raw_bsd driver should avoid calling
> bs->file itself:
> 
> return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID |
>        (sector_num << BDRV_SECTOR_BITS);
> 
> Let block.c:bdrv_co_get_block_status() call down into bs->file.
> 
> The problem is then the protocol cannot report unallocated sectors with
> this approach.
> 
> I think we want to preserve bs' offset while taking the other flags from
> bs->file (DATA, ZERO).

This would cause other changes.  For example, a qcow2 with full metadata
preallocation (i.e. all L2 tables are there but it points to holes)
would not return DATA anymore.  I think this is wrong, and especially a
change from the old is_allocated API.

However, a variant on this idea could be to return

   BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
        (sector_num << BDRV_SECTOR_BITS);

and then BDRV_BLOCK_RAW would mean "take DATA and ZERO from bs->file".

Paolo

> Peter, Paolo: What do you think of this approach?
Peter Lieven - Oct. 2, 2013, 3:34 p.m.
Am 02.10.2013 17:13, schrieb Paolo Bonzini:
> Il 02/10/2013 17:06, Stefan Hajnoczi ha scritto:
>> Sorry I didn't review this earlier but this flag looks hacky and I'm not
>> confident about merging the patch yet.
>>
>> The patch makes me wonder if the raw_bsd driver should avoid calling
>> bs->file itself:
>>
>> return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID |
>>        (sector_num << BDRV_SECTOR_BITS);
>>
>> Let block.c:bdrv_co_get_block_status() call down into bs->file.
>>
>> The problem is then the protocol cannot report unallocated sectors with
>> this approach.
>>
>> I think we want to preserve bs' offset while taking the other flags from
>> bs->file (DATA, ZERO).
> This would cause other changes.  For example, a qcow2 with full metadata
> preallocation (i.e. all L2 tables are there but it points to holes)
> would not return DATA anymore.  I think this is wrong, and especially a
> change from the old is_allocated API.
>
> However, a variant on this idea could be to return
>
>    BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
>         (sector_num << BDRV_SECTOR_BITS);
>
> and then BDRV_BLOCK_RAW would mean "take DATA and ZERO from bs->file".
I am fine with that.

Peter

Patch

diff --git a/block.c b/block.c
index 93e113a..7fa2e43 100644
--- a/block.c
+++ b/block.c
@@ -3161,7 +3161,7 @@  static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
 
     if (bs->file &&
         (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
-        (ret & BDRV_BLOCK_OFFSET_VALID)) {
+        (ret & BDRV_BLOCK_OFFSET_VALID) && !(ret & BDRV_BLOCK_RAW)) {
         ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
                                         *pnum, pnum);
         if (ret2 >= 0) {
@@ -3172,7 +3172,7 @@  static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
         }
     }
 
-    return ret;
+    return ret & ~BDRV_BLOCK_RAW;
 }
 
 /* Coroutine wrapper for bdrv_get_block_status() */
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index d4ace60..a9e0209 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -62,7 +62,11 @@  static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
                                             int64_t sector_num,
                                             int nb_sectors, int *pnum)
 {
-    return bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum);
+    int64_t ret = bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum);
+    if (ret < 0) {
+        return ret;
+    }
+    return ret | BDRV_BLOCK_RAW;
 }
 
 static int coroutine_fn raw_co_write_zeroes(BlockDriverState *bs,
diff --git a/include/block/block.h b/include/block/block.h
index f808550..cb7019b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -84,6 +84,8 @@  typedef struct BlockDevOps {
 /* 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
+ * BDRV_BLOCK_RAW: used internally to indicate that the request
+ *                 was piped through the raw driver
  *
  * 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.
@@ -105,6 +107,7 @@  typedef struct BlockDevOps {
 #define BDRV_BLOCK_DATA         1
 #define BDRV_BLOCK_ZERO         2
 #define BDRV_BLOCK_OFFSET_VALID 4
+#define BDRV_BLOCK_RAW          8
 #define BDRV_BLOCK_OFFSET_MASK  BDRV_SECTOR_MASK
 
 typedef enum {