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

[Paolo Bonzini]

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.

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