block/blkio: Set BlockDriver::has_variable_length to false

[PATCH for-7.2] block/blkio: Set BlockDriver::has_variable_length to false

Posted by Alberto Faria 1 year, 5 months ago

Setting it to true can cause the device size to be queried from libblkio
in otherwise fast paths, degrading performance. Set it to false and
require users to refresh the device size explicitly instead.

Fixes: 4c8f4fda0504 ("block/blkio: Tolerate device size changes")
Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Alberto Faria <afaria@redhat.com>
---
 block/blkio.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/block/blkio.c b/block/blkio.c
index 620fab28a7..5eae3adfaf 100644
--- a/block/blkio.c
+++ b/block/blkio.c
@@ -993,7 +993,6 @@ static void blkio_refresh_limits(BlockDriverState *bs, Error **errp)
     { \
         .format_name             = name, \
         .protocol_name           = name, \
-        .has_variable_length     = true, \
         .instance_size           = sizeof(BDRVBlkioState), \
         .bdrv_file_open          = blkio_file_open, \
         .bdrv_close              = blkio_close, \
-- 
2.38.1

Re: [PATCH for-7.2] block/blkio: Set BlockDriver::has_variable_length to false

Posted by Stefan Hajnoczi 1 year, 5 months ago

On Tue, 8 Nov 2022 at 09:45, Alberto Faria <afaria@redhat.com> wrote:
>
> Setting it to true can cause the device size to be queried from libblkio
> in otherwise fast paths, degrading performance. Set it to false and
> require users to refresh the device size explicitly instead.
>
> Fixes: 4c8f4fda0504 ("block/blkio: Tolerate device size changes")
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Alberto Faria <afaria@redhat.com>

If it's okay to set it to false then that makes me wonder why this
undocumented field exists at all. Can you or Kevin explain and
possibly follow up with a patch that documents the field?

Thanks,
Stefan

Re: [PATCH for-7.2] block/blkio: Set BlockDriver::has_variable_length to false

Posted by Kevin Wolf 1 year, 5 months ago

Am 10.11.2022 um 22:01 hat Stefan Hajnoczi geschrieben:
> On Tue, 8 Nov 2022 at 09:45, Alberto Faria <afaria@redhat.com> wrote:
> >
> > Setting it to true can cause the device size to be queried from libblkio
> > in otherwise fast paths, degrading performance. Set it to false and
> > require users to refresh the device size explicitly instead.
> >
> > Fixes: 4c8f4fda0504 ("block/blkio: Tolerate device size changes")
> > Suggested-by: Kevin Wolf <kwolf@redhat.com>
> > Signed-off-by: Alberto Faria <afaria@redhat.com>
> 
> If it's okay to set it to false then that makes me wonder why this
> undocumented field exists at all. Can you or Kevin explain and
> possibly follow up with a patch that documents the field?

It is for removable media backends (only host_cdrom has survived until
today; there used to be host_floppy, too) where the disk size can change
without an explicit block_resize when you change the medium.

I'm not sure how well this works in practice, but so far we've not
intentionally broken anything related to it that wasn't always broken.

Kevin

Re: [PATCH for-7.2] block/blkio: Set BlockDriver::has_variable_length to false

Posted by Kevin Wolf 1 year, 5 months ago

Am 08.11.2022 um 15:44 hat Alberto Faria geschrieben:
> Setting it to true can cause the device size to be queried from libblkio
> in otherwise fast paths, degrading performance. Set it to false and
> require users to refresh the device size explicitly instead.
> 
> Fixes: 4c8f4fda0504 ("block/blkio: Tolerate device size changes")
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Alberto Faria <afaria@redhat.com>

Thanks, applied to the block branch.

Kevin