[Ocfs2-devel] [PATCH] ocfs2: Add lockdep annotations
Joel Becker
Joel.Becker at oracle.com
Wed Jun 3 09:31:56 PDT 2009
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
More information about the Ocfs2-devel
mailing list