* [Qemu-devel] [PATCH] block: Avoid unecessary drv->bdrv_getlength() calls
@ 2013-10-29 11:35 Kevin Wolf
2013-10-29 12:02 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Kevin Wolf @ 2013-10-29 11:35 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, asias, armbru, stefanha
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 <kwolf@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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Avoid unecessary drv->bdrv_getlength() calls
2013-10-29 11:35 [Qemu-devel] [PATCH] block: Avoid unecessary drv->bdrv_getlength() calls Kevin Wolf
@ 2013-10-29 12:02 ` Paolo Bonzini
2013-10-29 12:12 ` Kevin Wolf
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2013-10-29 12:02 UTC (permalink / raw)
To: Kevin Wolf; +Cc: asias, qemu-devel, stefanha, armbru
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 <kwolf@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 <pbonzini@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Avoid unecessary drv->bdrv_getlength() calls
2013-10-29 12:02 ` Paolo Bonzini
@ 2013-10-29 12:12 ` Kevin Wolf
2013-11-04 7:24 ` Fam Zheng
0 siblings, 1 reply; 6+ messages in thread
From: Kevin Wolf @ 2013-10-29 12:12 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: asias, qemu-devel, stefanha, armbru
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 <kwolf@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 <pbonzini@redhat.com>
Thanks, good catch. I've added this now.
Kevin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Avoid unecessary drv->bdrv_getlength() calls
2013-10-29 12:12 ` Kevin Wolf
@ 2013-11-04 7:24 ` Fam Zheng
2013-11-04 11:18 ` Kevin Wolf
0 siblings, 1 reply; 6+ messages in thread
From: Fam Zheng @ 2013-11-04 7:24 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Paolo Bonzini, asias, qemu-devel, stefanha, armbru
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 <kwolf@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 <pbonzini@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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Avoid unecessary drv->bdrv_getlength() calls
2013-11-04 7:24 ` Fam Zheng
@ 2013-11-04 11:18 ` Kevin Wolf
2013-11-04 11:25 ` Fam Zheng
0 siblings, 1 reply; 6+ messages in thread
From: Kevin Wolf @ 2013-11-04 11:18 UTC (permalink / raw)
To: Fam Zheng; +Cc: Paolo Bonzini, asias, qemu-devel, stefanha, armbru
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 <kwolf@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 <pbonzini@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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Avoid unecessary drv->bdrv_getlength() calls
2013-11-04 11:18 ` Kevin Wolf
@ 2013-11-04 11:25 ` Fam Zheng
0 siblings, 0 replies; 6+ messages in thread
From: Fam Zheng @ 2013-11-04 11:25 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Paolo Bonzini, asias, qemu-devel, stefanha, armbru
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 <kwolf@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 <pbonzini@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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-11-04 11:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-29 11:35 [Qemu-devel] [PATCH] block: Avoid unecessary drv->bdrv_getlength() calls Kevin Wolf
2013-10-29 12:02 ` Paolo Bonzini
2013-10-29 12:12 ` Kevin Wolf
2013-11-04 7:24 ` Fam Zheng
2013-11-04 11:18 ` Kevin Wolf
2013-11-04 11:25 ` Fam Zheng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.