zio: rename "ioctl" to "flush"; remove `zio_ioctl()` #16064

robn · 2024-04-05T03:16:49Z

Motivation and Context

ZIO_TYPE_IOCTL has always been confusingly-named, as its only used for device flush, and really couldn't be used for anything else anyway. The presence of zio_ioctl and zio_flush compounded this confusion.

I'd had low-key plans to fix it up someday maybe, but then @amotin raised it in #16061, so here we are.

Description

Renames ZIO_TYPE_IOCTL and ZIO_PIPELINE_IOCTL to ZIO_TYPE_FLUSH and ZIO_PIPELINE_FLUSH
Renames z_ioctl IO queue to z_flush
Removes now-unused io_cmd field from struct zio
Removes use of DKIOCFLUSHWRITECACHE, since we now know what all these requests are
Removes zio_ioctl(), folds its functionality into zio_flush()
Various related naming and comment updates

I've tried to maintain the userspace API, not that there's much of it involved.

The use of X as the flag for flush in zio_impl.h and zpool-events.8 is kinda dubious, but I don't have any obviously better choice, and given the number of drive-by fixes I've done to these two docs in the last year or so I doubt many people will notice or care anyway.

How Has This Been Tested?

Compiled on Linux and FreeBSD, basic sanity check run completed.

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.

include/os/linux/zfs/sys/trace_common.h

module/zfs/zio.c

include/sys/fs/zfs.h

module/os/freebsd/zfs/vdev_file.c

module/os/linux/zfs/vdev_disk.c

It only had one user, zio_flush(), and there are no other vdev ioctls anyway. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Signed-off-by: Rob Norris <rob.norris@klarasystems.com>

There's no other options, so we can just always assume its a flush. Includes some light refactoring where a switch statement was doing control flow that no longer works. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Signed-off-by: Rob Norris <rob.norris@klarasystems.com>

Without DKIOCFLUSHWRITECACHE, we no longer need the compat header. Note that we're keeping the userspace SPL compat header, which is used by libefi. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Signed-off-by: Rob Norris <rob.norris@klarasystems.com>

The only possible ioctl is a flush, and any other kind of meta-operation introduced in the future is likely to have different semantics (much like trim did). So, lets just call it what it is. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Signed-off-by: Rob Norris <rob.norris@klarasystems.com>

robn · 2024-04-10T06:23:39Z

module/os/freebsd/zfs/vdev_geom.c

