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
signature.asc
Description: OpenPGP digital signature