* [PATCH] ocfs2: embed actual values into ocfs2_sysfile_lock_key names
@ 2025-06-23 14:54 Tetsuo Handa
2025-06-24 0:38 ` Heming Zhao
2025-06-30 2:21 ` Joseph Qi
0 siblings, 2 replies; 7+ messages in thread
From: Tetsuo Handa @ 2025-06-23 14:54 UTC (permalink / raw)
To: Mark Fasheh, Joel Becker, Joseph Qi, ocfs2-devel, LKML, Andrew Morton
Since lockdep_set_class() uses stringified key name via macro, calling
lockdep_set_class() with an array causes lockdep warning messages to
report variable name than actual index number.
Change ocfs2_init_locked_inode() to pass actual index number for better
readability of lockdep reports. This patch does not change behavior.
Before:
Chain exists of:
&ocfs2_sysfile_lock_key[args->fi_sysfile_type] --> jbd2_handle --> &oi->ip_xattr_sem
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&oi->ip_xattr_sem);
lock(jbd2_handle);
lock(&oi->ip_xattr_sem);
lock(&ocfs2_sysfile_lock_key[args->fi_sysfile_type]);
*** DEADLOCK ***
After:
Chain exists of:
&ocfs2_sysfile_lock_key[EXTENT_ALLOC_SYSTEM_INODE] --> jbd2_handle --> &oi->ip_xattr_sem
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&oi->ip_xattr_sem);
lock(jbd2_handle);
lock(&oi->ip_xattr_sem);
lock(&ocfs2_sysfile_lock_key[EXTENT_ALLOC_SYSTEM_INODE]);
*** DEADLOCK ***
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
fs/ocfs2/inode.c | 70 +++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 66 insertions(+), 4 deletions(-)
diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
index 12e5d1f73325..14bf440ea4df 100644
--- a/fs/ocfs2/inode.c
+++ b/fs/ocfs2/inode.c
@@ -50,8 +50,6 @@ struct ocfs2_find_inode_args
unsigned int fi_sysfile_type;
};
-static struct lock_class_key ocfs2_sysfile_lock_key[NUM_SYSTEM_INODES];
-
static int ocfs2_read_locked_inode(struct inode *inode,
struct ocfs2_find_inode_args *args);
static int ocfs2_init_locked_inode(struct inode *inode, void *opaque);
@@ -250,14 +248,77 @@ static int ocfs2_find_actor(struct inode *inode, void *opaque)
static int ocfs2_init_locked_inode(struct inode *inode, void *opaque)
{
struct ocfs2_find_inode_args *args = opaque;
+#ifdef CONFIG_LOCKDEP
+ static struct lock_class_key ocfs2_sysfile_lock_key[NUM_SYSTEM_INODES];
static struct lock_class_key ocfs2_quota_ip_alloc_sem_key,
ocfs2_file_ip_alloc_sem_key;
+#endif
inode->i_ino = args->fi_ino;
OCFS2_I(inode)->ip_blkno = args->fi_blkno;
- if (args->fi_sysfile_type != 0)
+#ifdef CONFIG_LOCKDEP
+ switch (args->fi_sysfile_type) {
+ case BAD_BLOCK_SYSTEM_INODE:
+ break;
+ case GLOBAL_INODE_ALLOC_SYSTEM_INODE:
+ lockdep_set_class(&inode->i_rwsem,
+ &ocfs2_sysfile_lock_key[GLOBAL_INODE_ALLOC_SYSTEM_INODE]);
+ break;
+ case SLOT_MAP_SYSTEM_INODE:
+ lockdep_set_class(&inode->i_rwsem,
+ &ocfs2_sysfile_lock_key[SLOT_MAP_SYSTEM_INODE]);
+ break;
+ case HEARTBEAT_SYSTEM_INODE:
+ lockdep_set_class(&inode->i_rwsem,
+ &ocfs2_sysfile_lock_key[HEARTBEAT_SYSTEM_INODE]);
+ break;
+ case GLOBAL_BITMAP_SYSTEM_INODE:
+ lockdep_set_class(&inode->i_rwsem,
+ &ocfs2_sysfile_lock_key[GLOBAL_BITMAP_SYSTEM_INODE]);
+ break;
+ case USER_QUOTA_SYSTEM_INODE:
+ lockdep_set_class(&inode->i_rwsem,
+ &ocfs2_sysfile_lock_key[USER_QUOTA_SYSTEM_INODE]);
+ break;
+ case GROUP_QUOTA_SYSTEM_INODE:
+ lockdep_set_class(&inode->i_rwsem,
+ &ocfs2_sysfile_lock_key[GROUP_QUOTA_SYSTEM_INODE]);
+ break;
+ case ORPHAN_DIR_SYSTEM_INODE:
+ lockdep_set_class(&inode->i_rwsem,
+ &ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE]);
+ break;
+ case EXTENT_ALLOC_SYSTEM_INODE:
lockdep_set_class(&inode->i_rwsem,
- &ocfs2_sysfile_lock_key[args->fi_sysfile_type]);
+ &ocfs2_sysfile_lock_key[EXTENT_ALLOC_SYSTEM_INODE]);
+ break;
+ case INODE_ALLOC_SYSTEM_INODE:
+ lockdep_set_class(&inode->i_rwsem,
+ &ocfs2_sysfile_lock_key[INODE_ALLOC_SYSTEM_INODE]);
+ break;
+ case JOURNAL_SYSTEM_INODE:
+ lockdep_set_class(&inode->i_rwsem,
+ &ocfs2_sysfile_lock_key[JOURNAL_SYSTEM_INODE]);
+ break;
+ case LOCAL_ALLOC_SYSTEM_INODE:
+ lockdep_set_class(&inode->i_rwsem,
+ &ocfs2_sysfile_lock_key[LOCAL_ALLOC_SYSTEM_INODE]);
+ break;
+ case TRUNCATE_LOG_SYSTEM_INODE:
+ lockdep_set_class(&inode->i_rwsem,
+ &ocfs2_sysfile_lock_key[TRUNCATE_LOG_SYSTEM_INODE]);
+ break;
+ case LOCAL_USER_QUOTA_SYSTEM_INODE:
+ lockdep_set_class(&inode->i_rwsem,
+ &ocfs2_sysfile_lock_key[LOCAL_USER_QUOTA_SYSTEM_INODE]);
+ break;
+ case LOCAL_GROUP_QUOTA_SYSTEM_INODE:
+ lockdep_set_class(&inode->i_rwsem,
+ &ocfs2_sysfile_lock_key[LOCAL_GROUP_QUOTA_SYSTEM_INODE]);
+ break;
+ default:
+ WARN_ONCE(1, "Unknown sysfile type %d\n", args->fi_sysfile_type);
+ }
if (args->fi_sysfile_type == USER_QUOTA_SYSTEM_INODE ||
args->fi_sysfile_type == GROUP_QUOTA_SYSTEM_INODE ||
args->fi_sysfile_type == LOCAL_USER_QUOTA_SYSTEM_INODE ||
@@ -267,6 +328,7 @@ static int ocfs2_init_locked_inode(struct inode *inode, void *opaque)
else
lockdep_set_class(&OCFS2_I(inode)->ip_alloc_sem,
&ocfs2_file_ip_alloc_sem_key);
+#endif
return 0;
}
--
2.49.0
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] ocfs2: embed actual values into ocfs2_sysfile_lock_key names
2025-06-23 14:54 [PATCH] ocfs2: embed actual values into ocfs2_sysfile_lock_key names Tetsuo Handa
@ 2025-06-24 0:38 ` Heming Zhao
2025-06-24 1:17 ` Tetsuo Handa
2025-06-30 2:21 ` Joseph Qi
1 sibling, 1 reply; 7+ messages in thread
From: Heming Zhao @ 2025-06-24 0:38 UTC (permalink / raw)
To: Tetsuo Handa, Mark Fasheh, Joel Becker, Joseph Qi, ocfs2-devel,
LKML, Andrew Morton
Hello,
Just from your code logic, the switch is unnecessary, and converting the input parameter into an "unsigned int" type is sufficient.
e.g.: (not test)
unsigned int type = args->fi_sysfile_type;
if (args->fi_sysfile_type != 0)
lockdep_set_class(&inode->i_rwsem,
&ocfs2_sysfile_lock_key[type]);
Thanks
- Heming
On 6/23/25 22:54, Tetsuo Handa wrote:
> Since lockdep_set_class() uses stringified key name via macro, calling
> lockdep_set_class() with an array causes lockdep warning messages to
> report variable name than actual index number.
>
> Change ocfs2_init_locked_inode() to pass actual index number for better
> readability of lockdep reports. This patch does not change behavior.
>
>
> Before:
>
> Chain exists of:
> &ocfs2_sysfile_lock_key[args->fi_sysfile_type] --> jbd2_handle --> &oi->ip_xattr_sem
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&oi->ip_xattr_sem);
> lock(jbd2_handle);
> lock(&oi->ip_xattr_sem);
> lock(&ocfs2_sysfile_lock_key[args->fi_sysfile_type]);
>
> *** DEADLOCK ***
>
> After:
>
> Chain exists of:
> &ocfs2_sysfile_lock_key[EXTENT_ALLOC_SYSTEM_INODE] --> jbd2_handle --> &oi->ip_xattr_sem
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&oi->ip_xattr_sem);
> lock(jbd2_handle);
> lock(&oi->ip_xattr_sem);
> lock(&ocfs2_sysfile_lock_key[EXTENT_ALLOC_SYSTEM_INODE]);
>
> *** DEADLOCK ***
>
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> fs/ocfs2/inode.c | 70 +++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 66 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
> index 12e5d1f73325..14bf440ea4df 100644
> --- a/fs/ocfs2/inode.c
> +++ b/fs/ocfs2/inode.c
> @@ -50,8 +50,6 @@ struct ocfs2_find_inode_args
> unsigned int fi_sysfile_type;
> };
>
> -static struct lock_class_key ocfs2_sysfile_lock_key[NUM_SYSTEM_INODES];
> -
> static int ocfs2_read_locked_inode(struct inode *inode,
> struct ocfs2_find_inode_args *args);
> static int ocfs2_init_locked_inode(struct inode *inode, void *opaque);
> @@ -250,14 +248,77 @@ static int ocfs2_find_actor(struct inode *inode, void *opaque)
> static int ocfs2_init_locked_inode(struct inode *inode, void *opaque)
> {
> struct ocfs2_find_inode_args *args = opaque;
> +#ifdef CONFIG_LOCKDEP
> + static struct lock_class_key ocfs2_sysfile_lock_key[NUM_SYSTEM_INODES];
> static struct lock_class_key ocfs2_quota_ip_alloc_sem_key,
> ocfs2_file_ip_alloc_sem_key;
> +#endif
>
> inode->i_ino = args->fi_ino;
> OCFS2_I(inode)->ip_blkno = args->fi_blkno;
> - if (args->fi_sysfile_type != 0)
> +#ifdef CONFIG_LOCKDEP
> + switch (args->fi_sysfile_type) {
> + case BAD_BLOCK_SYSTEM_INODE:
> + break;
> + case GLOBAL_INODE_ALLOC_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[GLOBAL_INODE_ALLOC_SYSTEM_INODE]);
> + break;
> + case SLOT_MAP_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[SLOT_MAP_SYSTEM_INODE]);
> + break;
> + case HEARTBEAT_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[HEARTBEAT_SYSTEM_INODE]);
> + break;
> + case GLOBAL_BITMAP_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[GLOBAL_BITMAP_SYSTEM_INODE]);
> + break;
> + case USER_QUOTA_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[USER_QUOTA_SYSTEM_INODE]);
> + break;
> + case GROUP_QUOTA_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[GROUP_QUOTA_SYSTEM_INODE]);
> + break;
> + case ORPHAN_DIR_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE]);
> + break;
> + case EXTENT_ALLOC_SYSTEM_INODE:
> lockdep_set_class(&inode->i_rwsem,
> - &ocfs2_sysfile_lock_key[args->fi_sysfile_type]);
> + &ocfs2_sysfile_lock_key[EXTENT_ALLOC_SYSTEM_INODE]);
> + break;
> + case INODE_ALLOC_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[INODE_ALLOC_SYSTEM_INODE]);
> + break;
> + case JOURNAL_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[JOURNAL_SYSTEM_INODE]);
> + break;
> + case LOCAL_ALLOC_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[LOCAL_ALLOC_SYSTEM_INODE]);
> + break;
> + case TRUNCATE_LOG_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[TRUNCATE_LOG_SYSTEM_INODE]);
> + break;
> + case LOCAL_USER_QUOTA_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[LOCAL_USER_QUOTA_SYSTEM_INODE]);
> + break;
> + case LOCAL_GROUP_QUOTA_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[LOCAL_GROUP_QUOTA_SYSTEM_INODE]);
> + break;
> + default:
> + WARN_ONCE(1, "Unknown sysfile type %d\n", args->fi_sysfile_type);
> + }
> if (args->fi_sysfile_type == USER_QUOTA_SYSTEM_INODE ||
> args->fi_sysfile_type == GROUP_QUOTA_SYSTEM_INODE ||
> args->fi_sysfile_type == LOCAL_USER_QUOTA_SYSTEM_INODE ||
> @@ -267,6 +328,7 @@ static int ocfs2_init_locked_inode(struct inode *inode, void *opaque)
> else
> lockdep_set_class(&OCFS2_I(inode)->ip_alloc_sem,
> &ocfs2_file_ip_alloc_sem_key);
> +#endif
>
> return 0;
> }
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] ocfs2: embed actual values into ocfs2_sysfile_lock_key names
2025-06-24 0:38 ` Heming Zhao
@ 2025-06-24 1:17 ` Tetsuo Handa
0 siblings, 0 replies; 7+ messages in thread
From: Tetsuo Handa @ 2025-06-24 1:17 UTC (permalink / raw)
To: Heming Zhao
Cc: Mark Fasheh, Joel Becker, Joseph Qi, ocfs2-devel, LKML, Andrew Morton
On 2025/06/24 9:38, Heming Zhao wrote:
> Hello,
>
> Just from your code logic, the switch is unnecessary, and converting
> the input parameter into an "unsigned int" type is sufficient.
>
> e.g.: (not test)
>
> unsigned int type = args->fi_sysfile_type;
>
> if (args->fi_sysfile_type != 0)
> lockdep_set_class(&inode->i_rwsem,
> &ocfs2_sysfile_lock_key[type]);
Excuse me, but you missed the point.
Your approach results in showing "type" instead of "args->fi_sysfile_type".
&ocfs2_sysfile_lock_key[type] --> jbd2_handle --> &oi->ip_xattr_sem
The point of this change is to show the actual value instead of variable name.
>> Before:
>>
>> &ocfs2_sysfile_lock_key[args->fi_sysfile_type] --> jbd2_handle --> &oi->ip_xattr_sem
>>
>> After:
>>
>> &ocfs2_sysfile_lock_key[EXTENT_ALLOC_SYSTEM_INODE] --> jbd2_handle --> &oi->ip_xattr_sem
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ocfs2: embed actual values into ocfs2_sysfile_lock_key names
2025-06-23 14:54 [PATCH] ocfs2: embed actual values into ocfs2_sysfile_lock_key names Tetsuo Handa
2025-06-24 0:38 ` Heming Zhao
@ 2025-06-30 2:21 ` Joseph Qi
2025-06-30 2:42 ` Heming Zhao
1 sibling, 1 reply; 7+ messages in thread
From: Joseph Qi @ 2025-06-30 2:21 UTC (permalink / raw)
To: Tetsuo Handa, Mark Fasheh, Joel Becker, ocfs2-devel, LKML, Andrew Morton
On 2025/6/23 22:54, Tetsuo Handa wrote:
> Since lockdep_set_class() uses stringified key name via macro, calling
> lockdep_set_class() with an array causes lockdep warning messages to
> report variable name than actual index number.
>
> Change ocfs2_init_locked_inode() to pass actual index number for better
> readability of lockdep reports. This patch does not change behavior.
>
>
> Before:
>
> Chain exists of:
> &ocfs2_sysfile_lock_key[args->fi_sysfile_type] --> jbd2_handle --> &oi->ip_xattr_sem
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&oi->ip_xattr_sem);
> lock(jbd2_handle);
> lock(&oi->ip_xattr_sem);
> lock(&ocfs2_sysfile_lock_key[args->fi_sysfile_type]);
>
> *** DEADLOCK ***
>
> After:
>
> Chain exists of:
> &ocfs2_sysfile_lock_key[EXTENT_ALLOC_SYSTEM_INODE] --> jbd2_handle --> &oi->ip_xattr_sem
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&oi->ip_xattr_sem);
> lock(jbd2_handle);
> lock(&oi->ip_xattr_sem);
> lock(&ocfs2_sysfile_lock_key[EXTENT_ALLOC_SYSTEM_INODE]);
>
> *** DEADLOCK ***
>
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Looks fine.
Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com>
> ---
> fs/ocfs2/inode.c | 70 +++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 66 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
> index 12e5d1f73325..14bf440ea4df 100644
> --- a/fs/ocfs2/inode.c
> +++ b/fs/ocfs2/inode.c
> @@ -50,8 +50,6 @@ struct ocfs2_find_inode_args
> unsigned int fi_sysfile_type;
> };
>
> -static struct lock_class_key ocfs2_sysfile_lock_key[NUM_SYSTEM_INODES];
> -
> static int ocfs2_read_locked_inode(struct inode *inode,
> struct ocfs2_find_inode_args *args);
> static int ocfs2_init_locked_inode(struct inode *inode, void *opaque);
> @@ -250,14 +248,77 @@ static int ocfs2_find_actor(struct inode *inode, void *opaque)
> static int ocfs2_init_locked_inode(struct inode *inode, void *opaque)
> {
> struct ocfs2_find_inode_args *args = opaque;
> +#ifdef CONFIG_LOCKDEP
> + static struct lock_class_key ocfs2_sysfile_lock_key[NUM_SYSTEM_INODES];
> static struct lock_class_key ocfs2_quota_ip_alloc_sem_key,
> ocfs2_file_ip_alloc_sem_key;
> +#endif
>
> inode->i_ino = args->fi_ino;
> OCFS2_I(inode)->ip_blkno = args->fi_blkno;
> - if (args->fi_sysfile_type != 0)
> +#ifdef CONFIG_LOCKDEP
> + switch (args->fi_sysfile_type) {
> + case BAD_BLOCK_SYSTEM_INODE:
> + break;
> + case GLOBAL_INODE_ALLOC_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[GLOBAL_INODE_ALLOC_SYSTEM_INODE]);
> + break;
> + case SLOT_MAP_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[SLOT_MAP_SYSTEM_INODE]);
> + break;
> + case HEARTBEAT_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[HEARTBEAT_SYSTEM_INODE]);
> + break;
> + case GLOBAL_BITMAP_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[GLOBAL_BITMAP_SYSTEM_INODE]);
> + break;
> + case USER_QUOTA_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[USER_QUOTA_SYSTEM_INODE]);
> + break;
> + case GROUP_QUOTA_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[GROUP_QUOTA_SYSTEM_INODE]);
> + break;
> + case ORPHAN_DIR_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE]);
> + break;
> + case EXTENT_ALLOC_SYSTEM_INODE:
> lockdep_set_class(&inode->i_rwsem,
> - &ocfs2_sysfile_lock_key[args->fi_sysfile_type]);
> + &ocfs2_sysfile_lock_key[EXTENT_ALLOC_SYSTEM_INODE]);
> + break;
> + case INODE_ALLOC_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[INODE_ALLOC_SYSTEM_INODE]);
> + break;
> + case JOURNAL_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[JOURNAL_SYSTEM_INODE]);
> + break;
> + case LOCAL_ALLOC_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[LOCAL_ALLOC_SYSTEM_INODE]);
> + break;
> + case TRUNCATE_LOG_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[TRUNCATE_LOG_SYSTEM_INODE]);
> + break;
> + case LOCAL_USER_QUOTA_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[LOCAL_USER_QUOTA_SYSTEM_INODE]);
> + break;
> + case LOCAL_GROUP_QUOTA_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[LOCAL_GROUP_QUOTA_SYSTEM_INODE]);
> + break;
> + default:
> + WARN_ONCE(1, "Unknown sysfile type %d\n", args->fi_sysfile_type);
> + }
> if (args->fi_sysfile_type == USER_QUOTA_SYSTEM_INODE ||
> args->fi_sysfile_type == GROUP_QUOTA_SYSTEM_INODE ||
> args->fi_sysfile_type == LOCAL_USER_QUOTA_SYSTEM_INODE ||
> @@ -267,6 +328,7 @@ static int ocfs2_init_locked_inode(struct inode *inode, void *opaque)
> else
> lockdep_set_class(&OCFS2_I(inode)->ip_alloc_sem,
> &ocfs2_file_ip_alloc_sem_key);
> +#endif
>
> return 0;
> }
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] ocfs2: embed actual values into ocfs2_sysfile_lock_key names
2025-06-30 2:21 ` Joseph Qi
@ 2025-06-30 2:42 ` Heming Zhao
2025-06-30 10:39 ` Tetsuo Handa
0 siblings, 1 reply; 7+ messages in thread
From: Heming Zhao @ 2025-06-30 2:42 UTC (permalink / raw)
To: Joseph Qi, Tetsuo Handa, Mark Fasheh, Joel Becker, ocfs2-devel,
LKML, Andrew Morton
I am not familiar with lockdep, and just have two questions regarding
the lockdep in ocfs2.
1>
there are three global "static struct lock_class_key" definitions:
- fs/ocfs2/inode.c : ocfs2_sysfile_lock_key[NUM_SYSTEM_INODES]
- fs/ocfs2/dlmglue.c: lockdep_keys[OCFS2_NUM_LOCK_TYPES]
- fs/ocfs2/sysfile.c: ocfs2_sysfile_cluster_lock_key[NUM_SYSTEM_INODES]
why did you env only trigger the ocfs2_sysfile_lock_key[] warning?
2>
It seems the existing CONFIG_DEBUG_LOCK_ALLOC is incorrect, it should be
replaced with CONFIG_LOCKDEP.
- Heming
On 6/30/25 10:21, Joseph Qi wrote:
>
>
> On 2025/6/23 22:54, Tetsuo Handa wrote:
>> Since lockdep_set_class() uses stringified key name via macro, calling
>> lockdep_set_class() with an array causes lockdep warning messages to
>> report variable name than actual index number.
>>
>> Change ocfs2_init_locked_inode() to pass actual index number for better
>> readability of lockdep reports. This patch does not change behavior.
>>
>>
>> Before:
>>
>> Chain exists of:
>> &ocfs2_sysfile_lock_key[args->fi_sysfile_type] --> jbd2_handle --> &oi->ip_xattr_sem
>>
>> Possible unsafe locking scenario:
>>
>> CPU0 CPU1
>> ---- ----
>> lock(&oi->ip_xattr_sem);
>> lock(jbd2_handle);
>> lock(&oi->ip_xattr_sem);
>> lock(&ocfs2_sysfile_lock_key[args->fi_sysfile_type]);
>>
>> *** DEADLOCK ***
>>
>> After:
>>
>> Chain exists of:
>> &ocfs2_sysfile_lock_key[EXTENT_ALLOC_SYSTEM_INODE] --> jbd2_handle --> &oi->ip_xattr_sem
>>
>> Possible unsafe locking scenario:
>>
>> CPU0 CPU1
>> ---- ----
>> lock(&oi->ip_xattr_sem);
>> lock(jbd2_handle);
>> lock(&oi->ip_xattr_sem);
>> lock(&ocfs2_sysfile_lock_key[EXTENT_ALLOC_SYSTEM_INODE]);
>>
>> *** DEADLOCK ***
>>
>>
>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>
> Looks fine.
>
> Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com>
>> ---
>> fs/ocfs2/inode.c | 70 +++++++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 66 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
>> index 12e5d1f73325..14bf440ea4df 100644
>> --- a/fs/ocfs2/inode.c
>> +++ b/fs/ocfs2/inode.c
>> @@ -50,8 +50,6 @@ struct ocfs2_find_inode_args
>> unsigned int fi_sysfile_type;
>> };
>>
>> -static struct lock_class_key ocfs2_sysfile_lock_key[NUM_SYSTEM_INODES];
>> -
>> static int ocfs2_read_locked_inode(struct inode *inode,
>> struct ocfs2_find_inode_args *args);
>> static int ocfs2_init_locked_inode(struct inode *inode, void *opaque);
>> @@ -250,14 +248,77 @@ static int ocfs2_find_actor(struct inode *inode, void *opaque)
>> static int ocfs2_init_locked_inode(struct inode *inode, void *opaque)
>> {
>> struct ocfs2_find_inode_args *args = opaque;
>> +#ifdef CONFIG_LOCKDEP
>> + static struct lock_class_key ocfs2_sysfile_lock_key[NUM_SYSTEM_INODES];
>> static struct lock_class_key ocfs2_quota_ip_alloc_sem_key,
>> ocfs2_file_ip_alloc_sem_key;
>> +#endif
>>
>> inode->i_ino = args->fi_ino;
>> OCFS2_I(inode)->ip_blkno = args->fi_blkno;
>> - if (args->fi_sysfile_type != 0)
>> +#ifdef CONFIG_LOCKDEP
>> + switch (args->fi_sysfile_type) {
>> + case BAD_BLOCK_SYSTEM_INODE:
>> + break;
>> + case GLOBAL_INODE_ALLOC_SYSTEM_INODE:
>> + lockdep_set_class(&inode->i_rwsem,
>> + &ocfs2_sysfile_lock_key[GLOBAL_INODE_ALLOC_SYSTEM_INODE]);
>> + break;
>> + case SLOT_MAP_SYSTEM_INODE:
>> + lockdep_set_class(&inode->i_rwsem,
>> + &ocfs2_sysfile_lock_key[SLOT_MAP_SYSTEM_INODE]);
>> + break;
>> + case HEARTBEAT_SYSTEM_INODE:
>> + lockdep_set_class(&inode->i_rwsem,
>> + &ocfs2_sysfile_lock_key[HEARTBEAT_SYSTEM_INODE]);
>> + break;
>> + case GLOBAL_BITMAP_SYSTEM_INODE:
>> + lockdep_set_class(&inode->i_rwsem,
>> + &ocfs2_sysfile_lock_key[GLOBAL_BITMAP_SYSTEM_INODE]);
>> + break;
>> + case USER_QUOTA_SYSTEM_INODE:
>> + lockdep_set_class(&inode->i_rwsem,
>> + &ocfs2_sysfile_lock_key[USER_QUOTA_SYSTEM_INODE]);
>> + break;
>> + case GROUP_QUOTA_SYSTEM_INODE:
>> + lockdep_set_class(&inode->i_rwsem,
>> + &ocfs2_sysfile_lock_key[GROUP_QUOTA_SYSTEM_INODE]);
>> + break;
>> + case ORPHAN_DIR_SYSTEM_INODE:
>> + lockdep_set_class(&inode->i_rwsem,
>> + &ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE]);
>> + break;
>> + case EXTENT_ALLOC_SYSTEM_INODE:
>> lockdep_set_class(&inode->i_rwsem,
>> - &ocfs2_sysfile_lock_key[args->fi_sysfile_type]);
>> + &ocfs2_sysfile_lock_key[EXTENT_ALLOC_SYSTEM_INODE]);
>> + break;
>> + case INODE_ALLOC_SYSTEM_INODE:
>> + lockdep_set_class(&inode->i_rwsem,
>> + &ocfs2_sysfile_lock_key[INODE_ALLOC_SYSTEM_INODE]);
>> + break;
>> + case JOURNAL_SYSTEM_INODE:
>> + lockdep_set_class(&inode->i_rwsem,
>> + &ocfs2_sysfile_lock_key[JOURNAL_SYSTEM_INODE]);
>> + break;
>> + case LOCAL_ALLOC_SYSTEM_INODE:
>> + lockdep_set_class(&inode->i_rwsem,
>> + &ocfs2_sysfile_lock_key[LOCAL_ALLOC_SYSTEM_INODE]);
>> + break;
>> + case TRUNCATE_LOG_SYSTEM_INODE:
>> + lockdep_set_class(&inode->i_rwsem,
>> + &ocfs2_sysfile_lock_key[TRUNCATE_LOG_SYSTEM_INODE]);
>> + break;
>> + case LOCAL_USER_QUOTA_SYSTEM_INODE:
>> + lockdep_set_class(&inode->i_rwsem,
>> + &ocfs2_sysfile_lock_key[LOCAL_USER_QUOTA_SYSTEM_INODE]);
>> + break;
>> + case LOCAL_GROUP_QUOTA_SYSTEM_INODE:
>> + lockdep_set_class(&inode->i_rwsem,
>> + &ocfs2_sysfile_lock_key[LOCAL_GROUP_QUOTA_SYSTEM_INODE]);
>> + break;
>> + default:
>> + WARN_ONCE(1, "Unknown sysfile type %d\n", args->fi_sysfile_type);
>> + }
>> if (args->fi_sysfile_type == USER_QUOTA_SYSTEM_INODE ||
>> args->fi_sysfile_type == GROUP_QUOTA_SYSTEM_INODE ||
>> args->fi_sysfile_type == LOCAL_USER_QUOTA_SYSTEM_INODE ||
>> @@ -267,6 +328,7 @@ static int ocfs2_init_locked_inode(struct inode *inode, void *opaque)
>> else
>> lockdep_set_class(&OCFS2_I(inode)->ip_alloc_sem,
>> &ocfs2_file_ip_alloc_sem_key);
>> +#endif
>>
>> return 0;
>> }
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] ocfs2: embed actual values into ocfs2_sysfile_lock_key names
2025-06-30 2:42 ` Heming Zhao
@ 2025-06-30 10:39 ` Tetsuo Handa
2025-06-30 13:55 ` Heming Zhao
0 siblings, 1 reply; 7+ messages in thread
From: Tetsuo Handa @ 2025-06-30 10:39 UTC (permalink / raw)
To: Heming Zhao
Cc: Joseph Qi, Mark Fasheh, Joel Becker, ocfs2-devel, LKML, Andrew Morton
On 2025/06/30 11:42, Heming Zhao wrote:
> I am not familiar with lockdep, and just have two questions regarding
> the lockdep in ocfs2.
>
> 1>
> there are three global "static struct lock_class_key" definitions:
> - fs/ocfs2/inode.c : ocfs2_sysfile_lock_key[NUM_SYSTEM_INODES]
> - fs/ocfs2/dlmglue.c: lockdep_keys[OCFS2_NUM_LOCK_TYPES]
> - fs/ocfs2/sysfile.c: ocfs2_sysfile_cluster_lock_key[NUM_SYSTEM_INODES]
>
> why did you env only trigger the ocfs2_sysfile_lock_key[] warning?
Because syzbot is reporting lockdep warning on ocfs2_sysfile_lock_key
at https://syzkaller.appspot.com/bug?extid=68c788938ba0326046a9 and
I couldn't find which lock_class_key syzbot is reporting.
Unless you want me to update all keys within this patch, you can submit
similar changes on lockdep_keys and ocfs2_sysfile_cluster_lock_key as
separate patches.
>
> 2>
> It seems the existing CONFIG_DEBUG_LOCK_ALLOC is incorrect, it should be
> replaced with CONFIG_LOCKDEP.
I couldn't catch what you mean. There are many modules which declare
"struct lock_class_key" under CONFIG_DEBUG_LOCK_ALLOC=y.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ocfs2: embed actual values into ocfs2_sysfile_lock_key names
2025-06-30 10:39 ` Tetsuo Handa
@ 2025-06-30 13:55 ` Heming Zhao
0 siblings, 0 replies; 7+ messages in thread
From: Heming Zhao @ 2025-06-30 13:55 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Joseph Qi, Mark Fasheh, Joel Becker, ocfs2-devel, LKML, Andrew Morton
On 6/30/25 18:39, Tetsuo Handa wrote:
> On 2025/06/30 11:42, Heming Zhao wrote:
>> I am not familiar with lockdep, and just have two questions regarding
>> the lockdep in ocfs2.
>>
>> 1>
>> there are three global "static struct lock_class_key" definitions:
>> - fs/ocfs2/inode.c : ocfs2_sysfile_lock_key[NUM_SYSTEM_INODES]
>> - fs/ocfs2/dlmglue.c: lockdep_keys[OCFS2_NUM_LOCK_TYPES]
>> - fs/ocfs2/sysfile.c: ocfs2_sysfile_cluster_lock_key[NUM_SYSTEM_INODES]
>>
>> why did you env only trigger the ocfs2_sysfile_lock_key[] warning?
>
> Because syzbot is reporting lockdep warning on ocfs2_sysfile_lock_key
> at https://syzkaller.appspot.com/bug?extid=68c788938ba0326046a9 and
> I couldn't find which lock_class_key syzbot is reporting.
>
> Unless you want me to update all keys within this patch, you can submit
> similar changes on lockdep_keys and ocfs2_sysfile_cluster_lock_key as
> separate patches.
>
>>
>> 2>
>> It seems the existing CONFIG_DEBUG_LOCK_ALLOC is incorrect, it should be
>> replaced with CONFIG_LOCKDEP.
>
> I couldn't catch what you mean. There are many modules which declare
> "struct lock_class_key" under CONFIG_DEBUG_LOCK_ALLOC=y.
>
I mean OCFS2 should use unified kernel configuration option.
- Heming
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-06-30 13:55 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-06-23 14:54 [PATCH] ocfs2: embed actual values into ocfs2_sysfile_lock_key names Tetsuo Handa
2025-06-24 0:38 ` Heming Zhao
2025-06-24 1:17 ` Tetsuo Handa
2025-06-30 2:21 ` Joseph Qi
2025-06-30 2:42 ` Heming Zhao
2025-06-30 10:39 ` Tetsuo Handa
2025-06-30 13:55 ` Heming Zhao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®