| Message ID | 20200206190502.389139-4-preichl@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: Remove wrappers for some semaphores | expand |
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> > 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); >
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); > >
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);
xfs_isilocked() will only check one lock type so it's needed to split the check into 2 calls. 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(-)