Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1189728 > unrolled thread

[RFC PATCH] nfs: avoid swap-over-NFS deadlock

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH] nfs: avoid swap-over-NFS deadlock "Jerome Marchand" <jmarchan@redhat.com> - 2015-07-22 10:20 +0200
    Re: [RFC PATCH] nfs: avoid swap-over-NFS deadlock Trond Myklebust <trond.myklebust@primarydata.com> - 2015-07-22 14:30 +0200
      Re: [RFC PATCH] nfs: avoid swap-over-NFS deadlock Jerome Marchand <jmarchan@redhat.com> - 2015-07-22 15:50 +0200
        Re: [RFC PATCH] nfs: avoid swap-over-NFS deadlock Mel Gorman <mgorman@techsingularity.net> - 2015-07-27 13:00 +0200

#1189728 — [RFC PATCH] nfs: avoid swap-over-NFS deadlock

Lockdep warns about a inconsistent {RECLAIM_FS-ON-W} ->
{IN-RECLAIM_FS-W} usage. The culpritt is the inode->i_mutex taken in
nfs_file_direct_write(). This code was introduced by commit a9ab5e840669
("nfs: page cache invalidation for dio").
This naive test patch avoid to take the mutex on a swapfile and makes
lockdep happy again. However I don't know much about NFS code and I
assume it's probably not the proper solution. Any thought?

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
 fs/nfs/direct.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index 38678d9..42324d4 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -974,7 +974,9 @@ ssize_t nfs_file_direct_write(struct kiocb *iocb, struct iov_iter *iter)
 	pos = iocb->ki_pos;
 	end = (pos + iov_iter_count(iter) - 1) >> PAGE_CACHE_SHIFT;
 
-	mutex_lock(&inode->i_mutex);
+	/* Don't take the mutex while in reclaim_FS */
+	if (!IS_SWAPFILE(inode))
+		mutex_lock(&inode->i_mutex);
 
 	result = nfs_sync_mapping(mapping);
 	if (result)
@@ -1014,7 +1016,8 @@ ssize_t nfs_file_direct_write(struct kiocb *iocb, struct iov_iter *iter)
 					      pos >> PAGE_CACHE_SHIFT, end);
 	}
 
-	mutex_unlock(&inode->i_mutex);
+	if (!IS_SWAPFILE(inode))
+		mutex_unlock(&inode->i_mutex);
 
 	if (!result) {
 		result = nfs_direct_wait(dreq);
@@ -1035,7 +1038,8 @@ ssize_t nfs_file_direct_write(struct kiocb *iocb, struct iov_iter *iter)
 out_release:
 	nfs_direct_req_release(dreq);
 out_unlock:
-	mutex_unlock(&inode->i_mutex);
+	if (!IS_SWAPFILE(inode))
+		mutex_unlock(&inode->i_mutex);
 	return result;
 }
 
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1189863

On Wed, Jul 22, 2015 at 4:10 AM, Jerome Marchand <jmarchan@redhat.com> wrote:
>
> Lockdep warns about a inconsistent {RECLAIM_FS-ON-W} ->
> {IN-RECLAIM_FS-W} usage. The culpritt is the inode->i_mutex taken in
> nfs_file_direct_write(). This code was introduced by commit a9ab5e840669
> ("nfs: page cache invalidation for dio").
> This naive test patch avoid to take the mutex on a swapfile and makes
> lockdep happy again. However I don't know much about NFS code and I
> assume it's probably not the proper solution. Any thought?
>
> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>

NFS is not the only O_DIRECT implementation to set the inode->i_mutex.
Why can't this be fixed in the generic swap code instead of adding
yet-another-exception-for-IS_SWAPFILE?

Cheers
  Trond
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1189956

[Multipart message — attachments visible in raw view] — view raw

On 07/22/2015 02:23 PM, Trond Myklebust wrote:
> On Wed, Jul 22, 2015 at 4:10 AM, Jerome Marchand <jmarchan@redhat.com> wrote:
>>
>> Lockdep warns about a inconsistent {RECLAIM_FS-ON-W} ->
>> {IN-RECLAIM_FS-W} usage. The culpritt is the inode->i_mutex taken in
>> nfs_file_direct_write(). This code was introduced by commit a9ab5e840669
>> ("nfs: page cache invalidation for dio").
>> This naive test patch avoid to take the mutex on a swapfile and makes
>> lockdep happy again. However I don't know much about NFS code and I
>> assume it's probably not the proper solution. Any thought?
>>
>> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
> 
> NFS is not the only O_DIRECT implementation to set the inode->i_mutex.
> Why can't this be fixed in the generic swap code instead of adding
> yet-another-exception-for-IS_SWAPFILE?

I meant to cc Mel. Just added him.

AFAIK NFS is the only filesystem that uses swap_activate. Other
swapfiles are handled more or less like block device (divided in a set
of contiguous ranges of disk block called swap extents), so there are
not affected by this possible deadlock.

Also nfs_direct_IO() is special in that it is called only from swap,
nfs_file_direct_write() however has other users.

Jerome
> 
> Cheers
>   Trond
> 

[toc] | [prev] | [next] | [standalone]

#1192961

On Wed, Jul 22, 2015 at 03:46:16PM +0200, Jerome Marchand wrote:
> On 07/22/2015 02:23 PM, Trond Myklebust wrote:
> > On Wed, Jul 22, 2015 at 4:10 AM, Jerome Marchand <jmarchan@redhat.com> wrote:
> >>
> >> Lockdep warns about a inconsistent {RECLAIM_FS-ON-W} ->
> >> {IN-RECLAIM_FS-W} usage. The culpritt is the inode->i_mutex taken in
> >> nfs_file_direct_write(). This code was introduced by commit a9ab5e840669
> >> ("nfs: page cache invalidation for dio").
> >> This naive test patch avoid to take the mutex on a swapfile and makes
> >> lockdep happy again. However I don't know much about NFS code and I
> >> assume it's probably not the proper solution. Any thought?
> >>
> >> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
> > 
> > NFS is not the only O_DIRECT implementation to set the inode->i_mutex.
> > Why can't this be fixed in the generic swap code instead of adding
> > yet-another-exception-for-IS_SWAPFILE?
> 
> I meant to cc Mel. Just added him.
> 

Can the full lockdep warning be included as it'll be easier to see then if
the generic swap code can somehow special case this? Currently, generic
swapping does not not need to care about how the filesystem locked.
For most filesystems, it's writing directly to the blocks on disk and
bypassing the FS. In the NFS case it'd be surprising to find that there
also are dirty pages in page cache that belong to the swap file as it's
going to cause corruption. If there is any special casing it would to only
attempt the invalidation in the !swap case and warn if mapping->nrpages. It
still would look a bit weird but safer than just not acquiring the mutex
and then potentially attempting an invalidation.

-- 
Mel Gorman
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web