[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 08/17] blkdebug: Set request_alignment during
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2 08/17] blkdebug: Set request_alignment during .bdrv_refresh_limits() |
Date: |
Tue, 21 Jun 2016 16:17:11 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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).
>
>> + 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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v2 09/17] iscsi: Set request_alignment during .bdrv_refresh_limits(), (continued)
Re: [Qemu-devel] [PATCH v2 00/17] Byte-based block limits, Kevin Wolf, 2016/06/21