Re: [Qemu-devel] [PATCH] block: Avoid unecessary drv-bdrv_getlength() calls

[Kevin Wolf]

Am 04.11.2013 um 08:24 hat Fam Zheng geschrieben:
 
 
 On Tue 29 Oct 2013 08:12:38 PM CST, Kevin Wolf wrote:
 Am 29.10.2013 um 13:02 hat Paolo Bonzini geschrieben:
 Il 29/10/2013 12:35, Kevin Wolf ha scritto:
 The block layer generally keeps the size of an image cached in
 bs-total_sectors so that it doesn't have to perform expensive
 operations to get the size whenever it needs it.
 
 This doesn't work however when using a backend that can change its size
 without qemu being aware of it, i.e. passthrough of removable media like
 CD-ROMs or floppy disks. For this reason, the caching is disabled when a
 removable device is used.
 
 It is obvious that checking whether the _guest_ device has removable
 media isn't the right thing to do when we want to know whether the size
 of the host backend can change. To make things worse, non-top-level
 BlockDriverStates never have any device attached, which makes qemu
 assume they are removable, so drv-bdrv_getlength() is always called on
 the protocol layer. In the case of raw-posix, this causes unnecessary
 lseek() system calls, which turned out to be rather expensive.
 
 This patch completely changes the logic and disables bs-total_sectors
 caching only for certain block driver types, for which a size change is
 expected: host_cdrom and host_floppy; also the raw format in case it
 sits on top of one of these protocols, but in the common case the nested
 bdrv_getlength() call on the protocol driver will use the cache again
 and avoid an expensive drv-bdrv_getlength() call.
 
 Signed-off-by: Kevin Wolf kw...@redhat.com
 
 raw-win32.c probably needs to have a .has_variable_length=true in
 bdrv_host_device.  Apart from that,
 
 Reviewed-by: Paolo Bonzini pbonz...@redhat.com
 
 Thanks, good catch. I've added this now.
 
 This breaks VMDK because it can't read description file: buffer is
 empty in bdrv_probe, when the file size is 512 bytes. (for
 raw-posix).

Perhaps we should replace bs-total_sectors with bs-total_byts one
day... For now, how about introducing a bdrv_getlength_bytes() that
always calls the driver and never converts bytes to sectors?

Kevin

Re: [Qemu-devel] [PATCH] block: Avoid unecessary drv-bdrv_getlength() calls

[Fam Zheng]

On 11/04/2013 07:18 PM, Kevin Wolf wrote:

Am 04.11.2013 um 08:24 hat Fam Zheng geschrieben:


On Tue 29 Oct 2013 08:12:38 PM CST, Kevin Wolf wrote:

Am 29.10.2013 um 13:02 hat Paolo Bonzini geschrieben:

Il 29/10/2013 12:35, Kevin Wolf ha scritto:

The block layer generally keeps the size of an image cached in
bs-total_sectors so that it doesn't have to perform expensive
operations to get the size whenever it needs it.

This doesn't work however when using a backend that can change its size
without qemu being aware of it, i.e. passthrough of removable media like
CD-ROMs or floppy disks. For this reason, the caching is disabled when a
removable device is used.

It is obvious that checking whether the _guest_ device has removable
media isn't the right thing to do when we want to know whether the size
of the host backend can change. To make things worse, non-top-level
BlockDriverStates never have any device attached, which makes qemu
assume they are removable, so drv-bdrv_getlength() is always called on
the protocol layer. In the case of raw-posix, this causes unnecessary
lseek() system calls, which turned out to be rather expensive.

This patch completely changes the logic and disables bs-total_sectors
caching only for certain block driver types, for which a size change is
expected: host_cdrom and host_floppy; also the raw format in case it
sits on top of one of these protocols, but in the common case the nested
bdrv_getlength() call on the protocol driver will use the cache again
and avoid an expensive drv-bdrv_getlength() call.

Signed-off-by: Kevin Wolf kw...@redhat.com

raw-win32.c probably needs to have a .has_variable_length=true in
bdrv_host_device.  Apart from that,

Reviewed-by: Paolo Bonzini pbonz...@redhat.com

Thanks, good catch. I've added this now.

This breaks VMDK because it can't read description file: buffer is
empty in bdrv_probe, when the file size is 512 bytes. (for
raw-posix).

