Re: [PATCH 00/19 v2] mkfs cleaning

From: Dave Chinner <david@fromorbit.com>
To: Jan Tulak <jtulak@redhat.com>
Cc: Eric Sandeen <sandeen@sandeen.net>, xfs-oss <xfs@oss.sgi.com>
Subject: Re: [PATCH 00/19 v2] mkfs cleaning
Date: Fri, 3 Jun 2016 10:53:24 +1000	[thread overview]
Message-ID: <20160603005324.GS12670@dastard> (raw)
In-Reply-To: <CACj3i715X7X65=BjSW8GZLhUF-YjQVShC-_Udmxe24XB1ACb1w@mail.gmail.com>

On Wed, Jun 01, 2016 at 03:19:34PM +0200, Jan Tulak wrote:
> On Tue, May 10, 2016 at 8:10 AM, Dave Chinner <david@fromorbit.com> wrote:
> 
> >
> > Looking at xfstests runs, new failures are:
> >
> > generic/054 -
> > generic/055 - both fail with:
> >
> > +*** mkfs failed: -l version=2,su=4096 ***
> >
> > and the .full file has this specific error:
> >
> > Illegal value 4096 for -l su option. value is too small
> >
> > indicating that we should be allowing (2^N * block size) log
> > stripe units to be set. This will be a limit configuration issue,
> > most likely needing fixing in mkfs.
> >
> 
> I'm looking on it, but this issue was introduced by Eric's fix for
> patch "mkfs:
> table based parsing for converted parameters":
> 
> > 
> The kernel enforces a max of XLOG_MAX_RECORD_BSIZE,
> > 
> and it should match the limits in L_SUNIT after all ...

So nobody got it right, then.  i.e. Eric's changes made it the same
as what was in my original patchset, which means *I got it wrong*,
too.

Really, it's not at all important who made the incorrect change or
why it was wrong - it's easy very make mistakes or get complex
things wrong. What matters is that we identify the problem code, we
fix the problem, and we try not to make the same error again.
Mistakes are a learning opportunity for everyone, not just the
person who wrote or reviewed the code.

> And looking on fs/xfs/xfs_super.c, the MIN value is enforced too. So maybe
> it is the test what needs fixing? Otherwise, I would put something
> like XFS_MIN_SECTORSIZE there.
> 
> See http://lxr.free-electrons.com/source/fs/xfs/xfs_super.c#L435

mp->m_logbsize is the incore log buffer size and this is checking
that the iclogsize passed in as a mount option is within the valid
range (16k->256k). It is not checking stripe unit alignment limits.
We can write an iclogbuf at any sector offset in the log; the log
stripe unit is simply an alignment guide. i.e. a lsunit = 4096 is 8
sector alignment, so the iclogbuf when written is padded out to 8
sector boundaries so that log writes are always aligned and sized to
the log stripe unit.

The only actual limit on iclogbuf size vs lsunit is that the
iclogbuf size must be >= than the lsunit, which limits the lsunit
to XLOG_MAX_RECORD_BSIZE. i.e valid range for specifying lsunit on
the command line is 1 <= lsunit <= XLOG_MAX_RECORD_BSIZE.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs