| Message ID | 20230407153303.391121-2-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: remove bdrv_co_get_geometry coroutines from I/O hot path | expand |
On Fri, Apr 07, 2023 at 05:32:56PM +0200, Paolo Bonzini wrote: > At the protocol level, has_variable_length only needs to be true in the > very special case of host CD-ROM drives, so that they do not need an > explicit monitor command to read the new size when a disc is loaded > in the tray. > > However, at the format level has_variable_length has to be true for all > raw blockdevs and for all filters, even though in practice the length > depends on the underlying file and thus will not change except in the > case of host CD-ROM drives. > > As a first step towards computing an accurate value of has_variable_length, > add the value into the BlockLimits structure and initialize the field > from the BlockDriver. My assumption here is that all other resizes (such as a block device that can be externally enlarged upon seeing the guest pause due to an ENOSPC condition on the current size) are NOT expected to have automatic reaction in qemu, but instead rely on some other external action (such as resuming after ENOSPC or an explicit monitor command) as the reason for why qemu eventually learns the new size. If my assumption is right, then you do sound correct in stating that CDROMs are special in that the guest OS can request the tray to be loaded and change the size from 0 to the size of the newly-inserted disc, with no intervening action or QMP command, and qemu must react to that size change. I'm asking because at one point, there was a proposal to extend the NBD protocol to allow dynamic resizing, somewhat similar to how a block device can be externally resized; there were questions on how much of a resize action has to be from client-to-server "please poll if the server has changed size recently" instead of server-to-client "by the way, the size just changed". I don't want NBD resize (if someone ever fleshes out the extension propsal into an actual implementation) to be hamstrung by an inability to reflect size changes initiated on the server side, rather than triggered at the client side. But since I think regular files and block devices have the same considerations, I don't think it is a show-stopper to this series. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > block.c | 2 +- > block/io.c | 6 ++++++ > include/block/block_int-common.h | 8 ++++++++ > 3 files changed, 15 insertions(+), 1 deletion(-) Reviewed-by: Eric Blake <eblake@redhat.com>
diff --git a/block.c b/block.c index 89a79c321fab..b1b7c7efe036 100644 --- a/block.c +++ b/block.c @@ -5849,7 +5849,7 @@ int64_t coroutine_fn bdrv_co_nb_sectors(BlockDriverState *bs) if (!drv) return -ENOMEDIUM; - if (drv->has_variable_length) { + if (bs->bl.has_variable_length) { int ret = bdrv_co_refresh_total_sectors(bs, bs->total_sectors); if (ret < 0) { return ret; diff --git a/block/io.c b/block/io.c index db438c765757..c49917c74677 100644 --- a/block/io.c +++ b/block/io.c @@ -182,6 +182,8 @@ void bdrv_refresh_limits(BlockDriverState *bs, Transaction *tran, Error **errp) drv->bdrv_aio_preadv || drv->bdrv_co_preadv_part) ? 1 : 512; + bs->bl.has_variable_length = drv->has_variable_length; + /* Take some limits from the children as a default */ have_limits = false; QLIST_FOREACH(c, &bs->children, next) { @@ -190,6 +192,10 @@ void bdrv_refresh_limits(BlockDriverState *bs, Transaction *tran, Error **errp) bdrv_merge_limits(&bs->bl, &c->bs->bl); have_limits = true; } + + if (c->role & BDRV_CHILD_FILTERED) { + bs->bl.has_variable_length |= c->bs->bl.has_variable_length; + } } if (!have_limits) { diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index ce51c1f7f999..95c934589571 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -855,6 +855,14 @@ typedef struct BlockLimits { /* maximum number of iovec elements */ int max_iov; + + /* + * true if the length of the underlying file can change, and QEMU + * is expected to adjust automatically. Mostly for CD-ROM drives, + * whose length is zero when the tray is empty (they don't need + * an explicit monitor command to load the disk inside the guest). + */ + bool has_variable_length; } BlockLimits; typedef struct BdrvOpBlocker BdrvOpBlocker;
At the protocol level, has_variable_length only needs to be true in the very special case of host CD-ROM drives, so that they do not need an explicit monitor command to read the new size when a disc is loaded in the tray. However, at the format level has_variable_length has to be true for all raw blockdevs and for all filters, even though in practice the length depends on the underlying file and thus will not change except in the case of host CD-ROM drives. As a first step towards computing an accurate value of has_variable_length, add the value into the BlockLimits structure and initialize the field from the BlockDriver. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- block.c | 2 +- block/io.c | 6 ++++++ include/block/block_int-common.h | 8 ++++++++ 3 files changed, 15 insertions(+), 1 deletion(-)