[PATCH 1/3] xfs: clear MS_ACTIVE after finishing log recovery

[PATCH 1/3] xfs: clear MS_ACTIVE after finishing log recovery

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Way back when we established inode block-map redo log items, it was
discovered that we needed to prevent the VFS from evicting inodes during
log recovery because any given inode might be have bmap redo items to
replay even if the inode has no link count and is ultimately deleted,
and any eviction of an unlinked inode causes the inode to be truncated
and freed too early.

To make this possible, we set MS_ACTIVE so that inodes would not be torn
down immediately upon release.  Unfortunately, this also results in the
quota inodes not being released at all if a later part of the mount
process should fail, because we never reclaim the inodes.  So, set
MS_ACTIVE right before we do the last part of log recovery and clear it
immediately after we finish the log recovery so that everything
will be torn down properly if we abort the mount.

Fixes: 17c12bcd30 ("xfs: when replaying bmap operations, don't let unlinked inodes get reaped")
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_log.c   |   11 +++++++++++
 fs/xfs/xfs_mount.c |   10 ----------
 2 files changed, 11 insertions(+), 10 deletions(-)


diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 0053bcf..4ebd0ba 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -749,9 +749,20 @@ xfs_log_mount_finish(
 		return 0;
 	}
 
+	/*
+	 * During the second phase of log recovery, we need iget and
+	 * iput to behave like they do for an active filesystem.
+	 * xfs_fs_drop_inode needs to be able to prevent the deletion
+	 * of inodes before we're done replaying log items on those
+	 * inodes.  Turn it off immediately after recovery finishes
+	 * so that we don't leak the quota inodes if subsequent mount
+	 * activities fail.
+	 */
+	mp->m_super->s_flags |= MS_ACTIVE;
 	error = xlog_recover_finish(mp->m_log);
 	if (!error)
 		xfs_log_work_queue(mp);
+	mp->m_super->s_flags &= ~MS_ACTIVE;
 
 	return error;
 }
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 40d4e8b..151a82db 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -945,15 +945,6 @@ xfs_mountfs(
 	}
 
 	/*
-	 * During the second phase of log recovery, we need iget and
-	 * iput to behave like they do for an active filesystem.
-	 * xfs_fs_drop_inode needs to be able to prevent the deletion
-	 * of inodes before we're done replaying log items on those
-	 * inodes.
-	 */
-	mp->m_super->s_flags |= MS_ACTIVE;
-
-	/*
 	 * Finish recovering the file system.  This part needed to be delayed
 	 * until after the root and real-time bitmap inodes were consistently
 	 * read in.
@@ -1028,7 +1019,6 @@ 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);

[PATCH 2/3] xfs: don't leak quotacheck dquots when cow recovery

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

If we fail a mount on account of cow recovery errors, it's possible that
a previous quotacheck left some dquots in memory.  The bailout clause of
xfs_mountfs forgets to purge these, and so we leak them.  Fix that.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_mount.c |    2 ++
 1 file changed, 2 insertions(+)


diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 151a82db..ea7d4b4 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -1024,6 +1024,8 @@ xfs_mountfs(
 	IRELE(rip);
 	cancel_delayed_work_sync(&mp->m_reclaim_work);
 	xfs_reclaim_inodes(mp, SYNC_WAIT);
+	/* Clean out dquots that might be in memory after quotacheck. */
+	xfs_qm_unmount(mp);
  out_log_dealloc:
 	mp->m_flags |= XFS_MOUNT_UNMOUNTING;
 	xfs_log_mount_cancel(mp);

[PATCH 3/3] xfs: evict all inodes involved with log redo item

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

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@oracle.com>
Cc: viro@ZenIV.linux.org.uk
---
 fs/inode.c         |    1 +
 fs/internal.h      |    1 -
 fs/xfs/xfs_log.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_log.c b/fs/xfs/xfs_log.c
index 4ebd0ba..1c594e3 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -757,12 +757,24 @@ xfs_log_mount_finish(
 	 * inodes.  Turn it off immediately after recovery finishes
 	 * 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 = xlog_recover_finish(mp->m_log);
 	if (!error)
 		xfs_log_work_queue(mp);
 	mp->m_super->s_flags &= ~MS_ACTIVE;
+	evict_inodes(mp->m_super);
 
 	return error;
 }
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 7b5d681..e730438 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2830,6 +2830,7 @@ static inline void lockdep_annotate_inode_mutex_key(struct inode *inode) { };
 #endif
 extern void unlock_new_inode(struct inode *);
 extern unsigned int get_next_ino(void);
+extern void evict_inodes(struct super_block *sb);
 
 extern void __iget(struct inode * inode);
 extern void iget_failed(struct inode *);

Re: [PATCH 3/3] xfs: evict all inodes involved with log redo item

[Dave Chinner]

