Re: [Qemu-block] [PATCH v2 08/17] blkdebug: Set request_alignment during .bdrv_refresh_limits()

[Kevin Wolf]

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 <address@hidden>
> 
> >>      /* 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

pgpnJtGFDfXtR.pgp
Description: PGP signature