Re: [PATCH v2 1/7] xfs: Add xfs_is_{i,io,mmap}locked functions

[Dave Chinner <david@fromorbit.com>]

On Mon, Feb 03, 2020 at 06:58:44PM +0100, Pavel Reichl wrote:
> Add xfs_is_ilocked(), xfs_is_iolocked() and xfs_is_mmaplocked()

The commit description is supposed to explain "Why?" rather than
describe what the code does.

So why are we adding these interfaces?

> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> Suggested-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_inode.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_inode.h |  3 +++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c5077e6326c7..80874c80df6d 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -372,6 +372,59 @@ xfs_isilocked(
>  	ASSERT(0);
>  	return 0;
>  }
> +
> +static inline bool
> +__xfs_is_ilocked(
> +	struct rw_semaphore	*rwsem,
> +	bool			shared,
> +	bool			excl)
> +{
> +	bool locked = false;
> +
> +	if (!rwsem_is_locked(rwsem))
> +		return false;
> +
> +	if (!debug_locks)
> +		return true;
> +
> +	if (shared)
> +		locked = lockdep_is_held_type(rwsem, 0);
> +
> +	if (excl)
> +		locked |= lockdep_is_held_type(rwsem, 1);
> +
> +	return locked;
> +}

This needs a comment explaining the reason why it is structured this
way. I can see quite clearly what it is doing, but why it is done
this way is not immediately apparent from the code.

In a few months, I'm not going to remember the reasons for this
code, and if the neither the code nor the commit description
explains the reasons why the code is like this, then it's really
quite difficult and time consuming to try to discover the reason for
the code being this way.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com