xfs
[Top] [All Lists]

Re: [PATCH 3/5 v2] superblock: introduce per-sb cache shrinker infrastru

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 3/5 v2] superblock: introduce per-sb cache shrinker infrastructure
From: Nick Piggin <npiggin@xxxxxxx>
Date: Thu, 27 May 2010 16:46:31 +1000
Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>, linux-kernel@xxxxxxxxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx, linux-mm@xxxxxxxxx, xfs@xxxxxxxxxxx
In-reply-to: <20100527061751.GK12087@dastard>
References: <1274777588-21494-1-git-send-email-david@xxxxxxxxxxxxx> <1274777588-21494-4-git-send-email-david@xxxxxxxxxxxxx> <20100526164116.GD22536@laptop> <20100526231214.GB1395@dastard> <20100527015335.GD1395@dastard> <20100527040120.GX31073@xxxxxxxxxxxxxxxxxx> <20100527061751.GK12087@dastard>
User-agent: Mutt/1.5.20 (2009-06-14)
On Thu, May 27, 2010 at 04:17:51PM +1000, Dave Chinner wrote:
> On Thu, May 27, 2010 at 05:01:20AM +0100, Al Viro wrote:
> > Um...  Maybe I'm dumb, but what's wrong with doing unregistration from
> > deactivate_locked_super(), right after the call of ->kill_sb()?  At that
> > point ->s_umount is already dropped, so we won't deadlock at all.
> > Shrinker rwsem will make sure that all shrinkers-in-progress will run
> > to completion, so we won't get a superblock freed under prune_super().
> > I don't particulary mind down_try_read() in prune_super(), but why not
> > make life obviously safer?
> > 
> > Am I missing something here?
> 
> I was worried about memory allocation in the ->kill_sb path
> deadlocking on the s_umount lock if it enters reclaim. e.g.  XFS
> inodes can still be dirty even after the VFS has disposed of them,
> and writing them back can require page cache allocation for the
> backing buffers. If allocation recurses back into the shrinker, we
> can deadlock on the s_umount lock.  This doesn't seem like an XFS
> specific problem, so I used a trylock to avoid that whole class of
> problems (same way the current shrinkers do).

If GFP_FS is set, we wouldn't touch the locks. It is a concern
though, if __GFP_FS allocations were previously permitted under
the exclusive lock.

 
> >From there, we can unregister the shrinker before calling ->kill_sb
> as per above. That, in turn, means that the unmount
> invalidate_inodes() vs shrinker race goes away and the iprune_sem is
> not needed in the new prune_icache_sb() function.  I'm pretty sure
> that I can now remove the iprune_sem, but I haven't written the
> patch to do that yet.

I do really like that aspect of your patch. It's nice to have the
shrinker always only operating against active supers. So I would
be in favour of your current scheme.


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