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 <ebl...@redhat.com> > > > > > > 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