Bug 981626 (send_discard_when_deleting_disks)

Adding some details from case:

AFAIK, lvm already has this feature where lvremove has a flag to discard blocks.

This bug https://bugzilla.redhat.com/show_bug.cgi?id=651493 added discard support to lvm2.

Default on rhev-h lvm.conf is

    # 1 enables; 0 disables.
    issue_discards = 0

So RHEV just need to implement it to make this possible.

To use the lvm issue_discards option we should make sure that:

1. lvm is issuing the discard ioctl *before* deallocating the segments of the lv (put it differently: it sends the discard ioctl before removing the lv from the mda)

2. lvm is waiting for all the ioctl to complete before removing the lv from the mda

3. the ioctl is synchronous, the kernel is non caching the requests: from the client point of view once the lv is removed from the mda there shouldn't be any pending io to the old segments (not even discards)

4. depending on the discard granularity this operation can take a long time (a storage server round-trip per discard granularity of the lv size), implication in vdsm is that to a certain extent this could become a long operation.

5. it seems that during lvremove with discard it's not possible to execute other lvm commands on the same host (write-lock) for the running time: this would block the normal vdsm operation


Thinking more about the issue_discards use case, it is more similar to the postZero operation we take in the deleteImage flow (more than lvremove): in fact in both cases the previous data will become unrecoverable (also for security reasons).

I think the best implementation would be using blkdiscard(8) over the activated lv, in fact:

- it maps 1:1 to the postZero operation (they both need to activate the lv, then dd takes much more time, but also blkdiscard takes some time to complete)

- the lv device map will pass the discard ioctl down to the pvs

- once the blkdiscard operation is terminated it is possible to quickly remove (no additional time required) the lv as before (issue_discards=0) 

- there's no fear of pending ioctl cached on the client side as once blkdiscard is finished we deactivate the lv (no other io allowed)

- blkdiscard fails explicitly (instead issue_discards fails silently)


VDSM has enough information to decide whether to use dd or blkdiscard to postZero a volume, question is if we want to make it automatic or if we want to configure it on the engine side.

Given comment 10 this bug is very similar to bug 1069557.

*** Bug 1226977 has been marked as a duplicate of this bug. ***

Hi Allon,

I'm not sure to understand why this is targeted to 4.0.0, as this seems quite far.
May you precise what is preventing from being shorter?

Thank you.

