add prefetch property

[shodanshok]

ZFS prefetch is currently governed by the zfs_prefetch_disable
tunable. However, this is a module-wide settings - if a specific
dataset benefits from prefetch, while others have issue with it,
an optimal solution does not exists.

This commit introduce the "prefetch" tri-state property, which enable
granular control (at dataset/volume level) for prefetching.

This patch does not remove the zfs_prefetch_disable, which reimains
a system-wide switch for enable/disable prefetch. However, to avoid
duplication, it would be preferable to deprecate and then remove
the module tunable.

Motivation and Context

Description

How Has This Been Tested?

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:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[gmelikov]

What's with backward compatibility - i.e. import pool with such defined property with pre-patch code?

[shodanshok]

What's with backward compatibility - i.e. import pool with such defined property with pre-patch code?

Good question. An unpatched ZFS seems to have no issue importing and using the pool - the prefetch property is simply ignored.

# patched (2.2 master)
root@debian12:~/zfs/zfs# zpool import tank
root@debian12:~/zfs/zfs# dmesg
[ 2174.204782] ZFS: Loaded module v2.2.99-1, ZFS pool version 5000, ZFS filesystem version 5
root@debian12:~/zfs/zfs# md5sum /tank/test/test.img
a7c960320c0ccac4b9879af3067d0bd2  /tank/test/test.img

# unpatched code (2.1.12)
root@debian12:~/zfs/zfs-2.1.12# zpool import tank
root@debian12:~/zfs/zfs-2.1.12# dmesg
[ 1640.430596] ZFS: Loaded module v2.1.12-1, ZFS pool version 5000, ZFS filesystem version 5
root@debian12:~/zfs/zfs-2.1.12# md5sum /tank/test/test.img
a7c960320c0ccac4b9879af3067d0bd2  /tank/test/test.img

[amotin]

I have no objections besides minor comments above and the fact that I don't like manual tuning for things that should behave out of the box. From time to time I think if we could properly wire POSIX fadvise API to allow applications to control prefetch.

I wonder, do you have particular workloads where prefetcher is hurting? Can prefetcher be improved to handle them better?

[shodanshok]

From time to time I think if we could properly wire POSIX fadvise API to allow applications to control prefetch.

Yeah, adding fadvise would be great. However, for these (many) applications not using it, manual prefetch control is useful.

I wonder, do you have particular workloads where prefetcher is hurting? Can prefetcher be improved to handle them better?

To tell the truth, no, it always worked well for me (or at least not so bad I noticed). However, it seems others have issues with it (see #15214) and I think it would be useful to let prefetch be a dataset property, so to be selectively disabled on the specific datasets/workloads were it cause issue (leaving the other with prefetch enabled).

Any feeling on (eventually) removing the module tunable? Thanks.

[rincebrain]

My only thought would be wanting to tune metadata versus data prefetch versus none at all, but I have no workloads that this impacts, so I have no suggestions about how much it matters...

[shodanshok]

My only thought would be wanting to tune metadata versus data prefetch versus none at all, but I have no workloads that this impacts, so I have no suggestions about how much it matters...

Do you think something as prefetch = none | metadata | all would be preferable? Or metadata should always be prefetched, even if prefetch = off?

[amotin]

Tri-state switch would be more functional. In support of metadata prefetch, we are now prefetching indirect blocks for write, and I remember when I implemented it it did improve write speeds by not delaying them while trying to read the indirect from pool already busy by async writes.

Speaking about terminology though, ZFS has two independent types of prefetch: prescient and speculative, and the parameter control only the last, while may be interpreted differently. Setting it to metadata could be interpreted as prefetching indirect blocks for data accesses, or all metadata accesses including L0. Just a thoughts.

[shodanshok]

Tri-state switch would be more functional.

Ok, I'll try to implement it.

Speaking about terminology though, ZFS has two independent types of prefetch: prescient and speculative, and the parameter control only the last

Sure, prescient prefetch is always (rightfully) enabled and this patch (nor the module tunable) change that.

[amotin]

Tri-state switch would be more functional.

Ok, I'll try to implement it.

The prefetcher itself can do only indirects already, as I have mentioned it does it for writes. So it should be trivial.

[amotin]

Looks good to me. Just needs the style fix and documentation in zfsprops man page.

[shodanshok]

Looks good to me. Just needs the style fix and documentation in zfsprops man page.

Done.

[shodanshok]

I just corrected a minor style issue.

[ixhamza]

Since there has been a change in zfs_prop_t, the libzfs.abi needs to be updated to avoid the checkabi failure.

[shodanshok]

Since there has been a change in zfs_prop_t, the libzfs.abi needs to be updated to avoid the checkabi failure.

Done, thanks.

[shodanshok]

Anything I can/should do to get review approval for this PR? Thanks.

[shodanshok]

@behlendorf any chances to review this patch? Thanks.

[behlendorf]

@shodanshok thanks for your patience. If you can rebase this on master and update the PR it should be good to go.

[shodanshok]

@shodanshok thanks for your patience. If you can rebase this on master and update the PR it should be good to go.

I just synched my branch to master via github "sync" feature. Thank you so much for reviewing this patch.

[behlendorf]

The CI failures here are unexpected and look unrelated, but we hit them on multiple builders so clearly something isn't quite right. I see this PR wasn't created on its own branch, I've gone ahead and opened a new PR #15436 to get a fresh test run with the minimal patch cleanly applied to the master branch. Let's see how that goes.

[shodanshok]

The CI failures here are unexpected and look unrelated, but we hit them on multiple builders so clearly something isn't quite right.

I noticed the build failures but from their logs I did not spot anything obviously related to this patch.

I see this PR wasn't created on its own branch, I've gone ahead and opened a new PR #15436 to get a fresh test run with the minimal patch cleanly applied to the master branch. Let's see how that goes.

Did I do something non-optimal with the clone/sync from master? Can the build failures be related to that? Are patches better committed in dedicated branches? Sorry if these are basic questions, just trying to better understand for future commits.

And thanks for taking the time to create #15436

[behlendorf]

It looks like the CI failures we caused by 797f55e which I just reverted from master with #15438. Assuming that we get a good CI run in #15436 which doesn't include that commit we should be in good shape.

Are patches better committed in dedicated branches?

Yes. When opening a new PR it's generally best to make the change on its own branch. This makes it easy to git rebase -i master your changes on the latest commits in the master branch, and it keeps each change logically separate from anything else you might be working on locally. For future any PRs please go ahead and create them on their own branch.