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
