From mboxrd@z Thu Jan 1 00:00:00 1970 From: Glauber Costa Subject: Re: [PATCH v8 16/34] xfs: convert buftarg LRU to generic code Date: Wed, 29 May 2013 14:51:58 +0400 Message-ID: <51A5DDCE.509@parallels.com> References: <1369391368-31562-1-git-send-email-glommer@openvz.org> <1369391368-31562-17-git-send-email-glommer@openvz.org> <20130525002759.GK24543@dastard> <51A4D3B5.6060802@parallels.com> <20130529101519.GA29466@dastard> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Cc: Glauber Costa , , Mel Gorman , , , , Johannes Weiner , Michal Hocko , Tejun Heo , Dave Chinner To: Dave Chinner Return-path: Received: from mx2.parallels.com ([199.115.105.18]:55835 "EHLO mx2.parallels.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965388Ab3E2KvJ (ORCPT ); Wed, 29 May 2013 06:51:09 -0400 In-Reply-To: <20130529101519.GA29466@dastard> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On 05/29/2013 02:15 PM, Dave Chinner wrote: > On Tue, May 28, 2013 at 09:26:37PM +0530, Glauber Costa wrote: >> On 05/25/2013 05:57 AM, Dave Chinner wrote: >>> On Fri, May 24, 2013 at 03:59:10PM +0530, Glauber Costa wrote: >>>> From: Dave Chinner >>>> >>>> Convert the buftarg LRU to use the new generic LRU list and take >>>> advantage of the functionality it supplies to make the buffer cache >>>> shrinker node aware. >>>> >>>> * v7: Add NUMA aware flag >>> >>> I know what is wrong with this patch that causes the unmount hang - >>> it's the handling of the _XBF_LRU_DISPOSE flag no longer being >>> modified atomically with the LRU lock. Hence there is a race where >>> we can either lose the _XBF_LRU_DISPOSE or not see it and hence we >>> can end up with code not detecting what list the buffer is on >>> correctly. >>> >>> I haven't had a chance to work out a fix for it yet. If this ends up >>> likely to hold up the patch set, Glauber, then feel free to drop it >>> from the series and I'll push a fixed version through the XFS tree >>> in due course.... >>> >>> Cheers, >>> >>> Dave. >>> >> Please let me know what you think about the following two (very coarse) >> patches. My idea is to expose more of the raw structures so XFS can do >> the locking itself when needed. > > No, I'd prefer not to do that. There's a big difference between a > callback that passes a pointer to an internal lock that protects the > list that the item being passed is on and exposing the guts of the > per node list and lock implementation to everyone.... > > As it is, the XFS buffer LRU reclaimation is modelled on the > inode/dentry cache lru reclaimation where the "on dispose list" flag > is managed by a lock in the inode/dentry and wraps around the > outside of the LRU locks. The simplest fix to XFS is to add a > spinlock to the buffer to handle this in the same way as inodes and > dentries. I think I might be able to do it in a way that avoids > the spin lock but I just haven't had time to look at it that closely. > > Cheers, > Ok. In the interest of having the series merged - we seem to be running out of problems - I see two courses of action: 1) Don't convert this to LRU at all, just convert to the new count/ scan interface, 2) Use a temporary spinlock, and you fix that later. I would actually prefer 2). Reason is that this patch actually do both, meaning I would have to rewrite the patch to do this scan / count loop without the new list_lru aid. Besides being more error-prone, it is of course a lot more work. Which one you prefer?