bz 276631 : GFS2: chmod hung - TRY 2
PATCH 1/1 bz 276631 : GFS2: chmod hung - TRY 2
Josef's right--my bad. Here is the corrected patch for 276631.
The problem boiled down to a race between the gdlm_init_threads() function initializing thread1 and its setting of blist = 1. Essentially, "if (current == ls->thread1)" was checked by the thread before the thread creator set ls->thread1.
Since thread1 is the only thread who is allowed to work on the blocking queue, and since neither thread thought it was thread1, no one was working on the queue. So everything just sat.
This patch reuses the ls->async_lock spin_lock to fix the race, and it fixes the problem. I've done more than 2000 iterations of the loop that was recreating the failure and it seems to work.
Dave Teigland brought up the question of whether we should do this another way. For example, by checking for the task name "lock_dlm1" instead. I'm open to opinions.
--
--
fs/gfs2/locking/dlm/thread.c +6 -0
@@ -279,8 +279,10 @@ static int gdlm_thread(void *data)/* Only thread1 is allowed to do blocking callbacks since gfs may wait for a completion callback within a blocking cb. */ +spin_lock (&ls->async_lock);if (current == ls->thread1) blist =1 ; +spin_unlock (&ls->async_lock);while (!kthread_should_stop ()) {set_current_state (TASK_INTERRUPTIBLE );
@@ -338,10 +340,12 @@ int gdlm_init_threads(struct gdlm_ls *lsstruct task_struct *p;int error; +spin_lock (&ls->async_lock); p =kthread_run (gdlm_thread, ls,"lock_dlm1" ); error =IS_ERR (p);if (error) {log_error ("can't start lock_dlm1 thread %d" , error); +spin_unlock (&ls->async_lock);return error; } ls->thread1 = p;
@@ -351,9 +355,11 @@ int gdlm_init_threads(struct gdlm_ls *lsif (error) {log_error ("can't start lock_dlm2 thread %d" , error);kthread_stop (ls->thread1); +spin_unlock (&ls->async_lock);return error; } ls->thread2 = p; +spin_unlock (&ls->async_lock);return 0 ; }
PATCH 1/1 bz 276631 : GFS2: chmod hung - TRY 2
Hi,
Now in the -nmw git tree. Thanks,
Steve.
Quoted 62 lines · Bob Peterson
Josef's right--my bad. Here is the corrected patch for 276631.
The problem boiled down to a race between the
gdlm_init_threads()function initializing thread1 and its setting of blist = 1. Essentially, "if (current ==ls->thread1)" was checked by the thread before the thread creator setls->thread1.Since thread1 is the only thread who is allowed to work on the blocking queue, and since neither thread thought it was thread1, no one was working on the queue. So everything just sat.
This patch reuses the
ls->async_lockspin_lockto fix the race, and it fixes the problem. I've done more than 2000 iterations of the loop that was recreating the failure and it seems to work.Dave Teigland brought up the question of whether we should do this another way. For example, by checking for the task name "
lock_dlm1" instead. I'm open to opinions.
--Signed-off-by Bob Peterson <[email protected]>--
diff -pura/fs/gfs2/locking/dlm/thread.cb/fs/gfs2/locking/dlm/thread.cfs/gfs2/locking/dlm/thread.c +6
-0@@ -279,8 +279,10 @@ static int gdlm_thread(void *data)/* Only thread1 is allowed to do blocking callbacks since gfs may wait for a completion callback within a blocking cb. */ +spin_lock (&ls->async_lock);if (current == ls->thread1) blist =1 ; +spin_unlock (&ls->async_lock);while (!kthread_should_stop ()) {set_current_state (TASK_INTERRUPTIBLE );@@ -338,10 +340,12 @@ int gdlm_init_threads(struct gdlm_ls *lsstruct task_struct *p;int error; +spin_lock (&ls->async_lock); p =kthread_run (gdlm_thread, ls,"lock_dlm1" ); error =IS_ERR (p);if (error) {log_error ("can't start lock_dlm1 thread %d" , error); +spin_unlock (&ls->async_lock);return error; } ls->thread1 = p;@@ -351,9 +355,11 @@ int gdlm_init_threads(struct gdlm_ls *lsif (error) {log_error ("can't start lock_dlm2 thread %d" , error);kthread_stop (ls->thread1); +spin_unlock (&ls->async_lock);return error; } ls->thread2 = p; +spin_unlock (&ls->async_lock);return 0 ; }
PATCH 1/1 bz 276631 : GFS2: chmod hung - TRY 2
In reply to Bob PetersonQuoted 28 lines · Bob Peterson
diff -pur
a/fs/gfs2/locking/dlm/thread.cb/fs/gfs2/locking/dlm/thread.cfs/gfs2/locking/dlm/thread.c +4
-0@@ -279,8 +279,10 @@ static int gdlm_thread(void *data)/* Only thread1 is allowed to do blocking callbacks since gfs may wait for a completion callback within a blocking cb. */ +spin_lock (&ls->async_lock);if (current == ls->thread1) blist =1 ; +spin_unlock (&ls->async_lock);while (!kthread_should_stop ()) {set_current_state (TASK_INTERRUPTIBLE );@@ -338,10 +340,12 @@ int gdlm_init_threads(struct gdlm_ls *lsstruct task_struct *p;int error; +spin_lock (&ls->async_lock); p =kthread_run (gdlm_thread, ls,"lock_dlm1" ); error =IS_ERR (p);if (error) {log_error ("can't start lock_dlm1 thread %d" , error); +spin_unlock (&ls->async_lock);return error; } ls->thread1 = p;
fs/gfs2/locking/dlm/thread.c int gdlm_init_threads(struct gdlm_ls *ls · line 362
Quoted 8 lines
fs/gfs2/locking/dlm/thread.c +2
-0@@ -351,9 +355,11 @@ int gdlm_init_threads(struct gdlm_ls *lsif (error) {log_error ("can't start lock_dlm2 thread %d" , error);kthread_stop (ls->thread1); +spin_unlock (&ls->async_lock);return error; } ls->thread2 = p; +spin_unlock (&ls->async_lock);
This is strange. First, it seems very likely to me that kthread_run could sleep, and almost certain that kthread_stop will sleep. Second, using a spinlock to signal a completion from one thread to another like this may be common with mutexes/completions, but not with spinlocks. I'd suggest one of two alternatives. Either have the thread check it's own name for "lock_dlm1", or add a new gdlm_thread1 function, i.e.
int _gdlm_thread(struct gdlm_ls *ls, int blist)
{
/* current function goes here */
}int gdlm_thread1(void *data)
{
return _gdlm_thread(data, 1);
}int gdlm_thread2(void *data)
{
return _gdlm_thread(data, 0);
}kthread_run(gdlm_thread1, ls, "lock_dlm1");kthread_run(gdlm_thread2, ls, "lock_dlm2");