[PATCH 00/19 v2] mkfs cleaning

[Jan Tulak]

On Fri, Jun 3, 2016 at 2:53 AM, Dave Chinner <david at fromorbit.com> wrote:

> 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 at 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.
>
> ​Yeah, I was just trying to get together all the info. ​


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

OK. Only, it seems we mixed lsu and lsunit. From manpage: lsunit is
specified in 512-bytes block units.

So the correct ranges for both lsunit and lsu should be:
1 <= lsunit <= BTOBB(XLOG_MAX_RECORD_BSIZE)
BBTOB(1) <= lsu <= XLOG_MAX_RECORD_BSIZE

Which will be further limited from the bottom by the physical block size,
but that check is already in place and working.

If this sounds ok, I will send the patch.

Cheers,
Jan


-- 
Jan Tulak
jtulak at redhat.com / jan at tulak.me
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://oss.sgi.com/pipermail/xfs/attachments/20160603/9d2dba03/attachment.html>