Skip to content
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

Improve performance for zpool trim on Linux #15843

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

usaleem-ix
Copy link
Contributor

Motivation and Context

On Linux, ZFS uses blkdev_issue_discard in vdev_disk_io_trim to issue trim command which is synchronus. In blkdev_issue_discard, submit_bio_wait is used to after calling __blkdev_issue_discard.

Description

This commit updates vdev_disk_io_trim to use __blkdev_issue_discard, which is asynchronous, and submit bio via vdev_submit_bio.

Unfortunately, there isn't any asynchronus version for blkdev_issue_secure_erase, so performance of secure trim will still suffer.

How Has This Been Tested?

CI test runs and manual runs of zpool trim <pool>

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

On Linux, ZFS uses blkdev_issue_discard in vdev_disk_io_trim to issue
trim command which is synchronus.

This commit updates vdev_disk_io_trim to use __blkdev_issue_discard,
which is asynchronus. Unfortunately there isn't any asynchronus
version for blkdev_issue_secure_erase, so performance of secure trim
will still suffer.

Signed-off-by: Umer Saleem <[email protected]>
config/kernel-blkdev.m4 Show resolved Hide resolved
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Feb 1, 2024
config/kernel-blkdev.m4 Show resolved Hide resolved
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Feb 2, 2024
@behlendorf behlendorf merged commit 06e25f9 into openzfs:master Feb 2, 2024
24 of 26 checks passed
@usaleem-ix usaleem-ix mentioned this pull request Feb 6, 2024
13 tasks
tonyhutter pushed a commit that referenced this pull request Feb 22, 2024
On Linux, ZFS uses blkdev_issue_discard in vdev_disk_io_trim to issue
trim command which is synchronous.

This commit updates vdev_disk_io_trim to use __blkdev_issue_discard,
which is asynchronous. Unfortunately there isn't any asynchronous
version for blkdev_issue_secure_erase, so performance of secure trim
will still suffer.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Umer Saleem <[email protected]>
Closes #15843
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
On Linux, ZFS uses blkdev_issue_discard in vdev_disk_io_trim to issue
trim command which is synchronous.

This commit updates vdev_disk_io_trim to use __blkdev_issue_discard,
which is asynchronous. Unfortunately there isn't any asynchronous
version for blkdev_issue_secure_erase, so performance of secure trim
will still suffer.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Umer Saleem <[email protected]>
Closes openzfs#15843
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
On Linux, ZFS uses blkdev_issue_discard in vdev_disk_io_trim to issue
trim command which is synchronous.

This commit updates vdev_disk_io_trim to use __blkdev_issue_discard,
which is asynchronous. Unfortunately there isn't any asynchronous
version for blkdev_issue_secure_erase, so performance of secure trim
will still suffer.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Umer Saleem <[email protected]>
Closes openzfs#15843
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
On Linux, ZFS uses blkdev_issue_discard in vdev_disk_io_trim to issue
trim command which is synchronous.

This commit updates vdev_disk_io_trim to use __blkdev_issue_discard,
which is asynchronous. Unfortunately there isn't any asynchronous
version for blkdev_issue_secure_erase, so performance of secure trim
will still suffer.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Umer Saleem <[email protected]>
Closes openzfs#15843
@robn
Copy link
Member

robn commented Apr 8, 2024

@usaleem-ix do you have any info on hand about the performance improvements this change gives? I'm not doubting that there's something there, I just want to understand the before & after a little better before I make any changes in this area. Thanks!

@usaleem-ix
Copy link
Contributor Author

@robn there was an issue where users with typically large pools were experiencing very slow trim operation. Before this change, we saw that ZFS uses synchronous TRIM KPIs on Linux (blkdev_issue_discard) (waiting for request completion), unlike FreeBSD, where it is properly asynchronous.

The problem with synchronous KPI is that ZFS currently has only 4 TRIM issue taskq threads, that means on Linux it can have only 4 TRIM requests per pool running same time. That may be OK for small non-fragmented pool, but for some cases it becomes a problem. So we introduced this patch to use asynchronous KPI (__blkdev_issue_discard).

Unfortunately, I don't have performance improvement numbers at hand for this.

@robn
Copy link
Member

robn commented Apr 8, 2024

No problem, I appreciate the info! That's about what my guess was too. Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants