Page MenuHomeFreeBSD

Use a callout instead of timeout(9) for delayed zio's.
ClosedPublic

Authored by jhb on Nov 28 2019, 8:22 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 10:11 AM
Unknown Object (File)
Wed, Nov 20, 3:28 PM
Unknown Object (File)
Wed, Nov 20, 3:19 PM
Unknown Object (File)
Wed, Nov 20, 1:38 PM
Unknown Object (File)
Mon, Nov 18, 8:27 AM
Unknown Object (File)
Oct 14 2024, 11:13 AM
Unknown Object (File)
Oct 2 2024, 6:15 PM
Unknown Object (File)
Oct 2 2024, 3:06 PM

Details

Test Plan
  • only compiled, not run-tested

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 28085
Build 26230: arc lint + arc unit

Event Timeline

While I can't say anything bad about this change, I think we should better follow ZoL who added taskq_dispatch_delay() in addition to taskq_dispatch(), backed by their equivalent of taskqueue_enqueue_timeout(). In this particular case your patch is probably more efficient. but it is not a testing path, so should not care about extra context switch more then code divergence.

Just to chime in with what @mav said. Once FreeBSD is merged with the soon to be OpenZFS repo, the missing features (rename -u, FreeBSD's ashift mechanism, etc) have been upstreamed and the port has been given a few months to soak - the current code in tree - based as it is on a near dead upstream - is going away.

Take a look at the current working branch:
https://github.com/zfsonfreebsd/ZoF/tree/projects/pr-rebase-20191128/module

The change looks good to me.
Regarding other topics mentioned, I do not think that the code in stable branches is going away soon.
Also, while I am hopeful that the ZoF code lands before FreeBSD 13, I do not see any harm in improving the current ZFS code before that.
And having plan B is always nice.

This revision is now accepted and ready to land.Dec 2 2019, 11:10 AM
sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio.c
733

Mmm, actually I think that we should assert here that the callout is not pending. That is, it was never scheduled or has already completed. There should be no way to get here if the zio has not finished its pipeline.

glebius added inline comments.
sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio.c
663

This needs to go under #ifdef _KERNEL, just like body of zio_delay_interrupt(), since the file is also compiled in userland.

I've done the same patch couple days ago :)

  • Change callout_drain to an assertion.
  • Add #ifdef guards to fix userland build.
This revision now requires review to proceed.Dec 11 2019, 10:17 PM
This revision is now accepted and ready to land.Dec 11 2019, 10:18 PM
jhb marked an inline comment as done.Dec 11 2019, 10:23 PM

For the purposes of retiring the old timeout_t, I'd rather use a smaller diff. I looked at the ZoF tree, but there's no instance of 'taskq_dispatch_delay' in zfs/zio.c (which I assumed was the same as this file), so I didn't see a way to extract a simple diff that would apply easily to the current ZFS code.

This revision was automatically updated to reflect the committed changes.