ocfs2: set OCFS2_LOCK_BLOCKED properly

for most cases ocfs2_generic_handle_bast() return 1 meaning that some down- converting work should be done. but when it return 0, the OCFS2_LOCK_BLOCKED is set inproperly.
OCFS2_LOCK_BLOCKED is cleared in a down-convertion(dc) is done. if no dc is needed any more(done by another lock request queued before this one), OCFS2_LOCK_BLOCKED stays there unexpectly.

the fix is that setting that flag when dc(s) is really needed.

Signed-off-by Wengang Wang <[email protected]>

---
 fs/ocfs2/dlmglue.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

fs/ocfs2/dlmglue.c +3 -2

index c5e4a49..e18eadf 100644
@@ -907,8 +907,6 @@ static int ocfs2_generic_handle_bast(struct ocfs2_lock_res *lockres,
 
 	assert_spin_locked(&lockres->l_lock);
 
-	lockres_or_flags(lockres, OCFS2_LOCK_BLOCKED);
-
 	if (level > lockres->l_blocking) {
 		/* only schedule a downconvert if we haven't already scheduled
 		 * one that goes low enough to satisfy the level we're

@@ -921,6 +919,9 @@ static int ocfs2_generic_handle_bast(struct ocfs2_lock_res *lockres,
 		lockres->l_blocking = level;
 	}
 
+	if (needs_downconvert)
+		lockres_or_flags(lockres, OCFS2_LOCK_BLOCKED);
+		
 	mlog_exit(needs_downconvert);
 	return needs_downconvert;
 }

woah... this looks like a good catch.

Mark?

Quoted 8 lines · Wengang Wang

for most cases ocfs2_generic_handle_bast() return 1 meaning that some down- converting work should be done. but when it return 0, the OCFS2_LOCK_BLOCKED is set inproperly.
OCFS2_LOCK_BLOCKED is cleared in a down-convertion(dc) is done. if no dc is needed any more(done by another lock request queued before this one), OCFS2_LOCK_BLOCKED stays there unexpectly.

the fix is that setting that flag when dc(s) is really needed.

Quoted 3 lines · Sunil Mushran

woah... this looks like a good catch.

Mark?

Yeah, looks like it fixes an error. Keep in mind that the bast function can be called multiple times, and sometimes when we're in the middle of a downconvert (or shortly after one). The "set BLOCKED always" code looks like it was trying to insure we never miss a contended lock, but it could be that we marked some needlessly.

Can you point to a test case or bug # describing the downside of not having this patch :)

Quoted 3 lines · Sunil Mushran

woah... this looks like a good catch.

Mark?

Yeah, looks like it fixes an error. Keep in mind that the bast function can be called multiple times, and sometimes when we're in the middle of a downconvert (or shortly after one). The "set BLOCKED always" code looks like it was trying to insure we never miss a contended lock, but it could be that we marked some needlessly.

Quoted 9 lines

Quoted 8 lines · Wengang Wang

for most cases ocfs2_generic_handle_bast() return 1 meaning that some down- converting work should be done. but when it return 0, the OCFS2_LOCK_BLOCKED is set inproperly.
OCFS2_LOCK_BLOCKED is cleared in a down-convertion(dc) is done. if no dc is needed any more(done by another lock request queued before this one), OCFS2_LOCK_BLOCKED stays there unexpectly.

the fix is that setting that flag when dc(s) is really needed.

Wengang,

Can you point to a test case or bug # describing the downside of not having this patch :)

Thanks,

	--Mark

--
Mark Fasheh

sorry that I have no test case and it's not from a bug. I found it when reading code.

actually I found it as a side product while I was working on freeze/thaw. and the main bug/problem is described in my email titled "ocfs2: fix __ocfs2_cluster_lock() dead lock".