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

linux/zvol_os: fix zvol queue limits initialization #16454

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

ixhamza
Copy link
Contributor

@ixhamza ixhamza commented Aug 15, 2024

Motivation and Context

zvol queue limits initialization depends on zv_volblocksize, but it is initialized later, leading to several limits being initialized with incorrect values, including max_discard_* limits. This also causes blkdiscard command to consistently fail, as blk_ioctl_discard reads bdev_max_discard_sectors() limits as 0, leading to failure. The fix is straightforward: initialize zv->zv_volblocksize early, before setting the queue limits.
This PR should fix zvol/zvol_misc/zvol_misc_trim failure on recent PRs, as the test case issues blkdiscard for a zvol. Additionally, zvol_misc_trim was recently enabled in 6c7d41a, which is why the issue wasn't identified earlier.

Description

How Has This Been Tested?

Before

truncate -s 2G /var/tmp/disk
zpool create tank /var/tmp/disk
zfs create -V 1G -o volblocksize=16k tank/zv
for file in /sys/block/zd0/queue/*; do
    case "$(basename "$file")" in
        discard_max_bytes|discard_max_hw_bytes|\
        optimal_io_size|physical_block_size)
            echo "$(basename "$file"): $(cat "$file")"
            ;;
    esac
done
=> discard_max_bytes: 0
=> discard_max_hw_bytes: 0
=> optimal_io_size: 0
=> physical_block_size: 512

blkdiscard --force /dev/zd0
=> blkdiscard: Operation forced, data will be lost!
=> blkdiscard: BLKDISCARD: /dev/zd0 ioctl failed: Operation not supported

After

truncate -s 2G /var/tmp/disk
zpool create tank /var/tmp/disk
zfs create -V 1G -o volblocksize=16k tank/zv
for file in /sys/block/zd0/queue/*; do
    case "$(basename "$file")" in
        discard_max_bytes|discard_max_hw_bytes|\
        optimal_io_size|physical_block_size)
            echo "$(basename "$file"): $(cat "$file")"
            ;;
    esac
done
=> discard_max_bytes: 268435456
=> discard_max_hw_bytes: 268435456
=> optimal_io_size: 16384
=> physical_block_size: 16384

blkdiscard --force /dev/zd0
=> blkdiscard: Operation forced, data will be lost!

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:

zvol queue limits initialization depends on `zv_volblocksize`, but it is
initialized later, leading to several limits being initialized with
incorrect values, including `max_discard_*` limits. This also causes
`blkdiscard` command to consistently fail, as `blk_ioctl_discard` reads
`bdev_max_discard_sectors()` limits as 0, leading to failure. The fix is
straightforward: initialize `zv->zv_volblocksize` early, before setting
the queue limits. This PR should fix `zvol/zvol_misc/zvol_misc_trim`
failure on recent PRs, as the test case issues `blkdiscard` for a zvol.
Additionally, `zvol_misc_trim` was recently enabled in `6c7d41a`,
which is why the issue wasn't identified earlier.

Signed-off-by: Ameer Hamza <[email protected]>
@tonyhutter
Copy link
Contributor

@ixhamza just wanted to let you know that the Fedora buildbots are basically down until #16450 and #16453 get merged (#16453 actually contains both the needed commits). You can either include those in your patchstack if you want a clean buildbot run (and we'll ignore them when merging this PR), or just wait until they get merged and re-base.

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Aug 15, 2024
@ixhamza
Copy link
Contributor Author

ixhamza commented Aug 15, 2024

@tonyhutter - Thanks for the update. This can wait. Once #16453 is merged, I’ll rebase.

@behlendorf
Copy link
Contributor

@ixhamza merged, please go ahead and rebase this.

@behlendorf behlendorf merged commit bdf4d6b into openzfs:master Aug 15, 2024
16 of 23 checks passed
@behlendorf
Copy link
Contributor

@ixhamza never mind, I see we got a clean run with the GitHub actions builders and this is a straightforward fix. Merged.

@ixhamza
Copy link
Contributor Author

ixhamza commented Aug 15, 2024

@behlendorf - Sorry, I just saw your message. Thanks for the quick merge.

@behlendorf
Copy link
Contributor

@ixhamza would you mind taking another look at this. It looks like we're still failing the zvol/zvol_misc/zvol_misc_trim test on the 6.10 kernel. For example, https://build.openzfs.org/builders/Fedora%2040%20x86_64%20%28TEST%29/builds/1020

@ixhamza
Copy link
Contributor Author

ixhamza commented Aug 19, 2024

@behlendorf - It's another issue in the zvol queue limits initialization. truenas@3af5c63 should fix it. I will open a PR once it passes GitHub actions in my repository.

@ixhamza
Copy link
Contributor Author

ixhamza commented Aug 20, 2024

#16462.

lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 4, 2024
zvol queue limits initialization depends on `zv_volblocksize`, but it is
initialized later, leading to several limits being initialized with
incorrect values, including `max_discard_*` limits. This also causes
`blkdiscard` command to consistently fail, as `blk_ioctl_discard` reads
`bdev_max_discard_sectors()` limits as 0, leading to failure. The fix is
straightforward: initialize `zv->zv_volblocksize` early, before setting
the queue limits. This PR should fix `zvol/zvol_misc/zvol_misc_trim`
failure on recent PRs, as the test case issues `blkdiscard` for a zvol.
Additionally, `zvol_misc_trim` was recently enabled in `6c7d41a`,
which is why the issue wasn't identified earlier.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Tony Hutter <[email protected]>
Signed-off-by: Ameer Hamza <[email protected]>
Closes openzfs#16454
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