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

[2.2] bdev_discard_supported: understand discard_granularity=0 #16111

Merged

Conversation

robn
Copy link
Member

@robn robn commented Apr 19, 2024

Motivation and Context

OpenZFS does not consider the discard_granularity property when enabling discard (TRIM), leading to the kernel rejecting discard requests with confusing warnings in the logs.

Backporting #16082 for 2.2.4, which i promised I would do in #16056.

Description

Kernel documentation for the discard_granularity property says:

A discard_granularity of 0 means that the device does not support discard functionality.

Some older kernels had drivers (notably loop, but also some USB-SATA adapters) that would set the QUEUE_FLAG_DISCARD capability flag, but have discard_granularity=0. Since 5.10 (torvalds/linux@b35fd7422c2f) the discard entry point blkdev_issue_discard() has had a check for this, which would immediately reject the call with EOPNOTSUPP, and throw a scary diagnostic message into the log. See #16068.

Since 6.8, the block layer sets a non-zero default for discard_granularity (torvalds/linux@3c407dc723bb), and a future kernel will remove the check entirely.

As such, there's no good reason for us to enable discard when discard_granularity=0. The kernel will never let the request go in anyway; better that we just disable it so we can report it properly to the user.

How Has This Been Tested?

Checked on 5.10.214, where loop devices have QUEUE_FLAG_DISCARD and discard_granularity=0:

$ uname -a
Linux quiz 5.10.214 #1 SMP Sun Mar 31 14:27:22 AEDT 2024 x86_64 GNU/Linux
$ zpool version
zfs-2.2.3-19_ga3525969c
zfs-kmod-2.2.3-19_ga3525969c
$ grep -H . /sys/block/loop0/queue/discard_{max_bytes,granularity}
/sys/block/loop0/queue/discard_max_bytes:4294966784
/sys/block/loop0/queue/discard_granularity:0
$ zpool create tank loop0
$ zpool status -t
  pool: tank
 state: ONLINE
config:

	NAME        STATE     READ WRITE CKSUM
	tank        ONLINE       0     0     0
	 loop0     ONLINE       0     0     0  (trim unsupported)

errors: No known data errors
$ zpool trim tank
cannot trim: no devices in pool support trim operations

Testing on #16082 should hold here.

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:

Kernel documentation for the discard_granularity property says:

    A discard_granularity of 0 means that the device does not support
    discard functionality.

Some older kernels had drivers (notably loop, but also some USB-SATA
adapters) that would set the QUEUE_FLAG_DISCARD capability flag, but
have discard_granularity=0. Since 5.10 (torvalds/linux@b35fd7422c2f) the
discard entry point blkdev_issue_discard() has had a check for this,
which would immediately reject the call with EOPNOTSUPP, and throw a
scary diagnostic message into the log. See openzfs#16068.

Since 6.8, the block layer sets a non-zero default for
discard_granularity (torvalds/linux@3c407dc723bb), and a future kernel
will remove the check entirely[1].

As such, there's no good reason for us to enable discard when
discard_granularity=0. The kernel will never let the request go in
anyway; better that we just disable it so we can report it properly to
the user.

1. https://patchwork.kernel.org/project/linux-block/patch/[email protected]/

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
(cherry picked from commit b181b2e)
@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Apr 19, 2024
@behlendorf behlendorf merged commit 72e4996 into openzfs:zfs-2.2.4-staging Apr 19, 2024
23 of 25 checks passed
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.

3 participants