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

[Kevin Wolf <kwolf@redhat.com>]

Am 14.06.2013 um 16:16 hat Paolo Bonzini geschrieben:
> Il 14/06/2013 04:31, Kevin Wolf ha scritto:
> >>> > > +    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.
> > 
> >>> > > +    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.
> 
> 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...

> 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