Re: [PATCH] ocfs2: embed actual values into ocfs2_sysfile_lock_key names

[Heming Zhao <heming.zhao@suse.com>]

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;
>   }