@@ -1053,7 +1053,7 @@ vdev_geom_io_intr(struct bio *bp)
 	/*
 	 * We have to split bio freeing into two parts, because the ABD code
 	 * cannot be called in this context and vdev_op_io_done is not called


With this PR vdev_op_io_done is now called for ZIO_TYPE_FLUSH, so this comment is wrong. I didn't want to just remove it, because maybe its pointing to a cleanup opportunity if this is no longer necessary. But I also don't know enough right now to just make the change. I'll come back to it soon, but if a FreeBSDer wants to step in and propose a change, that'd be even better!

This context is indeed not sleepable. Primarily it means no waiting memory allocations, etc, which would not be a problem here. But it also means no sleepable locks, and on FreeBSD ZFS' mutex_enter() called, for example, by zfs_refcount_remove_many(), called by abd_return_buf() is mapped into sleepable sx_xlock(). Since vdev_op_io_done() is now called for ZIO_TYPE_FLUSH, I guess it should be easier to remove this special case here and make vdev_geom_io_done() do everything.

robn · 2024-04-10T06:24:31Z

Feedback addressed, rebased to master, and updated following #16061.

There's no other options, so we can just always assume its a flush. Includes some light refactoring where a switch statement was doing control flow that no longer works. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Signed-off-by: Rob Norris <rob.norris@klarasystems.com> Closes #16064

Without DKIOCFLUSHWRITECACHE, we no longer need the compat header. Note that we're keeping the userspace SPL compat header, which is used by libefi. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Signed-off-by: Rob Norris <rob.norris@klarasystems.com> Closes #16064

The only possible ioctl is a flush, and any other kind of meta-operation introduced in the future is likely to have different semantics (much like trim did). So, lets just call it what it is. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Signed-off-by: Rob Norris <rob.norris@klarasystems.com> Closes #16064

It only had one user, zio_flush(), and there are no other vdev ioctls anyway. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Signed-off-by: Rob Norris <rob.norris@klarasystems.com> Closes openzfs#16064

There's no other options, so we can just always assume its a flush. Includes some light refactoring where a switch statement was doing control flow that no longer works. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Signed-off-by: Rob Norris <rob.norris@klarasystems.com> Closes openzfs#16064

Without DKIOCFLUSHWRITECACHE, we no longer need the compat header. Note that we're keeping the userspace SPL compat header, which is used by libefi. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Signed-off-by: Rob Norris <rob.norris@klarasystems.com> Closes openzfs#16064

The only possible ioctl is a flush, and any other kind of meta-operation introduced in the future is likely to have different semantics (much like trim did). So, lets just call it what it is. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Signed-off-by: Rob Norris <rob.norris@klarasystems.com> Closes openzfs#16064

robn changed the title ~~zio: rename "ioctl" to "flush" and clean up~~ zio: rename "ioctl" to "flush"; remove zio_ioctl() Apr 5, 2024

robn mentioned this pull request Apr 5, 2024

zinject: inject device errors into ioctls #16061

Merged

13 tasks

amotin reviewed Apr 5, 2024

View reviewed changes

include/os/linux/zfs/sys/trace_common.h Show resolved Hide resolved

module/zfs/zio.c Outdated Show resolved Hide resolved

robn force-pushed the zio-ioctl-flush-rename branch 2 times, most recently from f5d0ad5 to 064edaf Compare April 6, 2024 01:55

amotin reviewed Apr 7, 2024

View reviewed changes

module/zfs/zio.c Outdated Show resolved Hide resolved

robn force-pushed the zio-ioctl-flush-rename branch from 064edaf to af00474 Compare April 7, 2024 01:32

amotin approved these changes Apr 7, 2024

View reviewed changes

behlendorf requested changes Apr 8, 2024

View reviewed changes

include/sys/fs/zfs.h Outdated Show resolved Hide resolved

module/os/freebsd/zfs/vdev_file.c Show resolved Hide resolved

module/os/linux/zfs/vdev_disk.c Outdated Show resolved Hide resolved

module/os/linux/zfs/vdev_disk.c Outdated Show resolved Hide resolved

behlendorf added the Status: Code Review Needed Ready for review and testing label Apr 8, 2024

robn added 4 commits April 10, 2024 16:10

zio: remove zio_ioctl()

f5f92da

It only had one user, zio_flush(), and there are no other vdev ioctls anyway. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Signed-off-by: Rob Norris <rob.norris@klarasystems.com>

robn force-pushed the zio-ioctl-flush-rename branch from af00474 to 070d554 Compare April 10, 2024 06:17

robn commented Apr 10, 2024

View reviewed changes

behlendorf approved these changes Apr 11, 2024

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Apr 11, 2024

behlendorf closed this in cac416f Apr 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zio: rename "ioctl" to "flush"; remove `zio_ioctl()` #16064

zio: rename "ioctl" to "flush"; remove `zio_ioctl()` #16064

robn commented Apr 5, 2024 •

edited

Loading

robn Apr 10, 2024

amotin Apr 10, 2024

robn commented Apr 10, 2024

zio: rename "ioctl" to "flush"; remove zio_ioctl() #16064

zio: rename "ioctl" to "flush"; remove zio_ioctl() #16064

Conversation

robn commented Apr 5, 2024 • edited Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

robn Apr 10, 2024

Choose a reason for hiding this comment

amotin Apr 10, 2024

Choose a reason for hiding this comment

robn commented Apr 10, 2024

zio: rename "ioctl" to "flush"; remove `zio_ioctl()` #16064

zio: rename "ioctl" to "flush"; remove `zio_ioctl()` #16064

robn commented Apr 5, 2024 •

edited

Loading