[Ocfs2-devel] [PATCH] ocfs2: set OCFS2_LOCK_BLOCKED properly

[Ocfs2-devel] [PATCH] ocfs2: set OCFS2_LOCK_BLOCKED properly

[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.

Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
---
 fs/ocfs2/dlmglue.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
index c5e4a49..e18eadf 100644
--- a/fs/ocfs2/dlmglue.c
+++ b/fs/ocfs2/dlmglue.c
@@ -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;
 }
-- 
1.6.2.5

[Ocfs2-devel] [PATCH] ocfs2: set OCFS2_LOCK_BLOCKED properly

[Sunil Mushran]

woah... this looks like a good catch.

Mark?

Wengang Wang wrote:
> 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 <wen.gang.wang@oracle.com>
> ---
>  fs/ocfs2/dlmglue.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
> index c5e4a49..e18eadf 100644
> --- a/fs/ocfs2/dlmglue.c
> +++ b/fs/ocfs2/dlmglue.c
> @@ -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;
>  }
>   

[Ocfs2-devel] [PATCH] ocfs2: set OCFS2_LOCK_BLOCKED properly

[Mark Fasheh]

On Tue, Jan 05, 2010 at 09:50:33AM -0800, Sunil Mushran wrote:
> 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.


> Wengang Wang wrote:
> > 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

[Ocfs2-devel] [PATCH] ocfs2: set OCFS2_LOCK_BLOCKED properly

[Joel Becker]

On Wed, Jan 06, 2010 at 05:03:19PM -0800, Mark Fasheh wrote:
> On Tue, Jan 05, 2010 at 09:50:33AM -0800, Sunil Mushran wrote:
> > 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.

	I'm trying to come up with the converse.  That is, why
wouldn't BLOCKED need to be set, or why wouldn't it be set even if
needed.
	If we're not yet blocking (l_blocking == NL), or we need to go
lower, we need to downconvert.  Gotta set BLOCKED if it wasn't set.  If
we're already scheduled to go to level, the downconvert must already be
queued.  It should have set BLOCKED.  If it's already complete, we have
nothing to block.  If we then set BLOCKED, we're in trouble.
	Yeah, this seems safe.  Sorry for blathering about it ;-)

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

	Definitely want to record the failure case.

Joel

-- 

"Not everything that can be counted counts, and not everything
 that counts can be counted."
        - Albert Einstein 

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Ocfs2-devel] [PATCH] ocfs2: set OCFS2_LOCK_BLOCKED properly

[Wengang Wang]

Hi Mark,

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".

regards,
wengang.
On 10-01-06 17:03, Mark Fasheh wrote:
> On Tue, Jan 05, 2010 at 09:50:33AM -0800, Sunil Mushran wrote:
> > 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.
> 
> 
> > Wengang Wang wrote:
> > > 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

[Ocfs2-devel] [PATCH] ocfs2: set OCFS2_LOCK_BLOCKED properly

[Mark Fasheh]

On Thu, Jan 07, 2010 at 10:10:06AM +0800, Wengang Wang wrote:
> sorry that I have no test case and it's not from a bug.
> I found it when reading code.

Ok, I forgot to ack the patch previously:

Acked-by: Mark Fasheh <mfasheh@suse.com>

> 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".

Ok, I'll look there.
	--Mark

--
Mark Fasheh