Re: [RFC PATCH] xfs: Fix logbsize validation

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Aug 12, 2025 at 02:43:41PM +0200, cem@kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
> 
> An user reported an inconsistency while mounting a V2 log filesystem
> with logbsize equals to the stripe unit being used (192k).
> 
> The current validation algorithm for the log buffer size enforces the
> user to pass a power_of_2 value between [16k-256k].
> The manpage dictates the log buffer size must be a multiple of the log
> stripe unit, but doesn't specify it must be a power_of_2. Also, if
> logbsize is not specified at mount time, it will be set to
> max(32768, log_sunit), where log_sunit not necessarily is a power_of_2.
> 
> It does seem to me then that logbsize being a power_of_2 constraint must
> be relaxed if there is a configured log stripe unit, so this patch
> updates the logbsize validation logic to ensure that:
> 
> - It can only be set to a specific range [16k-256k]
> 
> - Will be aligned to log stripe unit when the latter is set,
>   and will be at least the same size as the log stripe unit.
> 
> - Enforce it to be power_of_2 aligned when log stripe unit is not set.
> 
> This is achieved by factoring out the logbsize validation to a separated
> function to avoid a big chain of if conditionals
> 
> While at it, update m_logbufs and m_logbsize conditionals in
> xfs_fs_validate_params from:
> 	(x != -1 && x != 0) to (x > 0)
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> 
> I am sending this as a RFC because although I did some basic testing,
> xfstests is still running, so I can't tell yet if it will fail on some configuration even though I am not expecting it to.

Heheh, how did that go?

>  fs/xfs/xfs_super.c | 57 ++++++++++++++++++++++++++++++++--------------
>  1 file changed, 40 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index bb0a82635a77..38d3d8a0b026 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1059,6 +1059,29 @@ xfs_fs_unfreeze(
>  	return 0;
>  }
>  
> +STATIC int
> +xfs_validate_logbsize(
> +	struct xfs_mount	*mp)
> +{
> +	int			logbsize = mp->m_logbsize;
> +	uint32_t		logsunit = mp->m_sb.sb_logsunit;

Why not access the fields directly instead of going through convenience
variables?

> +	if (logsunit > 1) {
> +		if (logbsize < logsunit ||
> +		    logbsize % logsunit) {
> +			xfs_warn(mp,
> +		"logbuf size must be a multiple of the log stripe unit");
> +			return -EINVAL;
> +		}
> +	} else {
> +		if (!is_power_of_2(logbsize)) {
> +		    xfs_warn(mp,

Odd indenting here  ^^^

> +		     "invalid logbufsize: %d [not a power of 2]", logbsize);
> +		    return -EINVAL;
> +		}
> +	}
> +	return 0;
> +}
>  /*
>   * This function fills in xfs_mount_t fields based on mount args.
>   * Note: the superblock _has_ now been read in.
> @@ -1067,16 +1090,13 @@ STATIC int
>  xfs_finish_flags(
>  	struct xfs_mount	*mp)
>  {
> -	/* Fail a mount where the logbuf is smaller than the log stripe */
>  	if (xfs_has_logv2(mp)) {
> -		if (mp->m_logbsize <= 0 &&
> -		    mp->m_sb.sb_logsunit > XLOG_BIG_RECORD_BSIZE) {
> -			mp->m_logbsize = mp->m_sb.sb_logsunit;
> -		} else if (mp->m_logbsize > 0 &&
> -			   mp->m_logbsize < mp->m_sb.sb_logsunit) {
> -			xfs_warn(mp,
> -		"logbuf size must be greater than or equal to log stripe size");
> -			return -EINVAL;
> +		if (mp->m_logbsize > 0) {
> +			if (xfs_validate_logbsize(mp))
> +				return -EINVAL;

If you're going to have xfs_validate_logbsize return an errno, then
capture it and return that, instead squashing them all to EINVAL.

AFAICT it's no big deal to have non-power-of-two log buffers, right?
I *think* everything looks ok, but I'm no expert in the bottom levels of
the xfs logging code. :/

--D

> +		} else {
> +			if (mp->m_sb.sb_logsunit > XLOG_BIG_RECORD_BSIZE)
> +				mp->m_logbsize = mp->m_sb.sb_logsunit;
>  		}
>  	} else {
>  		/* Fail a mount if the logbuf is larger than 32K */
> @@ -1628,8 +1648,7 @@ xfs_fs_validate_params(
>  		return -EINVAL;
>  	}
>  
> -	if (mp->m_logbufs != -1 &&
> -	    mp->m_logbufs != 0 &&
> +	if (mp->m_logbufs > 0 &&
>  	    (mp->m_logbufs < XLOG_MIN_ICLOGS ||
>  	     mp->m_logbufs > XLOG_MAX_ICLOGS)) {
>  		xfs_warn(mp, "invalid logbufs value: %d [not %d-%d]",
> @@ -1637,14 +1656,18 @@ xfs_fs_validate_params(
>  		return -EINVAL;
>  	}
>  
> -	if (mp->m_logbsize != -1 &&
> -	    mp->m_logbsize !=  0 &&
> +	/*
> +	 * We have not yet read the superblock, so we can't check against
> +	 * logsunit here.
> +	 */
> +	if (mp->m_logbsize > 0 &&
>  	    (mp->m_logbsize < XLOG_MIN_RECORD_BSIZE ||
> -	     mp->m_logbsize > XLOG_MAX_RECORD_BSIZE ||
> -	     !is_power_of_2(mp->m_logbsize))) {
> +	     mp->m_logbsize > XLOG_MAX_RECORD_BSIZE)) {
>  		xfs_warn(mp,
> -			"invalid logbufsize: %d [not 16k,32k,64k,128k or 256k]",
> -			mp->m_logbsize);
> +			"invalid logbufsize: %d [not in range %dk-%dk]",
> +			mp->m_logbsize,
> +			(XLOG_MIN_RECORD_BSIZE/1024),
> +			(XLOG_MAX_RECORD_BSIZE/1024));
>  		return -EINVAL;
>  	}
>  
> -- 
> 2.50.1
> 
>