On 2/6/20 1:05 PM, Pavel Reichl wrote:
> xfs_isilocked() will only check one lock type so it's needed to split
> the check into 2 calls.
I think it's worth documenting the apparent intent of these calls;
did the old call mean one or the other is locked? (given the '|')
or does it mean to test both?
Testing both individually does seem legit. The single caller of each
of these functions has already asserted:
ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
and then each also does:
xfs_ilock(ip, XFS_ILOCK_EXCL);
before calling these functions, so it is safe and reasonable to assume
that both locks are held, and the intent is to test each one.
Oh, and if we look at when the old form got introduced, git blame says
ecfea3f0c8c64ce7375f4be4506996968958bd01, and it did:
- ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
- ASSERT(direction == SHIFT_LEFT || direction == SHIFT_RIGHT);
+ ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL));
so really, this is just reverting that invalid change back to
valid individual ASSERTs.
I'll leave it up to Darrick whether he wants to massage the commit
log I guess, but please at least add a :
Fixes: ecfea3f0c8c6 ("xfs: split xfs_bmap_shift_extents")
Reviewed-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> Suggested-by: Dave Chinner <dchinner@xxxxxxxxxx>
> Signed-off-by: Pavel Reichl <preichl@xxxxxxxxxx>
> ---
> 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 bc2be29193aa..c9dc94f114ed 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_isilocked(ip, XFS_IOLOCK_EXCL));
> + ASSERT(xfs_isilocked(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_isilocked(ip, XFS_IOLOCK_EXCL));
> + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>
> if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> error = xfs_iread_extents(tp, ip, whichfork);
>
