New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: enable issue_discards=1 in /etc/lvm/lvm.conf #123

Open
sammcj opened this Issue Jan 3, 2019 · 5 comments

Comments

Projects
None yet
3 participants
@sammcj
Copy link

sammcj commented Jan 3, 2019

issue_discards should be enabled by default in /etc/lvm/lvm.conf, this allows TRIM/DISCARD and SCSI UNMAP IOCTLs through the LVM layer which is very important if you use SSDs or in many cases with thin provision iSCSI storage.

From lvm.conf:

"Configuration option devices/issue_discards.
Issue discards to PVs that are no longer used by an LV.
Discards are sent to an LV's underlying physical volumes when the LV
is no longer using the physical volumes' space, e.g. lvremove,
lvreduce. Discards inform the storage that a region is no longer
used. Storage that supports discards advertise the protocol-specific
way discards should be issued by the kernel (TRIM, UNMAP, or
WRITE SAME with UNMAP bit set). Not all storage will support or
benefit from discards, but SSDs and thinly provisioned LUNs
generally do. If enabled, discards will only be issued if both the
storage and kernel provide support."

Patch for change:

--- /etc/lvm/lvm.conf	2019-01-03 11:36:22.379998897 +1100
+++ /etc/lvm/lvm.conf.discards	2019-01-03 14:35:14.624804878 +1100
@@ -298,7 +298,7 @@
 	# benefit from discards, but SSDs and thinly provisioned LUNs
 	# generally do. If enabled, discards will only be issued if both the
 	# storage and kernel provide support.
-	issue_discards = 0
+	issue_discards = 1
 }

 # Configuration section allocation.

This is an improvement suggestion as per a brief discussion on the XCP-ng forums based on my XenServer and CentOS tuning experiences.

@sammcj sammcj changed the title enable issue_discards=1 in /etc/lvm/lvm.conf Feature request: enable issue_discards=1 in /etc/lvm/lvm.conf Jan 3, 2019

@sammcj

This comment has been minimized.

Copy link
Author

sammcj commented Jan 3, 2019

FYI we have been doing this with puppet as such:

  # Enable discard / unmap / trim at the LVM level
  file_line {'issue_discards':
    ensure   => 'present',
    line     => '  issue_discards = 1',
    path     => '/etc/lvm/lvm.conf',
    match    => /  issue_discards = 0/,
    multiple => false,
    replace  => true,
  }
@stormi

This comment has been minimized.

Copy link
Member

stormi commented Jan 3, 2019

Do you know why it's not the default in lvm.conf? Are there drawbacks to this setting?

@sammcj

This comment has been minimized.

Copy link
Author

sammcj commented Jan 6, 2019

No idea, I wouldn't be surprised if it was just a historical thing, it certainly annoys me having to change it on every machine I build.

AFAIK there are no drawbacks because if the underlying block device is not capable of the IOCTL it's simply not issued.

@Ultra2D

This comment has been minimized.

Copy link

Ultra2D commented Jan 11, 2019

Do you make any other changes for this to have any effect? Because this does not seem to work on lvmoiscsi when removing a VDI from a SR.

According to https://discussions.citrix.com/topic/378764-reclaiming-freed-space/#comment-1931283 lvremove is called without discard, unless you do a "Reclaim freed space" which sends discards even if issue_discards = 0 in lvm.conf. Reclaim freed space does work.

It also suggests changing lvutil.py, and indeed, after changing lvutil.py it does work for me.

adding the config_param "issues_discards=1" to the lvutil.py remove function, which makes it automatic and transparent

It would be nice to have a SR parameter (maybe an sr-config key) auto-trim=true or something so folks with cooperative storage platforms can maintain the benefit of thin provisioning at scale.

@sammcj

This comment has been minimized.

Copy link
Author

sammcj commented Jan 13, 2019

Ah yes - sorry lvutil.py is the other part I forgot in this suggestion - thank you for picking that up 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment