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

Message ID d35d1067-a6f6-09be-9c4c-3e08a3bec3be@sandeen.net
State New
Headers show

Commit Message

Eric Sandeen March 7, 2018, 11:32 p.m. UTC
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>
---


--
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

Comments

Eric Sandeen March 8, 2018, 12:41 a.m. UTC | #1
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
> 
--
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
Brian Foster March 15, 2018, 12:17 p.m. UTC | #2
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
--
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
Eric Sandeen March 15, 2018, 12:19 p.m. UTC | #3
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
--
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
Brian Foster March 15, 2018, 12:41 p.m. UTC | #4
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
--
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

Patch
diff mbox

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;
 }