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

[jhb]

Test Plan

• compiled, not run-tested

[cem]

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]

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.

[matthew.bryan_isilon.com]

In D22599#495079, @markj wrote:

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]

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 ↗

(On Diff #65023)

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]

sys/sys/fail.h

88 ↗

(On Diff #65023)

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]

sys/sys/fail.h

88 ↗

(On Diff #65023)

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?

[matthew.bryan_isilon.com]

sys/sys/fail.h

88 ↗

(On Diff #65023)

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]

Allocate callout lazily in fail_point_use_timeout_path.

[cem]

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 ↗

(On Diff #65467)

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]

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]

sys/kern/kern_fail.c

498–511 ↗

(On Diff #65467)

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]

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 ↗

(On Diff #65467)

Ok :)