Re: [PATCH 2/2] xfs: don't dirty snapshot logs for unlinked inode recovery

[Gao Xiang <hsiangkao@redhat.com>]

Hi folks,

On Wed, Mar 28, 2018 at 08:17:28AM +1100, Dave Chinner wrote:
> On Mon, Mar 26, 2018 at 08:46:49AM -0400, Brian Foster wrote:
> > On Sat, Mar 24, 2018 at 09:20:49AM -0700, Darrick J. Wong wrote:
> > > On Wed, Mar 07, 2018 at 05:33:48PM -0600, Eric Sandeen wrote:
> > > > Now that unlinked inode recovery is done outside of
> > > > log recovery, there is no need to dirty the log on
> > > > snapshots just to handle unlinked inodes.  This means
> > > > that readonly snapshots can be mounted without requiring
> > > > -o ro,norecovery to avoid the log replay that can't happen
> > > > on a readonly block device.
> > > > 
> > > > (unlinked inodes will just hang out in the agi buckets until
> > > > the next writable mount)
> > > 
> > > FWIW I put these two in a test kernel to see what would happen and
> > > generic/311 failures popped up.  It looked like the _check_scratch_fs
> > > found incorrect block counts on the snapshot(?)
> > > 
> > 
> > Interesting. Just a wild guess, but perhaps it has something to do with
> > lazy sb accounting..? I see we call xfs_initialize_perag_data() when
> > mounting an unclean fs.
> 
> The freeze is calls xfs_log_sbcount() which should update the
> superblock counters from the in-memory counters and write them to
> disk.
> 
> If they are out, I'm guessing it's because the in-memory per-ag
> reservations are not being returned to the global pool before the
> in-memory counters are summed during a freeze....
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

I spend some time on tracking this problem. I've made a quick
modification with per-AG reservation and tested with generic/311
it seems fine. My current question is that how such fsfreezed
images (with clean mount) work with old kernels without [PATCH 1/1]?
I'm afraid orphan inodes won't be freed with such old kernels....
Am I missing something?

Thanks,
Gao Xiang

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 06041834daa3..79d6d8858dcf 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -963,12 +963,17 @@ xfs_log_quiesce(
 	return xfs_log_cover(mp);
 }
 
-void
+int
 xfs_log_clean(
 	struct xfs_mount	*mp)
 {
-	xfs_log_quiesce(mp);
+	int ret;
+
+	ret = xfs_log_quiesce(mp);
+	if (ret)
+		return ret;
 	xfs_log_unmount_write(mp);
+	return 0;
 }
 
 /*
diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
index 044e02cb8921..4061a219bfde 100644
--- a/fs/xfs/xfs_log.h
+++ b/fs/xfs/xfs_log.h
@@ -139,7 +139,7 @@ bool	xfs_log_item_in_current_chkpt(struct xfs_log_item *lip);
 
 void	xfs_log_work_queue(struct xfs_mount *mp);
 int	xfs_log_quiesce(struct xfs_mount *mp);
-void	xfs_log_clean(struct xfs_mount *mp);
+int	xfs_log_clean(struct xfs_mount *mp);
 bool	xfs_log_check_lsn(struct xfs_mount *, xfs_lsn_t);
 bool	xfs_log_in_recovery(struct xfs_mount *);
 
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 97f31308de03..3ef21f589d6b 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3478,6 +3478,7 @@ xlog_recover_finish(
 						     : "internal");
 		log->l_flags &= ~XLOG_RECOVERY_NEEDED;
 	} else {
+		xlog_recover_process_iunlinks(log);
 		xfs_info(log->l_mp, "Ending clean mount");
 	}
 	return 0;
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 21b1d034aca3..0db1e7e0e0c8 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -884,7 +884,7 @@ xfs_fs_freeze(
 {
 	struct xfs_mount	*mp = XFS_M(sb);
 	unsigned int		flags;
-	int			ret;
+	int			error;
 
 	/*
 	 * The filesystem is now frozen far enough that memory reclaim
@@ -893,10 +893,25 @@ xfs_fs_freeze(
 	 */
 	flags = memalloc_nofs_save();
 	xfs_blockgc_stop(mp);
+
+	/* Get rid of any leftover CoW reservations... */
+	error = xfs_blockgc_free_space(mp, NULL);
+	if (error) {
+		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
+		return error;
+	}
+
+	/* Free the per-AG metadata reservation pool. */
+	error = xfs_fs_unreserve_ag_blocks(mp);
+	if (error) {
+		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
+		return error;
+	}
+
 	xfs_save_resvblks(mp);
-	ret = xfs_log_quiesce(mp);
+	error = xfs_log_clean(mp);
 	memalloc_nofs_restore(flags);
-	return ret;
+	return error;
 }
 
 STATIC int
@@ -904,10 +919,26 @@ xfs_fs_unfreeze(
 	struct super_block	*sb)
 {
 	struct xfs_mount	*mp = XFS_M(sb);
+	int error;
 
 	xfs_restore_resvblks(mp);
 	xfs_log_work_queue(mp);
+
+	/* Recover any CoW blocks that never got remapped. */
+	error = xfs_reflink_recover_cow(mp);
+	if (error) {
+		xfs_err(mp,
+			"Error %d recovering leftover CoW allocations.", error);
+		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
+		return error;
+	}
+
 	xfs_blockgc_start(mp);
+
+	/* Create the per-AG metadata reservation pool .*/
+	error = xfs_fs_reserve_ag_blocks(mp);
+	if (error && error != -ENOSPC)
+		return error;
 	return 0;
 }
 
@@ -1440,7 +1471,6 @@ xfs_fs_fill_super(
 #endif
 	}
 
-	/* Filesystem claims it needs repair, so refuse the mount. */
 	if (xfs_sb_version_needsrepair(&mp->m_sb)) {
 		xfs_warn(mp, "Filesystem needs repair.  Please run xfs_repair.");
 		error = -EFSCORRUPTED;
-- 
2.18.1