Re: [PATCH v8 2/8] block: consolidate blocksize properties consistency checks
Am 29.05.2020 um 12:56 hat Roman Kagan geschrieben: > On Fri, May 29, 2020 at 11:53:26AM +0200, Markus Armbruster wrote: > > Roman Kagan writes: > > > > > Several block device properties related to blocksize configuration must > > > be in certain relationship WRT each other: physical block must be no > > > smaller than logical block; min_io_size, opt_io_size, and > > > discard_granularity must be a multiple of a logical block. > > > > > > To ensure these requirements are met, add corresponding consistency > > > checks to blkconf_blocksizes, adjusting its signature to communicate > > > possible error to the caller. Also remove the now redundant consistency > > > checks from the specific devices. > > > > > > Signed-off-by: Roman Kagan > > > Reviewed-by: Eric Blake > > > Reviewed-by: Paul Durrant > > > --- > > > include/hw/block/block.h | 2 +- > > > hw/block/block.c | 30 +- > > > hw/block/fdc.c | 5 - > > > hw/block/nvme.c| 5 - > > > hw/block/swim.c| 5 - > > > hw/block/virtio-blk.c | 7 +-- > > > hw/block/xen-block.c | 6 +- > > > hw/ide/qdev.c | 5 - > > > hw/scsi/scsi-disk.c| 12 +--- > > > hw/usb/dev-storage.c | 5 - > > > tests/qemu-iotests/172.out | 2 +- > > > 11 files changed, 58 insertions(+), 26 deletions(-) > > > > > > diff --git a/include/hw/block/block.h b/include/hw/block/block.h > > > index d7246f3862..784953a237 100644 > > > --- a/include/hw/block/block.h > > > +++ b/include/hw/block/block.h > > > @@ -87,7 +87,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, > > > void *buf, hwaddr size, > > > bool blkconf_geometry(BlockConf *conf, int *trans, > > >unsigned cyls_max, unsigned heads_max, unsigned > > > secs_max, > > >Error **errp); > > > -void blkconf_blocksizes(BlockConf *conf); > > > +bool blkconf_blocksizes(BlockConf *conf, Error **errp); > > > bool blkconf_apply_backend_options(BlockConf *conf, bool readonly, > > > bool resizable, Error **errp); > > > > > > diff --git a/hw/block/block.c b/hw/block/block.c > > > index bf56c7612b..b22207c921 100644 > > > --- a/hw/block/block.c > > > +++ b/hw/block/block.c > > > @@ -61,7 +61,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, > > > void *buf, hwaddr size, > > > return true; > > > } > > > > > > -void blkconf_blocksizes(BlockConf *conf) > > > +bool blkconf_blocksizes(BlockConf *conf, Error **errp) > > > { > > > BlockBackend *blk = conf->blk; > > > BlockSizes blocksizes; > > > @@ -83,6 +83,34 @@ void blkconf_blocksizes(BlockConf *conf) > > > conf->logical_block_size = BDRV_SECTOR_SIZE; > > > } > > > } > > > + > > > +if (conf->logical_block_size > conf->physical_block_size) { > > > +error_setg(errp, > > > + "logical_block_size > physical_block_size not > > > supported"); > > > +return false; > > > +} > > > > Pardon me if this has been answered already for prior revisions: do we > > really support physical block sizes that are not a multiple of the > > logical block size? > > Both physical and logical block sizes are required to be powers of two, > so the former is certain to be a multiple of the latter. > > > > + > > > +if (!QEMU_IS_ALIGNED(conf->min_io_size, conf->logical_block_size)) { > > > +error_setg(errp, > > > + "min_io_size must be a multiple of > > > logical_block_size"); > > > +return false; > > > +} > > > + > > > +if (!QEMU_IS_ALIGNED(conf->opt_io_size, conf->logical_block_size)) { > > > +error_setg(errp, > > > + "opt_io_size must be a multiple of > > > logical_block_size"); > > > +return false; > > > +} > > > + > > > +if (conf->discard_granularity != -1 && > > > +!QEMU_IS_ALIGNED(conf->discard_granularity, > > > + conf->logical_block_size)) { > > > +error_setg(errp, "discard_granularity must be " > > > + "a multiple of logical_block_size"); > > > +return false; > > > +} > > > + > > > +return true; > > > } > > > > > > bool blkconf_apply_backend_options(BlockConf *conf, bool readonly, > > > diff --git a/hw/block/fdc.c b/hw/block/fdc.c > > > index c5fb9d6ece..8eda572ef4 100644 > > > --- a/hw/block/fdc.c > > > +++ b/hw/block/fdc.c > > > @@ -554,7 +554,10 @@ static void floppy_drive_realize(DeviceState *qdev, > > > Error **errp) > > > read_only = !blk_bs(dev->conf.blk) || > > > blk_is_read_only(dev->conf.blk); > > > } > > > > > > -blkconf_blocksizes(>conf); > > > +if (!blkconf_blocksizes(>conf, errp)) { > > > +return; > > > +} > > > + > > > if (dev->conf.logical_block_size != 512 || > > > dev->conf.physical_block_size != 512) > > > { > > > diff --git
Re: [PATCH v8 2/8] block: consolidate blocksize properties consistency checks
On Fri, May 29, 2020 at 11:53:26AM +0200, Markus Armbruster wrote: > Roman Kagan writes: > > > Several block device properties related to blocksize configuration must > > be in certain relationship WRT each other: physical block must be no > > smaller than logical block; min_io_size, opt_io_size, and > > discard_granularity must be a multiple of a logical block. > > > > To ensure these requirements are met, add corresponding consistency > > checks to blkconf_blocksizes, adjusting its signature to communicate > > possible error to the caller. Also remove the now redundant consistency > > checks from the specific devices. > > > > Signed-off-by: Roman Kagan > > Reviewed-by: Eric Blake > > Reviewed-by: Paul Durrant > > --- > > include/hw/block/block.h | 2 +- > > hw/block/block.c | 30 +- > > hw/block/fdc.c | 5 - > > hw/block/nvme.c| 5 - > > hw/block/swim.c| 5 - > > hw/block/virtio-blk.c | 7 +-- > > hw/block/xen-block.c | 6 +- > > hw/ide/qdev.c | 5 - > > hw/scsi/scsi-disk.c| 12 +--- > > hw/usb/dev-storage.c | 5 - > > tests/qemu-iotests/172.out | 2 +- > > 11 files changed, 58 insertions(+), 26 deletions(-) > > > > diff --git a/include/hw/block/block.h b/include/hw/block/block.h > > index d7246f3862..784953a237 100644 > > --- a/include/hw/block/block.h > > +++ b/include/hw/block/block.h > > @@ -87,7 +87,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void > > *buf, hwaddr size, > > bool blkconf_geometry(BlockConf *conf, int *trans, > >unsigned cyls_max, unsigned heads_max, unsigned > > secs_max, > >Error **errp); > > -void blkconf_blocksizes(BlockConf *conf); > > +bool blkconf_blocksizes(BlockConf *conf, Error **errp); > > bool blkconf_apply_backend_options(BlockConf *conf, bool readonly, > > bool resizable, Error **errp); > > > > diff --git a/hw/block/block.c b/hw/block/block.c > > index bf56c7612b..b22207c921 100644 > > --- a/hw/block/block.c > > +++ b/hw/block/block.c > > @@ -61,7 +61,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void > > *buf, hwaddr size, > > return true; > > } > > > > -void blkconf_blocksizes(BlockConf *conf) > > +bool blkconf_blocksizes(BlockConf *conf, Error **errp) > > { > > BlockBackend *blk = conf->blk; > > BlockSizes blocksizes; > > @@ -83,6 +83,34 @@ void blkconf_blocksizes(BlockConf *conf) > > conf->logical_block_size = BDRV_SECTOR_SIZE; > > } > > } > > + > > +if (conf->logical_block_size > conf->physical_block_size) { > > +error_setg(errp, > > + "logical_block_size > physical_block_size not > > supported"); > > +return false; > > +} > > Pardon me if this has been answered already for prior revisions: do we > really support physical block sizes that are not a multiple of the > logical block size? Both physical and logical block sizes are required to be powers of two, so the former is certain to be a multiple of the latter. > > + > > +if (!QEMU_IS_ALIGNED(conf->min_io_size, conf->logical_block_size)) { > > +error_setg(errp, > > + "min_io_size must be a multiple of logical_block_size"); > > +return false; > > +} > > + > > +if (!QEMU_IS_ALIGNED(conf->opt_io_size, conf->logical_block_size)) { > > +error_setg(errp, > > + "opt_io_size must be a multiple of logical_block_size"); > > +return false; > > +} > > + > > +if (conf->discard_granularity != -1 && > > +!QEMU_IS_ALIGNED(conf->discard_granularity, > > + conf->logical_block_size)) { > > +error_setg(errp, "discard_granularity must be " > > + "a multiple of logical_block_size"); > > +return false; > > +} > > + > > +return true; > > } > > > > bool blkconf_apply_backend_options(BlockConf *conf, bool readonly, > > diff --git a/hw/block/fdc.c b/hw/block/fdc.c > > index c5fb9d6ece..8eda572ef4 100644 > > --- a/hw/block/fdc.c > > +++ b/hw/block/fdc.c > > @@ -554,7 +554,10 @@ static void floppy_drive_realize(DeviceState *qdev, > > Error **errp) > > read_only = !blk_bs(dev->conf.blk) || > > blk_is_read_only(dev->conf.blk); > > } > > > > -blkconf_blocksizes(>conf); > > +if (!blkconf_blocksizes(>conf, errp)) { > > +return; > > +} > > + > > if (dev->conf.logical_block_size != 512 || > > dev->conf.physical_block_size != 512) > > { > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > > index 2f3100e56c..672650e162 100644 > > --- a/hw/block/nvme.c > > +++ b/hw/block/nvme.c > > @@ -1390,7 +1390,10 @@ static void nvme_realize(PCIDevice *pci_dev, Error > > **errp) > > host_memory_backend_set_mapped(n->pmrdev, true); > > } > > > > -
Re: [PATCH v8 2/8] block: consolidate blocksize properties consistency checks
Roman Kagan writes: > Several block device properties related to blocksize configuration must > be in certain relationship WRT each other: physical block must be no > smaller than logical block; min_io_size, opt_io_size, and > discard_granularity must be a multiple of a logical block. > > To ensure these requirements are met, add corresponding consistency > checks to blkconf_blocksizes, adjusting its signature to communicate > possible error to the caller. Also remove the now redundant consistency > checks from the specific devices. > > Signed-off-by: Roman Kagan > Reviewed-by: Eric Blake > Reviewed-by: Paul Durrant > --- > include/hw/block/block.h | 2 +- > hw/block/block.c | 30 +- > hw/block/fdc.c | 5 - > hw/block/nvme.c| 5 - > hw/block/swim.c| 5 - > hw/block/virtio-blk.c | 7 +-- > hw/block/xen-block.c | 6 +- > hw/ide/qdev.c | 5 - > hw/scsi/scsi-disk.c| 12 +--- > hw/usb/dev-storage.c | 5 - > tests/qemu-iotests/172.out | 2 +- > 11 files changed, 58 insertions(+), 26 deletions(-) > > diff --git a/include/hw/block/block.h b/include/hw/block/block.h > index d7246f3862..784953a237 100644 > --- a/include/hw/block/block.h > +++ b/include/hw/block/block.h > @@ -87,7 +87,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void > *buf, hwaddr size, > bool blkconf_geometry(BlockConf *conf, int *trans, >unsigned cyls_max, unsigned heads_max, unsigned > secs_max, >Error **errp); > -void blkconf_blocksizes(BlockConf *conf); > +bool blkconf_blocksizes(BlockConf *conf, Error **errp); > bool blkconf_apply_backend_options(BlockConf *conf, bool readonly, > bool resizable, Error **errp); > > diff --git a/hw/block/block.c b/hw/block/block.c > index bf56c7612b..b22207c921 100644 > --- a/hw/block/block.c > +++ b/hw/block/block.c > @@ -61,7 +61,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void > *buf, hwaddr size, > return true; > } > > -void blkconf_blocksizes(BlockConf *conf) > +bool blkconf_blocksizes(BlockConf *conf, Error **errp) > { > BlockBackend *blk = conf->blk; > BlockSizes blocksizes; > @@ -83,6 +83,34 @@ void blkconf_blocksizes(BlockConf *conf) > conf->logical_block_size = BDRV_SECTOR_SIZE; > } > } > + > +if (conf->logical_block_size > conf->physical_block_size) { > +error_setg(errp, > + "logical_block_size > physical_block_size not supported"); > +return false; > +} Pardon me if this has been answered already for prior revisions: do we really support physical block sizes that are not a multiple of the logical block size? > + > +if (!QEMU_IS_ALIGNED(conf->min_io_size, conf->logical_block_size)) { > +error_setg(errp, > + "min_io_size must be a multiple of logical_block_size"); > +return false; > +} > + > +if (!QEMU_IS_ALIGNED(conf->opt_io_size, conf->logical_block_size)) { > +error_setg(errp, > + "opt_io_size must be a multiple of logical_block_size"); > +return false; > +} > + > +if (conf->discard_granularity != -1 && > +!QEMU_IS_ALIGNED(conf->discard_granularity, > + conf->logical_block_size)) { > +error_setg(errp, "discard_granularity must be " > + "a multiple of logical_block_size"); > +return false; > +} > + > +return true; > } > > bool blkconf_apply_backend_options(BlockConf *conf, bool readonly, > diff --git a/hw/block/fdc.c b/hw/block/fdc.c > index c5fb9d6ece..8eda572ef4 100644 > --- a/hw/block/fdc.c > +++ b/hw/block/fdc.c > @@ -554,7 +554,10 @@ static void floppy_drive_realize(DeviceState *qdev, > Error **errp) > read_only = !blk_bs(dev->conf.blk) || > blk_is_read_only(dev->conf.blk); > } > > -blkconf_blocksizes(>conf); > +if (!blkconf_blocksizes(>conf, errp)) { > +return; > +} > + > if (dev->conf.logical_block_size != 512 || > dev->conf.physical_block_size != 512) > { > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > index 2f3100e56c..672650e162 100644 > --- a/hw/block/nvme.c > +++ b/hw/block/nvme.c > @@ -1390,7 +1390,10 @@ static void nvme_realize(PCIDevice *pci_dev, Error > **errp) > host_memory_backend_set_mapped(n->pmrdev, true); > } > > -blkconf_blocksizes(>conf); > +if (!blkconf_blocksizes(>conf, errp)) { > +return; > +} > + > if (!blkconf_apply_backend_options(>conf, > blk_is_read_only(n->conf.blk), > false, errp)) { > return; > diff --git a/hw/block/swim.c b/hw/block/swim.c > index 8f124782f4..74f56e8f46 100644 > --- a/hw/block/swim.c > +++ b/hw/block/swim.c > @@ -189,7 +189,10 @@ static void