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