Re: [Qemu-devel] [PATCHv5] block/get_block_status: avoid redundant callouts on raw devices

2013-10-08 Thread Stefan Hajnoczi 
On Mon, Oct 7, 2013 at 10:25 AM, Paolo Bonzini pbonz...@redhat.com wrote:
 Il 07/10/2013 07:59, Peter Lieven ha scritto:
 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.

 Signed-off-by: Peter Lieven p...@kamp.de
 ---
 v5: add a generic get_lba_status function in the raw driver which
 adds the BDRV_BLOCK_RAW flag. bdrv_co_get_block_status will
 handle the callout to bs-file then.

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

  block.c   |4 
  block/raw_bsd.c   |3 ++-
  include/block/block.h |4 
  3 files changed, 10 insertions(+), 1 deletion(-)

 diff --git a/block.c b/block.c
 index 93e113a..38a589e 100644
 --- a/block.c
 +++ b/block.c
 @@ -3147,6 +3147,10 @@ static int64_t coroutine_fn 
 bdrv_co_get_block_status(BlockDriverState *bs,
  return ret;
  }

 +if (ret  BDRV_BLOCK_RAW) {
 +return bdrv_get_block_status(bs-file, sector_num, nb_sectors, 
 pnum);

 Strictly speaking, this should probably do something like this:

   assert(ret  BDRV_BLOCK_OFFSET_VALID);
   return bdrv_get_block_status(bs-file, ret  BDRV_SECTOR_BITS,
nb_sectors, pnum);

 Or alternatively the raw driver should return just BDRV_BLOCK_RAW.

 As a third option, the raw driver could also return not just
 BDRV_BLOCK_RAW and BDRV_BLOCK_OFFSET_VALID, but also BDRV_BLOCK_DATA (so
 that the answer makes some sense even without going down to bs-file).

 But I'll let the block maintainers decide what to do.

The return value is a bitfield, so let's use those bits and take Option 3.

Stefan

Re: [Qemu-devel] [PATCHv5] block/get_block_status: avoid redundant callouts on raw devices

2013-10-08 Thread Peter Lieven 

On 07.10.2013 10:25, Paolo Bonzini wrote:

Il 07/10/2013 07:59, Peter Lieven ha scritto:

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.

Signed-off-by: Peter Lieven p...@kamp.de
---
v5: add a generic get_lba_status function in the raw driver which
 adds the BDRV_BLOCK_RAW flag. bdrv_co_get_block_status will
 handle the callout to bs-file then.

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

  block.c   |4 
  block/raw_bsd.c   |3 ++-
  include/block/block.h |4 
  3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 93e113a..38a589e 100644
--- a/block.c
+++ b/block.c
@@ -3147,6 +3147,10 @@ static int64_t coroutine_fn 
bdrv_co_get_block_status(BlockDriverState *bs,
  return ret;
  }
  
+if (ret  BDRV_BLOCK_RAW) {

+return bdrv_get_block_status(bs-file, sector_num, nb_sectors, pnum);

Strictly speaking, this should probably do something like this:

   assert(ret  BDRV_BLOCK_OFFSET_VALID);
   return bdrv_get_block_status(bs-file, ret  BDRV_SECTOR_BITS,
nb_sectors, pnum);

shouldn't the last line be:

  return bdrv_get_block_status(bs-file, ret  BDRV_SECTOR_BITS,
   *pnum, pnum);


This would of course require *pnum = nb_sectors in raw_co_get_block_status
?

Re: [Qemu-devel] [PATCHv5] block/get_block_status: avoid redundant callouts on raw devices

2013-10-08 Thread Paolo Bonzini 
Il 08/10/2013 14:05, Peter Lieven ha scritto:
 Strictly speaking, this should probably do something like this:

assert(ret  BDRV_BLOCK_OFFSET_VALID);
return bdrv_get_block_status(bs-file, ret  BDRV_SECTOR_BITS,
 nb_sectors, pnum);
 shouldn't the last line be:
 
   return bdrv_get_block_status(bs-file, ret  BDRV_SECTOR_BITS,
*pnum, pnum);
 
 
 This would of course require *pnum = nb_sectors in raw_co_get_block_status
 ?

Yes for both questions.

Paolo

[Qemu-devel] [PATCHv5] block/get_block_status: avoid redundant callouts on raw devices

2013-10-07 Thread Peter Lieven 
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.

Signed-off-by: Peter Lieven p...@kamp.de
---
v5: add a generic get_lba_status function in the raw driver which
adds the BDRV_BLOCK_RAW flag. bdrv_co_get_block_status will
handle the callout to bs-file then.

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

 block.c   |4 
 block/raw_bsd.c   |3 ++-
 include/block/block.h |4 
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 93e113a..38a589e 100644
--- a/block.c
+++ b/block.c
@@ -3147,6 +3147,10 @@ static int64_t coroutine_fn 
bdrv_co_get_block_status(BlockDriverState *bs,
 return ret;
 }
 
+if (ret  BDRV_BLOCK_RAW) {
+return bdrv_get_block_status(bs-file, sector_num, nb_sectors, pnum);
+}
+
 if (!(ret  BDRV_BLOCK_DATA)) {
 if (bdrv_has_zero_init(bs)) {
 ret |= BDRV_BLOCK_ZERO;
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index d4ace60..308d605 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -62,7 +62,8 @@ 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);
+return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
+   (sector_num  BDRV_SECTOR_BITS);
 }
 
 static int coroutine_fn raw_co_write_zeroes(BlockDriverState *bs,
diff --git a/include/block/block.h b/include/block/block.h
index f808550..003699e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -84,6 +84,9 @@ 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 answered by the raw driver and that one
+ * should look in bs-file directly.
  *
  * 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 +108,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 {
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv5] block/get_block_status: avoid redundant callouts on raw devices

2013-10-07 Thread Paolo Bonzini 
Il 07/10/2013 07:59, Peter Lieven ha scritto:
 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.
 
 Signed-off-by: Peter Lieven p...@kamp.de
 ---
 v5: add a generic get_lba_status function in the raw driver which
 adds the BDRV_BLOCK_RAW flag. bdrv_co_get_block_status will
 handle the callout to bs-file then.
 
 v4: use a flag to detect the raw driver instead of the strncmp
 hack
 
  block.c   |4 
  block/raw_bsd.c   |3 ++-
  include/block/block.h |4 
  3 files changed, 10 insertions(+), 1 deletion(-)
 
 diff --git a/block.c b/block.c
 index 93e113a..38a589e 100644
 --- a/block.c
 +++ b/block.c
 @@ -3147,6 +3147,10 @@ static int64_t coroutine_fn 
 bdrv_co_get_block_status(BlockDriverState *bs,
  return ret;
  }
  
 +if (ret  BDRV_BLOCK_RAW) {
 +return bdrv_get_block_status(bs-file, sector_num, nb_sectors, pnum);

Strictly speaking, this should probably do something like this:

  assert(ret  BDRV_BLOCK_OFFSET_VALID);
  return bdrv_get_block_status(bs-file, ret  BDRV_SECTOR_BITS,
   nb_sectors, pnum);

Or alternatively the raw driver should return just BDRV_BLOCK_RAW.

As a third option, the raw driver could also return not just
BDRV_BLOCK_RAW and BDRV_BLOCK_OFFSET_VALID, but also BDRV_BLOCK_DATA (so
that the answer makes some sense even without going down to bs-file).

But I'll let the block maintainers decide what to do.

Paolo

 +}
 +
  if (!(ret  BDRV_BLOCK_DATA)) {
  if (bdrv_has_zero_init(bs)) {
  ret |= BDRV_BLOCK_ZERO;
 diff --git a/block/raw_bsd.c b/block/raw_bsd.c
 index d4ace60..308d605 100644
 --- a/block/raw_bsd.c
 +++ b/block/raw_bsd.c
 @@ -62,7 +62,8 @@ 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);
 +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
 +   (sector_num  BDRV_SECTOR_BITS);
  }
  
  static int coroutine_fn raw_co_write_zeroes(BlockDriverState *bs,
 diff --git a/include/block/block.h b/include/block/block.h
 index f808550..003699e 100644
 --- a/include/block/block.h
 +++ b/include/block/block.h
 @@ -84,6 +84,9 @@ 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 answered by the raw driver and that one
 + * should look in bs-file directly.
   *
   * 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 +108,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 {

Re: [Qemu-devel] [PATCHv5] block/get_block_status: avoid redundant callouts on raw devices

2013-10-07 Thread Peter Lieven 

On 07.10.2013 10:25, Paolo Bonzini wrote:

Il 07/10/2013 07:59, Peter Lieven ha scritto:

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.

Signed-off-by: Peter Lieven p...@kamp.de
---
v5: add a generic get_lba_status function in the raw driver which
 adds the BDRV_BLOCK_RAW flag. bdrv_co_get_block_status will
 handle the callout to bs-file then.

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

  block.c   |4 
  block/raw_bsd.c   |3 ++-
  include/block/block.h |4 
  3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 93e113a..38a589e 100644
--- a/block.c
+++ b/block.c
@@ -3147,6 +3147,10 @@ static int64_t coroutine_fn 
bdrv_co_get_block_status(BlockDriverState *bs,
  return ret;
  }
  
+if (ret  BDRV_BLOCK_RAW) {

+return bdrv_get_block_status(bs-file, sector_num, nb_sectors, pnum);

Strictly speaking, this should probably do something like this:

   assert(ret  BDRV_BLOCK_OFFSET_VALID);
   return bdrv_get_block_status(bs-file, ret  BDRV_SECTOR_BITS,
nb_sectors, pnum);

Or alternatively the raw driver should return just BDRV_BLOCK_RAW.

As a third option, the raw driver could also return not just
BDRV_BLOCK_RAW and BDRV_BLOCK_OFFSET_VALID, but also BDRV_BLOCK_DATA (so
that the answer makes some sense even without going down to bs-file).

But I'll let the block maintainers decide what to do.

Okay, I will wait for their feedback.

Peter