Re: [PATCH v3 3/4] xfs: Fix bug when checking diff. locks

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Feb 07, 2020 at 01:25:06PM -0600, Eric Sandeen wrote:
> 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@redhat.com>

Yeah, we should've done "one test per assert" back then... :/

And please do massage the commit log.

--D

> > Suggested-by: Dave Chinner <dchinner@redhat.com>
> > Signed-off-by: Pavel Reichl <preichl@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 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);
> >