[RFC PATCH] xfs: Fix logbsize validation

[RFC PATCH] xfs: Fix logbsize validation

[cem]

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.

 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;
+
+	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,
+		     "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;
+		} 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

Re: [RFC PATCH] xfs: Fix logbsize validation

[Darrick J. Wong]

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

Re: [RFC PATCH] xfs: Fix logbsize validation

[Carlos Maiolino]

On Tue, Aug 12, 2025 at 07:16:55PM -0700, Darrick J. Wong wrote:
> 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?

quick group seems ok, I'll run something more complete today

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

I did this to pack the lines length a bit, I initially was writing it
inline to xfs_finish_flags(), then decided to move it to a different
function and I kept the local vars.

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

Yeah, sorry, will fix that.

> 
> > +		     "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.

Fair enough.

> 
> 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. :/

Heh, I couldn't find anything that really prevents log buffers to be
non-power-of-to, in fact, that's what happens today when we don't
specify a logbsize, but we have a non-power-of-2 log stripe unit.

FWIW, I thought a bit about the possibility to simply prevent log stripe
units to be a non-power-of-2, but the whole reason the user who reported
this hit this problem was that he's using a SSD reporting 192k stripe.
Quoting his own words:

"if you are wondering why I am using a 48b stripe, ask the ssd
manufacturer"

So, it seems to me that restricting log sunit to be power-of-2 aligned,
is not gonna play well with some hardware out there.


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

Re: [RFC PATCH] xfs: Fix logbsize validation

[Carlos Maiolino]

On Tue, Aug 12, 2025 at 07:16:55PM -0700, Darrick J. Wong wrote:
> 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?

Tests looks mostly fine, there were a few extra failures which seemed
all related to tests which uses a custom geometry, not compatible with
those I was forcing on all the tests, I'll double check if there is
anything suspicious, and if everything is ok, I'll send a proper non-rfc
version.

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

Re: [RFC PATCH] xfs: Fix logbsize validation

[Dave Chinner]

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.

The xfs(5) man page explicitly lists a set of fixed sizes that are
supported, all of which are a power of two.

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

log_sunit is set by mkfs, so the user cannot change that
dynmaically. LSU is generally something that should not be
overridden by users - it is set by mkfs for optimal performance on
devices that require specific write alignment to avoid costly RMW
cycles...

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

Have you tested what happens when LSU is set to, say 72kB and
logbsize is set to 216kB? (i.e. integer multiple of 3). What about a
LSU of 19kB on a 1kB filesystem and a logbsize of 247kB? 

These are weird and wacky configurations, but this change allows
users to create them. i.e. it greatly expands the test matrix for
these mount and mkfs options by adding an entirely new
dimension to LSU configuration testing matrix.

Does the benefit that this change provide outweigh the risk of
uncovering some hidden corner case in the iclog alignment code? And
failure here will result in journal corruption, and that may be very
difficult to detect as it will only be noticed if there are
subsequent recovery failures. That's a pretty nasty failure mode -
it's difficult to triage and find the root cause, and it guarantees
data loss and downtime for the user because it can only be fixed by
running xfs_repair to zero the log.

So, do the risks associated with greatly expanding the supported
iclogbuf/LSU configurations outweigh the benefits that users will
gain from having this expanded functionality?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC PATCH] xfs: Fix logbsize validation

[Carlos Maiolino]

On Thu, Aug 14, 2025 at 08:23:58AM +1000, Dave Chinner wrote:
> 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.
> 
> The xfs(5) man page explicitly lists a set of fixed sizes that are
> supported, all of which are a power of two.

Right, but yet if a log sunit is set, mount will automatically set to a
different value (if sunit is not any of those values). This is when it
gets confused.

> 
> > 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.
> 
> log_sunit is set by mkfs, so the user cannot change that
> dynmaically. LSU is generally something that should not be
> overridden by users - it is set by mkfs for optimal performance on
> devices that require specific write alignment to avoid costly RMW
> cycles...
> 
> > 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.
> 
> Have you tested what happens when LSU is set to, say 72kB and
> logbsize is set to 216kB? (i.e. integer multiple of 3). What about a
> LSU of 19kB on a 1kB filesystem and a logbsize of 247kB?

So far I did test only a 96k logbsize with 32k LSU, I need more time to
test different configurations, but I see where you are getting to.


> 
> These are weird and wacky configurations, but this change allows
> users to create them. i.e. it greatly expands the test matrix for
> these mount and mkfs options by adding an entirely new
> dimension to LSU configuration testing matrix.

Right, agreed.

> 
> Does the benefit that this change provide outweigh the risk of
> uncovering some hidden corner case in the iclog alignment code? And
> failure here will result in journal corruption, and that may be very
> difficult to detect as it will only be noticed if there are
> subsequent recovery failures. That's a pretty nasty failure mode -
> it's difficult to triage and find the root cause, and it guarantees
> data loss and downtime for the user because it can only be fixed by
> running xfs_repair to zero the log.
> 
> So, do the risks associated with greatly expanding the supported
> iclogbuf/LSU configurations outweigh the benefits that users will
> gain from having this expanded functionality?

Agree. It's not worth the increased complexity, I'll update the manpage
only to reflect the current behavior.

Thanks for the review Dave.

Carlos,

> 
> Cheers,
> 
> Dave.
> --
> Dave Chinner
> david@fromorbit.com