Re: [Qemu-devel] [PATCH 5/5] block: Move request_alignment into BlockLimit

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2867 bytes --]

On 06/14/2016 02:05 AM, Kevin Wolf wrote:

>>>>  static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>>>>  {
>>>> +    /* Inherit all limits except for request_alignment */
>>>> +    int request_alignment = bs->bl.request_alignment;
>>>> +
>>>>      bs->bl = bs->file->bs->bl;
>>>> +    bs->bl.request_alignment = request_alignment;
>>
>> Any ideas on how to fix the test, then?  Have two blkdebug devices
>> nested atop one another, since those are the devices where we can
>> explicitly override alignment?
> 
> Interesting idea. Maybe that's a good option if it works.
> 
>> (normally, you'd _expect_ the chain to
>> inherit the worst-case alignment of all BDS in the chain, so blkdebug is
>> the way around it).
> 
> Actually, I think there are two cases.
> 
> The first one is that you get a request and want to know what to do with
> it. Here you don't want to inherit the worst-case alignment. You'd
> rather want to enforce alignment only when it is actually needed. For
> example, if you have a cache=none backing file with 4k alignment, but a
> cache=writeback overlay, and you don't actually access the backing file
> with this request, it would be wasteful to align the request. You also
> don't really know that a driver will issue a misaligned request (or any
> request at all) to the lower layer just because it got called with one.
> 
> The second case is when you want to issue a request. For example, let's
> take the qcow2 cache here, which has 64k cached and needs to update a
> few bytes. Currently it always writes the full 64k (and with 1 MB
> clusters a full megabyte), but what it really should do is consider the
> worst-case alignment.
> 
> This is comparable to the difference between bdrv_opt_mem_align(), which
> is used in qemu_blockalign() where we want to create a buffer that works
> even in the worst case, and bdrv_min_mem_align(), which is used in
> bdrv_qiov_is_aligned() in order to determine whether we must align an
> existing request.
> 
>> That's the only thing left before I repost the series, so I may just
>> post the last patch as RFC and play with it a bit more...
> 
> And in the light of the above, maybe the solution is as easy as changing
> raw_refresh_limits() to set bs->bl.request_alignment = 1.

Or maybe split the main bdrv_refresh_limits() into two pieces: one that
merges limits from one BDS into another (the limits that are worth
merging, and in the correct direction: opt merges to MAX, max merges to
MIN_NON_ZERO, request_alignment is not merged), the other that calls
merge(bs, bs->file->bs); then have raw_refresh_limits() also use the
common merge functionality rather than straight copy.  Testing that
approach now...

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]