Enabling discard for the storage node allows the format drivers to
discard snapshots and other things, while configuration of the format
layer actually decides whether to actually discard data on request from
the host.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_block.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index b1f495b731..6e76571796 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1163,7 +1163,10 @@ qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src,
         if (qemuBlockStorageSourceGetBlockdevGetCacheProps(src, fileprops) < 0)
             goto cleanup;

-        if (virJSONValueObjectAdd(fileprops, "b:read-only", src->readonly, NULL) < 0)
+        if (virJSONValueObjectAdd(fileprops,
+                                  "b:read-only", src->readonly,
+                                  "s:discard", "unmap",
+                                  NULL) < 0)
             goto cleanup;
     }

-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 04/25/2018 11:15 AM, Peter Krempa wrote:
> Enabling discard for the storage node allows the format drivers to
> discard snapshots and other things, while configuration of the format
> layer actually decides whether to actually discard data on request from
> the host.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/qemu/qemu_block.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 

Isn't this related to the {disk|src}->discard value? Which we copied
from patch 3.

So you'd just be unconditionally setting here regardless of what was
configured?


John

> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index b1f495b731..6e76571796 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -1163,7 +1163,10 @@ qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src,
>          if (qemuBlockStorageSourceGetBlockdevGetCacheProps(src, fileprops) < 0)
>              goto cleanup;
> 
> -        if (virJSONValueObjectAdd(fileprops, "b:read-only", src->readonly, NULL) < 0)
> +        if (virJSONValueObjectAdd(fileprops,
> +                                  "b:read-only", src->readonly,
> +                                  "s:discard", "unmap",
> +                                  NULL) < 0)
>              goto cleanup;
>      }
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 05/02/2018 03:39 PM, John Ferlan wrote:
> 
> 
> On 04/25/2018 11:15 AM, Peter Krempa wrote:
>> Enabling discard for the storage node allows the format drivers to
>> discard snapshots and other things, while configuration of the format
>> layer actually decides whether to actually discard data on request from
>> the host.
>>
>> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>> ---
>>  src/qemu/qemu_block.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
> 
> Isn't this related to the {disk|src}->discard value? Which we copied
> from patch 3.
> 
> So you'd just be unconditionally setting here regardless of what was
> configured?
> 
> 
> John
> 


Hmm... so it seems the answer to my question is in the next patch.

TBH: The variation between names and knowing exactly which method is for
what condition - it's well, mind boggling. The terminology of storage
and format layer to go along with source source protocol and storage
node without much code documenting makes things challenging to follow.

So it seems for whatever reason this GetBackendProps is always wanting
to use "unmap"; whereas, GetBlockdevFormatCommonProps may use "unmap" or
"ignore".  And then, in the next patch a blockdev props and a backend
props are both generated for the same object - poof.

So assuming this dance is correct,

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, May 02, 2018 at 15:39:07 -0400, John Ferlan wrote:
> 
> 
> On 04/25/2018 11:15 AM, Peter Krempa wrote:
> > Enabling discard for the storage node allows the format drivers to
> > discard snapshots and other things, while configuration of the format
> > layer actually decides whether to actually discard data on request from
> > the host.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >  src/qemu/qemu_block.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> 
> Isn't this related to the {disk|src}->discard value? Which we copied
> from patch 3.
> 
> So you'd just be unconditionally setting here regardless of what was
> configured?

The actual discard value will be used for the "format" (qcow2/raw/...)
layer. QEMU always enables discard for the storage layers so that qcow2
can discard blocks from a deleted snapshot etc.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list