| Message ID | 20200420133214.28921-5-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Fix resize (extending) of short overlays | expand |
On Mon 20 Apr 2020 03:32:09 PM CEST, Kevin Wolf wrote: > If BDRV_REQ_ZERO_WRITE is set and we're extending the image, calling > qcow2_cluster_zeroize() with flags=0 does the right thing: It doesn't > undo any previous preallocation, but just adds the zero flag to all > relevant L2 entries. If an external data file is in use, a write_zeroes > request to the data file is made instead. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
20.04.2020 16:32, Kevin Wolf wrote: > If BDRV_REQ_ZERO_WRITE is set and we're extending the image, calling > qcow2_cluster_zeroize() with flags=0 does the right thing: It doesn't > undo any previous preallocation, but just adds the zero flag to all > relevant L2 entries. If an external data file is in use, a write_zeroes > request to the data file is made instead. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/qcow2.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/block/qcow2.c b/block/qcow2.c > index 6c6d6101ce..7a70c1c090 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -1726,6 +1726,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, > > bs->supported_zero_flags = header.version >= 3 ? > BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK : 0; > + bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE; > > /* Repair image if dirty */ > if (!(flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) && !bs->read_only && > @@ -4213,6 +4214,14 @@ 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) { > + ret = qcow2_cluster_zeroize(bs, old_length, offset - old_length, 0); Hmm. As I understand, qcow2_cluster_zeroize is unprepared to cluster-unaligned offset/size. I think we should handle it somehow. > + if (ret < 0) { > + error_setg_errno(errp, -ret, "Failed to zero out the new area"); > + goto fail; > + } > + } > + > if (prealloc != PREALLOC_MODE_OFF) { > /* Flush metadata before actually changing the image size */ > ret = qcow2_write_caches(bs); >
On Tue 21 Apr 2020 10:47:17 AM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: >> + if ((flags & BDRV_REQ_ZERO_WRITE) && offset > old_length) { >> + ret = qcow2_cluster_zeroize(bs, old_length, offset - old_length, 0); > > Hmm. As I understand, qcow2_cluster_zeroize is unprepared to > cluster-unaligned offset/size. I think we should handle it somehow. You're right, it actually hits an assertion :-/ I suppose you can simply round the size up to the next cluster. Berto
21.04.2020 13:50, Alberto Garcia wrote: > On Tue 21 Apr 2020 10:47:17 AM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: >>> + if ((flags & BDRV_REQ_ZERO_WRITE) && offset > old_length) { >>> + ret = qcow2_cluster_zeroize(bs, old_length, offset - old_length, 0); >> >> Hmm. As I understand, qcow2_cluster_zeroize is unprepared to >> cluster-unaligned offset/size. I think we should handle it somehow. > > You're right, it actually hits an assertion :-/ > > I suppose you can simply round the size up to the next cluster. > But we also need to handle unaligned old_length, we can't just align it down, we should write zero to existing cluster if it is NORMAL cluster..
diff --git a/block/qcow2.c b/block/qcow2.c index 6c6d6101ce..7a70c1c090 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1726,6 +1726,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, bs->supported_zero_flags = header.version >= 3 ? BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK : 0; + bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE; /* Repair image if dirty */ if (!(flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) && !bs->read_only && @@ -4213,6 +4214,14 @@ 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) { + ret = qcow2_cluster_zeroize(bs, old_length, offset - old_length, 0); + if (ret < 0) { + error_setg_errno(errp, -ret, "Failed to zero out the new area"); + goto fail; + } + } + if (prealloc != PREALLOC_MODE_OFF) { /* Flush metadata before actually changing the image size */ ret = qcow2_write_caches(bs);
If BDRV_REQ_ZERO_WRITE is set and we're extending the image, calling qcow2_cluster_zeroize() with flags=0 does the right thing: It doesn't undo any previous preallocation, but just adds the zero flag to all relevant L2 entries. If an external data file is in use, a write_zeroes request to the data file is made instead. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2.c | 9 +++++++++ 1 file changed, 9 insertions(+)