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
pgpWLgWAiqnG1.pgp
Description: PGP signature
