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@xxxxxxxxxx wrote:
> > From: Carlos Maiolino <cem@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> >
> > 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
> >
> >
