Page MenuHomeFreeBSD

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

Authored by jhb on Thu, Nov 28, 8:22 PM.

Details

Test Plan
  • only compiled, not run-tested

Diff Detail

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

Event Timeline

jhb created this revision.Thu, Nov 28, 8:22 PM
mav added a reviewer: mmacy.Thu, Nov 28, 10:30 PM

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

avg accepted this revision.Mon, Dec 2, 11:10 AM

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.Mon, Dec 2, 11:10 AM
avg added inline comments.Mon, Dec 2, 11:15 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 :)

jhb updated this revision to Diff 65509.Wed, Dec 11, 10:17 PM
  • Change callout_drain to an assertion.
  • Add #ifdef guards to fix userland build.
This revision now requires review to proceed.Wed, Dec 11, 10:17 PM
avg accepted this revision.Wed, Dec 11, 10:18 PM
This revision is now accepted and ready to land.Wed, Dec 11, 10:18 PM
jhb marked an inline comment as done.Wed, Dec 11, 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.