Additional limits on hole reporting

[behlendorf]

Motivation and Context

Issue #14512

Description

Holding the zp->z_rangelock as a RL_READER over the range 0-UINT64_MAX is sufficient to prevent the dnode from being re-dirtied by concurrent writers. To avoid potentially looping multiple times for external caller which do not take the rangelock holes are not reported after the first sync. While not optimal this is always functionally correct.

This change adds the missing rangelock calls on FreeBSD to zvol_cdev_ioctl().

How Has This Been Tested?

Verified the fix behaves as expected using the test case in #14512.

The check in dnode_is_dirty() could probably be further refined. However, we need to be careful about making it too expensive. It's reasonable to allow a safe early abort in the uncommon case where a constantly modified file is being searched for holes.

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.

[rincebrain]

I'm not sure, thinking about it, why we can't just return holes that were there after at most one sync, rather than returning EBUSY?

We're not allowed to return holes that didn't exist, and returning no holes is definitely always safe, but there's not some atomic operation for "seek to hole/data and write/read thing there", so I don't think you get any promises about it still being there after the call returns, just that it was when you did it.

I could be missing something, of course, it just seems a shame to have to say "no holes at all" for remotely busy files, when I don't think you get any promises about how long the call is accurate for...

[behlendorf]

I think you could make a reasonable case for that behavior. Once the system call has returned all bets are off anyway regarding if that hole still exists. My feeling however is that in this case we want to play it as safe as possible. Just to make sure we avoid any other unexpected issues.

Ideally, I think what we'd want to do is not force the sync at all. But instead walk the dirty records for the file and apply them to the current on-disk blocks to determine what the file will look like when it finally hits disk. Though that of course comes with it's own set of challenges since we need to be able to make some assertions about how the I/O pipeline will handle those records.

[behlendorf]

I'm not sure, thinking about it, why we can't just return holes that were there after at most one sync, rather than returning EBUSY?

I've updated the patch to handle this a little bit better. By overextending the rangelock to the end of file we can properly lock out any process appending to the file and at worst require a single sync. However I refrained from adding an ASSERT for this because Lustre uses this interface without the rangelock and we don't want to break that.