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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 27 May 2010 16:46:31 +1000
From:	Nick Piggin <npiggin@...e.de>
To:	Dave Chinner <david@...morbit.com>
Cc:	Al Viro <viro@...IV.linux.org.uk>, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, linux-mm@...ck.org, xfs@....sgi.com
Subject: Re: [PATCH 3/5 v2] superblock: introduce per-sb cache shrinker
 infrastructure

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.


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