On Fri, Aug 11, 2017 at 12:50:10PM -0700, Darrick J. Wong wrote:
> When we introduced the bmap redo log items, we set MS_ACTIVE on the
> mountpoint and XFS_IRECOVERY on the inode to prevent unlinked inodes
> from being truncated prematurely during log recovery. This also had the
> effect of putting linked inodes on the lru instead of evicting them.
>
> Unfortunately, we neglected to find all those unreferenced lru inodes
> and evict them after finishing log recovery, which means that we leak
> them if anything goes wrong in the rest of xfs_mountfs, because the lru
> is only cleaned out on unmount.
>
> Therefore, evict unreferenced inodes in the lru list immediately
> after clearing MS_ACTIVE.
>
> Fixes: 17c12bcd30 ("xfs: when replaying bmap operations, don't let unlinked inodes get reaped")
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> Cc: viro@xxxxxxxxxxxxxxxxxx
> ---
> v2: use the vfs evict_inodes instead of duplicating it
> ---
> fs/inode.c | 1 +
> fs/internal.h | 1 -
> fs/xfs/xfs_mount.c | 12 ++++++++++++
> include/linux/fs.h | 1 +
> 4 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 5037059..6a1626e 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -637,6 +637,7 @@ void evict_inodes(struct super_block *sb)
>
> dispose_list(&dispose);
> }
> +EXPORT_SYMBOL_GPL(evict_inodes);
>
> /**
> * invalidate_inodes - attempt to free all inodes on a superblock
> diff --git a/fs/internal.h b/fs/internal.h
> index 9676fe1..fedfe94 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -132,7 +132,6 @@ static inline bool atime_needs_update_rcu(const struct path *path,
> extern void inode_io_list_del(struct inode *inode);
>
> extern long get_nr_dirty_inodes(void);
> -extern void evict_inodes(struct super_block *);
> extern int invalidate_inodes(struct super_block *, bool);
>
> /*
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index a46c9d7..351e2c3 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -956,10 +956,22 @@ xfs_mountfs(
> * inodes. Turn it off immediately after xfs_log_mount_finish
> * so that we don't leak the quota inodes if subsequent mount
> * activities fail.
> + *
> + * We let all inodes involved in redo item processing end up on
> + * the LRU instead of being evicted immediately so that if we do
> + * something to an unlinked inode, the irele won't cause
> + * premature truncation and freeing of the inode, which results
> + * in log recovery failure. We have to evict the unreferenced
> + * lru inodes after clearing MS_ACTIVE because we don't
> + * otherwise clean up the lru if there's a subsequent failure in
> + * xfs_mountfs, which leads to us leaking the inodes if nothing
> + * else (e.g. quotacheck) references the inodes before the
> + * mount failure occurs.
> */
> mp->m_super->s_flags |= MS_ACTIVE;
> error = xfs_log_mount_finish(mp);
> mp->m_super->s_flags &= ~MS_ACTIVE;
> + evict_inodes(mp->m_super);
Shouldn't all this MS_ACTIVE flag and inode eviction stuff be put
inside xfs_log_mount_finish()? Seems to me like wrapping it aroudn
the outside is the wrong place to be putting it...
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
--
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
