linux/zvol_os.c: Fix max_discard_sectors limit for 6.8+ kernel #16462
Conversation
|
Would it not be better then to create and set |
module/os/linux/zfs/zvol_os.c
Outdated
| @@ -1266,6 +1266,8 @@ zvol_alloc_non_blk_mq(struct zvol_state_os *zso, zvol_queue_limits_t *limits) | |||
| zso->zvo_queue = zso->zvo_disk->queue; | |||
|
|
|||
| #ifndef HAVE_BLKDEV_QUEUE_LIMITS_FEATURES | |||
There was a problem hiding this comment.
I don't quite understand why I didn't just call zvol_queue_limits_apply(limits, zso->zvo_queue) here, since that was always the point of convert/apply: everything in one two places.
There was a problem hiding this comment.
Just curious, why set it in two places if the limits should already be set during initialization?
There was a problem hiding this comment.
The idea was that you would call _convert if you had a creation function that could take limits, and regardless, calling _apply afterward in all cases would get everything left over.
(see the comment: I assert in the last paragraph that its "called on all versions").
Oh yeah, this is all in fact redundant. zvol_alloc_non_blk_mq() calls zvol_queue_limits_apply() at the very end for all cases, so this extra stuff in the middle can go.
diff --git a/module/os/linux/zfs/zvol_os.c b/module/os/linux/zfs/zvol_os.c
index e04f64e23..122fda3ea 100644
--- module/os/linux/zfs/zvol_os.c
+++ module/os/linux/zfs/zvol_os.c
@@ -1251,7 +1251,6 @@ zvol_alloc_non_blk_mq(struct zvol_state_os *zso, zvol_queue_limits_t *limits)
zso->zvo_disk->minors = ZVOL_MINORS;
zso->zvo_queue = zso->zvo_disk->queue;
- zvol_queue_limits_apply(limits, zso->zvo_queue);
#elif defined(HAVE_BLK_ALLOC_DISK_2ARG)
struct queue_limits qlimits;
zvol_queue_limits_convert(limits, &qlimits);
@@ -1264,11 +1263,6 @@ zvol_alloc_non_blk_mq(struct zvol_state_os *zso, zvol_queue_limits_t *limits)
zso->zvo_disk = disk;
zso->zvo_disk->minors = ZVOL_MINORS;
zso->zvo_queue = zso->zvo_disk->queue;
-
-#ifndef HAVE_BLKDEV_QUEUE_LIMITS_FEATURES
- blk_queue_set_write_cache(zso->zvo_queue, B_TRUE);
-#endif
-
#else
zso->zvo_queue = blk_alloc_queue(NUMA_NO_NODE);
if (zso->zvo_queue == NULL)There was a problem hiding this comment.
@robn - Supporting different kernel versions has indeed made things quite complex. Hats off to you for managing it so efficiently. I will clean up as per your suggestion along with fixing the original issue.
There was a problem hiding this comment.
Oh sorry! I was just muttering out loud about my mistake; not actually suggesting you fix it!
(but if you want to anyway, I will be delighted and enthusiastically approve the PR 🙌)
@robn - That's a good point, and I was thinking the same, but I refrained from it since |
In kernels 6.8 and later, the zvol block device is allocated with qlimits passed during initialization. However, the zvol driver does not set `max_hw_discard_sectors`, which is necessary to properly initialize `max_discard_sectors`. This causes the `zvol_misc_trim` test to fail on 6.8+ kernels when invoking the `blkdiscard` command. Setting `max_hw_discard_sectors` in the `HAVE_BLK_ALLOC_DISK_2ARG` case resolve the issue. Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Rob Noris suggested that we could clean up redundant limits for the case of non-blk mq scenario. Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
|
@behlendorf - both Fedora tests are passing now, other test failures look unrelated. |
| @@ -1213,6 +1213,7 @@ zvol_queue_limits_convert(zvol_queue_limits_t *limits, | |||
| qlimits->io_opt = limits->zql_io_opt; | |||
| qlimits->physical_block_size = limits->zql_physical_block_size; | |||
| qlimits->max_discard_sectors = limits->zql_max_discard_sectors; | |||
| qlimits->max_hw_discard_sectors = limits->zql_max_discard_sectors; | |||
There was a problem hiding this comment.
Oh that's not what I meant but I like it better, nice!
Rob Noris suggested that we could clean up redundant limits for the case of non-blk mq scenario. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Tony Hutter <hutter2@llnl.gov> Reviewed-by: Rob Norris <robn@despairlabs.com> Signed-off-by: Ameer Hamza <ahamza@ixsystems.com> Closes #16462
In kernels 6.8 and later, the zvol block device is allocated with qlimits passed during initialization. However, the zvol driver does not set `max_hw_discard_sectors`, which is necessary to properly initialize `max_discard_sectors`. This causes the `zvol_misc_trim` test to fail on 6.8+ kernels when invoking the `blkdiscard` command. Setting `max_hw_discard_sectors` in the `HAVE_BLK_ALLOC_DISK_2ARG` case resolve the issue. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Tony Hutter <hutter2@llnl.gov> Reviewed-by: Rob Norris <robn@despairlabs.com> Signed-off-by: Ameer Hamza <ahamza@ixsystems.com> Closes openzfs#16462
Rob Noris suggested that we could clean up redundant limits for the case of non-blk mq scenario. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Tony Hutter <hutter2@llnl.gov> Reviewed-by: Rob Norris <robn@despairlabs.com> Signed-off-by: Ameer Hamza <ahamza@ixsystems.com> Closes openzfs#16462
Motivation and Context
In kernels 6.8 and later, the zvol block device is allocated with qlimits passed during initialization. However, the zvol driver does not set
max_hw_discard_sectors, which is necessary to properly initializemax_discard_sectors. This causes thezvol_misc_trimtest to fail on 6.8+ kernels when invoking theblkdiscardcommand. Settingmax_hw_discard_sectorsin theHAVE_BLK_ALLOC_DISK_2ARGcase resolve the issue.Description
How Has This Been Tested?
blkdiscardcommand on 6.8 kernels and onwards.zvol_misc_trimtest on Fedora using the 6.10 kernel.Types of changes
Checklist:
Signed-off-by.