| Message ID | 4e73b26693677b6067545590708adcba66da1c06.1536226505.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Misc reopen-related patches | expand |
On 06.09.18 11:37, Alberto Garcia wrote: > 'discard' is one of the basic BlockdevOptions available for all > drivers, but it's not handled by bdrv_reopen_prepare() so any attempt > to change it results in an error: > > (qemu) qemu-io virtio0 "reopen -o discard=on" > Cannot change the option 'discard' > > Since there's no reason why we shouldn't allow changing it and the > implementation is simple let's just do it. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) Reviewed-by: Max Reitz <mreitz@redhat.com>
On Thu 06 Sep 2018 11:37:08 AM CEST, Alberto Garcia <berto@igalia.com> wrote: > 'discard' is one of the basic BlockdevOptions available for all > drivers, but it's not handled by bdrv_reopen_prepare() so any attempt > to change it results in an error: > > (qemu) qemu-io virtio0 "reopen -o discard=on" > Cannot change the option 'discard' > > Since there's no reason why we shouldn't allow changing it and the > implementation is simple let's just do it. > > Signed-off-by: Alberto Garcia <berto@igalia.com> A side effect of this change that I hadn't noticed when I sent this patch: protocol nodes have the "discard" option set to "unmap" by default (by bdrv_inherited_options()), and that sets the BDRV_O_UNMAP flag. However that flag is cleared during reopen even though the "discard" option remains there. So thanks to this patch the flag correctly reflects the value of the option after reopen. Is it worth sending the patch again with an updated commit message that explains this? Berto
On 19.09.18 11:18, Alberto Garcia wrote: > On Thu 06 Sep 2018 11:37:08 AM CEST, Alberto Garcia <berto@igalia.com> wrote: >> 'discard' is one of the basic BlockdevOptions available for all >> drivers, but it's not handled by bdrv_reopen_prepare() so any attempt >> to change it results in an error: >> >> (qemu) qemu-io virtio0 "reopen -o discard=on" >> Cannot change the option 'discard' >> >> Since there's no reason why we shouldn't allow changing it and the >> implementation is simple let's just do it. >> >> Signed-off-by: Alberto Garcia <berto@igalia.com> > > A side effect of this change that I hadn't noticed when I sent this > patch: protocol nodes have the "discard" option set to "unmap" by > default (by bdrv_inherited_options()), and that sets the BDRV_O_UNMAP > flag. > > However that flag is cleared during reopen even though the "discard" > option remains there. So thanks to this patch the flag correctly > reflects the value of the option after reopen. > > Is it worth sending the patch again with an updated commit message that > explains this? I don't know this any better than you. :-) I have to admit that personally I don't care too much about overly exact commit messages, but I'm probably just wrong. Max
On Mon 24 Sep 2018 03:43:10 PM CEST, Max Reitz wrote: > On 19.09.18 11:18, Alberto Garcia wrote: >> On Thu 06 Sep 2018 11:37:08 AM CEST, Alberto Garcia <berto@igalia.com> wrote: >>> 'discard' is one of the basic BlockdevOptions available for all >>> drivers, but it's not handled by bdrv_reopen_prepare() so any attempt >>> to change it results in an error: >>> >>> (qemu) qemu-io virtio0 "reopen -o discard=on" >>> Cannot change the option 'discard' >>> >>> Since there's no reason why we shouldn't allow changing it and the >>> implementation is simple let's just do it. >>> >>> Signed-off-by: Alberto Garcia <berto@igalia.com> >> >> A side effect of this change that I hadn't noticed when I sent this >> patch: protocol nodes have the "discard" option set to "unmap" by >> default (by bdrv_inherited_options()), and that sets the BDRV_O_UNMAP >> flag. >> >> However that flag is cleared during reopen even though the "discard" >> option remains there. So thanks to this patch the flag correctly >> reflects the value of the option after reopen. >> >> Is it worth sending the patch again with an updated commit message that >> explains this? > > I don't know this any better than you. :-) > I have to admit that personally I don't care too much about overly exact > commit messages, but I'm probably just wrong. I decided not to resend the series after all, at least not just for this change. Berto
diff --git a/block.c b/block.c index 1c0f72454a..bd8467bed8 100644 --- a/block.c +++ b/block.c @@ -3155,6 +3155,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, BlockDriver *drv; QemuOpts *opts; QDict *orig_reopen_opts; + char *discard = NULL; bool read_only; assert(reopen_state != NULL); @@ -3177,6 +3178,15 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, update_flags_from_options(&reopen_state->flags, opts); + discard = qemu_opt_get_del(opts, "discard"); + if (discard != NULL) { + if (bdrv_parse_discard_flags(discard, &reopen_state->flags) != 0) { + error_setg(errp, "Invalid discard option"); + ret = -EINVAL; + goto error; + } + } + /* All other options (including node-name and driver) must be unchanged. * Put them back into the QDict, so that they are checked at the end * of this function. */ @@ -3290,6 +3300,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, error: qemu_opts_del(opts); qobject_unref(orig_reopen_opts); + g_free(discard); return ret; }
'discard' is one of the basic BlockdevOptions available for all drivers, but it's not handled by bdrv_reopen_prepare() so any attempt to change it results in an error: (qemu) qemu-io virtio0 "reopen -o discard=on" Cannot change the option 'discard' Since there's no reason why we shouldn't allow changing it and the implementation is simple let's just do it. Signed-off-by: Alberto Garcia <berto@igalia.com> --- block.c | 11 +++++++++++ 1 file changed, 11 insertions(+)