Re: [PATCH v8 16/34] xfs: convert buftarg LRU to generic code

[Glauber Costa <glommer@parallels.com>]

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 <dchinner@redhat.com>
>>>>
>>>> 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?