Re: [PATCH v2 6/7] xfs: update excl. lock check for IOLOCK and ILOCK

[Dave Chinner <david@fromorbit.com>]

On Mon, Feb 03, 2020 at 06:58:49PM +0100, Pavel Reichl wrote:
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> Suggested-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index c3638552b3c0..2d371f87e890 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -5829,7 +5829,8 @@ xfs_bmap_collapse_extents(
>  	if (XFS_FORCED_SHUTDOWN(mp))
>  		return -EIO;
>  
> -	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL));
> +	ASSERT(xfs_is_iolocked(ip, XFS_IOLOCK_EXCL) ||
> +		xfs_is_ilocked(ip, XFS_ILOCK_EXCL));

Hmmm. I think this is incorrect - a bug in the original code in that
xfs_isilocked() will only check one lock type and so this never
worked as intended.

That is, we should have both the IOLOCK and the ILOCK held here.
The IOLOCK is taken by the high level xfs_file_fallocate() code to
lock out IO, while ILOCK is taken cwinside the transaction to make
the metadata changes atomic.

Hence I think this should actually end up as:

	ASSERT(xfs_is_iolocked(ip, XFS_IOLOCK_EXCL));
	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));

>  
>  	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
>  		error = xfs_iread_extents(tp, ip, whichfork);
> @@ -5946,7 +5947,8 @@ xfs_bmap_insert_extents(
>  	if (XFS_FORCED_SHUTDOWN(mp))
>  		return -EIO;
>  
> -	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL));
> +	ASSERT(xfs_is_iolocked(ip, XFS_IOLOCK_EXCL) ||
> +		xfs_is_ilocked(ip, XFS_ILOCK_EXCL));

Same here.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com