Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

Joe Perches writes: > On Tue, 2017-12-12 at 08:43 +1100, Dave Chinner wrote: >> On Sat, Dec 09, 2017 at 09:00:18AM -0800, Joe Perches wrote: >> > On Sat, 2017-12-09 at 09:36 +1100, Dave Chinner wrote: >> > > 1. Using lockdep_set_novalidate_class() for anything other >> > >

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Mon, 2017-12-11 at 14:43 -0800, Matthew Wilcox wrote: > - There's no warning for the first paragraph of section 6: > > 6) Functions > > > Functions should be short and sweet, and do just one thing. They should > fit on one or two screenfuls of text (the ISO/ANSI screen size is

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Mon, 11 Dec 2017, Joe Perches wrote: > > - I don't understand the error for xa_head here: > > > > struct xarray { > > spinlock_t xa_lock; > > gfp_t xa_flags; > > void __rcu *xa_head; > > }; > > > >Do people really think that: > > > > struct

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Mon, Dec 11, 2017 at 02:12:28PM -0800, Joe Perches wrote: > On Tue, 2017-12-12 at 08:43 +1100, Dave Chinner wrote: > > On Sat, Dec 09, 2017 at 09:00:18AM -0800, Joe Perches wrote: > > > On Sat, 2017-12-09 at 09:36 +1100, Dave Chinner wrote: > > > > 1. Using

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Mon, 2017-12-11 at 14:43 -0800, Matthew Wilcox wrote: > On Mon, Dec 11, 2017 at 02:12:28PM -0800, Joe Perches wrote: > > Completely reasonable. Thanks. > > If we're doing "completely reasonable" complaints, then ... > > - I don't understand why plain 'unsigned' is deemed bad. That was a

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Mon, Dec 11, 2017 at 02:12:28PM -0800, Joe Perches wrote: > Completely reasonable. Thanks. If we're doing "completely reasonable" complaints, then ... - I don't understand why plain 'unsigned' is deemed bad. - The rule about all function parameters in prototypes having a name doesn't

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Tue, 2017-12-12 at 08:43 +1100, Dave Chinner wrote: > On Sat, Dec 09, 2017 at 09:00:18AM -0800, Joe Perches wrote: > > On Sat, 2017-12-09 at 09:36 +1100, Dave Chinner wrote: > > > 1. Using lockdep_set_novalidate_class() for anything other > > > than device->mutex will throw checkpatch

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Sun, Dec 10, 2017 at 08:23:15PM -0800, Matthew Wilcox wrote: > On Mon, Dec 11, 2017 at 10:57:45AM +1100, Dave Chinner wrote: > > i.e. the fact the cmpxchg failed may not have anything to do with a > > race condtion - it failed because the slot wasn't empty like we > > expected it to be. There

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Sat, Dec 09, 2017 at 09:00:18AM -0800, Joe Perches wrote: > On Sat, 2017-12-09 at 09:36 +1100, Dave Chinner wrote: > > 1. Using lockdep_set_novalidate_class() for anything other > > than device->mutex will throw checkpatch warnings. Nice. (*) > [] > > (*) checkpatch.pl is considered

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Mon, Dec 11, 2017 at 10:57:45AM +1100, Dave Chinner wrote: > i.e. the fact the cmpxchg failed may not have anything to do with a > race condtion - it failed because the slot wasn't empty like we > expected it to be. There can be any number of reasons the slot isn't > empty - the API should not

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Fri, Dec 08, 2017 at 03:01:31PM -0800, Matthew Wilcox wrote: > On Thu, Dec 07, 2017 at 11:38:43AM +1100, Dave Chinner wrote: > > > > cmpxchg is for replacing a known object in a store - it's not really > > > > intended for doing initial inserts after a lookup tells us there is > > > > nothing

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Sat, 2017-12-09 at 09:36 +1100, Dave Chinner wrote: > 1. Using lockdep_set_novalidate_class() for anything other > than device->mutex will throw checkpatch warnings. Nice. (*) [] > (*) checkpatch.pl is considered mostly harmful round here, too, > but that's another rant How so?

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Thu, Dec 07, 2017 at 11:38:43AM +1100, Dave Chinner wrote: > > > cmpxchg is for replacing a known object in a store - it's not really > > > intended for doing initial inserts after a lookup tells us there is > > > nothing in the store. The radix tree "insert only if empty" makes > > > sense

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Fri, Dec 08, 2017 at 12:35:07PM -0500, Alan Stern wrote: > On Fri, 8 Dec 2017, Byungchul Park wrote: > > > I'm sorry to hear that.. If I were you, I would also get > > annoyed. And.. thanks for explanation. > > > > But, I think assigning lock classes properly and checking > > relationship of

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Fri, 8 Dec 2017, Byungchul Park wrote: > I'm sorry to hear that.. If I were you, I would also get > annoyed. And.. thanks for explanation. > > But, I think assigning lock classes properly and checking > relationship of the classes to detect deadlocks is reasonable. > > In my opinion about

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On 12/8/2017 4:25 PM, Dave Chinner wrote: On Fri, Dec 08, 2017 at 01:45:52PM +0900, Byungchul Park wrote: On Fri, Dec 08, 2017 at 09:22:16AM +1100, Dave Chinner wrote: On Thu, Dec 07, 2017 at 11:06:34AM -0500, Theodore Ts'o wrote: On Wed, Dec 06, 2017 at 06:06:48AM -0800, Matthew Wilcox

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Fri, Dec 08, 2017 at 01:45:52PM +0900, Byungchul Park wrote: > On Fri, Dec 08, 2017 at 09:22:16AM +1100, Dave Chinner wrote: > > On Thu, Dec 07, 2017 at 11:06:34AM -0500, Theodore Ts'o wrote: > > > On Wed, Dec 06, 2017 at 06:06:48AM -0800, Matthew Wilcox wrote: > > > > > Unfortunately for you,

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Fri, Dec 08, 2017 at 09:22:16AM +1100, Dave Chinner wrote: > On Thu, Dec 07, 2017 at 11:06:34AM -0500, Theodore Ts'o wrote: > > On Wed, Dec 06, 2017 at 06:06:48AM -0800, Matthew Wilcox wrote: > > > > Unfortunately for you, I don't find arguments along the lines of > > > > "lockdep will save us"

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Thu, Dec 07, 2017 at 11:06:34AM -0500, Theodore Ts'o wrote: > On Wed, Dec 06, 2017 at 06:06:48AM -0800, Matthew Wilcox wrote: > > > Unfortunately for you, I don't find arguments along the lines of > > > "lockdep will save us" at all convincing. lockdep already throws > > > too many false

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Wed, Dec 06, 2017 at 06:06:48AM -0800, Matthew Wilcox wrote: > > Unfortunately for you, I don't find arguments along the lines of > > "lockdep will save us" at all convincing. lockdep already throws > > too many false positives to be useful as a tool that reliably and > > accurately points out

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Wed, Dec 06, 2017 at 06:06:48AM -0800, Matthew Wilcox wrote: > On Wed, Dec 06, 2017 at 07:44:04PM +1100, Dave Chinner wrote: > > On Tue, Dec 05, 2017 at 08:45:49PM -0800, Matthew Wilcox wrote: > > > That said, using xa_cmpxchg() in the dquot code looked like the right > > > thing to do? Since

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Wed, Dec 06, 2017 at 07:44:04PM +1100, Dave Chinner wrote: > On Tue, Dec 05, 2017 at 08:45:49PM -0800, Matthew Wilcox wrote: > > That said, using xa_cmpxchg() in the dquot code looked like the right > > thing to do? Since we'd dropped the qi mutex and the ILOCK, it looks > > entirely

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Tue, Dec 05, 2017 at 08:45:49PM -0800, Matthew Wilcox wrote: > On Wed, Dec 06, 2017 at 02:14:56PM +1100, Dave Chinner wrote: > > > The other conversions use the normal API instead of the advanced API, so > > > all of this gets hidden away. For example, the inode cache does this: > > > > Ah,

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Tue, Dec 05, 2017 at 08:45:49PM -0800, Matthew Wilcox wrote: > The dquot code is just going to have to live with the fact that there's > additional locking going on that it doesn't need. I'm open to getting > rid of the irqsafety, but we can't give up the spinlock protection > without giving

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Wed, Dec 06, 2017 at 02:14:56PM +1100, Dave Chinner wrote: > > The other conversions use the normal API instead of the advanced API, so > > all of this gets hidden away. For example, the inode cache does this: > > Ah, OK, that's not obvious from the code changes. :/ Yeah, it's a lot easier

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Tue, Dec 05, 2017 at 06:02:08PM -0800, Matthew Wilcox wrote: > On Wed, Dec 06, 2017 at 12:36:48PM +1100, Dave Chinner wrote: > > > - if (radix_tree_preload(GFP_NOFS)) > > > - return -ENOMEM; > > > - > > > INIT_LIST_HEAD(>list_node); > > > elem->key = key; > > > > > > -

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Wed, Dec 06, 2017 at 12:36:48PM +1100, Dave Chinner wrote: > > - if (radix_tree_preload(GFP_NOFS)) > > - return -ENOMEM; > > - > > INIT_LIST_HEAD(>list_node); > > elem->key = key; > > > > - spin_lock(>lock); > > - error = radix_tree_insert(>store, key, elem); > > -

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Tue, Dec 05, 2017 at 04:41:58PM -0800, Matthew Wilcox wrote: > From: Matthew Wilcox > > This eliminates a call to radix_tree_preload(). . > void > @@ -431,24 +424,24 @@ xfs_mru_cache_insert( > unsigned long key, > struct xfs_mru_cache_elem