flush: don't report flush error when disabling flush support

[robn]

[Sponsors: Klara, Inc., Wasabi Technology, Inc.]

Motivation and Context

The first time a device returns ENOTSUP in repsonse to a flush request, we set vdev_nowritecache so we don't issue flushes in the future and instead just pretend the succeeded. However, we still return an error for the initial flush, even though we just decided such errors are meaningless!

Description

So, when setting vdev_nowritecache in response to a flush error, also reset the error code to assume success.

Along the way, it seems there's no good reason for vdev_disk & vdev_geom to explicitly detect no support for flush and set vdev_nowritecache; just letting the error through to zio_vdev_io_assess() will cause it all to fall out nicely. So remove those checks.

This patch was suggested by @amotin in #16314. I'm unlikely to continue with that PR in that form, but this patch is still good and will be wanted when we do stat paying attention to flush errors. No reason not to include it now and get it off my plate!

How Has This Been Tested?

Direct testing against various unpublished flush error-response prototypes. Hard to test in OpenZFS proper, since nothing responds to flush errors.

Successful ZTS run against Linux 6.1.

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.

[amotin]

Thanks for remembering it. I am OK about this, but I am not completely certain that unifying this in OS-independent code is the right direction. Different OS and vdev types might have different ways of reporting it. I worry that mapping of Linux EOPNOTSUPP into ENOTSUP is too much of a special case. Or we expect other cases to translate whatever they have into ENOTSUP? If so, then why do we also check for ENOTTY?

[robn]

Yeah, this is a reasonable point.

I've split this PR into two commits. The actual change as described is in the first commit, and could stand alone.

The second changes things to make ENOTSUP the only way to signal that flush is not supported. For Linux, we then explicitly check for EOPNOTSUPP and ENOTTY, and convert. I could drop ENOTTY, as it seems to go back to ancient Linux where it was implemented with an ioctl-like interface to the block device, but I haven't done the study to really confirm that. As for EOPNOTSUPP, it's kind of redundant, but you are right that they aren't necessarily the same, and although we mix them throughout the code, we can easily just say that this a first tiny step towards cleaning them all up.

Or I can just drop the commit :)

[amotin]

Looks good to me, but once we go that way, would be nice to also patch zfs_file_deallocate() on both FreeBSD and Linux to use ENOTSUP instead of EOPNOTSUP (I honestly don't know which of them is better, so would be fine either way), plus on FreeBSD we need to map ENODEV into ENOTSUP there too.

[robn]

@amotin agreed.

FWIW, POSIX describes ENOTSUP and EOPNOTSUPP as:

[ENOTSUP]
Not supported (may be the same value as [EOPNOTSUPP]).

[EOPNOTSUPP]
Operation not supported on socket (may be the same value as [ENOTSUP]).

So I prefer ENOTSUP as the more general option. Nothing will care in practice, but consistency is good.