Change in vdsm[master]: storage: add discard support for vm disks

Idan Shaby has uploaded a new change for review.

Change subject: storage: add discard support for vm disks
......................................................................

storage: add discard support for vm disks

When the engine sends the field 'passDiscard' with the value 'true' as
one of the parameters of the vm's disk's drive, vdsm adds
discard='unmap' to the disk's drive.
With that, vdsm gives the vm disk the ability to support discard.

Note that vdsm can handle scenarios of sending passDiscard with the
values 'true' or 'false', alongside not sending it at all, which makes
it possible to work with old engines that don't support the feature.

For more information about the "Pass Discard" feature, please refer to
the feature page:
http://www.ovirt.org/develop/release-management/features/storage/pass-discar...

Change-Id: I215c12260f819538e40056ec16d0b9378287ccee
Bug-Url: https://bugzilla.redhat.com/1241106
Signed-off-by: Idan Shaby 
---
M tests/vmStorageTests.py
M vdsm/virt/vmdevices/storage.py
2 files changed, 23 insertions(+), 1 deletion(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/51/65751/1

diff --git a/tests/vmStorageTests.py b/tests/vmStorageTests.py
index aa289d3..3c91544 100644
--- a/tests/vmStorageTests.py
+++ b/tests/vmStorageTests.py
@@ -98,6 +98,22 @@
             """
         self.check({}, conf, xml, is_block_device=True)

+    def test_disk_with_pass_discard(self):
+        conf = drive_config(
+            serial='54-a672-23e5b495a9ea',
+            passDiscard='true',
+        )
+        xml = """
+            <disk device="disk" snapshot="no" type="block">
+                <source dev="/path/to/volume"/>
+                <target bus="virtio" dev="vda"/>
+                <serial>54-a672-23e5b495a9ea</serial>
+                <driver cache="none" discard="unmap" error_policy="stop"
+                        io="native" name="qemu" type="raw"/>
+            </disk>
+            """
+        self.check({}, conf, xml, is_block_device=True)
+
     def test_disk_file(self):
         conf = drive_config(
             serial='54-a672-23e5b495a9ea',
@@ -672,6 +688,7 @@
         'readonly': 'False',
         'shared': 'none',
         'type': 'disk',
+        'passDiscard': 'false',
     }
     conf.update(kw)
     return conf
@@ -685,4 +702,5 @@
         "format": format,
         "path": "/path/to/replica",
         "propagateErrors": "off",
+        'passDiscard': 'false',
     }
diff --git a/vdsm/virt/vmdevices/storage.py b/vdsm/virt/vmdevices/storage.py
index 78bf00c..dcf16c0 100644
--- a/vdsm/virt/vmdevices/storage.py
+++ b/vdsm/virt/vmdevices/storage.py
@@ -62,7 +62,8 @@
                  'index', 'name', 'optional', 'shared', 'truesize',
                  'volumeChain', 'baseVolumeID', 'serial', 'reqsize', 'cache',
                  '_blockDev', 'extSharedState', 'drv', 'sgio', 'GUID',
-                 'diskReplicate', '_diskType', 'hosts', 'protocol', 'auth')
+                 'diskReplicate', '_diskType', 'hosts', 'protocol', 'auth',
+                 'passDiscard')
     VOLWM_CHUNK_SIZE = (config.getint('irs', 'volume_utilization_chunk_mb') *
                         constants.MEGAB)
     VOLWM_FREE_PCT = 100 - config.getint('irs', 'volume_utilization_percent')
@@ -551,6 +552,9 @@
     elif drive['format']:
         driverAttrs['type'] = 'raw'

+    if 'passDiscard' in drive and drive['passDiscard'] == 'true':
+        driverAttrs['discard'] = 'unmap'
+
     try:
         driverAttrs['iothread'] = str(drive['specParams']['pinToIoThread'])
     except KeyError:

gerrit-hooks has posted comments on this change.

Change subject: storage: add discard support for vm disks
......................................................................

Patch Set 1:

* Update Tracker::#1241106::OK, status: NEW
* Check Bug-Url::OK
* Check Public Bug::#1241106::OK, public bug
* Check Product::IGNORE, not relevant for branch: master
* Check TM::IGNORE, not relevant for branch: master
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])

Yaniv Kaul has posted comments on this change.

Change subject: storage: add discard support for vm disks
......................................................................

Patch Set 1:

I'm not entirely sure why not just add it by default and be done with it.
Perhaps a VDSM option to turn it off if we find issues with it.

Idan Shaby has posted comments on this change.

Change subject: storage: add discard support for vm disks
......................................................................

Patch Set 1:

I am not sure that it is a good idea. If we wouldn't have the problem with WAD vs passDiscard, maybe we could do it, but that's not the case.

There's an entire logic in the engine's side regarding passing passDiscard for each disk (it's not yet on gerrit). In a nutshell, even if the disk is configured to pass discard in the engine, that doesn't mean that the engine will allow this and pass vdsm passDiscard='true'.
It depends on WAD and the underlying storage support for discard and 'discard zeroes the data' (that may have been changed).

Thus, making vdsm to run each vm disk with discard='unmap' by default is not possible.
Please see "Wipe After Delete and Pass Discard" in the feature page for more info.