[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2 08/17] blkdebug: Set request_alignment during
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] [PATCH v2 08/17] blkdebug: Set request_alignment during .bdrv_refresh_limits() |
Date: |
Tue, 21 Jun 2016 15:27:19 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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>
>
> ---
> 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
- Re: [Qemu-block] [PATCH v2 17/17] block: Move request_alignment into BlockLimit, (continued)
Re: [Qemu-block] [PATCH v2 00/17] Byte-based block limits, Kevin Wolf, 2016/06/21