Re: [Qemu-devel] [PATCH v3 3/9] block: Let write zeroes fallback work even with small max_transfer

[Eric Blake]

On 11/22/2016 07:16 AM, Kevin Wolf wrote:
> Am 17.11.2016 um 21:13 hat Eric Blake geschrieben:
>> Commit 443668ca rewrote the write_zeroes logic to guarantee that
>> an unaligned request never crosses a cluster boundary.  But
>> in the rewrite, the new code assumed that at most one iteration
>> would be needed to get to an alignment boundary.
>>

>> @@ -1257,8 +1262,6 @@ static int coroutine_fn 
>> bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>>
>>          if (ret == -ENOTSUP) {
>>              /* Fall back to bounce buffer if write zeroes is unsupported */
>> -            int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer,
>> -                                            MAX_WRITE_ZEROES_BOUNCE_BUFFER);
>>              BdrvRequestFlags write_flags = flags & ~BDRV_REQ_ZERO_WRITE;
> 
> Why do we even still bother with max_transfer in this function when we
> could just call bdrv_aligned_pwritev() and use its fragmentation code?

Hmm. bdrv_aligned_pwritev() asserts that its arguments are already
aligned, but for the head and tail, they might not be.  I agree that for
the bulk of the body, it may help, but it would take more thought on
refactoring if we want to have fragmentation at only one spot.

> 
> Of course, when bdrv_co_do_pwrite_zeroes() was written, your
> fragmentation code didn't exist yet, but today I think it would make
> more sense to use a single centralised version of it instead of
> reimplementing it here.
> 
> This doesn't make your fix less correct, but if we did things this way,
> the fix wouldn't even be needed because a single iteration (in this
> loop) would indeed always be enough.

Can I request to defer such refactoring to 2.9, while getting this patch
as-is into 2.8?

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

signature.asc
Description: OpenPGP digital signature