Patchwork [2/5] blockdev: add discard suboption to -drive

login
register
mail settings
Submitter Paolo Bonzini
Date Feb. 8, 2013, 1:06 p.m.
Message ID <1360328775-13144-3-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/219146/
State New
Headers show

Comments

Paolo Bonzini - Feb. 8, 2013, 1:06 p.m.
Add support for BDRV_O_UNMAP from the QEMU command-line.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 blockdev.c      | 11 +++++++++++
 qemu-options.hx |  2 ++
 2 files changed, 13 insertions(+)
Stefan Hajnoczi - Feb. 13, 2013, 10:39 a.m.
On Fri, Feb 08, 2013 at 02:06:12PM +0100, Paolo Bonzini wrote:
> @@ -1489,6 +1496,10 @@ QemuOptsList qemu_drive_opts = {
>              .type = QEMU_OPT_STRING,
>              .help = "disk image",
>          },{
> +            .name = "discard",
> +            .type = QEMU_OPT_STRING,
> +            .help = "discard operation (ignore/off, unmap/on)",

off/on aliases will be confusing when we plan to add "anchor" in the
future, making this non-boolean.  Please support ignore/unmap only.
Paolo Bonzini - Feb. 20, 2013, 2:09 p.m.
Il 13/02/2013 11:39, Stefan Hajnoczi ha scritto:
>> >              .type = QEMU_OPT_STRING,
>> >              .help = "disk image",
>> >          },{
>> > +            .name = "discard",
>> > +            .type = QEMU_OPT_STRING,
>> > +            .help = "discard operation (ignore/off, unmap/on)",
> off/on aliases will be confusing when we plan to add "anchor" in the
> future, making this non-boolean.  Please support ignore/unmap only.

My problem here is that "unmap" will not be the default but at the same
time it will be what people want to use.  Hence, it is important to have
an easy to remember and discover option; "discard=on" is immediately
understandable, while "discard=unmap" is not.

Paolo
Stefan Hajnoczi - Feb. 21, 2013, 8:48 a.m.
On Wed, Feb 20, 2013 at 03:09:51PM +0100, Paolo Bonzini wrote:
> Il 13/02/2013 11:39, Stefan Hajnoczi ha scritto:
> >> >              .type = QEMU_OPT_STRING,
> >> >              .help = "disk image",
> >> >          },{
> >> > +            .name = "discard",
> >> > +            .type = QEMU_OPT_STRING,
> >> > +            .help = "discard operation (ignore/off, unmap/on)",
> > off/on aliases will be confusing when we plan to add "anchor" in the
> > future, making this non-boolean.  Please support ignore/unmap only.
> 
> My problem here is that "unmap" will not be the default but at the same
> time it will be what people want to use.  Hence, it is important to have
> an easy to remember and discover option; "discard=on" is immediately
> understandable, while "discard=unmap" is not.

I understand.  The risk is that we end up confusing users rather than
helping, because the actual ignore/anchor/unmap modes need to be
understood to use this feature properly.

If you and Kevin like the off/on alias, go ahead.  It's not critical.

Stefan
Kevin Wolf - Feb. 21, 2013, 9:01 a.m.
On Thu, Feb 21, 2013 at 09:48:56AM +0100, Stefan Hajnoczi wrote:
> On Wed, Feb 20, 2013 at 03:09:51PM +0100, Paolo Bonzini wrote:
> > Il 13/02/2013 11:39, Stefan Hajnoczi ha scritto:
> > >> >              .type = QEMU_OPT_STRING,
> > >> >              .help = "disk image",
> > >> >          },{
> > >> > +            .name = "discard",
> > >> > +            .type = QEMU_OPT_STRING,
> > >> > +            .help = "discard operation (ignore/off, unmap/on)",
> > > off/on aliases will be confusing when we plan to add "anchor" in the
> > > future, making this non-boolean.  Please support ignore/unmap only.
> > 
> > My problem here is that "unmap" will not be the default but at the same
> > time it will be what people want to use.  Hence, it is important to have
> > an easy to remember and discover option; "discard=on" is immediately
> > understandable, while "discard=unmap" is not.
> 
> I understand.  The risk is that we end up confusing users rather than
> helping, because the actual ignore/anchor/unmap modes need to be
> understood to use this feature properly.
> 
> If you and Kevin like the off/on alias, go ahead.  It's not critical.

I actually think both are good points and am not sure which one is
more important. In such cases I tend to just accept whatever a proposed
patch contains.

Kevin

Patch

diff --git a/blockdev.c b/blockdev.c
index 63e6f1e..b4ae36a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -378,6 +378,13 @@  DriveInfo *drive_init(QemuOpts *opts, BlockInterfaceType block_default_type)
 	}
     }
 
+    if ((buf = qemu_opt_get(opts, "discard")) != NULL) {
+        if (bdrv_parse_discard_flags(buf, &bdrv_flags) != 0) {
+            error_report("invalid discard option");
+            return NULL;
+        }
+    }
+
     bdrv_flags |= BDRV_O_CACHE_WB;
     if ((buf = qemu_opt_get(opts, "cache")) != NULL) {
         if (bdrv_parse_cache_flags(buf, &bdrv_flags) != 0) {
@@ -1489,6 +1496,10 @@  QemuOptsList qemu_drive_opts = {
             .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, "
diff --git a/qemu-options.hx b/qemu-options.hx
index 046bdc0..d5a99c7 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -185,6 +185,8 @@  These options have the same definition as they have in @option{-hdachs}.
 @var{cache} is "none", "writeback", "unsafe", "directsync" or "writethrough" and controls how the host cache is used to access block data.
 @item aio=@var{aio}
 @var{aio} is "threads", or "native" and selects between pthread based disk I/O and native Linux AIO.
+@item discard=@var{discard}
+@var{discard} is one of "ignore" (or "off") or "unmap" (or "on") and controls whether @dfn{discard} (also known as @dfn{trim} or @dfn{unmap}) requests are ignored or passed to the filesystem.  Some machine types may not support discard requests.
 @item format=@var{format}
 Specify which disk @var{format} will be used rather than detecting
 the format.  Can be used to specifiy format=raw to avoid interpreting