Re: [PATCH v5 4/9] qcow2: Support BDRV_REQ_ZERO_WRITE for truncate

[Kevin Wolf]

Am 22.04.2020 um 18:14 hat Eric Blake geschrieben:
> On 4/22/20 10:58 AM, Kevin Wolf wrote:
> 
> > > > @@ -4214,6 +4215,35 @@ static int coroutine_fn 
> > > > qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
> > > >            g_assert_not_reached();
> > > >        }
> > > > +    if ((flags & BDRV_REQ_ZERO_WRITE) && offset > old_length) {
> > > > +        uint64_t zero_start = QEMU_ALIGN_UP(old_length, 
> > > > s->cluster_size);
> > > > +        uint64_t zero_end = QEMU_ALIGN_UP(offset, s->cluster_size);
> > > 
> > > This rounds up beyond the new size...
> > > 
> > > > +
> > > > +        /* Use zero clusters as much as we can */
> > > > +        ret = qcow2_cluster_zeroize(bs, zero_start, zero_end - 
> > > > zero_start, 0);
> > > 
> > > and then requests that the extra be zeroed.  Does that always work, even
> > > when it results in pdrv_co_pwrite_zeroes beyond the end of s->data_file?
> > 
> > You mean the data_file_is_raw() path in qcow2_cluster_zeroize()? It's
> > currently not a code path that is run because we only set
> > BDRV_REQ_ZERO_WRITE for truncate if the image has a backing file, and
> > data_file_is_raw() doesn't work with backing files.
> 
> Good point.
> 
> > 
> > But hypothetically, if someone called truncate with BDRV_REQ_ZERO_WRITE
> > for such a file, I think it would fail.
> > 
> > > If so,
> > > 
> > > Reviewed-by: Eric Blake <address@hidden>
> > > 
> > > otherwise, you may have to treat the tail specially, the same way you
> > > treated an unaligned head.
> > 
> > Actually, do I even need to round the tail?
> > 
> >      /* Caller must pass aligned values, except at image end */
> >      assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
> >      assert(QEMU_IS_ALIGNED(end_offset, s->cluster_size) ||
> >             end_offset == bs->total_sectors << BDRV_SECTOR_BITS);
> > 
> > So qcow2_cluster_zeroize() seems to accept the unaligned tail. It would
> > still set the zero flag for the partial last cluster and for the
> > external data file, bdrv_co_pwrite_zeroes() would have the correct size.
> 
> Then I'm in favor of NOT rounding the tail.  That's an easy enough change
> and we've now justified that it does what we want, so R-b stands with that
> one-line tweak.

Would have been too easy... bs->total_sectors isn't updated yet, so the
assertion does fail.

I can make the assertion check end_offset >= ... instead. That should
still check what we wanted to check here and allow the unaligned
extension.

This feels like the better option to me compared to updating
bs->total_sectors earlier and then undoing that change in every error
path.

Kevin