Re: [PATCH v2 4/5] xfs: Remove mrlock wrapper

[Dave Chinner]

On Sat, Oct 07, 2023 at 09:35:42PM +0100, Matthew Wilcox (Oracle) wrote:
> mrlock was an rwsem wrapper that also recorded whether the lock was
> held for read or write.  Now that we can ask the generic code whether
> the lock is held for read or write, we can remove this wrapper and use
> an rwsem directly.
> 
> As the comment says, we can't use lockdep to assert that the ILOCK is
> held for write, because we might be in a workqueue, and we aren't able
> to tell lockdep that we do in fact own the lock.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>

.....

> @@ -338,10 +338,14 @@ xfs_assert_ilocked(
>  	struct xfs_inode	*ip,
>  	uint			lock_flags)
>  {
> +	/*
> +	 * Sometimes we assert the ILOCK is held exclusively, but we're in
> +	 * a workqueue, so lockdep doesn't know we're the owner.
> +	 */
>  	if (lock_flags & XFS_ILOCK_SHARED)
> -		rwsem_assert_held(&ip->i_lock.mr_lock);
> +		rwsem_assert_held(&ip->i_lock);
>  	else if (lock_flags & XFS_ILOCK_EXCL)
> -		BUG_ON(!ip->i_lock.mr_writer);
> +		__rwsem_assert_held_write(&ip->i_lock);

It took me ages to work out that the comment related to the "__"
variant of rwsem_assert_held_write() function. I really dislike the
use of "__" prefixes for a function with slightly different,
non-obvious semantics to the parent - it's way too subtle for it to
be clear that this is what the comment is refering to.

In this case, we effectively have rwsem_assert_held_write() that
does lockdep checks, and __rwsem_assert_held_write() that does not
do lockdep checks. Either the former needs to say "lockdep" or the
latter needs "nolockdep" in the name to indicate to the reader the
intent of the code calling the checking function....

Otherwise the code looks fine.

-Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx