BRT EOPNOTSUPP more speaking return value

[oromenahar]

return EOPNOTSUPP if the storage pool don't support block cloning.

Motivation and Context

To improve readability and let other programs know what the real error is.

Description

Just changed the return value, if the storage pool don't support BRT.

How Has This Been Tested?

Tested if it still compiles on rocky linux 9.
Tests and docs maybe missing, will check this later.

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.

[behlendorf]

This is more accurate, and it looks to me like the latest coreutils will handle EOPNOTSUPP correctly when it bubbles all the way up. That said, the copy_file_range() man page doesn't mention EOPNOTSUPP as a possible errno so I'd rather not change this until we really understand the consequences. At a minimum I see this change breaks the error handling here for FreeBSD which is a problem.

[oromenahar]

@behlendorf yes I saw that, I was a little fast when writing this PR. I will go and check more about this, when changing the return values. I saw one or two more checks, where we should change the return values to a more speaking value. So this should be put on hold for now until I checked everything.

First, this explicit return value wouldn't bubble up, in the BSD code this check happens before zfs_clone_range is ever called. In the Linux part is it the same. This codes would be good for other tools which wanna use zfs_clone_range in the future, for example zfs send/recv with --clone.

EDIT: Also wanna check this here again: EXDEV BSD Maybe it could miss some errors and just do a generic copy, which shouldn't be done on specific errors.
Rebased it to the Linux reflink merge.

[robn]

I'm softly ok with this as I think its the right return for zfs_clone_range (as the internal API). The FreeBSD change looks right.

Regarding copy_file_range: zfs_clone_range returning EXDEV is a signal that the caller has asked for something reasonable, but it can't be serviced by a clone, and should always then fall back to a content copy. For Linux we're actually some distance away from the copy_file_range system call; pre 5.3, the kernel will sort out the return code, and after that we do the fallback ourselves. Your FreeBSD change does the same thing. EXDEV mostly isn't a valid return for copy_file_range directly.

Reading code to check all this has caused me to notice a possible bug on Linux 4.15-5.2. It looks like only -ENOTSUPP will trigger the fallback, not -EXDEV, so I think it has to be converted there too. That's not for this PR though, just something I saw on the way. I'll work up a test on some ancient VM later today and send a separate PR if necessary.

[behlendorf]

Thanks for the quick revision with the FreeBSD fix. I think we're okay with integrating this now after you've wrapped up any additional testing.

[oromenahar]

So checked the code, there are still a lot of EXDEV return codes, but they are more or less okay. EXDEV is more used as an generic error in zfs_clone_range and less than a specific error. Sometimes it can cause confusion if some of the EXDEV errors ever reach the userspace. That's not really a big deal, some people say error is error but to keep this in mind is a good idea. Maybe I have time later to think about the return values. I already thought about which could be changed to other values and marked them on my local copy.

EDIT: Also wanna check this here again: EXDEV BSD Maybe it could miss some errors and just do a generic copy, which shouldn't be done on specific errors.

this is fine as well. On all if this cases we can fallback to a genric old style copy on BSD and than the return value would never be EXDEV.

I'm finished my tests and we can merge this into master if you like.

[behlendorf]

@oromenahar thanks, I'll go ahead and merge this.