Perhaps we should replace bs-total_sectors with bs-total_byts one
day... For now, how about introducing a bdrv_getlength_bytes() that
always calls the driver and never converts bytes to sectors?
I think it will work. And is it correct to round up total_bytes to 
BDRV_SECTOR_SIZE before storing to total_sectors? From my view it's 
better than losing some bytes by rounding down.


Fam

Re: [Qemu-devel] [PATCH] block: Avoid unecessary drv-bdrv_getlength() calls

[Fam Zheng]

On Tue 29 Oct 2013 08:12:38 PM CST, Kevin Wolf wrote:

Am 29.10.2013 um 13:02 hat Paolo Bonzini geschrieben:

Il 29/10/2013 12:35, Kevin Wolf ha scritto:

The block layer generally keeps the size of an image cached in
bs-total_sectors so that it doesn't have to perform expensive
operations to get the size whenever it needs it.

This doesn't work however when using a backend that can change its size
without qemu being aware of it, i.e. passthrough of removable media like
CD-ROMs or floppy disks. For this reason, the caching is disabled when a
removable device is used.

It is obvious that checking whether the _guest_ device has removable
media isn't the right thing to do when we want to know whether the size
of the host backend can change. To make things worse, non-top-level
BlockDriverStates never have any device attached, which makes qemu
assume they are removable, so drv-bdrv_getlength() is always called on
the protocol layer. In the case of raw-posix, this causes unnecessary
lseek() system calls, which turned out to be rather expensive.

This patch completely changes the logic and disables bs-total_sectors
caching only for certain block driver types, for which a size change is
expected: host_cdrom and host_floppy; also the raw format in case it
sits on top of one of these protocols, but in the common case the nested
bdrv_getlength() call on the protocol driver will use the cache again
and avoid an expensive drv-bdrv_getlength() call.

Signed-off-by: Kevin Wolf kw...@redhat.com



raw-win32.c probably needs to have a .has_variable_length=true in
bdrv_host_device.  Apart from that,

Reviewed-by: Paolo Bonzini pbonz...@redhat.com


Thanks, good catch. I've added this now.


This breaks VMDK because it can't read description file: buffer is 
empty in bdrv_probe, when the file size is 512 bytes. (for raw-posix).


Fam

[Qemu-devel] [PATCH] block: Avoid unecessary drv-bdrv_getlength() calls

[Kevin Wolf]

The block layer generally keeps the size of an image cached in
bs-total_sectors so that it doesn't have to perform expensive
operations to get the size whenever it needs it.

This doesn't work however when using a backend that can change its size
without qemu being aware of it, i.e. passthrough of removable media like
CD-ROMs or floppy disks. For this reason, the caching is disabled when a
removable device is used.

It is obvious that checking whether the _guest_ device has removable
media isn't the right thing to do when we want to know whether the size
of the host backend can change. To make things worse, non-top-level
BlockDriverStates never have any device attached, which makes qemu
assume they are removable, so drv-bdrv_getlength() is always called on
the protocol layer. In the case of raw-posix, this causes unnecessary
lseek() system calls, which turned out to be rather expensive.

This patch completely changes the logic and disables bs-total_sectors
caching only for certain block driver types, for which a size change is
expected: host_cdrom and host_floppy; also the raw format in case it
sits on top of one of these protocols, but in the common case the nested
bdrv_getlength() call on the protocol driver will use the cache again
and avoid an expensive drv-bdrv_getlength() call.

Signed-off-by: Kevin Wolf kw...@redhat.com
---
 block.c   | 7 ---
 block/raw-posix.c | 9 ++---
 block/raw_bsd.c   | 1 +
 include/block/block_int.h | 3 +++
 4 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 366999b..da88be0 100644
--- a/block.c
+++ b/block.c
@@ -2868,9 +2868,10 @@ int64_t bdrv_getlength(BlockDriverState *bs)
 if (!drv)
 return -ENOMEDIUM;
 
