Re: [PATCH 2/9] xfs: lift writable fs check up into log worker task

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Jan 06, 2021 at 12:41:20PM -0500, Brian Foster wrote:
> The log covering helper checks whether the filesystem is writable to
> determine whether to cover the log. The helper is currently only
> called from the background log worker. In preparation to reuse the
> helper from freezing contexts, lift the check into xfs_log_worker().
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_log.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index b445e63cbc3c..4137ed007111 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1050,13 +1050,11 @@ xfs_log_space_wake(
>   * can't start trying to idle the log until both the CIL and AIL are empty.
>   */
>  static int

I think this is a predicate, right?  Should this function return a bool
instead of an int?

This function always confuses me slightly since it pushes us through the
covering state machine, and (I think) assumes that someone will force
the CIL and push the AIL if it returns zero. :)

To check my thinking further-- back in that thread I started about
setting and clearing log incompat flags, I think Dave was pushing me to
clear the log incompat flags just before we call xfs_sync_sb when the
log is in NEED2 state, right?

AFAICT the net effect of this series is to reorder the log code so that
xfs_log_quiesce covers the log (force cil, push ail, log two
transactions containing only the superblock), and adds an xfs_log_clean
that quiesces the log and then writes an unmount record after that.

Two callers whose behavior does not change with this series are: 1) The
log worker quiesces the log when it's idle; and 2) unmount quiesces the
log and then writes an unmount record so that the next mount knows it
can skip replay entirely.

The big difference is 3) freeze now only covers the log, whereas before
it would cover, write an unmount record, and immediately redirty the log
to force replay of the snapshot, right?

Assuming I understood all that, my next question is: Eric Sandeen was
working on a patchset to process unlinked inodes unconditionally on
mount so that frozen fs images can be written out with the unmount
record (because I guess people make ro snapshots of live fs images and
then balk when they have to make the snapshot rw to run log recovery.
Any thoughts about /that/? :)

--D

> -xfs_log_need_covered(xfs_mount_t *mp)
> +xfs_log_need_covered(
> +	struct xfs_mount	*mp)
>  {
> -	struct xlog	*log = mp->m_log;
> -	int		needed = 0;
> -
> -	if (!xfs_fs_writable(mp, SB_FREEZE_WRITE))
> -		return 0;
> +	struct xlog		*log = mp->m_log;
> +	int			needed = 0;
>  
>  	if (!xlog_cil_empty(log))
>  		return 0;
> @@ -1271,7 +1269,7 @@ xfs_log_worker(
>  	struct xfs_mount	*mp = log->l_mp;
>  
>  	/* dgc: errors ignored - not fatal and nowhere to report them */
> -	if (xfs_log_need_covered(mp)) {
> +	if (xfs_fs_writable(mp, SB_FREEZE_WRITE) && xfs_log_need_covered(mp)) {
>  		/*
>  		 * Dump a transaction into the log that contains no real change.
>  		 * This is needed to stamp the current tail LSN into the log
> -- 
> 2.26.2
>