[PATCH 0/2] xfs: don't require a dirty log on snapshots

[PATCH 0/2] xfs: don't require a dirty log on snapshots

[Eric Sandeen]

Today, freezing an xfs filesystem will add a dummy log entry
after it quiesces the filesystem, to trigger unlinked inode
recovery on the next mount.  This kind of sucks for readonly
snapshots on readonly devices; one must then specify 
"-o ro,norecovery" or mount will fail, because the log appears
dirty.  This is very nonobvious.

I propose improving the situation by moving unlinked inode recovery
out of log replay; check for and process unlinked inodes on every mount,
regardless of log dirtiness.  This way we can create snapshots with
clean logs.

There may be some devils in the details here but it passes a quick
sanity check, so let's see what reviewers think, and see what I've
overlooked...

Thanks,
-Eric

[PATCH 1/2] xfs: always check for and process unlinked inodes on mount

[Eric Sandeen]

Process any unlinked inodes unconditionally; this allows us to
skip dirtying the log on frozen filesystems and still have
proper recovery on the next mount.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 1937a93..2a645c0 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -5854,8 +5854,6 @@ static inline bool xlog_item_is_intent(struct xfs_log_item *lip)
 		 */
 		xfs_log_force(log->l_mp, XFS_LOG_SYNC);
 
-		xlog_recover_process_iunlinks(log);
-
 		xlog_recover_check_summary(log);
 
 		xfs_notice(log->l_mp, "Ending recovery (logdev: %s)",
@@ -5865,6 +5863,14 @@ static inline bool xlog_item_is_intent(struct xfs_log_item *lip)
 	} else {
 		xfs_info(log->l_mp, "Ending clean mount");
 	}
+
+	/*
+	 * Process any unlinked inodes unconditionally, this allows us to
+	 * skip dirtying the log on frozen filesystems and still have
+	 * proper recovery on the next mount.
+	 */
+	xlog_recover_process_iunlinks(log);
+
 	return 0;
 }
 

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

[Eric Sandeen]

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)

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 93588ea..5669525 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1419,9 +1419,10 @@ struct proc_xfs_info {
 
 /*
  * Second stage of a freeze. The data is already frozen so we only
- * need to take care of the metadata. Once that's done sync the superblock
- * to the log to dirty it in case of a crash while frozen. This ensures that we
- * will recover the unlinked inode lists on the next mount.
+ * need to take care of the metadata.
+ * Any unlinked inode lists will remain at this point, and be recovered
+ * on the next writable mount if we crash while frozen, or create
+ * a snapshot from the frozen filesystem.
  */
 STATIC int
 xfs_fs_freeze(
@@ -1431,7 +1432,7 @@ struct proc_xfs_info {
 
 	xfs_save_resvblks(mp);
 	xfs_quiesce_attr(mp);
-	return xfs_sync_sb(mp, true);
+	return 0;
 }
 
 STATIC int

Re: [PATCH 1/2] xfs: always check for and process unlinked inodes on mount

[Eric Sandeen]

I suppose this should be titled "... on log processing"

It happens during the log handling part of mount, but is outside
of dirty log replay.

On 3/7/18 5:32 PM, Eric Sandeen wrote:
> Process any unlinked inodes unconditionally; this allows us to
> skip dirtying the log on frozen filesystems and still have
> proper recovery on the next mount.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 1937a93..2a645c0 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -5854,8 +5854,6 @@ static inline bool xlog_item_is_intent(struct xfs_log_item *lip)
>  		 */
>  		xfs_log_force(log->l_mp, XFS_LOG_SYNC);
>  
> -		xlog_recover_process_iunlinks(log);
> -
>  		xlog_recover_check_summary(log);
>  
>  		xfs_notice(log->l_mp, "Ending recovery (logdev: %s)",
> @@ -5865,6 +5863,14 @@ static inline bool xlog_item_is_intent(struct xfs_log_item *lip)
>  	} else {
>  		xfs_info(log->l_mp, "Ending clean mount");
>  	}
> +
> +	/*
> +	 * Process any unlinked inodes unconditionally, this allows us to
> +	 * skip dirtying the log on frozen filesystems and still have
> +	 * proper recovery on the next mount.
> +	 */
> +	xlog_recover_process_iunlinks(log);
> +
>  	return 0;
>  }
>  
> 
> --
> 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/2] xfs: always check for and process unlinked inodes on mount