-if (bdrv_dev_has_removable_media(bs)) {
-if (drv-bdrv_getlength) {
-return drv-bdrv_getlength(bs);
+if (drv-has_variable_length) {
+int ret = refresh_total_sectors(bs, bs-total_sectors);
+if (ret  0) {
+return ret;
 }
 }
 return bs-total_sectors * BDRV_SECTOR_SIZE;
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 6f03fbf..f6d48bb 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1715,7 +1715,8 @@ static BlockDriver bdrv_host_floppy = {
 .bdrv_aio_flush= raw_aio_flush,
 
 .bdrv_truncate  = raw_truncate,
-.bdrv_getlength= raw_getlength,
+.bdrv_getlength  = raw_getlength,
+.has_variable_length = true,
 .bdrv_get_allocated_file_size
 = raw_get_allocated_file_size,
 
@@ -1824,7 +1825,8 @@ static BlockDriver bdrv_host_cdrom = {
 .bdrv_aio_flush= raw_aio_flush,
 
 .bdrv_truncate  = raw_truncate,
-.bdrv_getlength = raw_getlength,
+.bdrv_getlength  = raw_getlength,
+.has_variable_length = true,
 .bdrv_get_allocated_file_size
 = raw_get_allocated_file_size,
 
@@ -1951,7 +1953,8 @@ static BlockDriver bdrv_host_cdrom = {
 .bdrv_aio_flush= raw_aio_flush,
 
 .bdrv_truncate  = raw_truncate,
-.bdrv_getlength = raw_getlength,
+.bdrv_getlength  = raw_getlength,
+.has_variable_length = true,
 .bdrv_get_allocated_file_size
 = raw_get_allocated_file_size,
 
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 0078c1b..2265dcc 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -178,6 +178,7 @@ static BlockDriver bdrv_raw = {
 .bdrv_co_get_block_status = raw_co_get_block_status,
 .bdrv_truncate= raw_truncate,
 .bdrv_getlength   = raw_getlength,
+.has_variable_length  = true,
 .bdrv_get_info= raw_get_info,
 .bdrv_is_inserted = raw_is_inserted,
 .bdrv_media_changed   = raw_media_changed,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index a48731d..1666066 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -156,8 +156,11 @@ struct BlockDriver {
 
 const char *protocol_name;
 int (*bdrv_truncate)(BlockDriverState *bs, int64_t offset);
+
 int64_t (*bdrv_getlength)(BlockDriverState *bs);
+bool has_variable_length;
 int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
+
 int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
  const uint8_t *buf, int nb_sectors);
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] block: Avoid unecessary drv-bdrv_getlength() calls

[Paolo Bonzini]

Il 29/10/2013 12:35, Kevin Wolf ha scritto:
 The block layer generally keeps the size of an image cached in
 bs-total_sectors so that it doesn't have to perform expensive
 operations to get the size whenever it needs it.
 
 This doesn't work however when using a backend that can change its size
 without qemu being aware of it, i.e. passthrough of removable media like
 CD-ROMs or floppy disks. For this reason, the caching is disabled when a
 removable device is used.
 
 It is obvious that checking whether the _guest_ device has removable
 media isn't the right thing to do when we want to know whether the size
 of the host backend can change. To make things worse, non-top-level
 BlockDriverStates never have any device attached, which makes qemu
 assume they are removable, so drv-bdrv_getlength() is always called on
 the protocol layer. In the case of raw-posix, this causes unnecessary
 lseek() system calls, which turned out to be rather expensive.
 
 This patch completely changes the logic and disables bs-total_sectors
 caching only for certain block driver types, for which a size change is
 expected: host_cdrom and host_floppy; also the raw format in case it
 sits on top of one of these protocols, but in the common case the nested
 bdrv_getlength() call on the protocol driver will use the cache again
 and avoid an expensive drv-bdrv_getlength() call.
 
 Signed-off-by: Kevin Wolf kw...@redhat.com
 ---
  block.c   | 7 ---
  block/raw-posix.c | 9 ++---
  block/raw_bsd.c   | 1 +
  include/block/block_int.h | 3 +++
  4 files changed, 14 insertions(+), 6 deletions(-)
 
 diff --git a/block.c b/block.c
 index 366999b..da88be0 100644
 --- a/block.c
 +++ b/block.c
 @@ -2868,9 +2868,10 @@ int64_t bdrv_getlength(BlockDriverState *bs)
  if (!drv)
  return -ENOMEDIUM;
  
 -if (bdrv_dev_has_removable_media(bs)) {
 -if (drv-bdrv_getlength) {
 -return drv-bdrv_getlength(bs);
 +if (drv-has_variable_length) {
 +int ret = refresh_total_sectors(bs, bs-total_sectors);
 +if (ret  0) {
 +return ret;
  }
  }
  return bs-total_sectors * BDRV_SECTOR_SIZE;
 diff --git a/block/raw-posix.c b/block/raw-posix.c
 index 6f03fbf..f6d48bb 100644
 --- a/block/raw-posix.c
 +++ b/block/raw-posix.c
 @@ -1715,7 +1715,8 @@ static BlockDriver bdrv_host_floppy = {
  .bdrv_aio_flush  = raw_aio_flush,
  
  .bdrv_truncate  = raw_truncate,
 -.bdrv_getlength  = raw_getlength,
 +.bdrv_getlength  = raw_getlength,
 +.has_variable_length = true,
  .bdrv_get_allocated_file_size
  = raw_get_allocated_file_size,
  
 @@ -1824,7 +1825,8 @@ static BlockDriver bdrv_host_cdrom = {
  .bdrv_aio_flush  = raw_aio_flush,
  
  .bdrv_truncate  = raw_truncate,
 -.bdrv_getlength = raw_getlength,
 +.bdrv_getlength  = raw_getlength,
 +.has_variable_length = true,
  .bdrv_get_allocated_file_size
  = raw_get_allocated_file_size,
  
 @@ -1951,7 +1953,8 @@ static BlockDriver bdrv_host_cdrom = {
  .bdrv_aio_flush  = raw_aio_flush,
  
  .bdrv_truncate  = raw_truncate,
 -.bdrv_getlength = raw_getlength,
 +.bdrv_getlength  = raw_getlength,
 +.has_variable_length = true,
  .bdrv_get_allocated_file_size
  = raw_get_allocated_file_size,
  
 diff --git a/block/raw_bsd.c b/block/raw_bsd.c
 index 0078c1b..2265dcc 100644
 --- a/block/raw_bsd.c
 +++ b/block/raw_bsd.c
 @@ -178,6 +178,7 @@ static BlockDriver bdrv_raw = {
  .bdrv_co_get_block_status = raw_co_get_block_status,
  .bdrv_truncate= raw_truncate,
  .bdrv_getlength   = raw_getlength,
 +.has_variable_length  = true,
  .bdrv_get_info= raw_get_info,
  .bdrv_is_inserted = raw_is_inserted,
  .bdrv_media_changed   = raw_media_changed,
 diff --git a/include/block/block_int.h b/include/block/block_int.h
 index a48731d..1666066 100644
 --- a/include/block/block_int.h
 +++ b/include/block/block_int.h
 @@ -156,8 +156,11 @@ struct BlockDriver {
  
  const char *protocol_name;
  int (*bdrv_truncate)(BlockDriverState *bs, int64_t offset);
 +
  int64_t (*bdrv_getlength)(BlockDriverState *bs);
 +bool has_variable_length;
  int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
 +
  int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
   const uint8_t *buf, int nb_sectors);
  
 

raw-win32.c probably needs to have a .has_variable_length=true in
bdrv_host_device.  Apart from that,

Reviewed-by: Paolo Bonzini pbonz...@redhat.com

Re: [Qemu-devel] [PATCH] block: Avoid unecessary drv-bdrv_getlength() calls

[Kevin Wolf]

Am 29.10.2013 um 13:02 hat Paolo Bonzini geschrieben:
 Il 29/10/2013 12:35, Kevin Wolf ha scritto:
  The block layer generally keeps the size of an image cached in
  bs-total_sectors so that it doesn't have to perform expensive
  operations to get the size whenever it needs it.
  
  This doesn't work however when using a backend that can change its size
  without qemu being aware of it, i.e. passthrough of removable media like
  CD-ROMs or floppy disks. For this reason, the caching is disabled when a
  removable device is used.
  
  It is obvious that checking whether the _guest_ device has removable
  media isn't the right thing to do when we want to know whether the size
  of the host backend can change. To make things worse, non-top-level
  BlockDriverStates never have any device attached, which makes qemu
  assume they are removable, so drv-bdrv_getlength() is always called on
  the protocol layer. In the case of raw-posix, this causes unnecessary
  lseek() system calls, which turned out to be rather expensive.
  
  This patch completely changes the logic and disables bs-total_sectors
  caching only for certain block driver types, for which a size change is
  expected: host_cdrom and host_floppy; also the raw format in case it
  sits on top of one of these protocols, but in the common case the nested
  bdrv_getlength() call on the protocol driver will use the cache again
  and avoid an expensive drv-bdrv_getlength() call.
  
  Signed-off-by: Kevin Wolf kw...@redhat.com

 raw-win32.c probably needs to have a .has_variable_length=true in
 bdrv_host_device.  Apart from that,
 
 Reviewed-by: Paolo Bonzini pbonz...@redhat.com

Thanks, good catch. I've added this now.

Kevin