[Ocfs2-devel] [PATCH] ocfs2: Add lockdep annotations

[Joel Becker <Joel.Becker@oracle.com>]

On Tue, Jun 02, 2009 at 02:24:05PM +0200, Jan Kara wrote:
> Add lockdep support to OCFS2. The support also covers all of the cluster
> locks except for open locks, journal locks, and local quotafile locks. These
> are special because they are acquired for a node, not for a particular process
> and lockdep cannot deal with such type of locking.

	I like the idea of ocfs2 having lockdep on everything.

> -static void ocfs2_cluster_unlock(struct ocfs2_super *osb,
> -				 struct ocfs2_lock_res *lockres,
> -				 int level);
> +static void __ocfs2_cluster_unlock(struct ocfs2_super *osb,
> +				   struct ocfs2_lock_res *lockres,
> +				   int level, unsigned long ip);
> +static inline void ocfs2_cluster_unlock(struct ocfs2_super *osb,
> +					struct ocfs2_lock_res *lockres,
> +					int level)
> +{
> +	__ocfs2_cluster_unlock(osb, lockres, level, _RET_IP_);
> +}

	I don't know why 'ip' needs to be part of the function prototype
- every place you use it, you just pass _RET_IP_.

> @@ -1239,11 +1256,21 @@ static int ocfs2_wait_for_mask_interruptible(struct ocfs2_mask_waiter *mw,
>  	return ret;
>  }
>  
> -static int ocfs2_cluster_lock(struct ocfs2_super *osb,
> -			      struct ocfs2_lock_res *lockres,
> -			      int level,
> -			      u32 lkm_flags,
> -			      int arg_flags)
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> +static int __ocfs2_cluster_lock(struct ocfs2_super *osb,
> +				struct ocfs2_lock_res *lockres,
> +				int level,
> +				u32 lkm_flags,
> +				int arg_flags,
> +				int l_subclass,
> +				unsigned long ip)
> +#else
> +static int __ocfs2_cluster_lock(struct ocfs2_super *osb,
> +				struct ocfs2_lock_res *lockres,
> +				int level,
> +				u32 lkm_flags,
> +				int arg_flags)
> +#endif
>  {
>  	struct ocfs2_mask_waiter mw;
>  	int wait, catch_signals = !(osb->s_mount_opt & OCFS2_MOUNT_NOINTR);
> @@ -1386,13 +1413,45 @@ out:
>  	}
>  	ocfs2_update_lock_stats(lockres, level, &mw, ret);
>  
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> +	if (!ret && lockres->l_lockdep_map.key != NULL) {
> +		if (level == DLM_LOCK_PR)
> +			rwsem_acquire_read(&lockres->l_lockdep_map, l_subclass,
> +				!!(arg_flags & OCFS2_META_LOCK_NOQUEUE), ip);
> +		else
> +			rwsem_acquire(&lockres->l_lockdep_map, l_subclass,
> +				!!(arg_flags & OCFS2_META_LOCK_NOQUEUE), ip);
> +	}
> +#endif
>  	mlog_exit(ret);
>  	return ret;
>  }

	Here again you've wrapped an ifdef around whether 'ip' is in the
argument list or not.  Since it's always _RET_IP_, why not just leave it
out of the argument list and just pas _RET_IP_ to the rwsem_acquire
calls?  That also eliminates the ugly ifdefs and need for
__ocfs2_cluster_lock().

> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> +static void __ocfs2_cluster_unlock(struct ocfs2_super *osb,
> +				   struct ocfs2_lock_res *lockres,
> +				   int level,
> +				   unsigned long ip)
> +#else
> +static void __ocfs2_cluster_unlock(struct ocfs2_super *osb,
> +				   struct ocfs2_lock_res *lockres,
> +				   int level)
> +#endif
>  {
>  	unsigned long flags;
>  
> @@ -1401,6 +1460,10 @@ static void ocfs2_cluster_unlock(struct ocfs2_super *osb,
>  	ocfs2_dec_holders(lockres, level);
>  	ocfs2_downconvert_on_unlock(osb, lockres);
>  	spin_unlock_irqrestore(&lockres->l_lock, flags);
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> +	if (lockres->l_lockdep_map.key != NULL)
> +		rwsem_release(&lockres->l_lockdep_map, 1, ip);
> +#endif
>  	mlog_exit_void();
>  }

	Same here.

> @@ -2145,10 +2208,11 @@ static int ocfs2_assign_bh(struct inode *inode,
>   * returns < 0 error if the callback will never be called, otherwise
>   * the result of the lock will be communicated via the callback.
>   */
> -int ocfs2_inode_lock_full(struct inode *inode,
> -			 struct buffer_head **ret_bh,
> -			 int ex,
> -			 int arg_flags)
> +int ocfs2_inode_lock_full_nested(struct inode *inode,
> +				 struct buffer_head **ret_bh,
> +				 int ex,
> +				 int arg_flags,
> +				 int subclass)
>  {
>  	int status, level, acquired;
>  	u32 dlm_flags;
> @@ -2186,7 +2250,12 @@ int ocfs2_inode_lock_full(struct inode *inode,
>  	if (arg_flags & OCFS2_META_LOCK_NOQUEUE)
>  		dlm_flags |= DLM_LKF_NOQUEUE;
>  
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> +	status = __ocfs2_cluster_lock(osb, lockres, level, dlm_flags,
> +				      arg_flags, subclass, _RET_IP_);
> +#else
>  	status = ocfs2_cluster_lock(osb, lockres, level, dlm_flags, arg_flags);
> +#endif

	This hunk wouldn't be necessary either.

> --- a/fs/ocfs2/inode.c
> +++ b/fs/ocfs2/inode.c
> @@ -215,6 +215,8 @@ bail:
>  static int ocfs2_init_locked_inode(struct inode *inode, void *opaque)
>  {
>  	struct ocfs2_find_inode_args *args = opaque;
> +	static struct lock_class_key ocfs2_quota_ip_alloc_sem_key,
> +				     ocfs2_file_ip_alloc_sem_key;
>  
>  	mlog_entry("inode = %p, opaque = %p\n", inode, opaque);
>  
> @@ -223,6 +225,15 @@ static int ocfs2_init_locked_inode(struct inode *inode, void *opaque)
>  	if (args->fi_sysfile_type != 0)
>  		lockdep_set_class(&inode->i_mutex,
>  			&ocfs2_sysfile_lock_key[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 ||
> +	    args->fi_sysfile_type == LOCAL_GROUP_QUOTA_SYSTEM_INODE)
> +		lockdep_set_class(&OCFS2_I(inode)->ip_alloc_sem,
> +				  &ocfs2_quota_ip_alloc_sem_key);
> +	else
> +		lockdep_set_class(&OCFS2_I(inode)->ip_alloc_sem,
> +				  &ocfs2_file_ip_alloc_sem_key);

	These keys don't need to be intialized?

> diff --git a/fs/ocfs2/inode.h b/fs/ocfs2/inode.h
> index ea71525..7096516 100644
> --- a/fs/ocfs2/inode.h
> +++ b/fs/ocfs2/inode.h
> @@ -109,6 +109,14 @@ struct ocfs2_inode_info
>  /* Indicates that the metadata cache should be used as an array. */
>  #define OCFS2_INODE_CACHE_INLINE	0x00000080
>  
> +/* Locking subclasses of inode cluster lock */
> +enum {
> +	OI_LS_NORMAL,
> +	OI_LS_PARENT,
> +	OI_LS_RENAME1,
> +	OI_LS_RENAME2,
> +};
> +

	I couldn't see where OI_LS_NORMAL was ever used, until I 
realized that your #define of ocfs2_inode_lock_full hardcodes
OI_LS_NORMAL as '0'.  It should really use the subclass name, right?

Joel

-- 

"Senator let's be sincere,
 As much as you can."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127