[Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock

[Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock

[alex chen]

If ocfs2 lockres has not been initialized before calling ocfs2_dlm_lock,
the lock won't be dropped and then will lead umount hung. The case is
described below:

ocfs2_mknod
    ocfs2_mknod_locked
        __ocfs2_mknod_locked
            ocfs2_journal_access_di
            Failed because of -ENOMEM or other reasons, the inode lockres
            has not been initialized yet.

    iput(inode)
        ocfs2_evict_inode
            ocfs2_delete_inode
                ocfs2_inode_lock
                    ocfs2_inode_lock_full_nested
                        __ocfs2_cluster_lock
                        Succeeds and allocates a new dlm lockres.
            ocfs2_clear_inode
                ocfs2_open_unlock
                    ocfs2_drop_inode_locks
                        ocfs2_drop_lock
                        Since lockres has not been initialized, the lock
                        can't be dropped and the lockres can't be
                        migrated, thus umount will hang forever.

Signed-off-by: Alex Chen <alex.chen@huawei.com>
Reviewed-by: Joseph Qi <joseph.qi@huawei.com>
Reviewed-by: joyce.xue <xuejiufei@huawei.com>

---
 fs/ocfs2/dlmglue.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
index 11849a4..8b23aa2 100644
--- a/fs/ocfs2/dlmglue.c
+++ b/fs/ocfs2/dlmglue.c
@@ -1391,6 +1391,11 @@ static int __ocfs2_cluster_lock(struct ocfs2_super *osb,
 	int noqueue_attempted = 0;
 	int dlm_locked = 0;

+	if (!(lockres->l_flags & OCFS2_LOCK_INITIALIZED)) {
+		mlog_errno(-EINVAL);
+		return -EINVAL;
+	}
+
 	ocfs2_init_mask_waiter(&mw);

 	if (lockres->l_ops->flags & LOCK_TYPE_USES_LVB)
-- 
1.8.4.3

[Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock

[Andrew Morton]

On Mon, 30 Mar 2015 11:22:13 +0800 alex chen <alex.chen@huawei.com> wrote:

> --- a/fs/ocfs2/dlmglue.c
> +++ b/fs/ocfs2/dlmglue.c
> @@ -1391,6 +1391,11 @@ static int __ocfs2_cluster_lock(struct ocfs2_super *osb,
>  	int noqueue_attempted = 0;
>  	int dlm_locked = 0;
> 
> +	if (!(lockres->l_flags & OCFS2_LOCK_INITIALIZED)) {
> +		mlog_errno(-EINVAL);
> +		return -EINVAL;
> +	}

hm.  How about we do this?


From: Andrew Morton <akpm@linux-foundation.org>
Subject: ocfs2: make mlog_errno return the errno

ocfs2 does

	mlog_errno(v);
	return v;

in many places.  Change mlog_errno() so we can do

	return mlog_errno(v);

For some weird reason this patch reduces the size of ocfs2 by 6k:

akpm3:/usr/src/25> size fs/ocfs2/ocfs2.ko
   text    data     bss     dec     hex filename
1146613   82767  832192 2061572  1f7504 fs/ocfs2/ocfs2.ko-before
1140857   82767  832192 2055816  1f5e88 fs/ocfs2/ocfs2.ko-after

Cc: Mark Fasheh <mfasheh@suse.com>
Cc: Joel Becker <jlbec@evilplan.org>
Cc: alex chen <alex.chen@huawei.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/ocfs2/cluster/masklog.h |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff -puN fs/ocfs2/cluster/masklog.h~a fs/ocfs2/cluster/masklog.h
--- a/fs/ocfs2/cluster/masklog.h~a
+++ a/fs/ocfs2/cluster/masklog.h
@@ -196,13 +196,14 @@ extern struct mlog_bits mlog_and_bits, m
 	}								\
 } while (0)
 
-#define mlog_errno(st) do {						\
+#define mlog_errno(st) ({						\
 	int _st = (st);							\
 	if (_st != -ERESTARTSYS && _st != -EINTR &&			\
 	    _st != AOP_TRUNCATED_PAGE && _st != -ENOSPC &&		\
 	    _st != -EDQUOT)						\
 		mlog(ML_ERROR, "status = %lld\n", (long long)_st);	\
-} while (0)
+	st;								\
+})
 
 #define mlog_bug_on_msg(cond, fmt, args...) do {			\
 	if (cond) {							\
_

[Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock

[Joseph Qi]

Hi Andrew,

On 2015/4/1 6:13, Andrew Morton wrote:
> On Mon, 30 Mar 2015 11:22:13 +0800 alex chen <alex.chen@huawei.com> wrote:
> 
>> --- a/fs/ocfs2/dlmglue.c
>> +++ b/fs/ocfs2/dlmglue.c
>> @@ -1391,6 +1391,11 @@ static int __ocfs2_cluster_lock(struct ocfs2_super *osb,
>>  	int noqueue_attempted = 0;
>>  	int dlm_locked = 0;
>>
>> +	if (!(lockres->l_flags & OCFS2_LOCK_INITIALIZED)) {
>> +		mlog_errno(-EINVAL);
>> +		return -EINVAL;
>> +	}
> 
> hm.  How about we do this?
> 
> 
> From: Andrew Morton <akpm@linux-foundation.org>
> Subject: ocfs2: make mlog_errno return the errno
> 
> ocfs2 does
> 
> 	mlog_errno(v);
> 	return v;
> 
> in many places.  Change mlog_errno() so we can do
> 
> 	return mlog_errno(v);
> 
I don't think this is fit for all.
In many places it should do cleanup rather than just return the error
code.

> For some weird reason this patch reduces the size of ocfs2 by 6k:
> 
> akpm3:/usr/src/25> size fs/ocfs2/ocfs2.ko
>    text    data     bss     dec     hex filename
> 1146613   82767  832192 2061572  1f7504 fs/ocfs2/ocfs2.ko-before
> 1140857   82767  832192 2055816  1f5e88 fs/ocfs2/ocfs2.ko-after
> 
> Cc: Mark Fasheh <mfasheh@suse.com>
> Cc: Joel Becker <jlbec@evilplan.org>
> Cc: alex chen <alex.chen@huawei.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  fs/ocfs2/cluster/masklog.h |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff -puN fs/ocfs2/cluster/masklog.h~a fs/ocfs2/cluster/masklog.h
> --- a/fs/ocfs2/cluster/masklog.h~a
> +++ a/fs/ocfs2/cluster/masklog.h
> @@ -196,13 +196,14 @@ extern struct mlog_bits mlog_and_bits, m
>  	}								\
>  } while (0)
>  
> -#define mlog_errno(st) do {						\
> +#define mlog_errno(st) ({						\
>  	int _st = (st);							\
>  	if (_st != -ERESTARTSYS && _st != -EINTR &&			\
>  	    _st != AOP_TRUNCATED_PAGE && _st != -ENOSPC &&		\
>  	    _st != -EDQUOT)						\
>  		mlog(ML_ERROR, "status = %lld\n", (long long)_st);	\
> -} while (0)
> +	st;								\
> +})
>  
>  #define mlog_bug_on_msg(cond, fmt, args...) do {			\
>  	if (cond) {							\
> _
> 
> 
> .
> 

[Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock

[Andrew Morton]

On Wed, 1 Apr 2015 08:43:45 +0800 Joseph Qi <joseph.qi@huawei.com> wrote:

> > From: Andrew Morton <akpm@linux-foundation.org>
> > Subject: ocfs2: make mlog_errno return the errno
> > 
> > ocfs2 does
> > 
> > 	mlog_errno(v);
> > 	return v;
> > 
> > in many places.  Change mlog_errno() so we can do
> > 
> > 	return mlog_errno(v);
> > 
> I don't think this is fit for all.
> In many places it should do cleanup rather than just return the error
> code.

There are about 50 sites which can use this.

[Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock

[alex chen]

Hi Andrew,

On 2015/4/1 10:19, Andrew Morton wrote:
> On Wed, 1 Apr 2015 08:43:45 +0800 Joseph Qi <joseph.qi@huawei.com> wrote:
> 
>>> From: Andrew Morton <akpm@linux-foundation.org>
>>> Subject: ocfs2: make mlog_errno return the errno
>>>
>>> ocfs2 does
>>>
>>> 	mlog_errno(v);
>>> 	return v;
>>>
>>> in many places.  Change mlog_errno() so we can do
>>>
>>> 	return mlog_errno(v);
>>>
>> I don't think this is fit for all.
>> In many places it should do cleanup rather than just return the error
>> code.
> 
> There are about 50 sites which can use this.
> 

Can we define a new macro 'mlog_errno_return' as described below ?
In addition, ocfs2 does
	if (v)
		mlog_errno(v);
	return v;
in some places. In order to deal with this situation we can judge if
'st' is not equal to zero before printing log.

Thanks
Alex

---
 fs/ocfs2/cluster/masklog.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/ocfs2/cluster/masklog.h b/fs/ocfs2/cluster/masklog.h
index 2260fb9..269fef9 100644
--- a/fs/ocfs2/cluster/masklog.h
+++ b/fs/ocfs2/cluster/masklog.h
@@ -204,6 +204,15 @@ extern struct mlog_bits mlog_and_bits, mlog_not_bits;
 		mlog(ML_ERROR, "status = %lld\n", (long long)_st);	\
 } while (0)

+#define mlog_errno_return(st) ({					\
+	int _st = (st);							\
+	if (_st != 0 && _st != -ERESTARTSYS && _st != -EINTR &&		\
+	    _st != AOP_TRUNCATED_PAGE && _st != -ENOSPC &&		\
+	    _st != -EDQUOT)						\
+		mlog(ML_ERROR, "status = %lld\n", (long long)_st);	\
+	st;								\
+})
+
 #define mlog_bug_on_msg(cond, fmt, args...) do {			\
 	if (cond) {							\
 		mlog(ML_ERROR, "bug expression: " #cond "\n");		\
-- 
1.8.4.3

[Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock

[Andrew Morton]

On Thu, 2 Apr 2015 11:14:38 +0800 alex chen <alex.chen@huawei.com> wrote:

> >>>
> >> I don't think this is fit for all.
> >> In many places it should do cleanup rather than just return the error
> >> code.
> > 
> > There are about 50 sites which can use this.
> > 
> 
> Can we define a new macro 'mlog_errno_return' as described below ?
> In addition, ocfs2 does
> 	if (v)
> 		mlog_errno(v);
> 	return v;
> in some places. In order to deal with this situation we can judge if
> 'st' is not equal to zero before printing log.

Macros which hide control flow are evil, although in this case the
"return" in the name gives people info about what's happening.

But why bother?  This:

--- a/fs/ocfs2/cluster/masklog.h~ocfs2-make-mlog_errno-return-the-errno
+++ a/fs/ocfs2/cluster/masklog.h
@@ -196,13 +196,14 @@ extern struct mlog_bits mlog_and_bits, m
 	}								\
 } while (0)
 
-#define mlog_errno(st) do {						\
+#define mlog_errno(st) ({						\
 	int _st = (st);							\
 	if (_st != -ERESTARTSYS && _st != -EINTR &&			\
 	    _st != AOP_TRUNCATED_PAGE && _st != -ENOSPC &&		\
 	    _st != -EDQUOT)						\
 		mlog(ML_ERROR, "status = %lld\n", (long long)_st);	\
-} while (0)
+	st;								\
+})
 
 #define mlog_bug_on_msg(cond, fmt, args...) do {			\
 	if (cond) {							\


is clean, idiomatic and works great.  And with no other change it
shrinks the fs object code by 6k.



Gad, mlog_errno() is a *huge* source of bloat.  Look:

--- a/fs/ocfs2/cluster/masklog.h~a
+++ a/fs/ocfs2/cluster/masklog.h
@@ -183,27 +183,9 @@ extern struct mlog_bits mlog_and_bits, m
 	       task_pid_nr(current), __mlog_cpu_guess,			\
 	       __PRETTY_FUNCTION__, __LINE__ , ##args)
 
-#define mlog(mask, fmt, args...) do {					\
-	u64 __m = MLOG_MASK_PREFIX | (mask);				\
-	if ((__m & ML_ALLOWED_BITS) &&					\
-	    __mlog_test_u64(__m, mlog_and_bits) &&			\
-	    !__mlog_test_u64(__m, mlog_not_bits)) {			\
-		if (__m & ML_ERROR)					\
-			__mlog_printk(KERN_ERR, "ERROR: "fmt , ##args);	\
-		else if (__m & ML_NOTICE)				\
-			__mlog_printk(KERN_NOTICE, fmt , ##args);	\
-		else __mlog_printk(KERN_INFO, fmt , ##args);		\
-	}								\
-} while (0)
+#define mlog(mask, fmt, args...) do { } while (0)
 
-#define mlog_errno(st) ({						\
-	int _st = (st);							\
-	if (_st != -ERESTARTSYS && _st != -EINTR &&			\
-	    _st != AOP_TRUNCATED_PAGE && _st != -ENOSPC &&		\
-	    _st != -EDQUOT)						\
-		mlog(ML_ERROR, "status = %lld\n", (long long)_st);	\
-	st;								\
-})
+#define mlog_errno(st) do { } while (0)
 
 #define mlog_bug_on_msg(cond, fmt, args...) do {			\
 	if (cond) {							\


z:/usr/src/25> size fs/ocfs2/ocfs2.ko
   text    data     bss     dec     hex filename
1140849   82767  832192 2055808  1f5e80 fs/ocfs2/ocfs2.ko-before
 675402   82767  226104  984273   f04d1 fs/ocfs2/ocfs2.ko


It almost doubles the size of the object code!

Someone, please put this thing on a diet.  It doesn't all need to be
inlined.  We could just do

extern void __mlog_errno(int st);
#define mlog_errno(st) __mlog_errno(st, __LINE__)

and save hundreds of kilobytes of text.  It'll be faster too, due to the
reduced instruction cache footprint.

[Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock

[Junxiao Bi]

Hi Alex,

On 03/30/2015 11:22 AM, alex chen wrote:
> If ocfs2 lockres has not been initialized before calling ocfs2_dlm_lock,
> the lock won't be dropped and then will lead umount hung. The case is
> described below:
> 
> ocfs2_mknod
>     ocfs2_mknod_locked
>         __ocfs2_mknod_locked
>             ocfs2_journal_access_di
>             Failed because of -ENOMEM or other reasons, the inode lockres
>             has not been initialized yet.

If failed here, is OCFS2_I(inode)->ip_inode_lockres initialized?  If not
how can you break __ocfs2_cluster_lock with the following condition?

if (!(lockres->l_flags & OCFS2_LOCK_INITIALIZED))

Thanks,
Junxiao.

> 
>     iput(inode)
>         ocfs2_evict_inode
>             ocfs2_delete_inode
>                 ocfs2_inode_lock
>                     ocfs2_inode_lock_full_nested
>                         __ocfs2_cluster_lock
>                         Succeeds and allocates a new dlm lockres.
>             ocfs2_clear_inode
>                 ocfs2_open_unlock
>                     ocfs2_drop_inode_locks
>                         ocfs2_drop_lock
>                         Since lockres has not been initialized, the lock
>                         can't be dropped and the lockres can't be
>                         migrated, thus umount will hang forever.
> 
> Signed-off-by: Alex Chen <alex.chen@huawei.com>
> Reviewed-by: Joseph Qi <joseph.qi@huawei.com>
> Reviewed-by: joyce.xue <xuejiufei@huawei.com>
> 
> ---
>  fs/ocfs2/dlmglue.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
> index 11849a4..8b23aa2 100644
> --- a/fs/ocfs2/dlmglue.c
> +++ b/fs/ocfs2/dlmglue.c
> @@ -1391,6 +1391,11 @@ static int __ocfs2_cluster_lock(struct ocfs2_super *osb,
>  	int noqueue_attempted = 0;
>  	int dlm_locked = 0;
> 
> +	if (!(lockres->l_flags & OCFS2_LOCK_INITIALIZED)) {
> +		mlog_errno(-EINVAL);
> +		return -EINVAL;
> +	}
> +
>  	ocfs2_init_mask_waiter(&mw);
> 
>  	if (lockres->l_ops->flags & LOCK_TYPE_USES_LVB)
> 

[Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock

[alex chen]

Hi Junxiao,

On 2015/4/16 15:28, Junxiao Bi wrote:
> Hi Alex,
> 
> On 03/30/2015 11:22 AM, alex chen wrote:
>> If ocfs2 lockres has not been initialized before calling ocfs2_dlm_lock,
>> the lock won't be dropped and then will lead umount hung. The case is
>> described below:
>>
>> ocfs2_mknod
>>     ocfs2_mknod_locked
>>         __ocfs2_mknod_locked
>>             ocfs2_journal_access_di
>>             Failed because of -ENOMEM or other reasons, the inode lockres
>>             has not been initialized yet.
> 
> If failed here, is OCFS2_I(inode)->ip_inode_lockres initialized?  If not

The OCFS2_I(inode)->ip_inode_lockres is initialized as follows:
__ocfs2_mknod_locked
    ocfs2_populate_inode
        ocfs2_inode_lock_res_init
            ocfs2_lock_res_init_common
So if ocfs2_journal_access_di is failed, the ip_inode_lockres will not be
initialized.
In this situation, we should not allocate a new dlm lockres through calling
ocfs2_dlm_lock() in __ocfs2_cluster_lock(), otherwise it will lead umount
hung. So we need bread __ocfs2_cluster_lock() if the inode lockres is not
be initialized, that is the condition
(!(lockres->l_flags & OCFS2_LOCK_INITIALIZED)) is TRUE.

> how can you break __ocfs2_cluster_lock with the following condition?
> 
> if (!(lockres->l_flags & OCFS2_LOCK_INITIALIZED))
> 
> Thanks,
> Junxiao.
> 
>>
>>     iput(inode)
>>         ocfs2_evict_inode
>>             ocfs2_delete_inode
>>                 ocfs2_inode_lock
>>                     ocfs2_inode_lock_full_nested
>>                         __ocfs2_cluster_lock
>>                         Succeeds and allocates a new dlm lockres.
>>             ocfs2_clear_inode
>>                 ocfs2_open_unlock
>>                     ocfs2_drop_inode_locks
>>                         ocfs2_drop_lock
>>                         Since lockres has not been initialized, the lock
>>                         can't be dropped and the lockres can't be
>>                         migrated, thus umount will hang forever.
>>
>> Signed-off-by: Alex Chen <alex.chen@huawei.com>
>> Reviewed-by: Joseph Qi <joseph.qi@huawei.com>
>> Reviewed-by: joyce.xue <xuejiufei@huawei.com>
>>
>> ---
>>  fs/ocfs2/dlmglue.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
>> index 11849a4..8b23aa2 100644
>> --- a/fs/ocfs2/dlmglue.c
>> +++ b/fs/ocfs2/dlmglue.c
>> @@ -1391,6 +1391,11 @@ static int __ocfs2_cluster_lock(struct ocfs2_super *osb,
>>  	int noqueue_attempted = 0;
>>  	int dlm_locked = 0;
>>
>> +	if (!(lockres->l_flags & OCFS2_LOCK_INITIALIZED)) {
>> +		mlog_errno(-EINVAL);
>> +		return -EINVAL;
>> +	}
>> +
>>  	ocfs2_init_mask_waiter(&mw);
>>
>>  	if (lockres->l_ops->flags & LOCK_TYPE_USES_LVB)
>>
> 
> 
> .
> 

[Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock

[Junxiao Bi]

On 04/21/2015 10:54 AM, alex chen wrote:
> Hi Junxiao,
> 
> On 2015/4/16 15:28, Junxiao Bi wrote:
>> Hi Alex,
>>
>> On 03/30/2015 11:22 AM, alex chen wrote:
>>> If ocfs2 lockres has not been initialized before calling ocfs2_dlm_lock,
>>> the lock won't be dropped and then will lead umount hung. The case is
>>> described below:
>>>
>>> ocfs2_mknod
>>>     ocfs2_mknod_locked
>>>         __ocfs2_mknod_locked
>>>             ocfs2_journal_access_di
>>>             Failed because of -ENOMEM or other reasons, the inode lockres
>>>             has not been initialized yet.
>>
>> If failed here, is OCFS2_I(inode)->ip_inode_lockres initialized?  If not
> 
> The OCFS2_I(inode)->ip_inode_lockres is initialized as follows:
> __ocfs2_mknod_locked
>     ocfs2_populate_inode
>         ocfs2_inode_lock_res_init
>             ocfs2_lock_res_init_common
> So if ocfs2_journal_access_di is failed, the ip_inode_lockres will not be
> initialized.
> In this situation, we should not allocate a new dlm lockres through calling
> ocfs2_dlm_lock() in __ocfs2_cluster_lock(), otherwise it will lead umount
> hung. So we need bread __ocfs2_cluster_lock() if the inode lockres is not
> be initialized, that is the condition
> (!(lockres->l_flags & OCFS2_LOCK_INITIALIZED)) is TRUE.

Looks good. Just didn't remind that inode_info is init once when it is
allocated from "ocfs2_inode_cache". lockres->l_flags is init there.

Thanks,
Junxiao.

> 
>> how can you break __ocfs2_cluster_lock with the following condition?
>>
>> if (!(lockres->l_flags & OCFS2_LOCK_INITIALIZED))
>>
>> Thanks,
>> Junxiao.
>>
>>>
>>>     iput(inode)
>>>         ocfs2_evict_inode
>>>             ocfs2_delete_inode
>>>                 ocfs2_inode_lock
>>>                     ocfs2_inode_lock_full_nested
>>>                         __ocfs2_cluster_lock
>>>                         Succeeds and allocates a new dlm lockres.
>>>             ocfs2_clear_inode
>>>                 ocfs2_open_unlock
>>>                     ocfs2_drop_inode_locks
>>>                         ocfs2_drop_lock
>>>                         Since lockres has not been initialized, the lock
>>>                         can't be dropped and the lockres can't be
>>>                         migrated, thus umount will hang forever.
>>>
>>> Signed-off-by: Alex Chen <alex.chen@huawei.com>
>>> Reviewed-by: Joseph Qi <joseph.qi@huawei.com>
>>> Reviewed-by: joyce.xue <xuejiufei@huawei.com>
>>>
>>> ---
>>>  fs/ocfs2/dlmglue.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
>>> index 11849a4..8b23aa2 100644
>>> --- a/fs/ocfs2/dlmglue.c
>>> +++ b/fs/ocfs2/dlmglue.c
>>> @@ -1391,6 +1391,11 @@ static int __ocfs2_cluster_lock(struct ocfs2_super *osb,
>>>  	int noqueue_attempted = 0;
>>>  	int dlm_locked = 0;
>>>
>>> +	if (!(lockres->l_flags & OCFS2_LOCK_INITIALIZED)) {
>>> +		mlog_errno(-EINVAL);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>>  	ocfs2_init_mask_waiter(&mw);
>>>
>>>  	if (lockres->l_ops->flags & LOCK_TYPE_USES_LVB)
>>>
>>
>>
>> .
>>
> 

[Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock

[alex chen]

On 2015/4/22 9:46, Junxiao Bi wrote:
> On 04/21/2015 10:54 AM, alex chen wrote:
>> Hi Junxiao,
>>
>> On 2015/4/16 15:28, Junxiao Bi wrote:
>>> Hi Alex,
>>>
>>> On 03/30/2015 11:22 AM, alex chen wrote:
>>>> If ocfs2 lockres has not been initialized before calling ocfs2_dlm_lock,
>>>> the lock won't be dropped and then will lead umount hung. The case is
>>>> described below:
>>>>
>>>> ocfs2_mknod
>>>>     ocfs2_mknod_locked
>>>>         __ocfs2_mknod_locked
>>>>             ocfs2_journal_access_di
>>>>             Failed because of -ENOMEM or other reasons, the inode lockres
>>>>             has not been initialized yet.
>>>
>>> If failed here, is OCFS2_I(inode)->ip_inode_lockres initialized?  If not
>>
>> The OCFS2_I(inode)->ip_inode_lockres is initialized as follows:
>> __ocfs2_mknod_locked
>>     ocfs2_populate_inode
>>         ocfs2_inode_lock_res_init
>>             ocfs2_lock_res_init_common
>> So if ocfs2_journal_access_di is failed, the ip_inode_lockres will not be
>> initialized.
>> In this situation, we should not allocate a new dlm lockres through calling
>> ocfs2_dlm_lock() in __ocfs2_cluster_lock(), otherwise it will lead umount
>> hung. So we need bread __ocfs2_cluster_lock() if the inode lockres is not
>> be initialized, that is the condition
>> (!(lockres->l_flags & OCFS2_LOCK_INITIALIZED)) is TRUE.
> 
> Looks good. Just didn't remind that inode_info is init once when it is
> allocated from "ocfs2_inode_cache". lockres->l_flags is init there.
> 
> Thanks,
> Junxiao.
> 

Thank you for your remind. If the subject is "check if lockres->l_flags be
initialized to OCFS2_LOCK_INITIALIZED before calling ocfs2_dlm_lock" will
be better.

>>
>>> how can you break __ocfs2_cluster_lock with the following condition?
>>>
>>> if (!(lockres->l_flags & OCFS2_LOCK_INITIALIZED))
>>>
>>> Thanks,
>>> Junxiao.
>>>
>>>>
>>>>     iput(inode)
>>>>         ocfs2_evict_inode
>>>>             ocfs2_delete_inode
>>>>                 ocfs2_inode_lock
>>>>                     ocfs2_inode_lock_full_nested
>>>>                         __ocfs2_cluster_lock
>>>>                         Succeeds and allocates a new dlm lockres.
>>>>             ocfs2_clear_inode
>>>>                 ocfs2_open_unlock
>>>>                     ocfs2_drop_inode_locks
>>>>                         ocfs2_drop_lock
>>>>                         Since lockres has not been initialized, the lock
>>>>                         can't be dropped and the lockres can't be
>>>>                         migrated, thus umount will hang forever.
>>>>
>>>> Signed-off-by: Alex Chen <alex.chen@huawei.com>
>>>> Reviewed-by: Joseph Qi <joseph.qi@huawei.com>
>>>> Reviewed-by: joyce.xue <xuejiufei@huawei.com>
>>>>
>>>> ---
>>>>  fs/ocfs2/dlmglue.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
>>>> index 11849a4..8b23aa2 100644
>>>> --- a/fs/ocfs2/dlmglue.c
>>>> +++ b/fs/ocfs2/dlmglue.c
>>>> @@ -1391,6 +1391,11 @@ static int __ocfs2_cluster_lock(struct ocfs2_super *osb,
>>>>  	int noqueue_attempted = 0;
>>>>  	int dlm_locked = 0;
>>>>
>>>> +	if (!(lockres->l_flags & OCFS2_LOCK_INITIALIZED)) {
>>>> +		mlog_errno(-EINVAL);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>>  	ocfs2_init_mask_waiter(&mw);
>>>>
>>>>  	if (lockres->l_ops->flags & LOCK_TYPE_USES_LVB)
>>>>
>>>
>>>
>>> .
>>>
>>
> 
> 
> .
>