Add FreeBSD posix_fadvise support

[amotin]

This is a rebase/refresh of #13958 by @macdice.

As commit 320f0c6 did for Linux, connect POSIX_FADV_WILLNEED up to dmu_prefetch() on FreeBSD.

Signed-off-by: Thomas Munro [email protected]

Motivation and Context

See #13694 (née #9807) for motivation and context. The present PR is for feature parity on FreeBSD.

Description

Connects posix_fadvise() to dmu_prefetch().

Quite similar to Linux, but:

• ESPIPE case is already handled by kern_posix_fadvise() function before we get here.
• (offset, length) has already been converted to (start, end), and if length was 0 then end is OFF_MAX. Is it OK to call dmu_prefetch() with a very large size, or should we also try to clamp it to zp->z_size? It will be clamped by max prefetch size internally anyway.
• This is only a draft because I haven't yet figured out what, if anything, needs to be done about mapped files (I don't understand why for Linux something was done before dmu_prefetch() and not after it completes, and whether that is done for correctness or performance).

How Has This Been Tested?

I have tested on top of the main branch of FreeBSD, which most recently merged OpenZFS master a few days ago (openzfs/zfs commit c629f0b). I've tested with simple programs, and PostgreSQL which generates a WILLNEED advice in various circumstances.

I wonder if we should implement POSIX_FADV_DONTNEED to evict ranges from ARC to facilitate testing, but that'd be another PR.

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.

[robn]

I've been reading the Linux side a bit this evening, and I am almost certain that the call to generic_fadvise() is not for correctness. Maybe performance (though it looks tenuous), but not correctness.

First, generic_fadvise() wasn't available until Linux 5.4, so if it was for correctness, we're missing something there.

As for the behaviour, it seems the call does some error checks (which we also do), then gets into a switch on advice. For SEQUENTIAL, it just increases the "ideal amount of pages to readahead" (ra_pages), so it'll try and get more next time.

For WILLNEED, it calls force_page_cache_readahead(), which ends up calling zpl_readahead() to get pages in. But since we're already pulling those in, the best I think it can be is a sort of prefault - dmu_prefetch() will get stuff into the ARC, but the page readahead should also inflate it into the mapped pages, ready for use. How much does it help though, hard to say, because I guess they're going to be fighting with each other a little?

In any case, I don't think you're gonna break anything by leaving that shape out on the FreeBSD side.

[amotin]

@fuporovvStack Please take a look.

[behlendorf]

Looks good. I wondered about the call to generic_fadvise() myself, going back to the comments in the original issue it was added as a performance optimization to force populate the page cache. Which makes some sense.

If the file is mmaped, generic_fadvise is also called for page cache read-ahead besides dmu_prefetch.

So there is no correctness issue there.