[Brian Foster]

On Wed, Mar 07, 2018 at 05:32:29PM -0600, Eric Sandeen wrote:
> Process any unlinked inodes unconditionally; this allows us to
> skip dirtying the log on frozen filesystems and still have
> proper recovery on the next mount.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 1937a93..2a645c0 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -5854,8 +5854,6 @@ static inline bool xlog_item_is_intent(struct xfs_log_item *lip)
>  		 */
>  		xfs_log_force(log->l_mp, XFS_LOG_SYNC);
>  
> -		xlog_recover_process_iunlinks(log);
> -
>  		xlog_recover_check_summary(log);
>  
>  		xfs_notice(log->l_mp, "Ending recovery (logdev: %s)",
> @@ -5865,6 +5863,14 @@ static inline bool xlog_item_is_intent(struct xfs_log_item *lip)
>  	} else {
>  		xfs_info(log->l_mp, "Ending clean mount");
>  	}
> +
> +	/*
> +	 * Process any unlinked inodes unconditionally, this allows us to
> +	 * skip dirtying the log on frozen filesystems and still have
> +	 * proper recovery on the next mount.
> +	 */
> +	xlog_recover_process_iunlinks(log);
> +

The code seems fine. The only nit I have is maybe we should pull down
the "Ending ..." messages until after the iunlinks call.

That aside, this does introduce the same kind of mount delay we've been
batting around wrt to the agfl padding fixup thing. We already have the
scan in some places (perag reservation init calculations, cow blocks
scan), some of which may be able to be removed/replaced going forward. I
think this one falls into the latter category as well, but I'd be fine
with this for the time being so long as the benefit is valuable enough.

Have we considered anything like conditionally dirtying the log on
freeze only when there are open+unlinked files? It seems like that may
be uncommon enough to address the problem for snapshot users
(particularly the read-only use case mentioned in the cover letter), but
that's just a guess.

Brian

>  	return 0;
>  }
>  
> 
> --
> 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/2] xfs: always check for and process unlinked inodes on mount

[Eric Sandeen]

On 3/15/18 8:17 AM, Brian Foster wrote:
> Have we considered anything like conditionally dirtying the log on
> freeze only when there are open+unlinked files? It seems like that may
> be uncommon enough to address the problem for snapshot users
> (particularly the read-only use case mentioned in the cover letter), but
> that's just a guess.
> 
> Brian


I did consider that, and was weighing the advantages with the disadvantages,
namely unpredictable behavior for snapshots...  I'm not sure how uncommon
the situation really is.

Regarding the mount delay, Darrick suggested that maybe we need to get
all these scans into one place if possible, for efficiency.

-Eric

Re: [PATCH 1/2] xfs: always check for and process unlinked inodes on mount

[Brian Foster]

On Thu, Mar 15, 2018 at 08:19:40AM -0400, Eric Sandeen wrote:
> 
> 
> On 3/15/18 8:17 AM, Brian Foster wrote:
> > Have we considered anything like conditionally dirtying the log on
> > freeze only when there are open+unlinked files? It seems like that may
> > be uncommon enough to address the problem for snapshot users
> > (particularly the read-only use case mentioned in the cover letter), but
> > that's just a guess.
> > 
> > Brian
> 
> 
> I did consider that, and was weighing the advantages with the disadvantages,
> namely unpredictable behavior for snapshots...  I'm not sure how uncommon
> the situation really is.
> 

Ok.

> Regarding the mount delay, Darrick suggested that maybe we need to get
> all these scans into one place if possible, for efficiency.
> 

These buffers should be cached after the first scan, right? As Dave
mentioned on one of the previous threads, I think the I/O load is more
of a factor than anything else.

I'd rather see us try to eliminate these kinds of scans rather than
condense them into some kind of infrastructure that tries to
self-justify their existence. But as previously mentioned, I think it's
reasonable to add this one for now and optimize it away separately.

Brian

> -Eric
> --
> 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 2/2] xfs: don't dirty snapshot logs for unlinked inode recovery

[Darrick J. Wong]

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(?)

--D

> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 93588ea..5669525 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1419,9 +1419,10 @@ struct proc_xfs_info {
>  
>  /*
>   * Second stage of a freeze. The data is already frozen so we only
> - * need to take care of the metadata. Once that's done sync the superblock
> - * to the log to dirty it in case of a crash while frozen. This ensures that we
> - * will recover the unlinked inode lists on the next mount.
> + * need to take care of the metadata.
> + * Any unlinked inode lists will remain at this point, and be recovered
> + * on the next writable mount if we crash while frozen, or create
> + * a snapshot from the frozen filesystem.
>   */
>  STATIC int
>  xfs_fs_freeze(
> @@ -1431,7 +1432,7 @@ struct proc_xfs_info {
>  
>  	xfs_save_resvblks(mp);
>  	xfs_quiesce_attr(mp);
> -	return xfs_sync_sb(mp, true);
> +	return 0;
>  }
>  
>  STATIC int
> 
> --
> 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 2/2] xfs: don't dirty snapshot logs for unlinked inode recovery

[Brian Foster]

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.

Brian

> --D
> 
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > ---
> > 
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 93588ea..5669525 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -1419,9 +1419,10 @@ struct proc_xfs_info {
> >  
> >  /*
> >   * Second stage of a freeze. The data is already frozen so we only
> > - * need to take care of the metadata. Once that's done sync the superblock
> > - * to the log to dirty it in case of a crash while frozen. This ensures that we
> > - * will recover the unlinked inode lists on the next mount.
> > + * need to take care of the metadata.
> > + * Any unlinked inode lists will remain at this point, and be recovered
> > + * on the next writable mount if we crash while frozen, or create
> > + * a snapshot from the frozen filesystem.
> >   */
> >  STATIC int
> >  xfs_fs_freeze(
> > @@ -1431,7 +1432,7 @@ struct proc_xfs_info {
> >  
> >  	xfs_save_resvblks(mp);
> >  	xfs_quiesce_attr(mp);
> > -	return xfs_sync_sb(mp, true);
> > +	return 0;
> >  }
> >  
> >  STATIC int
> > 
> > --
> > 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
> --
> 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 2/2] xfs: don't dirty snapshot logs for unlinked inode recovery

[Dave Chinner]

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

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

[Gao Xiang]

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

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

[Eric Sandeen]

On 2/23/21 7:42 AM, Gao Xiang wrote:
> 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?

It's true, a snapshot created with these patches will not have their unlinked
inodes processed if mounted on an older kernel. I'm not sure how much of a
problem that is; the filesystem is not inconsistent, but some space is lost,
I guess. I'm not sure it's common to take a snapshot of a frozen filesystem on
one kernel and then move it back to an older kernel.  Maybe others have
thoughts on this.

-Eric

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

[Gao Xiang]

On Tue, Feb 23, 2021 at 08:40:56AM -0600, Eric Sandeen wrote:
> On 2/23/21 7:42 AM, Gao Xiang wrote:
> > 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?
> 
> It's true, a snapshot created with these patches will not have their unlinked
> inodes processed if mounted on an older kernel. I'm not sure how much of a
> problem that is; the filesystem is not inconsistent, but some space is lost,
> I guess. I'm not sure it's common to take a snapshot of a frozen filesystem on
> one kernel and then move it back to an older kernel.  Maybe others have
> thoughts on this.

My current thought might be only to write clean mount without
unlinked inodes when freezing, but leave log dirty if any
unlinked inodes exist as Brian mentioned before and don't
handle such case (?). I'd like to hear more comments about
this as well.

Thanks,
Gao Xiang

> 
> -Eric
> 

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

[Eric Sandeen]

On 2/23/21 9:03 AM, Gao Xiang wrote:
> On Tue, Feb 23, 2021 at 08:40:56AM -0600, Eric Sandeen wrote:
>> On 2/23/21 7:42 AM, Gao Xiang wrote:
>>> 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?
>>
>> It's true, a snapshot created with these patches will not have their unlinked
>> inodes processed if mounted on an older kernel. I'm not sure how much of a
>> problem that is; the filesystem is not inconsistent, but some space is lost,
>> I guess. I'm not sure it's common to take a snapshot of a frozen filesystem on
>> one kernel and then move it back to an older kernel.  Maybe others have
>> thoughts on this.
> 
> My current thought might be only to write clean mount without
> unlinked inodes when freezing, but leave log dirty if any
> unlinked inodes exist as Brian mentioned before and don't
> handle such case (?). I'd like to hear more comments about
> this as well.

I don't know if I had made this comment before ;) but I feel like that's even
more "surprise" (as in: gets further from the principle of least surprise)
and TBH I would rather not have that somewhat unpredictable behavior.

I think I'd rather /always/ make a dirty log than sometimes do it, other
times not. It'd just be more confusion for the admin IMHO.

Thanks,
-Eric

> Thanks,
> Gao Xiang
> 
>>
>> -Eric
>>
> 

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

[Gao Xiang]

On Tue, Feb 23, 2021 at 09:46:38AM -0600, Eric Sandeen wrote:
> 
> 
> On 2/23/21 9:03 AM, Gao Xiang wrote:
> > On Tue, Feb 23, 2021 at 08:40:56AM -0600, Eric Sandeen wrote:
> >> On 2/23/21 7:42 AM, Gao Xiang wrote:
> >>> 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?
> >>
> >> It's true, a snapshot created with these patches will not have their unlinked
> >> inodes processed if mounted on an older kernel. I'm not sure how much of a
> >> problem that is; the filesystem is not inconsistent, but some space is lost,
> >> I guess. I'm not sure it's common to take a snapshot of a frozen filesystem on
> >> one kernel and then move it back to an older kernel.  Maybe others have
> >> thoughts on this.
> > 
> > My current thought might be only to write clean mount without
> > unlinked inodes when freezing, but leave log dirty if any
> > unlinked inodes exist as Brian mentioned before and don't
> > handle such case (?). I'd like to hear more comments about
> > this as well.
> 
> I don't know if I had made this comment before ;) but I feel like that's even
> more "surprise" (as in: gets further from the principle of least surprise)
> and TBH I would rather not have that somewhat unpredictable behavior.
> 

Yeah, I saw that comment as well....

> I think I'd rather /always/ make a dirty log than sometimes do it, other
> times not. It'd just be more confusion for the admin IMHO.

Ok, some other alternative approaches I could think out in my mind
aren't trivial (e.g. some hack on log recovery, etc).. Any ideas /
thoughts about this are welcomed :) Thanks!

Thanks,
Gao Xiang

> 
> Thanks,
> -Eric
> 
> > Thanks,
> > Gao Xiang
> > 
> >>
> >> -Eric
> >>
> > 
> 

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

[Darrick J. Wong]

On Tue, Feb 23, 2021 at 09:46:38AM -0600, Eric Sandeen wrote:
> 
> 
> On 2/23/21 9:03 AM, Gao Xiang wrote:
> > On Tue, Feb 23, 2021 at 08:40:56AM -0600, Eric Sandeen wrote:
> >> On 2/23/21 7:42 AM, Gao Xiang wrote:
> >>> 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?
> >>
> >> It's true, a snapshot created with these patches will not have their unlinked
> >> inodes processed if mounted on an older kernel. I'm not sure how much of a
> >> problem that is; the filesystem is not inconsistent, but some space is lost,
> >> I guess. I'm not sure it's common to take a snapshot of a frozen filesystem on
> >> one kernel and then move it back to an older kernel.  Maybe others have
> >> thoughts on this.

Yes, I know of cloudy image generation factories that use old versions
of RHEL to generate images that are then frozen and copied to a
deployment system without an unmount.  I don't understand why they
insist that unmount is "too slow" but freeze isn't, nor why they then
file bugs that their instance deploy process is unacceptably slow
because of log recovery.

> > My current thought might be only to write clean mount without
> > unlinked inodes when freezing, but leave log dirty if any
> > unlinked inodes exist as Brian mentioned before and don't
> > handle such case (?). I'd like to hear more comments about
> > this as well.
> 
> I don't know if I had made this comment before ;) but I feel like that's even
> more "surprise" (as in: gets further from the principle of least surprise)
> and TBH I would rather not have that somewhat unpredictable behavior.
> 
> I think I'd rather /always/ make a dirty log than sometimes do it, other
> times not. It'd just be more confusion for the admin IMHO.

...but the next time anyone wants to introduce a new in/rocompat feature
flag for something inode related, then you can disable the "leave a
dirty log on freeze if there are unlinked inodes" behavior.

--D

> 
> Thanks,
> -Eric
> 
> > Thanks,
> > Gao Xiang
> > 
> >>
> >> -Eric
> >>
> >