| Message ID | 1465939839-30097-9-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 06/14 15:30, Eric Blake wrote: > We want to eventually stick request_alignment alongside other > BlockLimits, but first, we must ensure it is populated at the > same time as all other limits, rather than being a special case > that is set only when a block is first opened. > > qemu-iotests 77 is particularly sensitive to the fact that we > can specify an artificial alignment override in blkdebug, and > that override must continue to work even when limits are > refreshed on an already open device. > > A later patch will be altering when bs->request_alignment > defaults are set, so fall back to sector alignment if we have > not inherited anything yet. > > 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: > We want to eventually stick request_alignment alongside other > BlockLimits, but first, we must ensure it is populated at the > same time as all other limits, rather than being a special case > that is set only when a block is first opened. > > qemu-iotests 77 is particularly sensitive to the fact that we > can specify an artificial alignment override in blkdebug, and > that override must continue to work even when limits are > refreshed on an already open device. > > A later patch will be altering when bs->request_alignment > defaults are set, so fall back to sector alignment if we have > not inherited anything yet. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v2: new patch > --- > block/blkdebug.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/block/blkdebug.c b/block/blkdebug.c > index 20d25bd..1589fa7 100644 > --- a/block/blkdebug.c > +++ b/block/blkdebug.c > @@ -37,6 +37,7 @@ > typedef struct BDRVBlkdebugState { > int state; > int new_state; > + int align; > > QLIST_HEAD(, BlkdebugRule) rules[BLKDBG__MAX]; > QSIMPLEQ_HEAD(, BlkdebugRule) active_rules; > @@ -382,9 +383,10 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags, > } > > /* Set request alignment */ > - align = qemu_opt_get_size(opts, "align", bs->request_alignment); > - if (align > 0 && align < INT_MAX && !(align & (align - 1))) { > - bs->request_alignment = align; > + align = qemu_opt_get_size(opts, "align", > + bs->request_alignment ?: BDRV_SECTOR_SIZE); In the state as of this patch: How would bs->request_alignment ever be zero? It is always initialised as 512 (because blkdebug doesn't implement byte-based interfaces). Later in this series, you move the initialisation of the field to bdrv_refresh_limits(). However, the check still doesn't make sense because now it's always 0 and you always use the BDRV_SECTOR_SIZE fallback. I think you should either just unconditionally use BDRV_SECTOR_SIZE or even better just store 0 in s->align if the option isn't given. Your implementation of blkdebug_refresh_limits() already leaves the default bs->request_alignment alone if s->align == 0. > + if (align > 0 && align < INT_MAX && is_power_of_2(align)) { > + s->align = align; > } else { > error_setg(errp, "Invalid alignment"); > ret = -EINVAL; Kevin
On 06/21/2016 07:27 AM, Kevin Wolf wrote: > Am 14.06.2016 um 23:30 hat Eric Blake geschrieben: >> We want to eventually stick request_alignment alongside other >> BlockLimits, but first, we must ensure it is populated at the >> same time as all other limits, rather than being a special case >> that is set only when a block is first opened. >> >> qemu-iotests 77 is particularly sensitive to the fact that we >> can specify an artificial alignment override in blkdebug, and >> that override must continue to work even when limits are >> refreshed on an already open device. >> >> A later patch will be altering when bs->request_alignment >> defaults are set, so fall back to sector alignment if we have >> not inherited anything yet. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> /* Set request alignment */ >> - align = qemu_opt_get_size(opts, "align", bs->request_alignment); >> - if (align > 0 && align < INT_MAX && !(align & (align - 1))) { >> - bs->request_alignment = align; >> + align = qemu_opt_get_size(opts, "align", >> + bs->request_alignment ?: BDRV_SECTOR_SIZE); > > In the state as of this patch: How would bs->request_alignment ever be > zero? It is always initialised as 512 (because blkdebug doesn't > implement byte-based interfaces). Correct. > > Later in this series, you move the initialisation of the field to > bdrv_refresh_limits(). However, the check still doesn't make sense > because now it's always 0 and you always use the BDRV_SECTOR_SIZE > fallback. > Correct again. > I think you should either just unconditionally use BDRV_SECTOR_SIZE or > even better just store 0 in s->align if the option isn't given. Your > implementation of blkdebug_refresh_limits() already leaves the default > bs->request_alignment alone if s->align == 0. I like that idea; I guess that means I instead need to tweak the logic to test if "align" is present in opts (in which case it must be non-zero), or absent (in which case leaving things as 0 is a nicer approach than trying to guess which default to stick things to). > >> + if (align > 0 && align < INT_MAX && is_power_of_2(align)) { And while I'm at it, align > 0 is redundant with is_power_of_2(align); will simplify on v3.
Am 22.06.2016 um 00:17 hat Eric Blake geschrieben: > On 06/21/2016 07:27 AM, Kevin Wolf wrote: > > Am 14.06.2016 um 23:30 hat Eric Blake geschrieben: > >> We want to eventually stick request_alignment alongside other > >> BlockLimits, but first, we must ensure it is populated at the > >> same time as all other limits, rather than being a special case > >> that is set only when a block is first opened. > >> > >> qemu-iotests 77 is particularly sensitive to the fact that we > >> can specify an artificial alignment override in blkdebug, and > >> that override must continue to work even when limits are > >> refreshed on an already open device. > >> > >> A later patch will be altering when bs->request_alignment > >> defaults are set, so fall back to sector alignment if we have > >> not inherited anything yet. > >> > >> Signed-off-by: Eric Blake <eblake@redhat.com> > > >> /* Set request alignment */ > >> - align = qemu_opt_get_size(opts, "align", bs->request_alignment); > >> - if (align > 0 && align < INT_MAX && !(align & (align - 1))) { > >> - bs->request_alignment = align; > >> + align = qemu_opt_get_size(opts, "align", > >> + bs->request_alignment ?: BDRV_SECTOR_SIZE); > > > > In the state as of this patch: How would bs->request_alignment ever be > > zero? It is always initialised as 512 (because blkdebug doesn't > > implement byte-based interfaces). > > Correct. > > > > > Later in this series, you move the initialisation of the field to > > bdrv_refresh_limits(). However, the check still doesn't make sense > > because now it's always 0 and you always use the BDRV_SECTOR_SIZE > > fallback. > > > > Correct again. > > > I think you should either just unconditionally use BDRV_SECTOR_SIZE or > > even better just store 0 in s->align if the option isn't given. Your > > implementation of blkdebug_refresh_limits() already leaves the default > > bs->request_alignment alone if s->align == 0. > > I like that idea; I guess that means I instead need to tweak the logic > to test if "align" is present in opts (in which case it must be > non-zero), or absent (in which case leaving things as 0 is a nicer > approach than trying to guess which default to stick things to). Except that you don't have to check all of that explicitly, the default value for absent options is what the third parameter is for: align = qemu_opt_get_size(opts, "align", 0); Kevin
On 06/22/2016 04:05 AM, Kevin Wolf wrote: >>>> /* Set request alignment */ >>>> - align = qemu_opt_get_size(opts, "align", bs->request_alignment); >>>> - if (align > 0 && align < INT_MAX && !(align & (align - 1))) { >>>> - bs->request_alignment = align; >>>> + align = qemu_opt_get_size(opts, "align", >>>> + bs->request_alignment ?: BDRV_SECTOR_SIZE); >>> >>> I think you should either just unconditionally use BDRV_SECTOR_SIZE or >>> even better just store 0 in s->align if the option isn't given. Your >>> implementation of blkdebug_refresh_limits() already leaves the default >>> bs->request_alignment alone if s->align == 0. >> >> I like that idea; I guess that means I instead need to tweak the logic >> to test if "align" is present in opts (in which case it must be >> non-zero), or absent (in which case leaving things as 0 is a nicer >> approach than trying to guess which default to stick things to). > > Except that you don't have to check all of that explicitly, the default > value for absent options is what the third parameter is for: > > align = qemu_opt_get_size(opts, "align", 0); Previously, we explicitly reject the user passing in an explicit 'align':0. If I go with this approach of 0 as the default, then I may need to tweak docs. But that sounds better, so I'll try it.
diff --git a/block/blkdebug.c b/block/blkdebug.c index 20d25bd..1589fa7 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -37,6 +37,7 @@ typedef struct BDRVBlkdebugState { int state; int new_state; + int align; QLIST_HEAD(, BlkdebugRule) rules[BLKDBG__MAX]; QSIMPLEQ_HEAD(, BlkdebugRule) active_rules; @@ -382,9 +383,10 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags, } /* Set request alignment */ - align = qemu_opt_get_size(opts, "align", bs->request_alignment); - if (align > 0 && align < INT_MAX && !(align & (align - 1))) { - bs->request_alignment = align; + align = qemu_opt_get_size(opts, "align", + bs->request_alignment ?: BDRV_SECTOR_SIZE); + if (align > 0 && align < INT_MAX && is_power_of_2(align)) { + s->align = align; } else { error_setg(errp, "Invalid alignment"); ret = -EINVAL; @@ -720,6 +722,15 @@ static void blkdebug_refresh_filename(BlockDriverState *bs, QDict *options) bs->full_open_options = opts; } +static void blkdebug_refresh_limits(BlockDriverState *bs, Error **errp) +{ + BDRVBlkdebugState *s = bs->opaque; + + if (s->align) { + bs->request_alignment = s->align; + } +} + static int blkdebug_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, Error **errp) { @@ -738,6 +749,7 @@ static BlockDriver bdrv_blkdebug = { .bdrv_getlength = blkdebug_getlength, .bdrv_truncate = blkdebug_truncate, .bdrv_refresh_filename = blkdebug_refresh_filename, + .bdrv_refresh_limits = blkdebug_refresh_limits, .bdrv_aio_readv = blkdebug_aio_readv, .bdrv_aio_writev = blkdebug_aio_writev,
We want to eventually stick request_alignment alongside other BlockLimits, but first, we must ensure it is populated at the same time as all other limits, rather than being a special case that is set only when a block is first opened. qemu-iotests 77 is particularly sensitive to the fact that we can specify an artificial alignment override in blkdebug, and that override must continue to work even when limits are refreshed on an already open device. A later patch will be altering when bs->request_alignment defaults are set, so fall back to sector alignment if we have not inherited anything yet. Signed-off-by: Eric Blake <eblake@redhat.com> --- v2: new patch --- block/blkdebug.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)