Fix race in taskqueue_drain_timeout()

[• hselasky]

Summary

taskqueue_drain_timeout() fails to detect that the task restarted the timeout

[• hselasky]

Implement suggestions from @kib .

[• hselasky]

Fix style.

[kib]

sys/kern/subr_taskqueue.c

84

1<<1

304

Silently ignoring enqueue request and returning success is not reasonable KPI. IMO, an error must be returned there. Callers might require some adoption to the error.

[• hselasky]

Update as per @kib 's comments.

[bjk]

share/man/man9/taskqueue.9

227 ↗

(On Diff #20696)

s/Else/Otherwise,'

[• hselasky]

Address manpage comments.

[kib]

I think that this variant of the subr_taskqueue_enqueue_timeout is fine in isolation, i.e. if we ignore the callers.

Did you inspected all callers of the function in the head ? In fact, r306320 is what I mean when we discussed the issue elsewere, and I thnk that despite this change, all callers should be fixed to do the similar treatment to the 'stopped' flag as the mentioned revision.

[• hselasky]

@kib
Did you inspected all callers of the function in the head ?

There is currently only one place in the kernel where the return value of this function is checked and that is:

sys/fs/autofs/autofs.c:		error = taskqueue_enqueue_timeout(taskqueue_thread,
sys/fs/autofs/autofs.c-		    &ar->ar_task, autofs_timeout * hz);
sys/fs/autofs/autofs.c-		if (error != 0) {
sys/fs/autofs/autofs.c:			AUTOFS_WARN("taskqueue_enqueue_timeout() failed "
sys/fs/autofs/autofs.c-			    "with error %d", error);
sys/fs/autofs/autofs.c-		}

From what I can see, this patch will not cause a problem, because the place where the task is residing is a new allocated memory block, so taskqueue_enqueue_timeout() will always return zero.

FYI: r306320 doesn't fully fix the problem. We also need the reliable taskqueue drain.

[kib]

Ok.

trasz, note that return value from the taskqueue_enqueue_timeout is not an error now, and not quite an error after the patch in this revision is applied.

[• hselasky]

@kib
Do you think the setting of the DT_DRAIN_IN_PROGRESS flag should be in its own function?

--HPS

[kib]

In D8012#166287, @hselasky wrote:

Do you think the setting of the DT_DRAIN_IN_PROGRESS flag should be in its own function?

Do you mean, should you create two helper inline functions to set and clear the flag ? I do not think it is important since right now the functions are single-use.

[• hselasky]

In D8012#166290, @kib wrote:

In D8012#166287, @hselasky wrote:

Do you think the setting of the DT_DRAIN_IN_PROGRESS flag should be in its own function?

Do you mean, should you create two helper inline functions to set and clear the flag ? I do not think it is important since right now the functions are single-use.

Yes, I was just wondering if that would be useful in the case where you want to do flush instead of drain, like inserting a barrier. Right now there is no need for such a function.

I'll wait for @trasz to respond and then I'll push.

Thank you!

[trasz]

I don't think it'll break anything in autofs.

[• hselasky]

Fix inverted KASSERT() check.

[wblock]

head/share/man/man9/taskqueue.9

226 ↗

(On Diff #20805)

Passive -> active:

This function returns -1 if the queue is being drained.
Otherwise, the number of pending calls is returned.

[• hselasky]

@wblock
https://svnweb.freebsd.org/changeset/base/306634

[wblock]

In D8012#168259, @hselasky wrote:

@wblock
https://svnweb.freebsd.org/changeset/base/306634