On Tue, Feb 04, 2020 at 10:08:45AM -0600, Eric Sandeen wrote: > On 2/4/20 12:21 AM, Christoph Hellwig wrote: > >> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL)); > >> + ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL)); > > > > I think this is a very bad interface. Either we keep our good old > > xfs_isilocked that just operates on the inode and lock flags, or > > we use something that gets the actual lock passed. But an interface > > that encodes the lock in both the function called and the flags, and > > one that doesn't follow neither the XFS lock flags conventions nor > > the core kernel convention is just not very useful. > > I think this came out of Dave's suggestion on the previous patchset, > but I agree with you Chrisoph. Even if there is a future reason to > split it out into a function for each type, I don't see a reason to > do it now, and this interface is awkward. > > I'd prefer to keep xfs_isilocked() with the current calling convention and > just change its internals to use lockdep. Dave spotted a bug in the > current implementation, but I think that can be fixed. > > Splitting out the 3 lock testing functions seems to me like complexity > creep that doesn't need to be in this series. > > Dave, thoughts? All I care about is that we get rid of the mrlock_t. I'm not interested in bikeshedding the details to death. I've put my 2c worth in, if you don't like it, then that's fine and I'm not going to get upset about that. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
