xfs
[Top] [All Lists]

Re: [PATCH] xfs: unlock i_mutex in xfs_break_layouts

To: Christoph Hellwig <hch@xxxxxx>
Subject: Re: [PATCH] xfs: unlock i_mutex in xfs_break_layouts
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 8 Apr 2015 08:19:27 +1000
Cc: xfs@xxxxxxxxxxx, linux-nfs@xxxxxxxxxxxxxxx, viro@xxxxxxxxxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1428420944-20965-1-git-send-email-hch@xxxxxx>
References: <1428420944-20965-1-git-send-email-hch@xxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Apr 07, 2015 at 05:35:44PM +0200, Christoph Hellwig wrote:
> We want to drop all I/O path locks when recalling layouts, and that includes
> i_mutex for the write path.  Without this we get stuck processe when recalls
> take too long.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
.....
>               xfs_iunlock(ip, iolock);
> diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
> index 365dd57..981a657 100644
> --- a/fs/xfs/xfs_pnfs.c
> +++ b/fs/xfs/xfs_pnfs.c
> @@ -31,7 +31,8 @@
>  int
>  xfs_break_layouts(
>       struct inode            *inode,
> -     uint                    *iolock)
> +     uint                    *iolock,
> +     bool                    with_imutex)
>  {
>       struct xfs_inode        *ip = XFS_I(inode);
>       int                     error;
> @@ -40,8 +41,12 @@ xfs_break_layouts(
>  
>       while ((error = break_layout(inode, false) == -EWOULDBLOCK)) {
>               xfs_iunlock(ip, *iolock);
> +             if (with_imutex && (*iolock & XFS_IOLOCK_EXCL))
> +                     mutex_unlock(&inode->i_mutex);
>               error = break_layout(inode, true);
>               *iolock = XFS_IOLOCK_EXCL;
> +             if (with_imutex)
> +                     mutex_lock(&inode->i_mutex);
>               xfs_ilock(ip, *iolock);
>       }

That's kinda nasty, and it has no documentation explaining when or
why we'd need to drop the i_mutex. How are we supposed to know if we
need to drop the i_mutex or not? What happens if the upper VFS
layers change or we have a multiple call paths that have different
i_mutex contexts (i.e. one holds, another doesn't)?

Which makes me wonder - is this layout breaking stuff at the right
layer?

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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