linux: zvols: correctly detect flush requests

[Fabian-Gruenbichler]

Motivation and Context

a user reported that since ZFS 2.2.7, sync writes from a VM backed by a zvol are no longer honored as sync writes by ZFS. a bisect identified the commit introducing the regression (see below).

Description

since 4.10, bio->bi_opf needs to be checked to determine all kinds of flush requests. this was the case prior to the commit referenced below, but the order of ifdefs was not the usual one (newest up top), which might have caused this to slip through.

this fixes a regression when using zvols as Qemu block devices, but might have broken other use cases as well. the symptoms are that all sync writes from within a VM configured to use such a virtual block devices are ignored and treated as async writes by the host ZFS layer.

this can be verified using fio in sync mode inside the VM, for example with

fio --filename=/dev/sda --ioengine=libaio --loops=1 --size=10G --time_based --runtime=60 --group_reporting --stonewall --name=cc1 --description="CC1" --rw=write --bs=4k --direct=1 --iodepth=1 --numjobs=1 --sync=1

which shows an IOPS number way above what the physical device underneath supports, with "zpool iostat -r 1" on the hypervisor side showing no sync IO occuring during the benchmark.

with the regression fixed, both fio inside the VM and the IO stats on the host show the expected numbers.

Fixes: 846b598

How Has This Been Tested?

see above - configured a test VM with a disk backed by a zvol, bisected using the fio commandline above and verified the fix in the same fashion after identifying the culprit,

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.

[Fabian-Gruenbichler]

it would be great if this could be backported to 2.3.x (and 2.2.x, in case new releases will be cut there as well) once reviewed and merged into master!

tagging @robn @tonyhutter

[amotin]

As I can see, modern kernel define both REQ_OP_FLUSH and REQ_PREFLUSH, so it is not obvious to me which one is right these days. May be checking both the same as before in HAVE_REQ_OP_FLUSH case would be safer? If nothing else, the comment above the function is inconsistent with the proposed content.

[Fabian-Gruenbichler]

yes, the comment already got broken by an earlier kernel compat bump..

I tested both the variant for 4.10+ (just checking PREFLUSH, as it ended up in the final commit) and the one before (checking either OP or the flag), both seem to work. the kernel's op_is_flush (https://github.com/torvalds/linux/blob/master/include/linux/blk_types.h#L445) checks the flag, but with an additional bit being required. I am not too well-versed in block-layer internals, so either the pre 4.10 variant or the 4.10+ variant seem fine to me, since the latter is a subset of the former, and this would effectively revert to the ZFS <= 2.2.6 behaviour instead of the 2.2.7/2.3.0/master definitely broken one ;)

[tonyhutter]

op_is_flush() is present on 4.18-6.13 kernels (ZFS's supported versions), so that might be the way to go. This at least builds for me on the 6.13 kernel (no idea if it fixes underlying issue):

diff --git a/include/os/linux/kernel/linux/blkdev_compat.h b/include/os/linux/kernel/linux/blkdev_compat.h
index d96708c60..8b040f4d6 100644
--- a/include/os/linux/kernel/linux/blkdev_compat.h
+++ b/include/os/linux/kernel/linux/blkdev_compat.h
@@ -383,7 +383,7 @@ bio_set_flush(struct bio *bio)
 static inline boolean_t
 bio_is_flush(struct bio *bio)
 {
-       return (bio_op(bio) == REQ_OP_FLUSH);
+       return (op_is_flush(bio->bi_opf));
 }
 
 /*

I can't seem to find the equivalent op_is_flush() macro for the blk-mq path, so I'm assuming the existing req_op(rq) == REQ_OP_FLUSH check is ok there.

[Fabian-Gruenbichler]

can confirm that op_is_flush also behaves as expected, so switched to that and added the previous REQ_OP_FLUSH check back in.

[tonyhutter]

@Fabian-Gruenbichler this looks good. If you squash the commits it should be ready to merge.

[Fabian-Gruenbichler]

done! left the co-authored-by that github added, the change is trivial either way :) thanks for the speedy review