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

[Kevin Wolf]

Am 22.11.2016 um 14:22 hat Eric Blake geschrieben:
> 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.

Right, it should be more like bdrv_co_pwritev() then, but something that
uses the logic in bdrv_aligned_pwritev().

Using bdrv_co_pwritev() would mean that it's tracked as another request,
but I don't think that's a problem. Otherwise we'd have to factor that
part out.

> > 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?

Yes, the refactoring is definitely for 2.9.

Kevin

pgpQ57GIV5yXG.pgp
Description: PGP signature