Re: [PATCH 6/7] xfs: clean up setting m_readio_* / m_writeio_*

From: Eric Sandeen <sandeen@sandeen.net>
To: Christoph Hellwig <hch@lst.de>, linux-xfs@vger.kernel.org
Cc: Ian Kent <raven@themaw.net>
Subject: Re: [PATCH 6/7] xfs: clean up setting m_readio_* / m_writeio_*
Date: Fri, 25 Oct 2019 14:18:02 -0500	[thread overview]
Message-ID: <258d888c-e359-c264-33c3-910ebbd37bac@sandeen.net> (raw)
In-Reply-To: <20191025174026.31878-7-hch@lst.de>

On 10/25/19 12:40 PM, Christoph Hellwig wrote:
> Fill in the default _log values in xfs_parseargs similar to other
> defaults, and open code the updates based on the on-disk superblock
> in xfs_mountfs now that they are completely trivial.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_mount.c | 36 +++++-------------------------------
>  fs/xfs/xfs_super.c |  5 +++++
>  2 files changed, 10 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 9800401a7d6f..bae53fdd5d51 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -425,35 +425,6 @@ xfs_update_alignment(xfs_mount_t *mp)
>  	return 0;
>  }
>  
> -/*
> - * Set the default minimum read and write sizes unless
> - * already specified in a mount option.
> - * We use smaller I/O sizes when the file system
> - * is being used for NFS service (wsync mount option).
> - */
> -STATIC void
> -xfs_set_rw_sizes(xfs_mount_t *mp)
> -{
> -	xfs_sb_t	*sbp = &(mp->m_sb);
> -	int		writeio_log;
> -
> -	if (!(mp->m_flags & XFS_MOUNT_DFLT_IOSIZE)) {
> -		if (mp->m_flags & XFS_MOUNT_WSYNC)
> -			writeio_log = XFS_WRITEIO_LOG_WSYNC;
> -		else
> -			writeio_log = XFS_WRITEIO_LOG_LARGE;
> -	} else {
> -		writeio_log = mp->m_writeio_log;
> -	}
> -
> -	if (sbp->sb_blocklog > writeio_log) {
> -		mp->m_writeio_log = sbp->sb_blocklog;
> -	} else {
> -		mp->m_writeio_log = writeio_log;
> -	}
> -	mp->m_writeio_blocks = 1 << (mp->m_writeio_log - sbp->sb_blocklog);
> -}
> -
>  /*
>   * precalculate the low space thresholds for dynamic speculative preallocation.
>   */
> @@ -718,9 +689,12 @@ xfs_mountfs(
>  		goto out_remove_errortag;
>  
>  	/*
> -	 * Set the minimum read and write sizes
> +	 * Update the preferred write size based on the information from the
> +	 * on-disk superblock.
>  	 */
> -	xfs_set_rw_sizes(mp);
> +	mp->m_writeio_log =
> +		max_t(uint32_t, mp->m_sb.sb_blocklog, mp->m_writeio_log);
> +	mp->m_writeio_blocks = 1 << (mp->m_writeio_log - mp->m_sb.sb_blocklog);
>  
>  	/* set the low space thresholds for dynamic preallocation */
>  	xfs_set_low_space_thresholds(mp);
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 1467f4bebc41..83dbfcc5a02d 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -405,6 +405,11 @@ xfs_parseargs(
>  				XFS_MIN_IO_LOG, XFS_MAX_IO_LOG);
>  			return -EINVAL;
>  		}
> +	} else {
> +		if (mp->m_flags & XFS_MOUNT_WSYNC)
> +			mp->m_writeio_log = XFS_WRITEIO_LOG_WSYNC;
> +		else
> +			mp->m_writeio_log = XFS_WRITEIO_LOG_LARGE;
>  	}

Ok let's see, by here, if Opt_allocsize was specified, we set
mp->m_writeio_log to the specified value, else if Opt_wsync was set, we 
set m_writeio_log to XFS_WRITEIO_LOG_WSYNC (14), otherwise we default to
XFS_WRITEIO_LOG_LARGE (16).  So that's it for parseargs.

AFAICT we can't escape parseargs w/ writeio_log less than PAGE_SHIFT
(i.e. page size).

Then in xfs_mountfs, you have it reset to the max of sb_blocklog and
m_writeio_log.  i.e. it gets resized iff sb_blocklog is greater than
the current m_writeio_log, which has a minimum of page size.

IOWS, it only gets a new value in mountfs if block size is > page size.

Which is a little surprising and nonobvious and it makes me wonder
if you're intentionally future-proofing here, or if that's just weird.
:)

-Eric

>  	return 0;
> 