[Qemu-devel] [PATCH v3 08/10] block: Allow changing 'discard' on reopen

Alberto Garcia posted 10 patches 7 years, 2 months ago
[Qemu-devel] [PATCH v3 08/10] block: Allow changing 'discard' on reopen
Posted by Alberto Garcia 7 years, 2 months ago
'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(+)

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;
 }
 
-- 
2.11.0


Re: [Qemu-devel] [PATCH v3 08/10] block: Allow changing 'discard' on reopen
Posted by Alberto Garcia 7 years, 1 month ago
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

Re: [Qemu-devel] [PATCH v3 08/10] block: Allow changing 'discard' on reopen
Posted by Max Reitz 7 years, 1 month ago
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

Re: [Qemu-devel] [PATCH v3 08/10] block: Allow changing 'discard' on reopen
Posted by Alberto Garcia 7 years, 1 month ago
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

Re: [Qemu-devel] [PATCH v3 08/10] block: Allow changing 'discard' on reopen
Posted by Max Reitz 7 years, 1 month ago
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>