Page MenuHomeFreeBSD

Use callout(9) instead of deprecated timeout(9).
AcceptedPublic

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

Details

Reviewers
cem
markj
Test Plan
  • compiled, not run-tested

Diff Detail

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

Event Timeline

jhb created this revision.Thu, Nov 28, 8:30 PM
cem added a comment.Fri, Nov 29, 2:49 AM

I think I'd rather just kill this path entirely. The FAIL_POINT_USE_TIMEOUT_PATH flag, the fail_point_use_timeout_path() inline function, and the } else { clause with the timeout(9) call above. *Nothing* in tree uses it. *Nothing* in ISLN's downstream fork uses it. It isn't clear what it's supposed to do and doesn't have any use, let's just get rid of it.

markj accepted this revision.Mon, Dec 2, 6:07 PM
markj added a subscriber: matthew.bryan_isilon.com.
In D22599#494263, @cem wrote:

I think I'd rather just kill this path entirely. The FAIL_POINT_USE_TIMEOUT_PATH flag, the fail_point_use_timeout_path() inline function, and the } else { clause with the timeout(9) call above. *Nothing* in tree uses it. *Nothing* in ISLN's downstream fork uses it. It isn't clear what it's supposed to do and doesn't have any use, let's just get rid of it.

I do remember that OneFS used fail_point_use_timeout_path() at some point, but obviously my knowledge is a bit out of date. :)

Maybe @matthew.bryan_isilon.com has an opinion on this? The change looks fine to me in any case.

This revision is now accepted and ready to land.Mon, Dec 2, 6:07 PM

I do remember that OneFS used fail_point_use_timeout_path() at some point, but obviously my knowledge is a bit out of date. :)
Maybe @matthew.bryan_isilon.com has an opinion on this? The change looks fine to me in any case.

Looks like ISLN does have some driver code using it via fail_point_use_timeout_path().
I suspect this path was originally made for dodging certain limitations on how you can sleep down in the driver code. I'd recommend tracking down the owners of the test code that leverages it to do some manual testing, if you haven't already.

cem added a comment.Mon, Dec 2, 8:18 PM

I must have missed that function or some other use-of-grep failure, my bad.

I think it may be more clear semantically to separate out "queue-with-delay" from "sleep" as a failpoint type, but that's orthogonal to removing timeout(9).

sys/sys/fail.h
88

struct callout is pretty big, and most failpoints won't use fail_point_use_timeout_path(). Could we allocate the callout from heap memory on demand instead?

jhb added inline comments.Tue, Dec 3, 9:01 PM
sys/sys/fail.h
88

Is it safe to call malloc in the places that call fail_point_use_timeout_path()? My assumption was that the desire was to place fail points almost anywhere and to call in situations where malloc() is not permitted. What if we added a 'fail_point_init_timeout()' that called fail_point_init() and then allocated a callout. This would require code that wanted to use the timeout path to use a separate init call, but would avoid dealing with allocation failures, races with concurrent fails, etc.

cem added inline comments.Tue, Dec 3, 10:11 PM
sys/sys/fail.h
88

I think it's safe. Every case I looked at briefly in OneFS seems like a sleepable location (we're allocating other stuff directly adjacent to the call, usually sysctls).

It seems to me like fail_point_use_timeout_path() is intended to be used once at initialization time for a failpoint, directly after fail_point_init() (that's how it is used in OneFS). So yeah, a fail_point_init_timeout() would be completely adequate, I think.

@matthew.bryan_isilon.com , does that sound reasonable?

sys/sys/fail.h
88

Yeah; the users of this code path have some init code for all the FPs and sysctls they use. It's the call/evaluation of the FP that needs to work in a non-sleepable context. So: malloc in some init is fine; we just can't do the init during evaluation.

That's speaking from the ISLN side btw. I don't know what users may be in the wild, or what their presumptions are.

jhb updated this revision to Diff 65467.Tue, Dec 10, 7:43 PM

Allocate callout lazily in fail_point_use_timeout_path.

This revision now requires review to proceed.Tue, Dec 10, 7:43 PM
cem added a comment.Tue, Dec 10, 8:20 PM

I don't think there's anything functionally wrong with the change at this point.

We talked about just having two init functions, fail_point_init and fail_point_init_use_timeout_path instead, and dropping fail_point_use_timeout_path() — did you decide against that?

sys/kern/kern_fail.c
498–511

I'd just MALLOC_DECLARE(M_FAIL_POINT); in sys/fail.h and inline this into fail_point_use_timeout_path(). Or move fail_point_use_timeout_path from sys/fail.h to kern_fail.c and inline this. Either way, I don't think there's a lot of reason to separate the two. I don't think the performance is sensitive so the header inline function isn't especially crucial, IMO.

I'd also switch the if to a KASSERT(fp->fp_callout == NULL). The API is a use-once-at-init thing.

jhb added a comment.Wed, Dec 11, 7:48 PM
In D22599#497934, @cem wrote:

I don't think there's anything functionally wrong with the change at this point.
We talked about just having two init functions, fail_point_init and fail_point_init_use_timeout_path instead, and dropping fail_point_use_timeout_path() — did you decide against that?

I had parsed your comments as saying to do the change in use_timeout_path instead of having a separate init. I started with a separate init actually, but then the API gets all wrong because all the KFAIL, etc. macros that wrap fail_point_init() would have to have separate variants. It seems simpler to just do the alloc in the existing function.

jhb added inline comments.Wed, Dec 11, 7:49 PM
sys/kern/kern_fail.c
498–511

I was just trying to be minimally invasive in the patch. :) I'd probably prefer moving fail_point_use_timeout_path() here. I added the alloc here mostly to try to match the existing style and use the fp_malloc() and fp_free() macros.

cem accepted this revision.Wed, Dec 11, 9:03 PM
In D22599#498303, @jhb wrote:
In D22599#497934, @cem wrote:

I don't think there's anything functionally wrong with the change at this point.
We talked about just having two init functions, fail_point_init and fail_point_init_use_timeout_path instead, and dropping fail_point_use_timeout_path() — did you decide against that?

I had parsed your comments as saying to do the change in use_timeout_path instead of having a separate init. I started with a separate init actually, but then the API gets all wrong because all the KFAIL, etc. macros that wrap fail_point_init() would have to have separate variants. It seems simpler to just do the alloc in the existing function.

Ah, my mistake. I don’t think the KFAIL macros have overlap with this use case but I might be mistaken. This is fine with me.

sys/kern/kern_fail.c
498–511

Ok :)

This revision is now accepted and ready to land.Wed, Dec 11, 9:03 PM