| Message ID | 1465939839-30097-17-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 06/14 15:30, Eric Blake wrote: > The raw block driver was blindly copying all limits from bs->file, > even though: 1. the main bdrv_refresh_limits() already does this > for many of gthe limits, and 2. blindly copying from the children s/gthe/the ? > can weaken any stricter limits that were already inherited from > the backing dhain during the main bdrv_refresh_limits(). Also, > the next patch is about to move .request_alignment into > BlockLimits, and that is a limit that should NOT be copied from > other layers in the BDS chain. > > Solve the issue by factoring out a new bdrv_merge_limits(), > and using that function to properly merge limits when comparing > two BlockDriverState objects. > > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Fam Zheng <famz@redhat.com>
On 06/15/2016 11:50 PM, Fam Zheng wrote: > On Tue, 06/14 15:30, Eric Blake wrote: >> The raw block driver was blindly copying all limits from bs->file, >> even though: 1. the main bdrv_refresh_limits() already does this >> for many of gthe limits, and 2. blindly copying from the children > > s/gthe/the ? Yep. [Sometimes it's interesting to stick in a typo, just to see who notices. Other times it's just me fat-fingering things] > >> can weaken any stricter limits that were already inherited from >> the backing dhain during the main bdrv_refresh_limits(). Also, >> the next patch is about to move .request_alignment into >> BlockLimits, and that is a limit that should NOT be copied from >> other layers in the BDS chain. >> >> Solve the issue by factoring out a new bdrv_merge_limits(), >> and using that function to properly merge limits when comparing >> two BlockDriverState objects. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> > > Reviewed-by: Fam Zheng <famz@redhat.com> >
Am 14.06.2016 um 23:30 hat Eric Blake geschrieben: > The raw block driver was blindly copying all limits from bs->file, > even though: 1. the main bdrv_refresh_limits() already does this > for many of gthe limits, and 2. blindly copying from the children > can weaken any stricter limits that were already inherited from > the backing dhain during the main bdrv_refresh_limits(). Also, > the next patch is about to move .request_alignment into > BlockLimits, and that is a limit that should NOT be copied from > other layers in the BDS chain. > > Solve the issue by factoring out a new bdrv_merge_limits(), > and using that function to properly merge limits when comparing > two BlockDriverState objects. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v2: new patch > --- > include/block/block.h | 1 + > include/block/block_int.h | 4 ++-- > include/qemu/typedefs.h | 1 + > block/io.c | 31 +++++++++++++------------------ > block/raw_bsd.c | 4 ++-- > 5 files changed, 19 insertions(+), 22 deletions(-) > > diff --git a/include/block/block.h b/include/block/block.h > index 733a8ec..c1d4648 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -262,6 +262,7 @@ int64_t bdrv_nb_sectors(BlockDriverState *bs); > int64_t bdrv_getlength(BlockDriverState *bs); > int64_t bdrv_get_allocated_file_size(BlockDriverState *bs); > void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr); > +void bdrv_merge_limits(BlockLimits *dst, const BlockLimits *src); > void bdrv_refresh_limits(BlockDriverState *bs, Error **errp); > int bdrv_commit(BlockDriverState *bs); > int bdrv_change_backing_file(BlockDriverState *bs, Why does this need to be an external block layer interface? (block.h instead of block_int.h) Or in fact... > diff --git a/block/raw_bsd.c b/block/raw_bsd.c > index b1d5237..379ce8d 100644 > --- a/block/raw_bsd.c > +++ b/block/raw_bsd.c > @@ -152,7 +152,7 @@ static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) > > static void raw_refresh_limits(BlockDriverState *bs, Error **errp) > { > - bs->bl = bs->file->bs->bl; > + bdrv_merge_limits(&bs->bl, &bs->file->bs->bl); > } ...isn't this completely redundant because bdrv_refresh_limits() already called bdrv_merge_limits(&bs->bl, &bs->file->bs->bl)? We could simply remove the .bdrv_refresh_limits implementation from raw_bsd. And then bdrv_merge_limits() could even be static (I think factoring it out is a good idea anyway because it removes some duplication). Kevin
On 06/21/2016 08:12 AM, Kevin Wolf wrote: > Am 14.06.2016 um 23:30 hat Eric Blake geschrieben: >> The raw block driver was blindly copying all limits from bs->file, >> even though: 1. the main bdrv_refresh_limits() already does this >> for many of gthe limits, and 2. blindly copying from the children >> can weaken any stricter limits that were already inherited from >> the backing dhain during the main bdrv_refresh_limits(). Also, >> the next patch is about to move .request_alignment into >> BlockLimits, and that is a limit that should NOT be copied from >> other layers in the BDS chain. >> >> Solve the issue by factoring out a new bdrv_merge_limits(), >> and using that function to properly merge limits when comparing >> two BlockDriverState objects. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> > > Or in fact... > >> diff --git a/block/raw_bsd.c b/block/raw_bsd.c >> index b1d5237..379ce8d 100644 >> --- a/block/raw_bsd.c >> +++ b/block/raw_bsd.c >> @@ -152,7 +152,7 @@ static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) >> >> static void raw_refresh_limits(BlockDriverState *bs, Error **errp) >> { >> - bs->bl = bs->file->bs->bl; >> + bdrv_merge_limits(&bs->bl, &bs->file->bs->bl); >> } The blind copy was not completely redundant, but the argument that we don't want a blind copy but instead want to rely on a merge_limits does indeed mean that... > > ...isn't this completely redundant because bdrv_refresh_limits() already > called bdrv_merge_limits(&bs->bl, &bs->file->bs->bl)? We could simply > remove the .bdrv_refresh_limits implementation from raw_bsd. ...this sounds like a better approach. I'll try it for v3. > > And then bdrv_merge_limits() could even be static (I think factoring it > out is a good idea anyway because it removes some duplication). Yes, even if static, it still gets called twice, and makes for a nicer way to quickly see which direction limits are constrained in (MIN_NON_ZERO vs. MAX), as well as to make any further tweaks in later patches easier to do from one spot (for example, we may want to add logic that clamps an opt limit [which uses MAX logic] to never exceed a max limit [which uses MIN_NON_ZERO logic]).
diff --git a/include/block/block.h b/include/block/block.h index 733a8ec..c1d4648 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -262,6 +262,7 @@ int64_t bdrv_nb_sectors(BlockDriverState *bs); int64_t bdrv_getlength(BlockDriverState *bs); int64_t bdrv_get_allocated_file_size(BlockDriverState *bs); void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr); +void bdrv_merge_limits(BlockLimits *dst, const BlockLimits *src); void bdrv_refresh_limits(BlockDriverState *bs, Error **errp); int bdrv_commit(BlockDriverState *bs); int bdrv_change_backing_file(BlockDriverState *bs, diff --git a/include/block/block_int.h b/include/block/block_int.h index 0169019..88ef826 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -323,7 +323,7 @@ struct BlockDriver { QLIST_ENTRY(BlockDriver) list; }; -typedef struct BlockLimits { +struct BlockLimits { /* maximum number of bytes that can be discarded at once (since it * is signed, it must be < 2G, if set), should be multiple of * pdiscard_alignment, but need not be power of 2. May be 0 if no @@ -364,7 +364,7 @@ typedef struct BlockLimits { /* maximum number of iovec elements */ int max_iov; -} BlockLimits; +}; typedef struct BdrvOpBlocker BdrvOpBlocker; diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h index b113fcf..e6f72c2 100644 --- a/include/qemu/typedefs.h +++ b/include/qemu/typedefs.h @@ -14,6 +14,7 @@ typedef struct BdrvDirtyBitmap BdrvDirtyBitmap; typedef struct BlockBackend BlockBackend; typedef struct BlockBackendRootState BlockBackendRootState; typedef struct BlockDriverState BlockDriverState; +typedef struct BlockLimits BlockLimits; typedef struct BusClass BusClass; typedef struct BusState BusState; typedef struct CharDriverState CharDriverState; diff --git a/block/io.c b/block/io.c index 0b5c40d..c6c1f7b 100644 --- a/block/io.c +++ b/block/io.c @@ -67,6 +67,17 @@ static void bdrv_parent_drained_end(BlockDriverState *bs) } } +void bdrv_merge_limits(BlockLimits *dst, const BlockLimits *src) +{ + dst->opt_transfer = MAX(dst->opt_transfer, src->opt_transfer); + dst->max_transfer = MIN_NON_ZERO(dst->max_transfer, src->max_transfer); + dst->opt_mem_alignment = MAX(dst->opt_mem_alignment, + src->opt_mem_alignment); + dst->min_mem_alignment = MAX(dst->min_mem_alignment, + src->min_mem_alignment); + dst->max_iov = MIN_NON_ZERO(dst->max_iov, src->max_iov); +} + void bdrv_refresh_limits(BlockDriverState *bs, Error **errp) { BlockDriver *drv = bs->drv; @@ -88,11 +99,7 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp) error_propagate(errp, local_err); return; } - bs->bl.opt_transfer = bs->file->bs->bl.opt_transfer; - bs->bl.max_transfer = bs->file->bs->bl.max_transfer; - bs->bl.min_mem_alignment = bs->file->bs->bl.min_mem_alignment; - bs->bl.opt_mem_alignment = bs->file->bs->bl.opt_mem_alignment; - bs->bl.max_iov = bs->file->bs->bl.max_iov; + bdrv_merge_limits(&bs->bl, &bs->file->bs->bl); } else { bs->bl.min_mem_alignment = 512; bs->bl.opt_mem_alignment = getpagesize(); @@ -107,19 +114,7 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp) error_propagate(errp, local_err); return; } - bs->bl.opt_transfer = MAX(bs->bl.opt_transfer, - bs->backing->bs->bl.opt_transfer); - bs->bl.max_transfer = MIN_NON_ZERO(bs->bl.max_transfer, - bs->backing->bs->bl.max_transfer); - bs->bl.opt_mem_alignment = - MAX(bs->bl.opt_mem_alignment, - bs->backing->bs->bl.opt_mem_alignment); - bs->bl.min_mem_alignment = - MAX(bs->bl.min_mem_alignment, - bs->backing->bs->bl.min_mem_alignment); - bs->bl.max_iov = - MIN(bs->bl.max_iov, - bs->backing->bs->bl.max_iov); + bdrv_merge_limits(&bs->bl, &bs->backing->bs->bl); } /* Then let the driver override it */ diff --git a/block/raw_bsd.c b/block/raw_bsd.c index b1d5237..379ce8d 100644 --- a/block/raw_bsd.c +++ b/block/raw_bsd.c @@ -1,6 +1,6 @@ /* BlockDriver implementation for "raw" * - * Copyright (C) 2010, 2013, Red Hat, Inc. + * Copyright (C) 2010-2016 Red Hat, Inc. * Copyright (C) 2010, Blue Swirl <blauwirbel@gmail.com> * Copyright (C) 2009, Anthony Liguori <aliguori@us.ibm.com> * @@ -152,7 +152,7 @@ static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) static void raw_refresh_limits(BlockDriverState *bs, Error **errp) { - bs->bl = bs->file->bs->bl; + bdrv_merge_limits(&bs->bl, &bs->file->bs->bl); } static int raw_truncate(BlockDriverState *bs, int64_t offset)
The raw block driver was blindly copying all limits from bs->file, even though: 1. the main bdrv_refresh_limits() already does this for many of gthe limits, and 2. blindly copying from the children can weaken any stricter limits that were already inherited from the backing dhain during the main bdrv_refresh_limits(). Also, the next patch is about to move .request_alignment into BlockLimits, and that is a limit that should NOT be copied from other layers in the BDS chain. Solve the issue by factoring out a new bdrv_merge_limits(), and using that function to properly merge limits when comparing two BlockDriverState objects. Signed-off-by: Eric Blake <eblake@redhat.com> --- v2: new patch --- include/block/block.h | 1 + include/block/block_int.h | 4 ++-- include/qemu/typedefs.h | 1 + block/io.c | 31 +++++++++++++------------------ block/raw_bsd.c | 4 ++-- 5 files changed, 19 insertions(+), 22 deletions(-)