Re: [PATCH v2 2/7] xfs: Update checking excl. locks for ilock

From: Dave Chinner <david@fromorbit.com>
To: Eric Sandeen <sandeen@sandeen.net>
Cc: Christoph Hellwig <hch@infradead.org>,
	Pavel Reichl <preichl@redhat.com>,
	linux-xfs@vger.kernel.org, Dave Chinner <dchinner@redhat.com>
Subject: Re: [PATCH v2 2/7] xfs: Update checking excl. locks for ilock
Date: Wed, 5 Feb 2020 08:06:32 +1100	[thread overview]
Message-ID: <20200204210632.GL20628@dread.disaster.area> (raw)
In-Reply-To: <80f1173e-9181-c8cc-c06f-cbbfaa46a138@sandeen.net>

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@fromorbit.com