qcow2: Discard freed clusters
cover qcow2: Discard freed clusters
This series adds options to make qcow2 discard freed clusters, in several categories. By default, only freed clusters related to snapshots (i.e. mainly snapshot deletion) are discarded.
Kevin Wolf (5):
Revert "block: Disable driver-specific options for 1.5"
qcow2: Add refcount update reason to all callers
qcow2: Options to enable discard for freed clusters
qcow2: Batch discards
block: Always enable discard on the protocol level
block.c | 2 +- block/qcow2-cluster.c | 38 +++++++++---- block/qcow2-refcount.c | 139 ++++++++++++++++++++++++++++++++++++++++------- block/qcow2-snapshot.c | 6 +- block/qcow2.c | 30 +++++++++- block/qcow2.h | 32 ++++++++++- blockdev.c | 118 ++-------------------------------------- tests/qemu-iotests/group | 2 +- 8 files changed, 214 insertions(+), 153 deletions(-)
Signature · 1 line
1.8.1.4
PATCH 1/5 Revert "block: Disable driver-specific options for 1.5"
This reverts commit 8ec7d390b0d50b5e5b4b1d8dba7ba40d64a70875.
--- blockdev.c | 118 ++--------------------------------------------- tests/qemu-iotests/group | 2 +- 2 files changed, 5 insertions(+), 115 deletions(-)
blockdev.c +4 -114
@@ -1734,120 +1734,10 @@ QemuOptsList qemu_drive_opts = { .name = "drive", .head = QTAILQ_HEAD_INITIALIZER(qemu_drive_opts.head), .desc = { - { - .name = "bus", - .type = QEMU_OPT_NUMBER, - .help = "bus number", - },{ - .name = "unit", - .type = QEMU_OPT_NUMBER, - .help = "unit number (i.e. lun for scsi)", - },{ - .name = "if", - .type = QEMU_OPT_STRING, - .help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)", - },{ - .name = "index", - .type = QEMU_OPT_NUMBER, - .help = "index number", - },{ - .name = "cyls", - .type = QEMU_OPT_NUMBER, - .help = "number of cylinders (ide disk geometry)", - },{ - .name = "heads", - .type = QEMU_OPT_NUMBER, - .help = "number of heads (ide disk geometry)", - },{ - .name = "secs", - .type = QEMU_OPT_NUMBER, - .help = "number of sectors (ide disk geometry)", - },{ - .name = "trans", - .type = QEMU_OPT_STRING, - .help = "chs translation (auto, lba. none)", - },{ - .name = "media", - .type = QEMU_OPT_STRING, - .help = "media type (disk, cdrom)", - },{ - .name = "snapshot", - .type = QEMU_OPT_BOOL, - .help = "enable/disable snapshot mode", - },{ - .name = "file", - .type = QEMU_OPT_STRING, - .help = "disk image", - },{ - .name = "discard", - .type = QEMU_OPT_STRING, - .help = "discard operation (ignore/off, unmap/on)", - },{ - .name = "cache", - .type = QEMU_OPT_STRING, - .help = "host cache usage (none, writeback, writethrough, " - "directsync, unsafe)", - },{ - .name = "aio", - .type = QEMU_OPT_STRING, - .help = "host AIO implementation (threads, native)", - },{ - .name = "format", - .type = QEMU_OPT_STRING, - .help = "disk format (raw, qcow2, ...)", - },{ - .name = "serial", - .type = QEMU_OPT_STRING, - .help = "disk serial number", - },{ - .name = "rerror", - .type = QEMU_OPT_STRING, - .help = "read error action", - },{ - .name = "werror", - .type = QEMU_OPT_STRING, - .help = "write error action", - },{ - .name = "addr", - .type = QEMU_OPT_STRING, - .help = "pci address (virtio only)", - },{ - .name = "readonly", - .type = QEMU_OPT_BOOL, - .help = "open drive file as read-only", - },{ - .name = "iops", - .type = QEMU_OPT_NUMBER, - .help = "limit total I/O operations per second", - },{ - .name = "iops_rd", - .type = QEMU_OPT_NUMBER, - .help = "limit read operations per second", - },{ - .name = "iops_wr", - .type = QEMU_OPT_NUMBER, - .help = "limit write operations per second", - },{ - .name = "bps", - .type = QEMU_OPT_NUMBER, - .help = "limit total bytes per second", - },{ - .name = "bps_rd", - .type = QEMU_OPT_NUMBER, - .help = "limit read bytes per second", - },{ - .name = "bps_wr", - .type = QEMU_OPT_NUMBER, - .help = "limit write bytes per second", - },{ - .name = "copy-on-read", - .type = QEMU_OPT_BOOL, - .help = "copy read data from backing file into image file", - },{ - .name = "boot", - .type = QEMU_OPT_BOOL, - .help = "(deprecated, ignored)", - }, +/* +* no elements => accept any params +* validation will happen later +*/ {/* end of list */ } }, };
tests/qemu-iotests/group +1 -1
@@ -57,7 +57,7 @@ 048 img auto quick 049 rw auto 050 rw auto backing quick -#051 rw auto +051 rw auto 052 rw auto backing 053 rw auto 054 rw auto
Signature · 1 line
1.8.1.4
PATCH 2/5 qcow2: Add refcount update reason to all callers
In reply to Kevin WolfThis adds a refcount update reason to all callers of update_refcounts(), so that a follow-up patch can use this information to decide whether clusters that reach a refcount of 0 should be discarded in the image file.
--- block/qcow2-cluster.c | 16 +++++++++------ block/qcow2-refcount.c | 55 +++++++++++++++++++++++++++++++------------------- block/qcow2-snapshot.c | 6 ++++-- block/qcow2.c | 3 ++- block/qcow2.h | 16 ++++++++++++--- 5 files changed, 63 insertions(+), 33 deletions(-)
block/qcow2-cluster.c +10 -6
@@ -98,14 +98,16 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, goto fail; }g_free (s->l1_table); -qcow2_free_clusters (bs, s->l1_table_offset, s->l1_size *sizeof (uint64_t )); +qcow2_free_clusters (bs, s->l1_table_offset, s->l1_size *sizeof (uint64_t ), +QCOW2_DISCARD_OTHER ); s->l1_table_offset = new_l1_table_offset; s->l1_table = new_l1_table; s->l1_size = new_l1_size;return 0 ; fail:g_free (new_l1_table); -qcow2_free_clusters (bs, new_l1_table_offset, new_l1_size2); +qcow2_free_clusters (bs, new_l1_table_offset, new_l1_size2, +QCOW2_DISCARD_OTHER );return ret; }
@@ -548,7 +550,8 @@ static int get_cluster_table(BlockDriverState *bs, uint64_t offset,/* Then decrease the refcount of the old table */ if (l2_offset) { -qcow2_free_clusters (bs, l2_offset, s->l2_size *sizeof (uint64_t )); +qcow2_free_clusters (bs, l2_offset, s->l2_size *sizeof (uint64_t ), +QCOW2_DISCARD_OTHER ); } }
@@ -718,7 +721,8 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)*/ if (j !=0 ) {for (i =0 ; i < j; i++) { -qcow2_free_any_clusters (bs,be64_to_cpu (old_cluster[i]),1 ); +qcow2_free_any_clusters (bs,be64_to_cpu (old_cluster[i]),1 , +QCOW2_DISCARD_OTHER ); } }
@@ -1339,7 +1343,7 @@ static int discard_single_l2(BlockDriverState *bs, uint64_t offset, l2_table[l2_index + i] =cpu_to_be64 (0 );/* Then decrease the refcount */ -qcow2_free_any_clusters (bs, old_offset,1 ); +qcow2_free_any_clusters (bs, old_offset,1 ,QCOW2_DISCARD_REQUEST ); } ret =qcow2_cache_put (bs, s->l2_table_cache, (void **) &l2_table);
@@ -1415,7 +1419,7 @@ static int zero_single_l2(BlockDriverState *bs, uint64_t offset,qcow2_cache_entry_mark_dirty (s->l2_table_cache, l2_table);if (old_offset &QCOW_OFLAG_COMPRESSED ) { l2_table[l2_index + i] =cpu_to_be64 (QCOW_OFLAG_ZERO ); -qcow2_free_any_clusters (bs, old_offset,1 ); +qcow2_free_any_clusters (bs, old_offset,1 ,QCOW2_DISCARD_REQUEST ); }else { l2_table[l2_index + i] |=cpu_to_be64 (QCOW_OFLAG_ZERO ); }
block/qcow2-refcount.c +34 -21
169 lines not on this page · raw message
block/qcow2-snapshot.c +4 -2
@@ -262,7 +262,8 @@ static int qcow2_write_snapshots(BlockDriverState *bs) }/* free the old snapshot table */ -qcow2_free_clusters (bs, s->snapshots_offset, s->snapshots_size); +qcow2_free_clusters (bs, s->snapshots_offset, s->snapshots_size, +QCOW2_DISCARD_SNAPSHOT ); s->snapshots_offset = snapshots_offset; s->snapshots_size = snapshots_size;return 0 ;
@@ -569,7 +570,8 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)if (ret <0 ) {return ret; } -qcow2_free_clusters (bs, sn.l1_table_offset, sn.l1_size *sizeof (uint64_t )); +qcow2_free_clusters (bs, sn.l1_table_offset, sn.l1_size *sizeof (uint64_t ), +QCOW2_DISCARD_SNAPSHOT );/* must update the copied flag on the current cluster offsets */ ret =qcow2_update_snapshot_refcount (bs, s->l1_table_offset, s->l1_size,0 );
block/qcow2.c +2 -1
@@ -1196,7 +1196,8 @@ static int preallocate(BlockDriverState *bs) ret =qcow2_alloc_cluster_link_l2 (bs, meta);if (ret <0 ) { -qcow2_free_any_clusters (bs, meta->alloc_offset, meta->nb_clusters); +qcow2_free_any_clusters (bs, meta->alloc_offset, meta->nb_clusters, +QCOW2_DISCARD_NEVER );return ret; }
block/qcow2.h +13 -3
@@ -129,6 +129,15 @@ enum { QCOW2_COMPAT_FEAT_MASK = QCOW2_COMPAT_LAZY_REFCOUNTS, }; +enum qcow2_discard_type { +QCOW2_DISCARD_NEVER =0 , +QCOW2_DISCARD_ALWAYS , +QCOW2_DISCARD_REQUEST , +QCOW2_DISCARD_SNAPSHOT , +QCOW2_DISCARD_OTHER , +QCOW2_DISCARD_MAX +}; + typedef struct Qcow2Feature { uint8_t type; uint8_t bit;
@@ -349,9 +358,10 @@ int qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset, int nb_clusters);int64_t qcow2_alloc_bytes (BlockDriverState *bs,int size);void qcow2_free_clusters (BlockDriverState *bs, -int64_t offset,int64_t size); -void qcow2_free_any_clusters (BlockDriverState *bs, -uint64_t cluster_offset,int nb_clusters); +int64_t offset,int64_t size, +enum qcow2_discard_type type); +void qcow2_free_any_clusters (BlockDriverState *bs,uint64_t l2_entry, +int nb_clusters,enum qcow2_discard_type type);int qcow2_update_snapshot_refcount (BlockDriverState *bs,int64_t l1_table_offset,int l1_size,int addend);
Signature · 1 line
1.8.1.4
PATCH 3/5 qcow2: Options to enable discard for freed clusters
In reply to Kevin WolfDeleted snapshots are discarded in the image file by default, discard requests take their default from the -drive discard=... option and other places that free clusters must always be enabled explicitly.
--- block/qcow2-refcount.c | 5 +++++ block/qcow2.c | 26 ++++++++++++++++++++++++++ block/qcow2.h | 5 +++++ 3 files changed, 36 insertions(+)
block/qcow2-refcount.c +5 -0
@@ -488,6 +488,11 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, s->free_cluster_index = cluster_index; } refcount_block[block_index] =cpu_to_be16 (refcount); +if (refcount ==0 && s->discard_passthrough[type]) { +/* Try discarding, ignore errors */ +/* FIXME Doing this cluster by cluster will be painfully slow */ +bdrv_discard (bs->file, cluster_offset,1 ); + } } ret =0 ;
block/qcow2.c +26 -0
@@ -295,6 +295,22 @@ static QemuOptsList qcow2_runtime_opts = { .type = QEMU_OPT_BOOL, .help = "Postpone refcount updates", }, + { + .name = QCOW2_OPT_DISCARD_REQUEST, + .type = QEMU_OPT_BOOL, + .help = "Pass guest discard requests to the layer below", + }, + { + .name = QCOW2_OPT_DISCARD_SNAPSHOT, + .type = QEMU_OPT_BOOL, + .help = "Generate discard requests when snapshot related space " + "is freed", + }, + { + .name = QCOW2_OPT_DISCARD_OTHER, + .type = QEMU_OPT_BOOL, + .help = "Generate discard requests when other clusters are freed", + }, {/* end of list */ } }, };
@@ -532,6 +548,16 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags) s->use_lazy_refcounts =qemu_opt_get_bool (opts,QCOW2_OPT_LAZY_REFCOUNTS , (s->compatible_features &QCOW2_COMPAT_LAZY_REFCOUNTS )); + s->discard_passthrough[QCOW2_DISCARD_NEVER ] = false, + s->discard_passthrough[QCOW2_DISCARD_ALWAYS ] = true, + s->discard_passthrough[QCOW2_DISCARD_REQUEST ] = +qemu_opt_get_bool (opts,QCOW2_OPT_DISCARD_REQUEST , + flags &BDRV_O_UNMAP ), + s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT ] = +qemu_opt_get_bool (opts,QCOW2_OPT_DISCARD_SNAPSHOT , true), + s->discard_passthrough[QCOW2_DISCARD_OTHER ] = +qemu_opt_get_bool (opts,QCOW2_OPT_DISCARD_OTHER , false), +qemu_opts_del (opts);if (s->use_lazy_refcounts && s->qcow_version <3 ) {
block/qcow2.h +5 -0
@@ -60,6 +60,9 @@#define QCOW2_OPT_LAZY_REFCOUNTS "lazy_refcounts" +#define QCOW2_OPT_DISCARD_REQUEST "pass_discard_request" +#define QCOW2_OPT_DISCARD_SNAPSHOT "pass_discard_snapshot" +#define QCOW2_OPT_DISCARD_OTHER "pass_discard_other" typedef struct QCowHeader { uint32_t magic;
@@ -187,6 +190,8 @@ typedef struct BDRVQcowState {int qcow_version;bool use_lazy_refcounts; +bool discard_passthrough[QCOW2_DISCARD_MAX ]; +uint64_t incompatible_features;uint64_t compatible_features;uint64_t autoclear_features;
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 3/5] qcow2: Options to enable discard for freed clusters
Il 13/06/2013 07:47, Kevin Wolf ha scritto:
block/qcow2.c static int qcow2_open(BlockDriverState *bs, QDict *options, int flags) · line 555
Quoted 5 lines
block/qcow2.c +5
-0+ s->discard_passthrough[QCOW2_DISCARD_NEVER ] = false, + s->discard_passthrough[QCOW2_DISCARD_ALWAYS ] = true, + s->discard_passthrough[QCOW2_DISCARD_REQUEST ] = +qemu_opt_get_bool (opts,QCOW2_OPT_DISCARD_REQUEST , + flags & BDRV_O_UNMAP),
I think there should not be two ways to enable it, it is confusing.
block/qcow2.c static int qcow2_open(BlockDriverState *bs, QDict *options, int flags) · line 559
Quoted 4 lines
block/qcow2.c +4
-0+ s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT ] = +qemu_opt_get_bool (opts,QCOW2_OPT_DISCARD_SNAPSHOT , true), + s->discard_passthrough[QCOW2_DISCARD_OTHER ] = + qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false),
Please document the defaults in qcow2_runtime_opts. (BTW, what is the rationale?)
Paolo
Re: [Qemu-devel] [PATCH 3/5] qcow2: Options to enable discard for freed clusters
Am 14.06.2013 um 00:10 hat Paolo Bonzini geschrieben:
Quoted 8 lines
Il 13/06/2013 07:47, Kevin Wolf ha scritto:
block/qcow2.c static int qcow2_open(BlockDriverState *bs, QDict *options, int flags) · line 555
Quoted 5 lines
block/qcow2.c +5
-0+ s->discard_passthrough[QCOW2_DISCARD_NEVER ] = false, + s->discard_passthrough[QCOW2_DISCARD_ALWAYS ] = true, + s->discard_passthrough[QCOW2_DISCARD_REQUEST ] = +qemu_opt_get_bool (opts,QCOW2_OPT_DISCARD_REQUEST , + flags & BDRV_O_UNMAP),I think there should not be two ways to enable it, it is confusing.
Hm, yes... But it's also confusing to have qcow2 provide an incomplete set of categories. Maybe we shouldn't have introduced -drive discard=... as a global option to begin with.
Quoted 7 lines
block/qcow2.c static int qcow2_open(BlockDriverState *bs, QDict *options, int flags) · line 559
Quoted 4 lines
block/qcow2.c +4
-0+ s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT ] = +qemu_opt_get_bool (opts,QCOW2_OPT_DISCARD_SNAPSHOT , true), + s->discard_passthrough[QCOW2_DISCARD_OTHER ] = + qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false),Please document the defaults in
qcow2_runtime_opts. (BTW, what is the rationale?)
The idea was that discard is slow and therefore disabled by default, except when you're doing an expensive snapshot operation that can potentially free a lot of space at once with not too many requests, so there it's enabled. And if you said -drive discard=on, you obviously want guest requests to take effect.
We could let QCOW2_OPT_DISCARD_OTHER default to BDRV_O_UNMAP as well if you prefer.
Kevin
Re: [Qemu-devel] [PATCH 3/5] qcow2: Options to enable discard for freed clusters
Il 14/06/2013 04:31, Kevin Wolf ha scritto:
Quoted 18 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
block/qcow2.c static int qcow2_open(BlockDriverState *bs, QDict *options, int flags) · line 555
Quoted 5 lines
block/qcow2.c +5
-0+ s->discard_passthrough[QCOW2_DISCARD_NEVER ] = false, + s->discard_passthrough[QCOW2_DISCARD_ALWAYS ] = true, + s->discard_passthrough[QCOW2_DISCARD_REQUEST ] = +qemu_opt_get_bool (opts,QCOW2_OPT_DISCARD_REQUEST , + flags & BDRV_O_UNMAP),I think there should not be two ways to enable it, it is confusing.
Hm, yes... But it's also confusing to have qcow2 provide an incomplete set of categories. Maybe we shouldn't have introduced -drive discard=... as a global option to begin with.
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
block/qcow2.c static int qcow2_open(BlockDriverState *bs, QDict *options, int flags) · line 559
Quoted 4 lines
block/qcow2.c +4
-0+ s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT ] = +qemu_opt_get_bool (opts,QCOW2_OPT_DISCARD_SNAPSHOT , true), + s->discard_passthrough[QCOW2_DISCARD_OTHER ] = + qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false),Please document the defaults in
qcow2_runtime_opts. (BTW, what is the rationale?)
Quoted 8 lines
The idea was that discard is slow and therefore disabled by default, except when you're doing an expensive snapshot operation that can potentially free a lot of space at once with not too many requests, so there it's enabled. And if you said -drive discard=on, you obviously want guest requests to take effect.
We could let
QCOW2_OPT_DISCARD_OTHERdefault toBDRV_O_UNMAPas well if you prefer.
It looks like QCOW2_OPT_DISCARD_OTHER is a rare case, so I don't mind leaving it as default to false. It won't waste more than a few clusters.
In the end discard_snapshot and discard_other should rarely be needed in practice, so I don't think having discard=... is a mistake. Too many knobs won't really be needed.
In fact, perhaps we do not need discard_snapshot and discard_request, only discard_other. discard_snapshot can be replaced by file.discard=ignore, discard_request by discard=unmap.
Paolo
Re: [Qemu-devel] [PATCH 3/5] qcow2: Options to enable discard for freed clusters
Am 14.06.2013 um 16:16 hat Paolo Bonzini geschrieben:
Quoted 25 lines
Il 14/06/2013 04:31, Kevin Wolf ha scritto:
Quoted 24 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
block/qcow2.c static int qcow2_open(BlockDriverState *bs, QDict *options, int flags) · line 555
Quoted 5 lines
block/qcow2.c +5
-0+ s->discard_passthrough[QCOW2_DISCARD_NEVER ] = false, + s->discard_passthrough[QCOW2_DISCARD_ALWAYS ] = true, + s->discard_passthrough[QCOW2_DISCARD_REQUEST ] = +qemu_opt_get_bool (opts,QCOW2_OPT_DISCARD_REQUEST , + flags & BDRV_O_UNMAP),I think there should not be two ways to enable it, it is confusing.
Hm, yes... But it's also confusing to have qcow2 provide an incomplete set of categories. Maybe we shouldn't have introduced -drive discard=... as a global option to begin with.
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
block/qcow2.c static int qcow2_open(BlockDriverState *bs, QDict *options, int flags) · line 559
Quoted 4 lines
block/qcow2.c +4
-0+ s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT ] = +qemu_opt_get_bool (opts,QCOW2_OPT_DISCARD_SNAPSHOT , true), + s->discard_passthrough[QCOW2_DISCARD_OTHER ] = + qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false),Please document the defaults in
qcow2_runtime_opts. (BTW, what is the rationale?)The idea was that discard is slow and therefore disabled by default, except when you're doing an expensive snapshot operation that can potentially free a lot of space at once with not too many requests, so there it's enabled. And if you said -drive discard=on, you obviously want guest requests to take effect.
Quoted 5 lines
Quoted 2 lines
We could let
QCOW2_OPT_DISCARD_OTHERdefault toBDRV_O_UNMAPas well if you prefer.It looks like
QCOW2_OPT_DISCARD_OTHERis a rare case, so I don't mind leaving it as default to false. It won't waste more than a few clusters.
Yes, it's generally relatively rare, like growing L1 or refcount table. There is one case where it should trigger a lot, though: Overwriting clusters of a compressed image.
Hm, though actually it doesn't make a lot of sense there. The freed cluster will immediately be used by the next write. Maybe COW should actually be QCOW2_OPT_DISCARD_NEVER...
Quoted 7 lines
In the end
discard_snapshotanddiscard_othershould rarely be needed in practice, so I don't think having discard=... is a mistake. Too many knobs won't really be needed.In fact, perhaps we do not need
discard_snapshotanddiscard_request, onlydiscard_other.discard_snapshotcan be replaced by file.discard=ignore,discard_requestby discard=unmap.
This is only true if you rule out some combination as useless. For example you would say that if you want to process guest requests, you always want to have snapshots discarded as well. You also assume that nobody wants the current behaviour (free clusters in qcow2 metadata, but don't send discards to raw-posix).
Isn't this assuming a bit too much?
To be clear, I don't expect these knobs to be used much either, but I have some feeling that some people (including us while debugging or asking questions) may be glad later to have such low-level options that control each layer separately.
Kevin
Re: [Qemu-devel] [PATCH 3/5] qcow2: Options to enable discard for freed clusters
Il 14/06/2013 10:31, Kevin Wolf ha scritto:
Quoted 10 lines
Quoted 2 lines
It looks like
QCOW2_OPT_DISCARD_OTHERis a rare case, so I don't mind leaving it as default to false. It won't waste more than a few clusters.Yes, it's generally relatively rare, like growing L1 or refcount table. There is one case where it should trigger a lot, though: Overwriting clusters of a compressed image.
Hm, though actually it doesn't make a lot of sense there. The freed cluster will immediately be used by the next write. Maybe COW should actually be
QCOW2_OPT_DISCARD_NEVER...
Sounds reasonable.
Quoted 14 lines
Quoted 7 lines
In the end
discard_snapshotanddiscard_othershould rarely be needed in practice, so I don't think having discard=... is a mistake. Too many knobs won't really be needed.In fact, perhaps we do not need
discard_snapshotanddiscard_request, onlydiscard_other.discard_snapshotcan be replaced by file.discard=ignore,discard_requestby discard=unmap.This is only true if you rule out some combination as useless. For example you would say that if you want to process guest requests, you always want to have snapshots discarded as well. You also assume that nobody wants the current behaviour (free clusters in qcow2 metadata, but don't send discards to raw-posix).
Quoted 6 lines
Isn't this assuming a bit too much?
To be clear, I don't expect these knobs to be used much either, but I have some feeling that some people (including us while debugging or asking questions) may be glad later to have such low-level options that control each layer separately.
Yeah, you're right. They're definitely useful to have, even if it is "just in case".
Paolo
Re: [Qemu-devel] [PATCH 3/5] qcow2: Options to enable discard for freed clusters
In reply to Kevin Wolfblock/qcow2.c static int qcow2_open(BlockDriverState *bs, QDict *options, int flags) · line 551
Quoted 5 lines · Kevin Wolf
block/qcow2.c +1
-0@@ -532,6 +548,16 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags) s->use_lazy_refcounts =qemu_opt_get_bool (opts,QCOW2_OPT_LAZY_REFCOUNTS , (s->compatible_features &QCOW2_COMPAT_LAZY_REFCOUNTS )); + s->discard_passthrough[QCOW2_DISCARD_NEVER] = false,
comma instead of semicolon?
block/qcow2.h typedef struct BDRVQcowState { · line 194
Quoted 6 lines
block/qcow2.h +2
-0@@ -187,6 +190,8 @@ typedef struct BDRVQcowState {int qcow_version;bool use_lazy_refcounts; +bool discard_passthrough[QCOW2_DISCARD_MAX ]; +
Neat solution to specifying discard behavior.
Stefan
Re: [Qemu-devel] [PATCH 3/5] qcow2: Options to enable discard for freed clusters
Am 17.06.2013 um 17:41 hat Stefan Hajnoczi geschrieben:
Quoted 8 lines · Kevin Wolf
block/qcow2.c static int qcow2_open(BlockDriverState *bs, QDict *options, int flags) · line 551
Quoted 5 lines
block/qcow2.c +1
-0@@ -532,6 +548,16 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags) s->use_lazy_refcounts =qemu_opt_get_bool (opts,QCOW2_OPT_LAZY_REFCOUNTS , (s->compatible_features &QCOW2_COMPAT_LAZY_REFCOUNTS )); + s->discard_passthrough[QCOW2_DISCARD_NEVER] = false,comma instead of semicolon?
Oh. Isn't C a nice language? :-)
I'll fix that.
Kevin
PATCH 4/5 qcow2: Batch discards
In reply to Kevin WolfThis optimises the discard operation for freed clusters by batching discard requests (both snapshot deletion and bdrv_discard end up updating the refcounts cluster by cluster).
Note that we don't discard asynchronously, but keep s->lock held. This is to avoid that a freed cluster is reallocated and written to while the discard is still in flight.
--- block/qcow2-cluster.c | 22 ++++++++++--- block/qcow2-refcount.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++-- block/qcow2.c | 1 + block/qcow2.h | 11 +++++++ 4 files changed, 112 insertions(+), 7 deletions(-)
block/qcow2-cluster.c +18 -4
@@ -1374,18 +1374,25 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset, nb_clusters =size_to_clusters (s, end_offset - offset); + s->cache_discards = true; +/* Each L2 table is handled by its own loop iteration */ while (nb_clusters >0 ) { ret =discard_single_l2 (bs, offset, nb_clusters);if (ret <0 ) { -return ret; + goto fail; } nb_clusters -= ret; offset += (ret * s->cluster_size); } -return 0 ; + ret =0 ; +fail: + s->cache_discards = false; +qcow2_process_discards (bs, ret); + +return ret; }/*
@@ -1447,15 +1454,22 @@ int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors)/* Each L2 table is handled by its own loop iteration */ nb_clusters =size_to_clusters (s, nb_sectors <<BDRV_SECTOR_BITS ); + s->cache_discards = true; +while (nb_clusters >0 ) { ret =zero_single_l2 (bs, offset, nb_clusters);if (ret <0 ) { -return ret; + goto fail; } nb_clusters -= ret; offset += (ret * s->cluster_size); } -return 0 ; + ret =0 ; +fail: + s->cache_discards = false; +qcow2_process_discards (bs, ret); + +return ret; }
block/qcow2-refcount.c +82 -3
115 lines not on this page · raw message
block/qcow2.c +1 -0
@@ -486,6 +486,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags) }QLIST_INIT (&s->cluster_allocs); +QTAILQ_INIT (&s->discards);/* read qcow2 extensions */ if (qcow2_read_extensions (bs, header.header_length, ext_end,NULL )) {
block/qcow2.h +11 -0
@@ -147,6 +147,13 @@ typedef struct Qcow2Feature {char name[46 ]; }QEMU_PACKED Qcow2Feature; +typedef struct Qcow2DiscardRegion { +BlockDriverState *bs; +uint64_t offset; +uint64_t bytes; +QTAILQ_ENTRY (Qcow2DiscardRegion) next; +}Qcow2DiscardRegion ; + typedef struct BDRVQcowState { int cluster_bits; int cluster_size;
@@ -199,6 +206,8 @@ typedef struct BDRVQcowState {size_t unknown_header_fields_size;void * unknown_header_fields; QLIST_HEAD(, Qcow2UnknownHeaderExtension) unknown_header_ext; + QTAILQ_HEAD (, Qcow2DiscardRegion) discards; +bool cache_discards; } BDRVQcowState;/* XXX: use std qcow open function ? */
@@ -374,6 +383,8 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,int qcow2_check_refcounts (BlockDriverState *bs,BdrvCheckResult *res,BdrvCheckMode fix); +void qcow2_process_discards (BlockDriverState *bs,int ret); +/* qcow2-cluster.c functions */ int qcow2_grow_l1_table (BlockDriverState *bs,uint64_t min_size,bool exact_size);
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 4/5] qcow2: Batch discards
Quoted 7 lines · Kevin Wolf
block/qcow2-refcount.c +3
-0@@ -420,6 +420,77 @@ fail_block:return ret; } +void qcow2_process_discards (BlockDriverState *bs,int ret) +{ +BDRVQcowState *s = bs->opaque;
block/qcow2-refcount.c fail_block: · line 435
Quoted 10 lines
block/qcow2-refcount.c +10
-0+Qcow2DiscardRegion *d, *next; + + QTAILQ_FOREACH_SAFE(d, &s->discards, next, next) { +QTAILQ_REMOVE (&s->discards, d, next); + +/* Discard is optional, ignore the return value */ +if (ret >=0 ) { +fprintf (stderr,"discard: %lx + %lx\n" , + d->offset >>BDRV_SECTOR_BITS , + d->bytes >>BDRV_SECTOR_BITS );
Debug code. This doesn't happen with --enable-trace-backend=stderr ;).
PATCH 5/5 block: Always enable discard on the protocol level
In reply to Kevin WolfTurning on discard options in qcow2 doesn't help a lot when the discard requests that it issues are thrown away by the raw-posix layer. This patch always enables discard functionality on the protocol level so that it's the image format's responsibility to send (or not) discard requests. Requests sent by the guest will be allowed or ignored by the top level BlockDriverState, which depends on the discard=... option like before.
In particular, this means that even without specifying options, the qcow2 default of discarding deleted snapshots actually takes effect now, both for qemu and qemu-img.
--- block.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
block.c +1 -1
@@ -1045,7 +1045,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,extract_subqdict (options, &file_options,"file." ); ret =bdrv_file_open (&file, filename, file_options, -bdrv_open_flags (bs, flags)); +bdrv_open_flags (bs, flags |BDRV_O_UNMAP ));if (ret <0 ) { goto fail; }
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 5/5] block: Always enable discard on the protocol level
Il 13/06/2013 07:47, Kevin Wolf ha scritto:
Quoted 23 lines
Turning on discard options in qcow2 doesn't help a lot when the discard requests that it issues are thrown away by the raw-posix layer. This patch always enables discard functionality on the protocol level so that it's the image format's responsibility to send (or not) discard requests. Requests sent by the guest will be allowed or ignored by the top level BlockDriverState, which depends on the discard=... option like before.
In particular, this means that even without specifying options, the qcow2 default of discarding deleted snapshots actually takes effect now, both for qemu and qemu-img.
Signed-off-by Kevin Wolf <[email protected]>--- block.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)block.c +0
-0
block.c int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options, · line 1051
Quoted 8 lines
block.c +1
-1@@ -1045,7 +1045,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,extract_subqdict (options, &file_options,"file." ); ret =bdrv_file_open (&file, filename, file_options, -bdrv_open_flags (bs, flags)); +bdrv_open_flags (bs, flags |BDRV_O_UNMAP ));if (ret <0 ) { goto fail; }
Can you still disable it with -drive file.discard=ignore?
Paolo
Re: [Qemu-devel] [PATCH 5/5] block: Always enable discard on the protocol level
Am 14.06.2013 um 00:06 hat Paolo Bonzini geschrieben:
Quoted 25 lines
Il 13/06/2013 07:47, Kevin Wolf ha scritto:
Quoted 24 lines
Turning on discard options in qcow2 doesn't help a lot when the discard requests that it issues are thrown away by the raw-posix layer. This patch always enables discard functionality on the protocol level so that it's the image format's responsibility to send (or not) discard requests. Requests sent by the guest will be allowed or ignored by the top level BlockDriverState, which depends on the discard=... option like before.
In particular, this means that even without specifying options, the qcow2 default of discarding deleted snapshots actually takes effect now, both for qemu and qemu-img.
Signed-off-by Kevin Wolf <[email protected]>--- block.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)block.c +0
-0@@ -1045,7 +1045,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,extract_subqdict (options, &file_options,"file." );
Quoted 9 lines
block.c int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options, · line 1051
Quoted 7 lines
block.c +1
-1ret =bdrv_file_open (&file, filename, file_options, -bdrv_open_flags (bs, flags)); +bdrv_open_flags (bs, flags |BDRV_O_UNMAP ));if (ret <0 ) { goto fail; }Can you still disable it with -drive file.discard=ignore?
This requires a few more changes (basically moving BDRV_O_UNMAP from flags into a boolean in the QDict), but I think eventually we'll want to allow this.
Kevin
Hi, Kevin
In normal case, qcow2 is based on raw-posix file which don't support
discard operation, so this serial doesn't change much in this case, still real space is not freed in snapshot delete, and it completes fast. Instead, tt affects the case that some protocol below support discard, so delete will be more clean but slower in that case. Is this true? Maybe add this in commit message?
Quoted 21 lines
This series adds options to make qcow2 discard freed clusters, in several categories. By default, only freed clusters related to snapshots (i.e. mainly snapshot deletion) are discarded.
Kevin Wolf (5):
Revert "block: Disable driver-specific options for 1.5"
qcow2: Add refcount update reason to all callers
qcow2: Options to enable discard for freed clusters
qcow2: Batch discards
block: Always enable discard on the protocol levelblock.c | 2 +- block/qcow2-cluster.c | 38 +++++++++---- block/qcow2-refcount.c | 139 ++++++++++++++++++++++++++++++++++++++++------- block/qcow2-snapshot.c | 6 +- block/qcow2.c | 30 +++++++++- block/qcow2.h | 32 ++++++++++- blockdev.c | 118 ++-------------------------------------- tests/qemu-iotests/group | 2 +- 8 files changed, 214 insertions(+), 153 deletions(-)
Signature · 3 lines
Best Regards Wenchao Xia
Am 18.06.2013 um 05:03 hat Wenchao Xia geschrieben:
Quoted 3 lines
Hi, Kevin
In normal case, qcow2 is based on raw-posix file which don't support
discard operation
It does, see raw_aio_discard().
Quoted 5 lines
, so this serial doesn't change much in this case,
still real space is not freed in snapshot delete, and it completes fast. Instead, tt affects the case that some protocol below support discard, so delete will be more clean but slower in that case. Is this true? Maybe add this in commit message?
Yes, it becomes a little bit slower, but not much in my tests.
Kevin