xfs
[Top] [All Lists]

Re: [PATCH 2/3] xfs: fix directory inode iolock lockdep false positive

To: Dave Chinner <david@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
Subject: Re: [PATCH 2/3] xfs: fix directory inode iolock lockdep false positive
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Wed, 19 Feb 2014 13:25:16 -0500
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1392783402-4726-3-git-send-email-david@xxxxxxxxxxxxx>
References: <1392783402-4726-1-git-send-email-david@xxxxxxxxxxxxx> <1392783402-4726-3-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0
On 02/18/2014 11:16 PM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> The change to add the IO lock to protect the directory extent map
> during readdir operations has cause lockdep to have a heart attack
> as it now sees a different locking order on inodes w.r.t. the
> mmap_sem because readdir has a different ordering to write().
> 
> Add a new lockdep class for directory inodes to avoid this false
> positive.
> 
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---

Hey Dave,

I'm not terribly familiar with lockdep, but I hit the attached "possible
circular locking dependency detected" warning when running with this patch.

(Reproduces by running generic/001 after a reboot).

Brian

>  fs/xfs/xfs_iops.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 9ddfb81..bb3bb65 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -48,6 +48,18 @@
>  #include <linux/fiemap.h>
>  #include <linux/slab.h>
>  
> +/*
> + * Directories have different lock order w.r.t. mmap_sem compared to regular
> + * files. This is due to readdir potentially triggering page faults on a user
> + * buffer inside filldir(), and this happens with the ilock on the directory
> + * held. For regular files, the lock order is the other way around - the
> + * mmap_sem is taken during the page fault, and then we lock the ilock to do
> + * block mapping. Hence we need a different class for the directory ilock so
> + * that lockdep can tell them apart.
> + */
> +static struct lock_class_key xfs_nondir_ilock_class;
> +static struct lock_class_key xfs_dir_ilock_class;
> +
>  static int
>  xfs_initxattrs(
>       struct inode            *inode,
> @@ -1191,6 +1203,7 @@ xfs_setup_inode(
>       xfs_diflags_to_iflags(inode, ip);
>  
>       ip->d_ops = ip->i_mount->m_nondir_inode_ops;
> +     lockdep_set_class(&ip->i_lock.mr_lock, &xfs_nondir_ilock_class);
>       switch (inode->i_mode & S_IFMT) {
>       case S_IFREG:
>               inode->i_op = &xfs_inode_operations;
> @@ -1198,6 +1211,7 @@ xfs_setup_inode(
>               inode->i_mapping->a_ops = &xfs_address_space_operations;
>               break;
>       case S_IFDIR:
> +             lockdep_set_class(&ip->i_lock.mr_lock, &xfs_dir_ilock_class);
>               if (xfs_sb_version_hasasciici(&XFS_M(inode->i_sb)->m_sb))
>                       inode->i_op = &xfs_dir_ci_inode_operations;
>               else
> 


Attachment: messages.lockdep
Description: Text document

<Prev in Thread] Current Thread [Next in Thread>