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@xxxxxxxxxx>
Yeah, we should've done "one test per assert" back then... :/
And please do massage the commit log.
--D
> > 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);
> >
