SPL fixes #553

tuxoko · 2016-05-21T01:22:41Z

Restore CALLOUT_FLAG_ABSOLUTE in cv_timedwait_hires
Fix splat-condvar on Linux 4.7
Fix race in taskq_destroy and dynamic spawing

In 39cd90e, I mistakenly disabled the ability of using absolute expire time in cv_timedwait_hires. I don't quite sure why I did that, so let's restore it. Signed-off-by: Chunwei Chen <[email protected]>

Use schedule_timeout_interruptible instead of schedule for busy waiting. Otherwise, it will cause "BUG: workqueue lockup" on Linux 4.7 pre-rc. This should also reduce CPU usage in case we need to wait for a long time. Signed-off-by: Chunwei Chen <[email protected]> Fix openzfs#552

dweeezil · 2016-05-21T03:40:06Z

Looking at the taskq_destroy/spawning issue only: I'm wondering whether you really need tqt_is_dynamic. It would seem that taskq_destroy could only do the wait-for-nspawn==0 loop if TASKQ_DYNAMIC were set. Similarly the taskq_thread could only decrement nspawn if TASKQ_DYNAMIC is set although it ought to be harmless to decrement it no matter what since nspawn should only be looked at for a TASKQ_DYNAMIC taskq.

tuxoko · 2016-05-21T03:45:25Z

Well, taskq_create will create one thread even if it's TASKQ_DYNAMIC.

dweeezil · 2016-05-21T16:54:56Z

@tuxoko Interesting you should mention the single thread created for dynamic taskqs. There doesn't seem to be much of a need them to have a thread at all. What are your thoughts on 345f9be which changes things a few things in your patch? I've run the splat tests, ztest and a few hours (so far) of the test suite with it.

tuxoko · 2016-05-23T18:25:03Z

@dweeezil
Spawning one thread is probably and old relics before we spawn thread during taskq_dispatch.
I'm not sure about disable TASKQ_DYNAMIC when spl_taskq_thread_dynamic==0 though, we might want to be able to change that without reload the module. If you really dislike the tqt_is_dynamic, we can reset ns_spawn after wait_event(tq->tq_wait_waitq, tq->tq_nthreads == count);

tuxoko · 2016-05-23T21:21:00Z

@dweeezil
See if you like this one.
I also add another patch for taskq_wait_outstanding.

dweeezil · 2016-05-23T21:42:23Z

module/spl/spl-taskq.c

