[PATCH] xfs: unset MS_ACTIVE if mount fails

[PATCH] xfs: unset MS_ACTIVE if mount fails

[Darrick J. Wong]

As part of the inode block map intent log item recovery process, we had
to set the IRECOVERY flag to prevent an unlinked inode from being
truncated during the first iput call.  This required us to set MS_ACTIVE
so that iput puts the inode on the lru instead of immediately evicting
the inode.

Unfortunately, if the mount fails later on, the inodes that have been
loaded (root dir and realtime) actually need to be evicted since we're
aborting the mount.  If we don't clear MS_ACTIVE in the failure step,
those inodes are not evicted and therefore leak.   The leak was found
by running xfs/130 and rmmoding xfs immediately after the test.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_mount.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index fc78739..b341f10 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -1009,6 +1009,7 @@ xfs_mountfs(
  out_quota:
 	xfs_qm_unmount_quotas(mp);
  out_rtunmount:
+	mp->m_super->s_flags &= ~MS_ACTIVE;
 	xfs_rtunmount_inodes(mp);
  out_rele_rip:
 	IRELE(rip);

Re: [PATCH] xfs: unset MS_ACTIVE if mount fails

[Christoph Hellwig]

On Mon, Oct 17, 2016 at 06:10:51PM -0700, Darrick J. Wong wrote:
> As part of the inode block map intent log item recovery process, we had
> to set the IRECOVERY flag to prevent an unlinked inode from being
> truncated during the first iput call.  This required us to set MS_ACTIVE
> so that iput puts the inode on the lru instead of immediately evicting
> the inode.
> 
> Unfortunately, if the mount fails later on, the inodes that have been
> loaded (root dir and realtime) actually need to be evicted since we're
> aborting the mount.  If we don't clear MS_ACTIVE in the failure step,
> those inodes are not evicted and therefore leak.   The leak was found
> by running xfs/130 and rmmoding xfs immediately after the test.

Actually that was an item I wanted to sort out about reflink before
we got it merged..

Can you explain why we even added the MS_ACTIVE flag?  Is it
to fool iput_final into keeping the inode on the lru?  I'd really
prefer if we could just do it in our own ->drop_inode call then
instead of messing with MS_ACTIVE.

Re: [PATCH] xfs: unset MS_ACTIVE if mount fails

[Darrick J. Wong]

On Mon, Oct 17, 2016 at 10:24:13PM -0700, Christoph Hellwig wrote:
> On Mon, Oct 17, 2016 at 06:10:51PM -0700, Darrick J. Wong wrote:
> > As part of the inode block map intent log item recovery process, we had
> > to set the IRECOVERY flag to prevent an unlinked inode from being
> > truncated during the first iput call.  This required us to set MS_ACTIVE
> > so that iput puts the inode on the lru instead of immediately evicting
> > the inode.
> > 
> > Unfortunately, if the mount fails later on, the inodes that have been
> > loaded (root dir and realtime) actually need to be evicted since we're
> > aborting the mount.  If we don't clear MS_ACTIVE in the failure step,
> > those inodes are not evicted and therefore leak.   The leak was found
> > by running xfs/130 and rmmoding xfs immediately after the test.
> 
> Actually that was an item I wanted to sort out about reflink before
> we got it merged..
> 
> Can you explain why we even added the MS_ACTIVE flag?  Is it
> to fool iput_final into keeping the inode on the lru?

In a way, yes.  We don't need to keep it on the LRU necessarily, we
just need it not to think "Oh, this inode has i_nlink == 0 so we can
truncate all the blocks and free the inode" prior to the end of bmap
intent processing.

> I'd really prefer if we could just do it in our own ->drop_inode call
> then instead of messing with MS_ACTIVE.

The difficulty here is that without MS_ACTIVE, the vfs will always
evict the inode when i_count == 0.  I suppose bmap intent processing
could bump up i_count and stash the inode somewhere to prevent the
i_count from hitting zero until after we're done recovering the log...
...but otherwise I'm sorta stumped about how to do that.

(Not so much 'stumped' as 'getting a headcold', bleargh)

--D

Re: [PATCH] xfs: unset MS_ACTIVE if mount fails

[Christoph Hellwig]

On Mon, Oct 17, 2016 at 10:38:49PM -0700, Darrick J. Wong wrote:
> In a way, yes.  We don't need to keep it on the LRU necessarily, we
> just need it not to think "Oh, this inode has i_nlink == 0 so we can
> truncate all the blocks and free the inode" prior to the end of bmap
> intent processing.
> 
> > I'd really prefer if we could just do it in our own ->drop_inode call
> > then instead of messing with MS_ACTIVE.
> 
> The difficulty here is that without MS_ACTIVE, the vfs will always
> evict the inode when i_count == 0.  I suppose bmap intent processing
> could bump up i_count and stash the inode somewhere to prevent the
> i_count from hitting zero until after we're done recovering the log...
> ...but otherwise I'm sorta stumped about how to do that.

Let me try to understand the issue here.

We get a final iput for an inode that recover once i_count hits zero.
We then want to hit the "if (!drop && (sb->s_flags & MS_ACTIVE)) {"
conditional there to add it to the lru, or at least not truncate it.
I guess for now we'll need MS_ACTIVE early here, yes.

So reluctantly:

Reviewed-by: Christoph Hellwig <hch@lst.de>