| Message ID | 20200601214251.4167140-2-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: rework inode flushing to make inode reclaim fully asynchronous | expand |
On Tue, Jun 02, 2020 at 07:42:22AM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > In tracking down a problem in this patchset, I discovered we are > reclaiming dirty stale inodes. This wasn't discovered until inodes > were always attached to the cluster buffer and then the rcu callback > that freed inodes was assert failing because the inode still had an > active pointer to the cluster buffer after it had been reclaimed. > > Debugging the issue indicated that this was a pre-existing issue > resulting from the way the inodes are handled in xfs_inactive_ifree. > When we free a cluster buffer from xfs_ifree_cluster, all the inodes > in cache are marked XFS_ISTALE. Those that are clean have nothing > else done to them and so eventually get cleaned up by background > reclaim. i.e. it is assumed we'll never dirty/relog an inode marked > XFS_ISTALE. > > On journal commit dirty stale inodes as are handled by both > buffer and inode log items to run though xfs_istale_done() and > removed from the AIL (buffer log item commit) or the log item will > simply unpin it because the buffer log item will clean it. What happens > to any specific inode is entirely dependent on which log item wins > the commit race, but the result is the same - stale inodes are > clean, not attached to the cluster buffer, and not in the AIL. Hence > inode reclaim can just free these inodes without further care. > > However, if the stale inode is relogged, it gets dirtied again and > relogged into the CIL. Most of the time this isn't an issue, because > relogging simply changes the inode's location in the current > checkpoint. Problems arise, however, when the CIL checkpoints > between two transactions in the xfs_inactive_ifree() deferops > processing. This results in the XFS_ISTALE inode being redirtied > and inserted into the CIL without any of the other stale cluster > buffer infrastructure being in place. > > Hence on journal commit, it simply gets unpinned, so it remains > dirty in memory. Everything in inode writeback avoids XFS_ISTALE > inodes so it can't be written back, and it is not tracked in the AIL > so there's not even a trigger to attempt to clean the inode. Hence > the inode just sits dirty in memory until inode reclaim comes along, > sees that it is XFS_ISTALE, and goes to reclaim it. This reclaiming > of a dirty inode caused use after free, list corruptions and other > nasty issues later in this patchset. > > Hence this patch addresses a violation of the "never log XFS_ISTALE > inodes" caused by the deferops processing rolling a transaction > and relogging a stale inode in xfs_inactive_free. It also adds a > bunch of asserts to catch this problem in debug kernels so that > we don't reintroduce this problem in future. > > Reproducer for this issue was generic/558 on a v4 filesystem. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/libxfs/xfs_trans_inode.c | 2 ++ > fs/xfs/xfs_icache.c | 3 ++- > fs/xfs/xfs_inode.c | 25 ++++++++++++++++++++++--- > 3 files changed, 26 insertions(+), 4 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c > index b5dfb66548422..4504d215cd590 100644 > --- a/fs/xfs/libxfs/xfs_trans_inode.c > +++ b/fs/xfs/libxfs/xfs_trans_inode.c > @@ -36,6 +36,7 @@ xfs_trans_ijoin( > > ASSERT(iip->ili_lock_flags == 0); > iip->ili_lock_flags = lock_flags; > + ASSERT(!xfs_iflags_test(ip, XFS_ISTALE)); > > /* > * Get a log_item_desc to point at the new item. > @@ -89,6 +90,7 @@ xfs_trans_log_inode( > > ASSERT(ip->i_itemp != NULL); > ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL)); > + ASSERT(!xfs_iflags_test(ip, XFS_ISTALE)); > > /* > * Don't bother with i_lock for the I_DIRTY_TIME check here, as races > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c > index 0a5ac6f9a5834..dbba4c1946386 100644 > --- a/fs/xfs/xfs_icache.c > +++ b/fs/xfs/xfs_icache.c > @@ -1141,7 +1141,7 @@ xfs_reclaim_inode( > goto out_ifunlock; > xfs_iunpin_wait(ip); > } > - if (xfs_iflags_test(ip, XFS_ISTALE) || xfs_inode_clean(ip)) { > + if (xfs_inode_clean(ip)) { > xfs_ifunlock(ip); > goto reclaim; > } > @@ -1228,6 +1228,7 @@ xfs_reclaim_inode( > xfs_ilock(ip, XFS_ILOCK_EXCL); > xfs_qm_dqdetach(ip); > xfs_iunlock(ip, XFS_ILOCK_EXCL); > + ASSERT(xfs_inode_clean(ip)); > > __xfs_inode_free(ip); > return error; > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 64f5f9a440aed..53a1d64782c35 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -1740,10 +1740,31 @@ xfs_inactive_ifree( > return error; > } > > + /* > + * We do not hold the inode locked across the entire rolling transaction > + * here. We only need to hold it for the first transaction that > + * xfs_ifree() builds, which may mark the inode XFS_ISTALE if the > + * underlying cluster buffer is freed. Relogging an XFS_ISTALE inode > + * here breaks the relationship between cluster buffer invalidation and > + * stale inode invalidation on cluster buffer item journal commit > + * completion, and can result in leaving dirty stale inodes hanging > + * around in memory. > + * > + * We have no need for serialising this inode operation against other > + * operations - we freed the inode and hence reallocation is required > + * and that will serialise on reallocating the space the deferops need > + * to free. Hence we can unlock the inode on the first commit of > + * the transaction rather than roll it right through the deferops. This > + * avoids relogging the XFS_ISTALE inode. Hmm. What defer ops causes a transaction roll? Is it the EFI that frees the inode cluster blocks? > + * > + * We check that xfs_ifree() hasn't grown an internal transaction roll > + * by asserting that the inode is still locked when it returns. > + */ > xfs_ilock(ip, XFS_ILOCK_EXCL); > - xfs_trans_ijoin(tp, ip, 0); > + xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL); This looks right to me since we should be marking the inode free in the first transaction and therefore should not keep it attached to the transaction... Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > error = xfs_ifree(tp, ip); > + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL)); > if (error) { > /* > * If we fail to free the inode, shut down. The cancel > @@ -1756,7 +1777,6 @@ xfs_inactive_ifree( > xfs_force_shutdown(mp, SHUTDOWN_META_IO_ERROR); > } > xfs_trans_cancel(tp); > - xfs_iunlock(ip, XFS_ILOCK_EXCL); > return error; > } > > @@ -1774,7 +1794,6 @@ xfs_inactive_ifree( > xfs_notice(mp, "%s: xfs_trans_commit returned error %d", > __func__, error); > > - xfs_iunlock(ip, XFS_ILOCK_EXCL); > return 0; > } > > -- > 2.26.2.761.g0e0b3e54be >
On Mon, Jun 01, 2020 at 09:30:52PM -0700, Darrick J. Wong wrote: > On Tue, Jun 02, 2020 at 07:42:22AM +1000, Dave Chinner wrote: > > --- a/fs/xfs/xfs_inode.c > > +++ b/fs/xfs/xfs_inode.c > > @@ -1740,10 +1740,31 @@ xfs_inactive_ifree( > > return error; > > } > > > > + /* > > + * We do not hold the inode locked across the entire rolling transaction > > + * here. We only need to hold it for the first transaction that > > + * xfs_ifree() builds, which may mark the inode XFS_ISTALE if the > > + * underlying cluster buffer is freed. Relogging an XFS_ISTALE inode > > + * here breaks the relationship between cluster buffer invalidation and > > + * stale inode invalidation on cluster buffer item journal commit > > + * completion, and can result in leaving dirty stale inodes hanging > > + * around in memory. > > + * > > + * We have no need for serialising this inode operation against other > > + * operations - we freed the inode and hence reallocation is required > > + * and that will serialise on reallocating the space the deferops need > > + * to free. Hence we can unlock the inode on the first commit of > > + * the transaction rather than roll it right through the deferops. This > > + * avoids relogging the XFS_ISTALE inode. > > Hmm. What defer ops causes a transaction roll? Is it the EFI that > frees the inode cluster blocks? Yeah, xfs_difree_inode_chunk() calls xfs_bmap_add_free() which goes through the deferops to free the inode chunk extent(s). I suspect that we can also get xfs_alloc_fix_freelist() deferring AGFL frees here too. I basically just assume anything that allocates or frees extents is likely to have at least one deferop as a result of this.... > > + * > > + * We check that xfs_ifree() hasn't grown an internal transaction roll > > + * by asserting that the inode is still locked when it returns. > > + */ > > xfs_ilock(ip, XFS_ILOCK_EXCL); > > - xfs_trans_ijoin(tp, ip, 0); > > + xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL); > > This looks right to me since we should be marking the inode free in the > first transaction and therefore should not keep it attached to the > transaction... *nod* That was my thinking about the problem, too. > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> Thanks! -Dave.
On Tue, Jun 02, 2020 at 07:42:22AM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > In tracking down a problem in this patchset, I discovered we are > reclaiming dirty stale inodes. This wasn't discovered until inodes > were always attached to the cluster buffer and then the rcu callback > that freed inodes was assert failing because the inode still had an > active pointer to the cluster buffer after it had been reclaimed. > > Debugging the issue indicated that this was a pre-existing issue > resulting from the way the inodes are handled in xfs_inactive_ifree. > When we free a cluster buffer from xfs_ifree_cluster, all the inodes > in cache are marked XFS_ISTALE. Those that are clean have nothing > else done to them and so eventually get cleaned up by background > reclaim. i.e. it is assumed we'll never dirty/relog an inode marked > XFS_ISTALE. > > On journal commit dirty stale inodes as are handled by both > buffer and inode log items to run though xfs_istale_done() and > removed from the AIL (buffer log item commit) or the log item will > simply unpin it because the buffer log item will clean it. What happens > to any specific inode is entirely dependent on which log item wins > the commit race, but the result is the same - stale inodes are > clean, not attached to the cluster buffer, and not in the AIL. Hence > inode reclaim can just free these inodes without further care. > > However, if the stale inode is relogged, it gets dirtied again and > relogged into the CIL. Most of the time this isn't an issue, because > relogging simply changes the inode's location in the current > checkpoint. Problems arise, however, when the CIL checkpoints > between two transactions in the xfs_inactive_ifree() deferops > processing. This results in the XFS_ISTALE inode being redirtied > and inserted into the CIL without any of the other stale cluster > buffer infrastructure being in place. > > Hence on journal commit, it simply gets unpinned, so it remains > dirty in memory. Everything in inode writeback avoids XFS_ISTALE > inodes so it can't be written back, and it is not tracked in the AIL > so there's not even a trigger to attempt to clean the inode. Hence > the inode just sits dirty in memory until inode reclaim comes along, > sees that it is XFS_ISTALE, and goes to reclaim it. This reclaiming > of a dirty inode caused use after free, list corruptions and other > nasty issues later in this patchset. > > Hence this patch addresses a violation of the "never log XFS_ISTALE > inodes" caused by the deferops processing rolling a transaction > and relogging a stale inode in xfs_inactive_free. It also adds a > bunch of asserts to catch this problem in debug kernels so that > we don't reintroduce this problem in future. > > Reproducer for this issue was generic/558 on a v4 filesystem. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- Reviewed-by: Brian Foster <bfoster@redhat.com> > fs/xfs/libxfs/xfs_trans_inode.c | 2 ++ > fs/xfs/xfs_icache.c | 3 ++- > fs/xfs/xfs_inode.c | 25 ++++++++++++++++++++++--- > 3 files changed, 26 insertions(+), 4 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c > index b5dfb66548422..4504d215cd590 100644 > --- a/fs/xfs/libxfs/xfs_trans_inode.c > +++ b/fs/xfs/libxfs/xfs_trans_inode.c > @@ -36,6 +36,7 @@ xfs_trans_ijoin( > > ASSERT(iip->ili_lock_flags == 0); > iip->ili_lock_flags = lock_flags; > + ASSERT(!xfs_iflags_test(ip, XFS_ISTALE)); > > /* > * Get a log_item_desc to point at the new item. > @@ -89,6 +90,7 @@ xfs_trans_log_inode( > > ASSERT(ip->i_itemp != NULL); > ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL)); > + ASSERT(!xfs_iflags_test(ip, XFS_ISTALE)); > > /* > * Don't bother with i_lock for the I_DIRTY_TIME check here, as races > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c > index 0a5ac6f9a5834..dbba4c1946386 100644 > --- a/fs/xfs/xfs_icache.c > +++ b/fs/xfs/xfs_icache.c > @@ -1141,7 +1141,7 @@ xfs_reclaim_inode( > goto out_ifunlock; > xfs_iunpin_wait(ip); > } > - if (xfs_iflags_test(ip, XFS_ISTALE) || xfs_inode_clean(ip)) { > + if (xfs_inode_clean(ip)) { > xfs_ifunlock(ip); > goto reclaim; > } > @@ -1228,6 +1228,7 @@ xfs_reclaim_inode( > xfs_ilock(ip, XFS_ILOCK_EXCL); > xfs_qm_dqdetach(ip); > xfs_iunlock(ip, XFS_ILOCK_EXCL); > + ASSERT(xfs_inode_clean(ip)); > > __xfs_inode_free(ip); > return error; > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 64f5f9a440aed..53a1d64782c35 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -1740,10 +1740,31 @@ xfs_inactive_ifree( > return error; > } > > + /* > + * We do not hold the inode locked across the entire rolling transaction > + * here. We only need to hold it for the first transaction that > + * xfs_ifree() builds, which may mark the inode XFS_ISTALE if the > + * underlying cluster buffer is freed. Relogging an XFS_ISTALE inode > + * here breaks the relationship between cluster buffer invalidation and > + * stale inode invalidation on cluster buffer item journal commit > + * completion, and can result in leaving dirty stale inodes hanging > + * around in memory. > + * > + * We have no need for serialising this inode operation against other > + * operations - we freed the inode and hence reallocation is required > + * and that will serialise on reallocating the space the deferops need > + * to free. Hence we can unlock the inode on the first commit of > + * the transaction rather than roll it right through the deferops. This > + * avoids relogging the XFS_ISTALE inode. > + * > + * We check that xfs_ifree() hasn't grown an internal transaction roll > + * by asserting that the inode is still locked when it returns. > + */ > xfs_ilock(ip, XFS_ILOCK_EXCL); > - xfs_trans_ijoin(tp, ip, 0); > + xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL); > > error = xfs_ifree(tp, ip); > + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL)); > if (error) { > /* > * If we fail to free the inode, shut down. The cancel > @@ -1756,7 +1777,6 @@ xfs_inactive_ifree( > xfs_force_shutdown(mp, SHUTDOWN_META_IO_ERROR); > } > xfs_trans_cancel(tp); > - xfs_iunlock(ip, XFS_ILOCK_EXCL); > return error; > } > > @@ -1774,7 +1794,6 @@ xfs_inactive_ifree( > xfs_notice(mp, "%s: xfs_trans_commit returned error %d", > __func__, error); > > - xfs_iunlock(ip, XFS_ILOCK_EXCL); > return 0; > } > > -- > 2.26.2.761.g0e0b3e54be >
diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c index b5dfb66548422..4504d215cd590 100644 --- a/fs/xfs/libxfs/xfs_trans_inode.c +++ b/fs/xfs/libxfs/xfs_trans_inode.c @@ -36,6 +36,7 @@ xfs_trans_ijoin( ASSERT(iip->ili_lock_flags == 0); iip->ili_lock_flags = lock_flags; + ASSERT(!xfs_iflags_test(ip, XFS_ISTALE)); /* * Get a log_item_desc to point at the new item. @@ -89,6 +90,7 @@ xfs_trans_log_inode( ASSERT(ip->i_itemp != NULL); ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL)); + ASSERT(!xfs_iflags_test(ip, XFS_ISTALE)); /* * Don't bother with i_lock for the I_DIRTY_TIME check here, as races diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c index 0a5ac6f9a5834..dbba4c1946386 100644 --- a/fs/xfs/xfs_icache.c +++ b/fs/xfs/xfs_icache.c @@ -1141,7 +1141,7 @@ xfs_reclaim_inode( goto out_ifunlock; xfs_iunpin_wait(ip); } - if (xfs_iflags_test(ip, XFS_ISTALE) || xfs_inode_clean(ip)) { + if (xfs_inode_clean(ip)) { xfs_ifunlock(ip); goto reclaim; } @@ -1228,6 +1228,7 @@ xfs_reclaim_inode( xfs_ilock(ip, XFS_ILOCK_EXCL); xfs_qm_dqdetach(ip); xfs_iunlock(ip, XFS_ILOCK_EXCL); + ASSERT(xfs_inode_clean(ip)); __xfs_inode_free(ip); return error; diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 64f5f9a440aed..53a1d64782c35 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -1740,10 +1740,31 @@ xfs_inactive_ifree( return error; } + /* + * We do not hold the inode locked across the entire rolling transaction + * here. We only need to hold it for the first transaction that + * xfs_ifree() builds, which may mark the inode XFS_ISTALE if the + * underlying cluster buffer is freed. Relogging an XFS_ISTALE inode + * here breaks the relationship between cluster buffer invalidation and + * stale inode invalidation on cluster buffer item journal commit + * completion, and can result in leaving dirty stale inodes hanging + * around in memory. + * + * We have no need for serialising this inode operation against other + * operations - we freed the inode and hence reallocation is required + * and that will serialise on reallocating the space the deferops need + * to free. Hence we can unlock the inode on the first commit of + * the transaction rather than roll it right through the deferops. This + * avoids relogging the XFS_ISTALE inode. + * + * We check that xfs_ifree() hasn't grown an internal transaction roll + * by asserting that the inode is still locked when it returns. + */ xfs_ilock(ip, XFS_ILOCK_EXCL); - xfs_trans_ijoin(tp, ip, 0); + xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL); error = xfs_ifree(tp, ip); + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL)); if (error) { /* * If we fail to free the inode, shut down. The cancel @@ -1756,7 +1777,6 @@ xfs_inactive_ifree( xfs_force_shutdown(mp, SHUTDOWN_META_IO_ERROR); } xfs_trans_cancel(tp); - xfs_iunlock(ip, XFS_ILOCK_EXCL); return error; } @@ -1774,7 +1794,6 @@ xfs_inactive_ifree( xfs_notice(mp, "%s: xfs_trans_commit returned error %d", __func__, error); - xfs_iunlock(ip, XFS_ILOCK_EXCL); return 0; }