- tq->tq_nspawn--;
- spin_unlock_irqrestore(&tq->tq_lock, flags);
+ if (taskq_thread_create(tq) == NULL) {
+ /* restore spawning count if failed */


I wondered at first the context of this comment (when looking at the original patch) and thought "restore to what"? How about:

/* cancel the request to spawn if thread creation fails */

I don't see how that makes it more understandable. The creation is already failed, and we are not actively cancel anything.

The original comment was just fine once I thought about it for a moment.

dweeezil · 2016-05-23T21:45:12Z

@tuxoko LGTM.

While taskq_destroy would wait for dynamic_taskq to finish its tasks, but it does not implies the thread being spawned is up and running. This will cause taskq to be freed before the thread can exit. We fix this by using tq_nspawn to indicate how many threads are being spawned before they are inserted to the thread list. And have taskq_destroy to wait for it to drop to zero. Signed-off-by: Chunwei Chen <[email protected]> Fix openzfs#550

wait_event is a macro, so the current implementation will cause re-evaluation of tq_next_id every time it wakes up. This would cause taskq_wait_outstanding(tq, 0) to be equivalent to taskq_wait(tq) Signed-off-by: Chunwei Chen <[email protected]>

behlendorf · 2016-05-24T15:56:29Z

module/spl/spl-condvar.c

- /* get abs expire time */
- tim += gethrtime();
+ if (!(flag & CALLOUT_FLAG_ABSOLUTE))
+ tim += gethrtime();


I'm not sure how I missed this when I looked over 39cd90e but yes we definitely need to restore this.

behlendorf · 2016-05-24T16:19:29Z

@tuxoko these three LGTM.

e726590 Fix taskq_wait_outstanding re-evaluate tq_next_id
a815c53 Fix race between taskq_destroy and dynamic spawning thread
c1da2df Restore CALLOUT_FLAG_ABSOLUTE in cv_timedwait_hires

However, I'd like to hold off on this one until things settle down in the mainline kernel.

fc6269d Use schedule_timeout_interruptible for busy wait in splat-condvar.c

tuxoko · 2016-05-24T16:59:56Z

I'm fine with that.

In 39cd90e, I mistakenly disabled the ability of using absolute expire time in cv_timedwait_hires. I don't quite sure why I did that, so let's restore it. Signed-off-by: Chunwei Chen <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Signed-off-by: Tim Chase <[email protected]> Issue #553

While taskq_destroy would wait for dynamic_taskq to finish its tasks, but it does not implies the thread being spawned is up and running. This will cause taskq to be freed before the thread can exit. We fix this by using tq_nspawn to indicate how many threads are being spawned before they are inserted to the thread list. And have taskq_destroy to wait for it to drop to zero. Signed-off-by: Chunwei Chen <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Signed-off-by: Tim Chase <[email protected]> Issue #553 Closes #550

wait_event is a macro, so the current implementation will cause re- evaluation of tq_next_id every time it wakes up. This would cause taskq_wait_outstanding(tq, 0) to be equivalent to taskq_wait(tq) Signed-off-by: Chunwei Chen <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Signed-off-by: Tim Chase <[email protected]> Issue #553

behlendorf · 2016-05-24T20:03:37Z

Merged as:

b3a22a0 Fix taskq_wait_outstanding re-evaluate tq_next_id
5ce028b Fix race between taskq_destroy and dynamic spawning thread
872e0cc Restore CALLOUT_FLAG_ABSOLUTE in cv_timedwait_hires

wait_event is a macro, so the current implementation will cause re- evaluation of tq_next_id every time it wakes up. This would cause taskq_wait_outstanding(tq, 0) to be equivalent to taskq_wait(tq) Signed-off-by: Chunwei Chen <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Signed-off-by: Tim Chase <[email protected]> Issue openzfs#553

While taskq_destroy would wait for dynamic_taskq to finish its tasks, but it does not implies the thread being spawned is up and running. This will cause taskq to be freed before the thread can exit. We fix this by using tq_nspawn to indicate how many threads are being spawned before they are inserted to the thread list. And have taskq_destroy to wait for it to drop to zero. Signed-off-by: Chunwei Chen <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Signed-off-by: Tim Chase <[email protected]> Issue openzfs#553 Closes openzfs#550

wait_event is a macro, so the current implementation will cause re- evaluation of tq_next_id every time it wakes up. This would cause taskq_wait_outstanding(tq, 0) to be equivalent to taskq_wait(tq) Signed-off-by: Chunwei Chen <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Signed-off-by: Tim Chase <[email protected]> Issue openzfs#553

While taskq_destroy would wait for dynamic_taskq to finish its tasks, but it does not implies the thread being spawned is up and running. This will cause taskq to be freed before the thread can exit. We fix this by using tq_nspawn to indicate how many threads are being spawned before they are inserted to the thread list. And have taskq_destroy to wait for it to drop to zero. Signed-off-by: Chunwei Chen <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Signed-off-by: Tim Chase <[email protected]> Issue openzfs#553 Closes openzfs#550

Chunwei Chen added 2 commits May 20, 2016 16:35

Restore CALLOUT_FLAG_ABSOLUTE in cv_timedwait_hires

c1da2df

In 39cd90e, I mistakenly disabled the ability of using absolute expire time in cv_timedwait_hires. I don't quite sure why I did that, so let's restore it. Signed-off-by: Chunwei Chen <[email protected]>

tuxoko force-pushed the sss branch from ef86feb to e4082f9 Compare May 23, 2016 21:18

tuxoko force-pushed the sss branch from e4082f9 to 3def006 Compare May 23, 2016 21:37

dweeezil reviewed May 23, 2016
View reviewed changes

Chunwei Chen added 2 commits May 23, 2016 15:01

tuxoko force-pushed the sss branch from 3def006 to e726590 Compare May 23, 2016 22:02

behlendorf reviewed May 24, 2016
View reviewed changes

behlendorf closed this May 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SPL fixes #553

SPL fixes #553

tuxoko commented May 21, 2016

dweeezil commented May 21, 2016

tuxoko commented May 21, 2016

dweeezil commented May 21, 2016

tuxoko commented May 23, 2016

tuxoko commented May 23, 2016

dweeezil May 23, 2016

tuxoko May 23, 2016

dweeezil May 23, 2016

dweeezil commented May 23, 2016

behlendorf May 24, 2016

behlendorf commented May 24, 2016

tuxoko commented May 24, 2016

behlendorf commented May 24, 2016

SPL fixes #553

SPL fixes #553

Conversation

tuxoko commented May 21, 2016

dweeezil commented May 21, 2016

tuxoko commented May 21, 2016

dweeezil commented May 21, 2016

tuxoko commented May 23, 2016

tuxoko commented May 23, 2016

dweeezil May 23, 2016

Choose a reason for hiding this comment

tuxoko May 23, 2016

Choose a reason for hiding this comment

dweeezil May 23, 2016

Choose a reason for hiding this comment

dweeezil commented May 23, 2016

behlendorf May 24, 2016

Choose a reason for hiding this comment

behlendorf commented May 24, 2016

tuxoko commented May 24, 2016

behlendorf commented May 24, 2016