<div dir="ltr"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jun 3, 2016 at 2:53 AM, Dave Chinner <span dir="ltr"><<a href="mailto:david@fromorbit.com" target="_blank">david@fromorbit.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><span class="">On Wed, Jun 01, 2016 at 03:19:34PM +0200, Jan Tulak wrote:<br>
> On Tue, May 10, 2016 at 8:10 AM, Dave Chinner <<a href="mailto:david@fromorbit.com">david@fromorbit.com</a>> wrote:<br>
><br>
> ><br>
> > Looking at xfstests runs, new failures are:<br>
> ><br>
> > generic/054 -<br>
> > generic/055 - both fail with:<br>
> ><br>
> > +*** mkfs failed: -l version=2,su=4096 ***<br>
> ><br>
> > and the .full file has this specific error:<br>
> ><br>
> > Illegal value 4096 for -l su option. value is too small<br>
> ><br>
> > indicating that we should be allowing (2^N * block size) log<br>
> > stripe units to be set. This will be a limit configuration issue,<br>
> > most likely needing fixing in mkfs.<br>
> ><br>
><br>
> ​I'm looking on it, but this issue was introduced by Eric's fix for<br>
> patch "mkfs:<br>
> table based parsing for converted parameters":<br>
><br>
> ​> ​<br>
> The kernel enforces a max of XLOG_MAX_RECORD_BSIZE,<br>
> ​> ​<br>
> and it should match the limits in L_SUNIT after all ...<br>
<br>
</span>So nobody got it right, then.  i.e. Eric's changes made it the same<br>
as what was in my original patchset, which means *I got it wrong*,<br>
too.<br>
<br>
Really, it's not at all important who made the incorrect change or<br>
why it was wrong - it's easy very make mistakes or get complex<br>
things wrong. What matters is that we identify the problem code, we<br>
fix the problem, and we try not to make the same error again.<br>
Mistakes are a learning opportunity for everyone, not just the<br>
person who wrote or reviewed the code.<br>
<span class=""><br></span></blockquote><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline">​Yeah, I was just trying to get together all the info. ​</div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><span class="">
> And looking on fs/xfs/xfs_super.c, the MIN value is enforced too. So maybe<br>
> it is the test what needs fixing? Otherwise, I would put something<br>
> like XFS_MIN_SECTORSIZE there.<br>
><br>
> See <a href="http://lxr.free-electrons.com/source/fs/xfs/xfs_super.c#L435" rel="noreferrer" target="_blank">http://lxr.free-electrons.com/source/fs/xfs/xfs_super.c#L435</a><br>
<br>
</span>mp->m_logbsize is the incore log buffer size and this is checking<br>
that the iclogsize passed in as a mount option is within the valid<br>
range (16k->256k). It is not checking stripe unit alignment limits.<br>
We can write an iclogbuf at any sector offset in the log; the log<br>
stripe unit is simply an alignment guide. i.e. a lsunit = 4096 is 8<br>
sector alignment, so the iclogbuf when written is padded out to 8<br>
sector boundaries so that log writes are always aligned and sized to<br>
the log stripe unit.<br>
<br>
The only actual limit on iclogbuf size vs lsunit is that the<br>
iclogbuf size must be >= than the lsunit, which limits the lsunit<br>
to XLOG_MAX_RECORD_BSIZE. i.e valid range for specifying lsunit on<br>
the command line is 1 <= lsunit <= XLOG_MAX_RECORD_BSIZE.<br>
<div class=""><div class="h5"></div></div></blockquote></div><div class="gmail_extra"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">OK. Only, it seems we mixed lsu and lsunit. From manpage: lsunit is specified in 512-bytes block units. </div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">So the correct ranges for both lsunit and lsu should be:</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><span style="font-family:arial,sans-serif">1 <= lsunit <= BTOBB(XLOG_MAX_RECORD_BSIZE)</span></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><span style="font-family:arial,sans-serif">BBTOB(1) <= lsu <= </span><span style="font-family:arial,sans-serif">XLOG_MAX_RECORD_BSIZE</span></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><span style="font-family:arial,sans-serif"><br></span></div><div class="gmail_default">Which will be further limited from the bottom by the physical block size, but that check is already in place and working.</div><div class="gmail_extra"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">If this sounds ok, I will send the patch.</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">Cheers,</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">Jan</div><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div>Jan Tulak<br></div><a href="mailto:jtulak@redhat.com" target="_blank">jtulak@redhat.com</a> / <a href="mailto:jan@tulak.me" target="_blank">jan@tulak.me</a></div></div></div></div>
</div></div>