All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: NeilBrown <neilb@suse.de>
Cc: Jan Kara <jack@suse.cz>, Jeff Layton <jlayton@redhat.com>,
	Trond Myklebust <trond.myklebust@primarydata.com>,
	"J. Bruce Fields" <bfields@fieldses.org>,
	Mel Gorman <mgorman@suse.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-mm@kvack.org, linux-nfs@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH/RFC 0/5] Support loop-back NFS mounts - take 2
Date: Thu, 24 Apr 2014 11:20:22 +1000	[thread overview]
Message-ID: <20140424012022.GX15995@dastard> (raw)
In-Reply-To: <20140423022441.4725.89693.stgit@notabene.brown>

On Wed, Apr 23, 2014 at 12:40:58PM +1000, NeilBrown wrote:
> This is a somewhat shorter patchset for loop-back NFS support than
> last time, thanks to the excellent feedback and particularly to Dave
> Chinner.  Thanks.
> 
> Avoiding the wait-for-congestion which can trigger a livelock is much
> the same, though I've reduced the cases in which the wait is
> by-passed.
> I did this using current->backing_dev_info which is otherwise serving
> no purpose on the current kernel.
> 
> Avoiding the deadlocks has been turned on its head.
> Instead of nfsd checking if it is a loop-back mount and setting
> PF_FSTRANS, which then needs lots of changes too PF_FSTRANS and
> __GFP_FS handling, it is now NFS which checks for a loop-back
> filesystem.
> 
> There is more verbosity in that patch (Fifth of Five) but the essence
> is that nfs_release_page will now not wait indefinitely for a COMMIT
> request to complete when sent to the local host.  It still waits a
> little while as some delay can be important. But it won't wait
> forever.
> The duration of "a little while" is currently 100ms, though I do
> wonder if a bigger number would serve just as well.
> 
> Unlike the previous series, this set should remove deadlocks that
> could happen during the actual fail-over process.  This is achieved by
> having nfs_release_page monitor the connection and if it changes from
> a remote to a local connection, or just disconnects, then it will
> timeout.  It currently polls every second, though this probably could
> be longer too.  It only needs to be the same order of magnitude as the
> time it takes node failure to be detected and failover to happen, and
> I suspect that is closer to 1 minute.  So maybe a 10 or 20 second poll
> interval would be just as good.
> 
> Implementing this timeout requires some horrible code as the
> wait_on_bit functions don't support timeouts.  If the general approach
> is found acceptable I'll explore ways to improve the timeout code.
> 
> Comments, criticism, etc very welcome as always,

Looks much less intrusive to me, and doesn't appear to affect any
other filesystem or the recursion patterns of memory reclaim,
so I like it very much more than the previous patchset. Nice work!
:)

The code changes are really outside my area of expertise now, so I
don't really feel qualified to review the changes. However, consider
the overall approach:

Acked-by: Dave Chinner <dchinner@redhat.com>

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

WARNING: multiple messages have this Message-ID (diff)
From: Dave Chinner <david@fromorbit.com>
To: NeilBrown <neilb@suse.de>
Cc: Jan Kara <jack@suse.cz>, Jeff Layton <jlayton@redhat.com>,
	Trond Myklebust <trond.myklebust@primarydata.com>,
	"J. Bruce Fields" <bfields@fieldses.org>,
	Mel Gorman <mgorman@suse.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-mm@kvack.org, linux-nfs@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH/RFC 0/5] Support loop-back NFS mounts - take 2
Date: Thu, 24 Apr 2014 11:20:22 +1000	[thread overview]
Message-ID: <20140424012022.GX15995@dastard> (raw)
In-Reply-To: <20140423022441.4725.89693.stgit@notabene.brown>

