Re: [PATCH 00/19 v2] mkfs cleaning

[Jan Tulak <jtulak@xxxxxxxxxx>]

On Fri, Jun 3, 2016 at 2:53 AM, Dave Chinner <david@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxÂ/ jan@xxxxxxxx