On Mon, Aug 14, 2017 at 03:07:19PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> 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.

That's because if we fail xfs_mountfs() we haven't yet set up
sb->s_root so generic_shutdown_super() won't call evict_inodes(),
right? Is there anything else we might miss from the generic
shutdown path that we need to do here?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/3] xfs: evict all inodes involved with log redo item

[Darrick J. Wong]

On Tue, Aug 15, 2017 at 12:16:02PM +1000, Dave Chinner wrote:
> On Mon, Aug 14, 2017 at 03:07:19PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > 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.
> 
> That's because if we fail xfs_mountfs() we haven't yet set up
> sb->s_root so generic_shutdown_super() won't call evict_inodes(),
> right? Is there anything else we might miss from the generic
> shutdown path that we need to do here?

I don't /think/ so?  Maybe I should sleep on that, though. :)

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] xfs: clear MS_ACTIVE after finishing log recovery

[Brian Foster]

On Mon, Aug 14, 2017 at 03:07:03PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Way back when we established inode block-map redo log items, it was
> discovered that we needed to prevent the VFS from evicting inodes during
> log recovery because any given inode might be have bmap redo items to
> replay even if the inode has no link count and is ultimately deleted,
> and any eviction of an unlinked inode causes the inode to be truncated
> and freed too early.
> 
> To make this possible, we set MS_ACTIVE so that inodes would not be torn
> down immediately upon release.  Unfortunately, this also results in the
> quota inodes not being released at all if a later part of the mount
> process should fail, because we never reclaim the inodes.  So, set
> MS_ACTIVE right before we do the last part of log recovery and clear it
> immediately after we finish the log recovery so that everything
> will be torn down properly if we abort the mount.
> 
> Fixes: 17c12bcd30 ("xfs: when replaying bmap operations, don't let unlinked inodes get reaped")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log.c   |   11 +++++++++++
>  fs/xfs/xfs_mount.c |   10 ----------
>  2 files changed, 11 insertions(+), 10 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 0053bcf..4ebd0ba 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -749,9 +749,20 @@ xfs_log_mount_finish(
>  		return 0;
>  	}
>  
> +	/*
> +	 * During the second phase of log recovery, we need iget and
> +	 * iput to behave like they do for an active filesystem.
> +	 * xfs_fs_drop_inode needs to be able to prevent the deletion
> +	 * of inodes before we're done replaying log items on those
> +	 * inodes.  Turn it off immediately after recovery finishes
> +	 * so that we don't leak the quota inodes if subsequent mount
> +	 * activities fail.
> +	 */
> +	mp->m_super->s_flags |= MS_ACTIVE;
>  	error = xlog_recover_finish(mp->m_log);
>  	if (!error)
>  		xfs_log_work_queue(mp);
> +	mp->m_super->s_flags &= ~MS_ACTIVE;
>  
>  	return error;
>  }
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 40d4e8b..151a82db 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -945,15 +945,6 @@ xfs_mountfs(
>  	}
>  
>  	/*
> -	 * During the second phase of log recovery, we need iget and
> -	 * iput to behave like they do for an active filesystem.
> -	 * xfs_fs_drop_inode needs to be able to prevent the deletion
> -	 * of inodes before we're done replaying log items on those
> -	 * inodes.
> -	 */
> -	mp->m_super->s_flags |= MS_ACTIVE;
> -
> -	/*
>  	 * Finish recovering the file system.  This part needed to be delayed
>  	 * until after the root and real-time bitmap inodes were consistently
>  	 * read in.
> @@ -1028,7 +1019,6 @@ 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);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] xfs: evict all inodes involved with log redo item

[Brian Foster]

On Mon, Aug 14, 2017 at 03:07:19PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> 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@oracle.com>
> Cc: viro@ZenIV.linux.org.uk
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/inode.c         |    1 +
>  fs/internal.h      |    1 -
>  fs/xfs/xfs_log.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_log.c b/fs/xfs/xfs_log.c
> index 4ebd0ba..1c594e3 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -757,12 +757,24 @@ xfs_log_mount_finish(
>  	 * inodes.  Turn it off immediately after recovery finishes
>  	 * 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 = xlog_recover_finish(mp->m_log);
>  	if (!error)
>  		xfs_log_work_queue(mp);
>  	mp->m_super->s_flags &= ~MS_ACTIVE;
> +	evict_inodes(mp->m_super);
>  
>  	return error;
>  }
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 7b5d681..e730438 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2830,6 +2830,7 @@ static inline void lockdep_annotate_inode_mutex_key(struct inode *inode) { };
>  #endif
>  extern void unlock_new_inode(struct inode *);
>  extern unsigned int get_next_ino(void);
> +extern void evict_inodes(struct super_block *sb);
>  
>  extern void __iget(struct inode * inode);
>  extern void iget_failed(struct inode *);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html