On Wed, Apr 23, 2014 at 12:40:58PM +1000, NeilBrown wrote:
> This is a somewhat shorter patchset for loop-back NFS support than
> last time, thanks to the excellent feedback and particularly to Dave
> Chinner.  Thanks.
> 
> Avoiding the wait-for-congestion which can trigger a livelock is much
> the same, though I've reduced the cases in which the wait is
> by-passed.
> I did this using current->backing_dev_info which is otherwise serving
> no purpose on the current kernel.
> 
> Avoiding the deadlocks has been turned on its head.
> Instead of nfsd checking if it is a loop-back mount and setting
> PF_FSTRANS, which then needs lots of changes too PF_FSTRANS and
> __GFP_FS handling, it is now NFS which checks for a loop-back
> filesystem.
> 
> There is more verbosity in that patch (Fifth of Five) but the essence
> is that nfs_release_page will now not wait indefinitely for a COMMIT
> request to complete when sent to the local host.  It still waits a
> little while as some delay can be important. But it won't wait
> forever.
> The duration of "a little while" is currently 100ms, though I do
> wonder if a bigger number would serve just as well.
> 
> Unlike the previous series, this set should remove deadlocks that
> could happen during the actual fail-over process.  This is achieved by
> having nfs_release_page monitor the connection and if it changes from
> a remote to a local connection, or just disconnects, then it will
> timeout.  It currently polls every second, though this probably could
> be longer too.  It only needs to be the same order of magnitude as the
> time it takes node failure to be detected and failover to happen, and
> I suspect that is closer to 1 minute.  So maybe a 10 or 20 second poll
> interval would be just as good.
> 
> Implementing this timeout requires some horrible code as the
> wait_on_bit functions don't support timeouts.  If the general approach
> is found acceptable I'll explore ways to improve the timeout code.
> 
> Comments, criticism, etc very welcome as always,

Looks much less intrusive to me, and doesn't appear to affect any
other filesystem or the recursion patterns of memory reclaim,
so I like it very much more than the previous patchset. Nice work!
:)

The code changes are really outside my area of expertise now, so I
don't really feel qualified to review the changes. However, consider
the overall approach:

Acked-by: Dave Chinner <dchinner@redhat.com>

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

  parent reply	other threads:[~2014-04-24  1:21 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-04-23  2:40 [PATCH/RFC 0/5] Support loop-back NFS mounts - take 2 NeilBrown
2014-04-23  2:40 ` NeilBrown
2014-04-23  2:40 ` [PATCH 5/5] NFS: avoid deadlocks with loop-back mounted NFS filesystems NeilBrown
2014-04-23  2:40   ` NeilBrown
2014-04-23  2:40 ` [PATCH 3/5] nfsd: Only set PF_LESS_THROTTLE when really needed NeilBrown
2014-04-23  2:40   ` NeilBrown
2014-05-06 20:54   ` J. Bruce Fields
2014-05-06 20:54     ` J. Bruce Fields
2014-05-12  1:05     ` NeilBrown
2014-05-06 21:05   ` Rik van Riel
2014-05-06 21:05     ` Rik van Riel
2014-05-12  1:04     ` NeilBrown
2014-05-12 15:32       ` Jan Kara
2014-05-12 15:32         ` Jan Kara
2014-04-23  2:40 ` [PATCH 2/5] SUNRPC: track whether a request is coming from a loop-back interface NeilBrown
2014-04-23  2:40   ` NeilBrown
2014-04-23  2:40 ` [PATCH 4/5] SUNRPC: track when a client connection is routed to the local host NeilBrown
2014-04-23  2:40   ` NeilBrown
2014-04-23 13:44   ` Anna Schumaker
2014-04-23 13:44     ` Anna Schumaker
2014-04-23 23:14     ` NeilBrown
2014-04-23 23:14       ` NeilBrown
2014-04-24 12:46       ` Anna Schumaker
2014-04-24 12:46         ` Anna Schumaker
2014-04-23  2:40 ` [PATCH 1/5] MM: avoid throttling reclaim for loop-back nfsd threads NeilBrown
2014-04-23  2:40   ` NeilBrown
2014-04-23 22:03   ` Andrew Morton
2014-04-23 22:03     ` Andrew Morton
2014-04-23 22:47     ` NeilBrown
2014-04-24  1:20 ` Dave Chinner [this message]
2014-04-24  1:20   ` [PATCH/RFC 0/5] Support loop-back NFS mounts - take 2 Dave Chinner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140424012022.GX15995@dastard \
    --to=david@fromorbit.com \
    --cc=akpm@linux-foundation.org \
    --cc=bfields@fieldses.org \
    --cc=jack@suse.cz \
    --cc=jlayton@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=mgorman@suse.com \
    --cc=neilb@suse.de \
    --cc=trond.myklebust@primarydata.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.