Linux-NFS Archive on lore.kernel.org
 help / color / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org, cgroups@vger.kernel.org,
	linux-mm@kvack.org, Johannes Weiner <hannes@cmpxchg.org>,
	Michal Hocko <mhocko@kernel.org>,
	Vladimir Davydov <vdavydov.dev@gmail.com>,
	Roman Gushchin <guro@fb.com>, Shakeel Butt <shakeelb@google.com>,
	Chris Down <chris@chrisdown.name>,
	Yang Shi <yang.shi@linux.alibaba.com>, Tejun Heo <tj@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	"Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
	Konstantin Khorenko <khorenko@virtuozzo.com>,
	Kirill Tkhai <ktkhai@virtuozzo.com>,
	Trond Myklebust <trond.myklebust@hammerspace.com>,
	Anna Schumaker <anna.schumaker@netapp.com>,
	"J. Bruce Fields" <bfields@fieldses.org>,
	Chuck Lever <chuck.lever@oracle.com>,
	linux-nfs@vger.kernel.org,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH] mm: fix hanging shrinker management on long do_shrink_slab
Date: Fri, 6 Dec 2019 13:09:53 +1100
Message-ID: <20191206020953.GS2695@dread.disaster.area> (raw)
In-Reply-To: <4e2d959a-0b0e-30aa-59b4-8e37728e9793@virtuozzo.com>

[please cc me on future shrinker infrastructure modifications]

On Mon, Dec 02, 2019 at 07:36:03PM +0300, Andrey Ryabinin wrote:
> 
> On 11/30/19 12:45 AM, Pavel Tikhomirov wrote:
> > We have a problem that shrinker_rwsem can be held for a long time for
> > read in shrink_slab, at the same time any process which is trying to
> > manage shrinkers hangs.
> > 
> > The shrinker_rwsem is taken in shrink_slab while traversing shrinker_list.
> > It tries to shrink something on nfs (hard) but nfs server is dead at
> > these moment already and rpc will never succeed. Generally any shrinker
> > can take significant time to do_shrink_slab, so it's a bad idea to hold
> > the list lock here.

registering/unregistering a shrinker is not a performance critical
task. If a shrinker is blocking for a long time, then we need to
work to fix the shrinker implementation because blocking is a much
bigger problem than just register/unregister.

> > The idea of the patch is to inc a refcount to the chosen shrinker so it
> > won't disappear and release shrinker_rwsem while we are in
> > do_shrink_slab, after that we will reacquire shrinker_rwsem, dec
> > the refcount and continue the traversal.

This is going to cause a *lot* of traffic on the shrinker rwsem.
It's already a pretty hot lock on large machines under memory
pressure (think thousands of tasks all doing direct reclaim across
hundreds of CPUs), and so changing them to cycle the rwsem on every
shrinker that will only make this worse. Esepcially when we consider
that there may be hundreds to thousands of registered shrinker
instances on large machines.

As an example of how frequent cycling of a global lock in shrinker
instances causes issues, we used to take references to superblock
shrinker count invocations to guarantee existence. This was found to
be a scalability limitation when lots of near-empty superblocks were
present in a system (see commit d23da150a37c ("fs/superblock: avoid
locking counting inodes and dentries before reclaiming them")).

This alleviated the problem for a while, but soon we had problems
with just taking a reference to the superblock in the callbacks that
did actual work. Hence we changed it to just take a per-superblock
rwsem to get rid of the global sb_lock spinlock in this path. See
commit eb6ef3df4faa ("trylock_super(): replacement for
grab_super_passive()". Now we don't have a scalability problem.

IOWs, we already know that cycling a global rwsem on every
individual shrinker invocation is going to cause noticable
scalability problems. Hence I don't think that this sort of "cycle
the global rwsem faster to reduce [un]register latency" solution is
going to fly because of the runtime performance regressions it will
introduce....

> I don't think this patch solves the problem, it only fixes one minor symptom of it.
> The actual problem here the reclaim hang in the nfs.

The nfs client is waiting on the NFS server to respond. It may
actually be that the server has hung, not the client...

> It means that any process, including kswapd, may go into nfs inode reclaim and stuck there.

*nod*

> I think this should be handled on nfs/vfs level by making  inode eviction during reclaim more asynchronous.

That's what we are trying to do with similar blocking based issues
in XFS inode reclaim. It's not simple, though, because these days
memory reclaim is like a bowl full of spaghetti covered with a
delicious sauce of non-obvious heuristics and broken
functionality....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

  parent reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20191129214541.3110-1-ptikhomirov@virtuozzo.com>
2019-12-02 16:36 ` Andrey Ryabinin
2019-12-03  0:13   ` Shakeel Butt
2019-12-03 11:03     ` Kirill Tkhai
2019-12-06  2:09   ` Dave Chinner [this message]
2019-12-06 10:09     ` Michal Hocko
2019-12-06 17:11     ` Shakeel Butt
2019-12-10  1:20       ` Dave Chinner
2019-12-19 10:35         ` Pavel Tikhomirov
     [not found] ` <20191204083514.GC25242@dhcp22.suse.cz>
2019-12-19 10:20   ` Pavel Tikhomirov

Reply instructions:

You may reply publically 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=20191206020953.GS2695@dread.disaster.area \
    --to=david@fromorbit.com \
    --cc=akpm@linux-foundation.org \
    --cc=anna.schumaker@netapp.com \
    --cc=aryabinin@virtuozzo.com \
    --cc=bfields@fieldses.org \
    --cc=cgroups@vger.kernel.org \
    --cc=chris@chrisdown.name \
    --cc=chuck.lever@oracle.com \
    --cc=guro@fb.com \
    --cc=hannes@cmpxchg.org \
    --cc=khorenko@virtuozzo.com \
    --cc=kirill.shutemov@linux.intel.com \
    --cc=ktkhai@virtuozzo.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=mhocko@kernel.org \
    --cc=ptikhomirov@virtuozzo.com \
    --cc=shakeelb@google.com \
    --cc=tglx@linutronix.de \
    --cc=tj@kernel.org \
    --cc=trond.myklebust@hammerspace.com \
    --cc=vdavydov.dev@gmail.com \
    --cc=viro@zeniv.linux.org.uk \
    --cc=yang.shi@linux.alibaba.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

Linux-NFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nfs/0 linux-nfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nfs linux-nfs/ https://lore.kernel.org/linux-nfs \
		linux-nfs@vger.kernel.org
	public-inbox-index linux-nfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-nfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git