Skip to contentSkip to conversation

qcow2: Discard freed clusters

PATCH Superseded Kevin Wolf 4 participants 18 messages Jun 13, 2013 – Jun 18, 2013 Block layer

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Kevin Wolf

author
#0 lore raw

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
author
#1 lore raw

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
index 9937311..4cb592f 100644
@@ -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
index 387b050..f487a8f 100644
@@ -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
author
#2 lore raw

PATCH 2/5 qcow2: Add refcount update reason to all callers

In reply to Kevin Wolf

This 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
index 76f30e5..26de1c1 100644
@@ -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
index 992a5c8..0caac90 100644
@@ -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
index 0fa5cb2..e28ea47 100644
@@ -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
index 6959c6a..64a6479 100644
@@ -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
author
#3 lore raw

PATCH 3/5 qcow2: Options to enable discard for freed clusters

In reply to Kevin Wolf

Deleted 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
index 6d35e49..7488988 100644
@@ -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
index e28ea47..62e6753 100644
@@ -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
index 64a6479..6f91b9a 100644
@@ -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
#4 lore raw

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

author
#5 lore raw

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

#6 lore raw

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_OTHER default to BDRV_O_UNMAP as 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

author
#7 lore raw

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_OTHER default to BDRV_O_UNMAP as 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.

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_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.

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

#8 lore raw

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_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.

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_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.

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

#9 lore raw

Re: [Qemu-devel] [PATCH 3/5] qcow2: Options to enable discard for freed clusters

In reply to Kevin Wolf

block/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

author
#10 lore raw

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

author
#11 lore raw

PATCH 4/5 qcow2: Batch discards

In reply to Kevin Wolf

This 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
index 26de1c1..e86bddc 100644
@@ -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
index 62e6753..de93d81 100644
@@ -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
index 6f91b9a..3b2d5cd 100644
@@ -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
#12 lore raw

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 ;).

author
#13 lore raw

PATCH 5/5 block: Always enable discard on the protocol level

In reply to Kevin Wolf

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.

---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
block.c +1 -1
index 79ad33d..0a7cf2f 100644
@@ -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
#14 lore raw

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.

---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
block.c +0 -0
index 79ad33d..0a7cf2f 100644

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

author
#15 lore raw

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.

---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
block.c +0 -0
index 79ad33d..0a7cf2f 100644
@@ -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 -1
     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?

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

#16 lore raw
In reply to Kevin Wolf

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 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 · 3 lines
Best Regards

Wenchao Xia
author
#17 lore raw

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