* [Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock
@ 2015-03-30 3:22 alex chen
2015-03-31 22:13 ` Andrew Morton
2015-04-16 7:28 ` Junxiao Bi
0 siblings, 2 replies; 10+ messages in thread
From: alex chen @ 2015-03-30 3:22 UTC (permalink / raw)
To: ocfs2-devel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock
2015-03-30 3:22 [Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock alex chen
@ 2015-03-31 22:13 ` Andrew Morton
2015-04-01 0:43 ` Joseph Qi
2015-04-16 7:28 ` Junxiao Bi
1 sibling, 1 reply; 10+ messages in thread
From: Andrew Morton @ 2015-03-31 22:13 UTC (permalink / raw)
To: ocfs2-devel
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) { \
_
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock
2015-03-31 22:13 ` Andrew Morton
@ 2015-04-01 0:43 ` Joseph Qi
2015-04-01 2:19 ` Andrew Morton
0 siblings, 1 reply; 10+ messages in thread
From: Joseph Qi @ 2015-04-01 0:43 UTC (permalink / raw)
To: ocfs2-devel
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) { \
> _
>
>
> .
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock
2015-04-01 0:43 ` Joseph Qi
@ 2015-04-01 2:19 ` Andrew Morton
2015-04-02 3:14 ` alex chen
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Morton @ 2015-04-01 2:19 UTC (permalink / raw)
To: ocfs2-devel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock
2015-04-01 2:19 ` Andrew Morton
@ 2015-04-02 3:14 ` alex chen
2015-04-02 3:45 ` Andrew Morton
0 siblings, 1 reply; 10+ messages in thread
From: alex chen @ 2015-04-02 3:14 UTC (permalink / raw)
To: ocfs2-devel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock
2015-04-02 3:14 ` alex chen
@ 2015-04-02 3:45 ` Andrew Morton
0 siblings, 0 replies; 10+ messages in thread
From: Andrew Morton @ 2015-04-02 3:45 UTC (permalink / raw)
To: ocfs2-devel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock
2015-03-30 3:22 [Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock alex chen
2015-03-31 22:13 ` Andrew Morton
@ 2015-04-16 7:28 ` Junxiao Bi
2015-04-21 2:54 ` alex chen
1 sibling, 1 reply; 10+ messages in thread
From: Junxiao Bi @ 2015-04-16 7:28 UTC (permalink / raw)
To: ocfs2-devel
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)
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock
2015-04-16 7:28 ` Junxiao Bi
@ 2015-04-21 2:54 ` alex chen
2015-04-22 1:46 ` Junxiao Bi
0 siblings, 1 reply; 10+ messages in thread
From: alex chen @ 2015-04-21 2:54 UTC (permalink / raw)
To: ocfs2-devel
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)
>>
>
>
> .
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock
2015-04-21 2:54 ` alex chen
@ 2015-04-22 1:46 ` Junxiao Bi
2015-04-22 8:14 ` alex chen
0 siblings, 1 reply; 10+ messages in thread
From: Junxiao Bi @ 2015-04-22 1:46 UTC (permalink / raw)
To: ocfs2-devel
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)
>>>
>>
>>
>> .
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock
2015-04-22 1:46 ` Junxiao Bi
@ 2015-04-22 8:14 ` alex chen
0 siblings, 0 replies; 10+ messages in thread
From: alex chen @ 2015-04-22 8:14 UTC (permalink / raw)
To: ocfs2-devel
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)
>>>>
>>>
>>>
>>> .
>>>
>>
>
>
> .
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-04-22 8:14 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-30 3:22 [Ocfs2-devel] [PATCH] ocfs2: check if the ocfs2 lock resource be initialized before calling ocfs2_dlm_lock alex chen
2015-03-31 22:13 ` Andrew Morton
2015-04-01 0:43 ` Joseph Qi
2015-04-01 2:19 ` Andrew Morton
2015-04-02 3:14 ` alex chen
2015-04-02 3:45 ` Andrew Morton
2015-04-16 7:28 ` Junxiao Bi
2015-04-21 2:54 ` alex chen
2015-04-22 1:46 ` Junxiao Bi
2015-04-22 8:14 ` alex chen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.