| Message ID | 20230607152627.468786-2-andrey.drobyshev@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-img: map: implement support for compressed clusters | expand |
On 6/7/23 17:26, Andrey Drobyshev wrote: > Functions qcow2_get_host_offset(), get_cluster_offset() explicitly > report compressed cluster types when data is compressed. However, this > information is never passed further. Let's make use of it by adding new > BDRV_BLOCK_COMPRESSED flag for bdrv_block_status(), so that caller may > know that the data range is compressed. In particular, we're going to > use this flag to tweak "qemu-img map" output. > > This new flag is only being utilized by qcow and qcow2 formats, as only > these two support compression. > > Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> > --- > block/qcow.c | 5 ++++- > block/qcow2.c | 3 +++ > include/block/block-common.h | 3 +++ > 3 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/block/qcow.c b/block/qcow.c > index 3644bbf5cb..8416bcc2c3 100644 > --- a/block/qcow.c > +++ b/block/qcow.c > @@ -549,7 +549,10 @@ qcow_co_block_status(BlockDriverState *bs, bool want_zero, > if (!cluster_offset) { > return 0; > } > - if ((cluster_offset & QCOW_OFLAG_COMPRESSED) || s->crypto) { > + if (cluster_offset & QCOW_OFLAG_COMPRESSED) { > + return BDRV_BLOCK_DATA | BDRV_BLOCK_COMPRESSED; > + } > + if (s->crypto) { > return BDRV_BLOCK_DATA; > } > *map = cluster_offset | index_in_cluster; > diff --git a/block/qcow2.c b/block/qcow2.c > index e23edd48c2..8e01adc610 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -2162,6 +2162,9 @@ qcow2_co_block_status(BlockDriverState *bs, bool want_zero, int64_t offset, > { > status |= BDRV_BLOCK_RECURSE; > } > + if (type == QCOW2_SUBCLUSTER_COMPRESSED) { > + status |= BDRV_BLOCK_COMPRESSED; > + } > return status; > } > > diff --git a/include/block/block-common.h b/include/block/block-common.h > index e15395f2cb..f7a4e7d4db 100644 > --- a/include/block/block-common.h > +++ b/include/block/block-common.h > @@ -282,6 +282,8 @@ typedef enum { > * layer rather than any backing, set by block layer > * BDRV_BLOCK_EOF: the returned pnum covers through end of file for this > * layer, set by block layer > + * BDRV_BLOCK_COMPRESSED: the underlying data is compressed; only valid for > + * the formats supporting compression: qcow, qcow2 > * > * Internal flags: > * BDRV_BLOCK_RAW: for use by passthrough drivers, such as raw, to request > @@ -317,6 +319,7 @@ typedef enum { > #define BDRV_BLOCK_ALLOCATED 0x10 > #define BDRV_BLOCK_EOF 0x20 > #define BDRV_BLOCK_RECURSE 0x40 > +#define BDRV_BLOCK_COMPRESSED 0x80 > > typedef QTAILQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue; > Reviewed-by: Denis V. Lunev <den@openvz.org>
On 6/21/23 19:08, Denis V. Lunev wrote: > On 6/7/23 17:26, Andrey Drobyshev wrote: >> Functions qcow2_get_host_offset(), get_cluster_offset() explicitly >> report compressed cluster types when data is compressed. However, this >> information is never passed further. Let's make use of it by adding new >> BDRV_BLOCK_COMPRESSED flag for bdrv_block_status(), so that caller may >> know that the data range is compressed. In particular, we're going to >> use this flag to tweak "qemu-img map" output. >> >> This new flag is only being utilized by qcow and qcow2 formats, as only >> these two support compression. >> >> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> >> --- >> block/qcow.c | 5 ++++- >> block/qcow2.c | 3 +++ >> include/block/block-common.h | 3 +++ >> 3 files changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/block/qcow.c b/block/qcow.c >> index 3644bbf5cb..8416bcc2c3 100644 >> --- a/block/qcow.c >> +++ b/block/qcow.c >> @@ -549,7 +549,10 @@ qcow_co_block_status(BlockDriverState *bs, bool >> want_zero, >> if (!cluster_offset) { >> return 0; >> } >> - if ((cluster_offset & QCOW_OFLAG_COMPRESSED) || s->crypto) { >> + if (cluster_offset & QCOW_OFLAG_COMPRESSED) { >> + return BDRV_BLOCK_DATA | BDRV_BLOCK_COMPRESSED; >> + } >> + if (s->crypto) { >> return BDRV_BLOCK_DATA; >> } >> *map = cluster_offset | index_in_cluster; >> diff --git a/block/qcow2.c b/block/qcow2.c >> index e23edd48c2..8e01adc610 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -2162,6 +2162,9 @@ qcow2_co_block_status(BlockDriverState *bs, >> bool want_zero, int64_t offset, >> { >> status |= BDRV_BLOCK_RECURSE; >> } >> + if (type == QCOW2_SUBCLUSTER_COMPRESSED) { >> + status |= BDRV_BLOCK_COMPRESSED; >> + } >> return status; >> } >> diff --git a/include/block/block-common.h >> b/include/block/block-common.h >> index e15395f2cb..f7a4e7d4db 100644 >> --- a/include/block/block-common.h >> +++ b/include/block/block-common.h >> @@ -282,6 +282,8 @@ typedef enum { >> * layer rather than any backing, set by >> block layer >> * BDRV_BLOCK_EOF: the returned pnum covers through end of file for >> this >> * layer, set by block layer >> + * BDRV_BLOCK_COMPRESSED: the underlying data is compressed; only >> valid for >> + * the formats supporting compression: qcow, >> qcow2 >> * >> * Internal flags: >> * BDRV_BLOCK_RAW: for use by passthrough drivers, such as raw, to >> request >> @@ -317,6 +319,7 @@ typedef enum { >> #define BDRV_BLOCK_ALLOCATED 0x10 >> #define BDRV_BLOCK_EOF 0x20 >> #define BDRV_BLOCK_RECURSE 0x40 >> +#define BDRV_BLOCK_COMPRESSED 0x80 >> typedef QTAILQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) >> BlockReopenQueue; > Reviewed-by: Denis V. Lunev <den@openvz.org> Looking into the second patch I have found that I was a too fast here :) The comment is misleading and the patch is incomplete. static inline bool TSA_NO_TSA block_driver_can_compress(BlockDriver *drv) { return drv->bdrv_co_pwritev_compressed || drv->bdrv_co_pwritev_compressed_part; } which means that 1 257 block/copy-on-read.c <<GLOBAL>> .bdrv_co_pwritev_compressed = cor_co_pwritev_compressed, 2 1199 block/qcow.c <<GLOBAL>> .bdrv_co_pwritev_compressed = qcow_co_pwritev_compressed, 3 255 block/throttle.c <<GLOBAL>> .bdrv_co_pwritev_compressed = throttle_co_pwritev_compressed, 4 3108 block/vmdk.c <<GLOBAL>> .bdrv_co_pwritev_compressed = vmdk_co_pwritev_compressed, 1 6121 block/qcow2.c <<GLOBAL>> .bdrv_co_pwritev_compressed_part...cow2_co_pwritev_compressed_part, We have missed at least VMDK images.
On 6/21/23 20:46, Denis V. Lunev wrote: > On 6/21/23 19:08, Denis V. Lunev wrote: >> On 6/7/23 17:26, Andrey Drobyshev wrote: >>> Functions qcow2_get_host_offset(), get_cluster_offset() explicitly >>> report compressed cluster types when data is compressed. However, this >>> information is never passed further. Let's make use of it by adding new >>> BDRV_BLOCK_COMPRESSED flag for bdrv_block_status(), so that caller may >>> know that the data range is compressed. In particular, we're going to >>> use this flag to tweak "qemu-img map" output. >>> >>> This new flag is only being utilized by qcow and qcow2 formats, as only >>> these two support compression. >>> >>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> >>> --- >>> block/qcow.c | 5 ++++- >>> block/qcow2.c | 3 +++ >>> include/block/block-common.h | 3 +++ >>> 3 files changed, 10 insertions(+), 1 deletion(-) >>> >>> diff --git a/block/qcow.c b/block/qcow.c >>> index 3644bbf5cb..8416bcc2c3 100644 >>> --- a/block/qcow.c >>> +++ b/block/qcow.c >>> @@ -549,7 +549,10 @@ qcow_co_block_status(BlockDriverState *bs, bool >>> want_zero, >>> if (!cluster_offset) { >>> return 0; >>> } >>> - if ((cluster_offset & QCOW_OFLAG_COMPRESSED) || s->crypto) { >>> + if (cluster_offset & QCOW_OFLAG_COMPRESSED) { >>> + return BDRV_BLOCK_DATA | BDRV_BLOCK_COMPRESSED; >>> + } >>> + if (s->crypto) { >>> return BDRV_BLOCK_DATA; >>> } >>> *map = cluster_offset | index_in_cluster; >>> diff --git a/block/qcow2.c b/block/qcow2.c >>> index e23edd48c2..8e01adc610 100644 >>> --- a/block/qcow2.c >>> +++ b/block/qcow2.c >>> @@ -2162,6 +2162,9 @@ qcow2_co_block_status(BlockDriverState *bs, >>> bool want_zero, int64_t offset, >>> { >>> status |= BDRV_BLOCK_RECURSE; >>> } >>> + if (type == QCOW2_SUBCLUSTER_COMPRESSED) { >>> + status |= BDRV_BLOCK_COMPRESSED; >>> + } >>> return status; >>> } >>> diff --git a/include/block/block-common.h >>> b/include/block/block-common.h >>> index e15395f2cb..f7a4e7d4db 100644 >>> --- a/include/block/block-common.h >>> +++ b/include/block/block-common.h >>> @@ -282,6 +282,8 @@ typedef enum { >>> * layer rather than any backing, set by >>> block layer >>> * BDRV_BLOCK_EOF: the returned pnum covers through end of file for >>> this >>> * layer, set by block layer >>> + * BDRV_BLOCK_COMPRESSED: the underlying data is compressed; only >>> valid for >>> + * the formats supporting compression: qcow, >>> qcow2 >>> * >>> * Internal flags: >>> * BDRV_BLOCK_RAW: for use by passthrough drivers, such as raw, to >>> request >>> @@ -317,6 +319,7 @@ typedef enum { >>> #define BDRV_BLOCK_ALLOCATED 0x10 >>> #define BDRV_BLOCK_EOF 0x20 >>> #define BDRV_BLOCK_RECURSE 0x40 >>> +#define BDRV_BLOCK_COMPRESSED 0x80 >>> typedef QTAILQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) >>> BlockReopenQueue; >> Reviewed-by: Denis V. Lunev <den@openvz.org> > Looking into the second patch I have found that I was a too fast here :) > > The comment is misleading and the patch is incomplete. > > static inline bool TSA_NO_TSA block_driver_can_compress(BlockDriver *drv) > { > return drv->bdrv_co_pwritev_compressed || > drv->bdrv_co_pwritev_compressed_part; > } > > which means that > > 1 257 block/copy-on-read.c <<GLOBAL>> > .bdrv_co_pwritev_compressed = cor_co_pwritev_compressed, > 2 1199 block/qcow.c <<GLOBAL>> > .bdrv_co_pwritev_compressed = qcow_co_pwritev_compressed, > 3 255 block/throttle.c <<GLOBAL>> > .bdrv_co_pwritev_compressed = throttle_co_pwritev_compressed, > 4 3108 block/vmdk.c <<GLOBAL>> > .bdrv_co_pwritev_compressed = vmdk_co_pwritev_compressed, > 1 6121 block/qcow2.c <<GLOBAL>> > .bdrv_co_pwritev_compressed_part...cow2_co_pwritev_compressed_part, > > We have missed at least VMDK images. Thanks, my bad I didn't check that more thoroughly, I was mainly looking in the docs when making this conclusion. man qemu-img: > Only the formats qcow and qcow2 support compression. The com‐ > pression is read-only. It means that if a compressed sector is > rewritten, then it is rewritten as uncompressed data. Apparently man pages got a bit outdated on this.
diff --git a/block/qcow.c b/block/qcow.c index 3644bbf5cb..8416bcc2c3 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -549,7 +549,10 @@ qcow_co_block_status(BlockDriverState *bs, bool want_zero, if (!cluster_offset) { return 0; } - if ((cluster_offset & QCOW_OFLAG_COMPRESSED) || s->crypto) { + if (cluster_offset & QCOW_OFLAG_COMPRESSED) { + return BDRV_BLOCK_DATA | BDRV_BLOCK_COMPRESSED; + } + if (s->crypto) { return BDRV_BLOCK_DATA; } *map = cluster_offset | index_in_cluster; diff --git a/block/qcow2.c b/block/qcow2.c index e23edd48c2..8e01adc610 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2162,6 +2162,9 @@ qcow2_co_block_status(BlockDriverState *bs, bool want_zero, int64_t offset, { status |= BDRV_BLOCK_RECURSE; } + if (type == QCOW2_SUBCLUSTER_COMPRESSED) { + status |= BDRV_BLOCK_COMPRESSED; + } return status; } diff --git a/include/block/block-common.h b/include/block/block-common.h index e15395f2cb..f7a4e7d4db 100644 --- a/include/block/block-common.h +++ b/include/block/block-common.h @@ -282,6 +282,8 @@ typedef enum { * layer rather than any backing, set by block layer * BDRV_BLOCK_EOF: the returned pnum covers through end of file for this * layer, set by block layer + * BDRV_BLOCK_COMPRESSED: the underlying data is compressed; only valid for + * the formats supporting compression: qcow, qcow2 * * Internal flags: * BDRV_BLOCK_RAW: for use by passthrough drivers, such as raw, to request @@ -317,6 +319,7 @@ typedef enum { #define BDRV_BLOCK_ALLOCATED 0x10 #define BDRV_BLOCK_EOF 0x20 #define BDRV_BLOCK_RECURSE 0x40 +#define BDRV_BLOCK_COMPRESSED 0x80 typedef QTAILQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
Functions qcow2_get_host_offset(), get_cluster_offset() explicitly report compressed cluster types when data is compressed. However, this information is never passed further. Let's make use of it by adding new BDRV_BLOCK_COMPRESSED flag for bdrv_block_status(), so that caller may know that the data range is compressed. In particular, we're going to use this flag to tweak "qemu-img map" output. This new flag is only being utilized by qcow and qcow2 formats, as only these two support compression. Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> --- block/qcow.c | 5 ++++- block/qcow2.c | 3 +++ include/block/block-common.h | 3 +++ 3 files changed, 10 insertions(+), 1 deletion(-)