On Mon, Oct 17, 2016 at 10:54:18AM -0500, Eric Sandeen wrote:
> On 10/17/16 10:02 AM, Brian Foster wrote:
> > Filesystem shutdown testing on an older distro kernel has uncovered an
> > imbalanced locking pattern for the inode flush lock in
> > xfs_reclaim_inode(). Specifically, there is a double unlock sequence
> > between the call to xfs_iflush_abort() and xfs_reclaim_inode() at the
> > "reclaim:" label.
> >
> > This actually does not cause obvious problems on current kernels due to
> > the current flush lock implementation. Older kernels use a counting
> > based flush lock mechanism, however, which effectively breaks the lock
> > indefinitely when an already unlocked flush lock is repeatedly unlocked.
> > Though this only currently occurs on filesystem shutdown, it has
> > reproduced the effect of elevating an fs shutdown to a system-wide crash
> > or hang.
> >
> > Because this problem exists on filesystem shutdown and thus only after
> > unrelated catastrophic failure, issue the simple fix to reacquire the
> > flush lock in xfs_reclaim_inode() before jumping to the reclaim code.
> > Add an assert to xfs_ifunlock() to help prevent future occurrences of
> > the same problem. Finally, update xfs_reclaim_inode() to bitwise-OR the
> > reclaim flag to avoid smashing the flush lock in the process (which is
> > based on an inode flag in current kernels). This avoids a (spurious)
> > failure of the newly introduced xfs_ifunlock() assertion.
> >
> > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> > Reported-by: Zorro Lang <zlang@xxxxxxxxxx>
> > ---
> > fs/xfs/xfs_icache.c | 3 ++-
> > fs/xfs/xfs_inode.h | 11 ++++++-----
> > 2 files changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> > index 14796b7..7375313 100644
> > --- a/fs/xfs/xfs_icache.c
> > +++ b/fs/xfs/xfs_icache.c
> > @@ -982,6 +982,7 @@ restart:
> > if (XFS_FORCED_SHUTDOWN(ip->i_mount)) {
> > xfs_iunpin_wait(ip);
>
> I suppose comments here might help...
>
> Other callers of xfs_iflush_abort include:
>
> /*
> * Unlocks the flush lock
> */
>
> and immediately re-locking it here might be worth explaining as well.
>
Indeed, I'll add something.
> > xfs_iflush_abort(ip, false);
> > + xfs_iflock(ip);
> > goto reclaim;
> > }
> > if (xfs_ipincount(ip)) {
>
> > @@ -1044,7 +1045,7 @@ reclaim:
> > * skip.
> > */
> > spin_lock(&ip->i_flags_lock);
> > - ip->i_flags = XFS_IRECLAIM;
> > + ip->i_flags |= XFS_IRECLAIM;
> > ip->i_ino = 0;
> > spin_unlock(&ip->i_flags_lock);
> >
>
> I think xfs_inode_free() should get the same |= treatment?
>
Yeah, I think that makes sense. That would allow the
ASSERT(!xfs_isiflocked(ip)) check in __xfs_inode_free() to actually
work. Thanks!
Brian
> -Eric
>
>
> > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> > index f14c1de..71e8a81 100644
> > --- a/fs/xfs/xfs_inode.h
> > +++ b/fs/xfs/xfs_inode.h
> > @@ -246,6 +246,11 @@ static inline bool xfs_is_reflink_inode(struct xfs_inode *ip)
> > * Synchronize processes attempting to flush the in-core inode back to disk.
> > */
> >
> > +static inline int xfs_isiflocked(struct xfs_inode *ip)
> > +{
> > + return xfs_iflags_test(ip, XFS_IFLOCK);
> > +}
> > +
> > extern void __xfs_iflock(struct xfs_inode *ip);
> >
> > static inline int xfs_iflock_nowait(struct xfs_inode *ip)
> > @@ -261,16 +266,12 @@ static inline void xfs_iflock(struct xfs_inode *ip)
> >
> > static inline void xfs_ifunlock(struct xfs_inode *ip)
> > {
> > + ASSERT(xfs_isiflocked(ip));
> > xfs_iflags_clear(ip, XFS_IFLOCK);
> > smp_mb();
> > wake_up_bit(&ip->i_flags, __XFS_IFLOCK_BIT);
> > }
> >
> > -static inline int xfs_isiflocked(struct xfs_inode *ip)
> > -{
> > - return xfs_iflags_test(ip, XFS_IFLOCK);
> > -}
> > -
> > /*
> > * Flags for inode locking.
> > * Bit ranges: 1<<1 - 1<<16-1 -- iolock/ilock modes (bitfield)
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
