1. Patchew
  2. linux
  3. View series

[PATCH] ocfs2: embed actual values into ocfs2_sysfile_lock_key names

Tetsuo Handa posted 1 patch 3 months, 2 weeks ago 
fs/ocfs2/inode.c | 70 +++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 66 insertions(+), 4 deletions(-)
[PATCH] ocfs2: embed actual values into ocfs2_sysfile_lock_key names
Posted by Tetsuo Handa 3 months, 2 weeks ago 
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
Re: [PATCH] ocfs2: embed actual values into ocfs2_sysfile_lock_key names
Posted by Joseph Qi 3 months, 1 week ago 

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;
>  }
Re: [PATCH] ocfs2: embed actual values into ocfs2_sysfile_lock_key names
Posted by Heming Zhao 3 months, 1 week ago 
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;
>>   }
> 
>
Re: [PATCH] ocfs2: embed actual values into ocfs2_sysfile_lock_key names
Posted by Tetsuo Handa 3 months, 1 week ago 
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.
Re: [PATCH] ocfs2: embed actual values into ocfs2_sysfile_lock_key names
Posted by Heming Zhao 3 months, 1 week ago 
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
Re: [PATCH] ocfs2: embed actual values into ocfs2_sysfile_lock_key names
Posted by Heming Zhao 3 months, 2 weeks ago 
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;
>   }
Re: [PATCH] ocfs2: embed actual values into ocfs2_sysfile_lock_key names
Posted by Tetsuo Handa 3 months, 2 weeks ago 
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
>>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.