(In reply to Nicolas Ecarnot from comment #16)
> Hi Allon,
> 
> I'm not sure to understand why this is targeted to 4.0.0, as this seems
> quite far.
> May you precise what is preventing from being shorter?
> 
> Thank you.
It is hardly targeted - notice the conditional devel nack.
Once (if?) we have a sensible design for this, we can start considering timelines.

As per comment 4 setting LVM's "issue_discards = 1" should do.

(In reply to Julio Entrena Perez from comment #18)
> As per comment 4 setting LVM's "issue_discards = 1" should do.
See the rest of the discussion. This a partial solution at best.

Dup of bug 872530 ?

(In reply to Yaniv Kaul from comment #20)
> Dup of bug 872530 ?
Nope.
Bug 872530 is about a disk that's being removed from the system. This one is about a live guest removing files from its filesystem and passing down the discard to the storage.

(In reply to Allon Mureinik from comment #21)
> (In reply to Yaniv Kaul from comment #20)
> > Dup of bug 872530 ?
> Nope.
> Bug 872530 is about a disk that's being removed from the system. This one is
> about a live guest removing files from its filesystem and passing down the
> discard to the storage.

Sorry, too many open windows. If this isn't a dup it's at the very lest closely related.

*** This bug has been marked as a duplicate of bug 872530 ***

Hi Yaniv,

IMHO, this bug is not a duplicate of bug 872530.

Bug 872530 is about the way we wipe disks.
This bug is about whether to issue a discard call or not after a disk removal.
Note that we don't wipe every disk. We wipe a disk (on a block domain, of course) only if it is marked as "wipe after delete".

The plan was to add the vdsm config file an option to issue a discard call for a removed disk (and in the future, also in the new/edit disk window, right beneath the "wipe after delete" check box, for example).

And as for bug 872530, the plan is to support new wipe methods for removed disks (might also be in the config file).

Of course that if the disk has the "wipe after delete" option enabled, and the user chose (as planned for bug 872530) to issue "discard -z" on a removed disk instead of dd, another discard call won't be performed.

Therefore, I think we should reopen this bug.

Idan is right. Forgot that one can just simply discard to re-gain allocated space.

I wonder why not send discard always - why do we need a checkbox for it?

That's because sending discard has performance implications.

From qemu's change log[1]: "'discard' commands...can cause performance degradation and fragmentation".

From libvirt's documentation[2]: "The guest time may be delayed, unless the OS has explicit handling of lost ticks".

So by default it should be false, to keep the same performance that we had previously.

[1] http://wiki.qemu.org/ChangeLog/1.5 (under the System emulation -> Block devices)
[2] https://libvirt.org/formatdomain.html (under Time keeping -> discard)

(In reply to Idan Shaby from comment #30)
> That's because sending discard has performance implications.

That's a thing of the past in most scenarios, for two reasons:
1. Today's enterprise storages are VERY efficient in discarding. Those are not the plain old SSDs which used to do very inefficient garbage collection when discarding.
2. The discard from the guest is now rarely done in realtime. In Linux, 'fstrim' would be set on a cron job (as opposed to mounting the filesystem - XFS or EXT4 - with discard on)

In any case, this SPECIFIC RFE is about discarding when the disk is removed - which won't hurt the performance of the VM most likely.
I do agree it needs an on/off switch, possibly per storage domain. We can probably keep it off by default.

> 
> From qemu's change log[1]: "'discard' commands...can cause performance
> degradation and fragmentation".
> 
> From libvirt's documentation[2]: "The guest time may be delayed, unless the
> OS has explicit handling of lost ticks".
> 
> So by default it should be false, to keep the same performance that we had
> previously.
> 
> [1] http://wiki.qemu.org/ChangeLog/1.5 (under the System emulation -> Block
> devices)
> [2] https://libvirt.org/formatdomain.html (under Time keeping -> discard)

*** Bug 1321608 has been marked as a duplicate of this bug. ***

> This feature adds a new parameter to the vdsm config file - "discard_enable", which is false by default.
>
> When changed to true for a specific host, for each disk/snapshot that is going to be removed, that host will issue a "blkdiscard" call right before removing it in order to free the used space from the thinly provisioned lun that the storage domain is built on.

*************

Let's say I have a cluster(s) with lots of fiberchannel hosts.  I get rid of a VM, which deletes its virtual disk.  Does every host in the data center do the appropriate LVM commands, and then I set discard_enable = true on one host so it tells the underlying storage about it?  Or do I need discard_enable = true on the SPM host, which means I need it everywhere since I won't know in advance which host takes the SPM role?

Hi Greg, the second option is the right one - you need discard_enable = true on the SPM host, which means that you need it everywhere since you don't know in advance which host takes the SPM role.

Anyway, this feature is merged only on master, so that it will be available only on 4.0 for now.

OK, thanks.  If it's off by default, would it be possible to set up a choice at install time to set it?  Or some way to persist vdsm.conf so admins can set it once and it's set everywhere in the datacenter after that?

thanks

(In reply to Greg Scott from comment #36)
> OK, thanks.  If it's off by default, would it be possible to set up a choice
> at install time to set it?  Or some way to persist vdsm.conf so admins can
> set it once and it's set everywhere in the datacenter after that?

In 4.0, you can configure (and persist) vdsm.conf via Cockpit (on RHEVH NGN).
In 4.1, we'll make it more granular (per storage domain, per disk) - similar to wipe-after-delete setting. Idan - please make sure there's a 4.1 RFE for this. 
> 
> thanks

Please see bug 1342919 .

Can you please open a test only ticket on 4.0 describing how to enable discard, so QE can try it out? We have customer looking to use this feature with 4.0.

(In reply to Yaniv Dary from comment #39)
> Can you please open a test only ticket on 4.0 describing how to enable
> discard, so QE can try it out? We have customer looking to use this feature
> with 4.0.
All the info is in the doctext, why do we need yet another bug?

(In reply to Allon Mureinik from comment #40)
> (In reply to Yaniv Dary from comment #39)
> > Can you please open a test only ticket on 4.0 describing how to enable
> > discard, so QE can try it out? We have customer looking to use this feature
> > with 4.0.
> All the info is in the doctext, why do we need yet another bug?

We need a ticket to describe what to test that we can target to 4.0.z.

I'll retarget. THIS bz represents the 4.0 offering. The full-fledged feature, including GUI etc is tracked by bug 1342919.

Tested the following:

1. On the storage server, created a 30G thin LUN. Exposed the LUN to the hyper-visors
2. Created a VM and installed RHEL OS.
3. Created, attached and activated a direct LUN with virtIO-SCSI interface to the VM. Checked 'Enable SCSI Pass-Through', in the direct LUN attachment phase.
4. Created an ext4 file system on the attached direct LUN disk. Added it to /etc/fstab and specified 'discard' instead of 'defaults': 

/dev/sdX                    /mount_point                   ext4    discard        0 0

And executed 'mount -a'
	
5. Created a 5G file on the new disk.
6. Examined the actual LUN size on the storage server for the exposed LUN.
7. On the VM, deleted the 5G file.
8. Once the file got deleted, executed 'sync' command on the guest.

Results:
LUN actual size, in the storage server decreased back to the size it was before the file creation



==========================
Used:
vdsm-4.18.12-1.el7ev.x86_64
rhevm-4.0.4-0.1.el7ev.noarch
XtremIO storage
RHEL7.2 guest

(In reply to Elad from comment #43)
> Tested the following:
> 
> 1. On the storage server, created a 30G thin LUN. Exposed the LUN to the
> hyper-visors
> 2. Created a VM and installed RHEL OS.
> 3. Created, attached and activated a direct LUN with virtIO-SCSI interface
> to the VM. Checked 'Enable SCSI Pass-Through', in the direct LUN attachment
> phase.
> 4. Created an ext4 file system on the attached direct LUN disk. Added it to
> /etc/fstab and specified 'discard' instead of 'defaults': 
> 
> /dev/sdX                    /mount_point                   ext4    discard  
> 0 0
> 
> And executed 'mount -a'
> 	
> 5. Created a 5G file on the new disk.
> 6. Examined the actual LUN size on the storage server for the exposed LUN.
> 7. On the VM, deleted the 5G file.
> 8. Once the file got deleted, executed 'sync' command on the guest.
> 
> Results:
> LUN actual size, in the storage server decreased back to the size it was
> before the file creation
> 
> 
> 
> ==========================
> Used:
> vdsm-4.18.12-1.el7ev.x86_64
> rhevm-4.0.4-0.1.el7ev.noarch
> XtremIO storage
> RHEL7.2 guest

This is not the right test. The test should be per this feature - send discard when the virtual disk is DELETED, not when contents within the virtual disk are (i.e., passthrough of the DISCARD command from the guest all the way to the storage.

I mistakenly tested discard for direct LUN. Will test for virtual disk

Also note that this virtual disk should be an image, not a direct lun.

1) Added 'discard_enable = true' to vdsm.conf and restarted vdsmd service 
2) Created a block domain based on a thin LUN on a discard supported storage server
3) Created a virtual disk on the block domain and attached it to a RHEL guest
4) Checked the actual LUN size on the storage server for later comparison
5) Wrote to virtual the disk from the guest. LUN actual size increased
6) Deleted the virtual disk

Result: LUN actual size got reduced to the size it was before virtual disk creation.

Tested with FC and iSCSI

==========================
Used:
vdsm-4.18.12-1.el7ev.x86_64
rhevm-4.0.4-0.1.el7ev.noarch
XtremIO storage
RHEL7.2 guest

Elad - did you test with qcow2, raw, with extended disk, with multiple LUNs in the SD, with 1TB disk, multiple disk deletions, the impact on the storage (performance, bandwidth in iSCSI and FC), etc.?

I tested the basic scenario here:
A raw preallocated virtual disk, not extended, single LUN in SD (35G for iSCSI and 80G for FC), a single disk deletion. 

Did not examine the impact on the storage, but other than the deletion itself, which impact should be tested here?

(In reply to Elad from comment #51)
> I tested the basic scenario here:
> A raw preallocated virtual disk, not extended, single LUN in SD (35G for
> iSCSI and 80G for FC), a single disk deletion. 
> 
> Did not examine the impact on the storage, but other than the deletion
> itself, which impact should be tested here?

1. That the SPM doesn't choke.
2. That the interface to the storage (FC or iSCSI) doesn't choke. Compare it with the same command using the old 'dd' - how much bandwidth it's taking.

(In reply to Yaniv Kaul from comment #52)
> (In reply to Elad from comment #51)
> > I tested the basic scenario here:
> > A raw preallocated virtual disk, not extended, single LUN in SD (35G for
> > iSCSI and 80G for FC), a single disk deletion. 
> > 
> > Did not examine the impact on the storage, but other than the deletion
> > itself, which impact should be tested here?
> 
> 1. That the SPM doesn't choke.
> 2. That the interface to the storage (FC or iSCSI) doesn't choke. Compare it
> with the same command using the old 'dd' - how much bandwidth it's taking.

Why comparing to 'dd'? We are not wiping the disk here. We are just discarding the lv.

(In reply to Idan Shaby from comment #53)
> (In reply to Yaniv Kaul from comment #52)
> > (In reply to Elad from comment #51)
> > > I tested the basic scenario here:
> > > A raw preallocated virtual disk, not extended, single LUN in SD (35G for
> > > iSCSI and 80G for FC), a single disk deletion. 
> > > 
> > > Did not examine the impact on the storage, but other than the deletion
> > > itself, which impact should be tested here?
> > 
> > 1. That the SPM doesn't choke.
> > 2. That the interface to the storage (FC or iSCSI) doesn't choke. Compare it
> > with the same command using the old 'dd' - how much bandwidth it's taking.
> 
> Why comparing to 'dd'? We are not wiping the disk here. We are just
> discarding the lv.

There's nothing else to compare to...

(In reply to Yaniv Kaul from comment #54)
> (In reply to Idan Shaby from comment #53)
> > (In reply to Yaniv Kaul from comment #52)
> > > (In reply to Elad from comment #51)
> > > > I tested the basic scenario here:
> > > > A raw preallocated virtual disk, not extended, single LUN in SD (35G for
> > > > iSCSI and 80G for FC), a single disk deletion. 
> > > > 
> > > > Did not examine the impact on the storage, but other than the deletion
> > > > itself, which impact should be tested here?
> > > 
> > > 1. That the SPM doesn't choke.
> > > 2. That the interface to the storage (FC or iSCSI) doesn't choke. Compare it
> > > with the same command using the old 'dd' - how much bandwidth it's taking.
> > 
> > Why comparing to 'dd'? We are not wiping the disk here. We are just
> > discarding the lv.
> 
> There's nothing else to compare to...

Right, but previously we didn't do anything instead of discarding the lv, so I am not sure if we can gain anything from comparing it to a very long process like "dd".
IMHO, we should not compare it to anything. But I agree that it's a good idea to inspect everything else that you mentioned (performance etc.).

Tested with the following code:
----------------------------------------
rhevm-4.0.4.2-0.1.el7ev.noarch
vdsm-4.18.13-1.el7ev.x86_64


Tested with the following scenario:

Steps to Reproduce:
Tested according to Test Plan below. Tests included
Basic discard disk with Thin disk
Basic discard disk with Preallocated disk
Discard disk of multiple disks on same SD
Discard disk on SD comprised of multiple LUNs
Discard disk after extending LUN, SD and Disk
Discard disk and Performance
Discard disk failed discard operation


https://polarion.engineering.redhat.com/polarion/#/project/RHEVM3/wiki/Storage_4_0/4_1_Storage_Support%20discard%20when%20deleting%20virtual%20disks%20from%20block%20based%20storage%20domains

Actual results:
All operations were successfull.

Expected results:




Moving to VERIFIED!

Kevin - did you actually see the benefit, that the discard freed up space on the storage?

(In reply to Kevin Alon Goldblatt from comment #56)
> Tested with the following code:
> ----------------------------------------
> rhevm-4.0.4.2-0.1.el7ev.noarch
> vdsm-4.18.13-1.el7ev.x86_64
> 
> 
> Tested with the following scenario:
> 
> Steps to Reproduce:
> Tested according to Test Plan below. Tests included
> Basic discard disk with Thin disk
> Basic discard disk with Preallocated disk
> Discard disk of multiple disks on same SD
> Discard disk on SD comprised of multiple LUNs
> Discard disk after extending LUN, SD and Disk
> Discard disk and Performance
> Discard disk failed discard operation
> 
> 
> https://polarion.engineering.redhat.com/polarion/#/project/RHEVM3/wiki/
> Storage_4_0/
> 4_1_Storage_Support%20discard%20when%20deleting%20virtual%20disks%20from%20bl
> ock%20based%20storage%20domains
> 
> Actual results:
> All operations were successfull.
> 
> Expected results:
> 
> 
> 
> 
> Moving to VERIFIED!

Hello,

For mere users like me, waiting for this feature for years [1], it would be nice if we could have an access to the tests results (polarion...).

On our block storage, we don't mind security considerations, so every task related to conscientiously wipe out every bit of a LV before deleting is not as important to us as what Idan tested in the first place, ie. make sure the block storage thin LUN is showing its available space back after LV deletion.

[1] http://lists.ovirt.org/pipermail/users/2014-November/029595.html

Aharon, I am not sure if and what we can expose here -> your call.

Nicolas, if security is not in your priority, I would use wipe after delete = false for my disks and discard_enable = true in vdsm conf file, for now.
In the future we will make it possible to configure discard per storage domains [1].
For more info, please refer to this [2] feature page.

[1] Bug 1342919 - (configure_discard_per_domain_and_disk) [RFE] Make discard configurable by a storage domain rather than a host
[2] http://www.ovirt.org/develop/release-management/features/storage/discard-after-delete/

(In reply to Yaniv Kaul from comment #57)
> Kevin - did you actually see the benefit, that the discard freed up space on
> the storage?
Yes I confirmed via the Storage Provider (XtremIO that the space-in-use on the thinly provisioned LUN is reclaimed after the each of the discard operations