From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail202.messagelabs.com (mail202.messagelabs.com [216.82.254.227]) by kanga.kvack.org (Postfix) with ESMTP id B19826B01CC for ; Thu, 27 May 2010 02:46:38 -0400 (EDT) Date: Thu, 27 May 2010 16:46:31 +1000 From: Nick Piggin Subject: Re: [PATCH 3/5 v2] superblock: introduce per-sb cache shrinker infrastructure Message-ID: <20100527064631.GK22536@laptop> References: <1274777588-21494-1-git-send-email-david@fromorbit.com> <1274777588-21494-4-git-send-email-david@fromorbit.com> <20100526164116.GD22536@laptop> <20100526231214.GB1395@dastard> <20100527015335.GD1395@dastard> <20100527040120.GX31073@ZenIV.linux.org.uk> <20100527061751.GK12087@dastard> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100527061751.GK12087@dastard> Sender: owner-linux-mm@kvack.org To: Dave Chinner Cc: Al Viro , linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, xfs@oss.sgi.com List-ID: 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, 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: email@kvack.org