On Thu, Jun 9, 2016 at 6:23 PM, Eric Sandeen <sandeen@xxxxxxxxxxx> wrote:
>
>
> On 6/9/16 8:41 AM, Jan Tulak wrote:
>> lsunit was already tested, but lsu was not. So a thing like -l su=4097 was
>> possible. This commit adds a check to fix it.
>>
>> Signed-off-by: Jan Tulak <jtulak@xxxxxxxxxx>
>> ---
>> mkfs/xfs_mkfs.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>> index 455bf11..b9b50fe 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -2875,6 +2875,15 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
>> lsunit = dsunit;
>> }
>>
>> + if (lsu) {
>> + if (lsu % blocksize != 0) {
>> + fprintf(stderr,
>> + _("log stripe unit (%d) must be a multiple of the block size (%d)\n"),
>> + lsu, blocksize);
>> + exit(1);
>> + }
>> + }
>> +
>
>
> Hm, I'm not quite seeing the behavior you say you're fixing;
> you give the example of -l su=4097, but:
>
> # mkfs/mkfs.xfs -f -dfile,name=fsfile,size=128m -l su=4097
> Illegal value 4097 for -l su option. value is too small
>
> So that's really not the relevant test case.
>
Well, but 4097 is not too small - 4096 should pass and few days ago, I
send a patch to fix that ("mkfs: fix -l su minval"). But yeah, I
didn't realized that when writing the commit message and didn't told
you that. :-)
>
> And in these cases it is caught and does fail:
>
> # mkfs/mkfs.xfs -f -dfile,name=fsfile,size=128m -l su=63k
> log stripe unit (64512) must be a multiple of the block size (4096)
>
> # mkfs/mkfs.xfs -f -dfile,name=fsfile,size=128m -l su=64512
> log stripe unit (64512) must be a multiple of the block size (4096)
>
> But in this case it does not:
>
> # mkfs/mkfs.xfs -f -dfile,name=fsfile,size=128m -l su=16385
> meta-data=fsfile isize=512 agcount=4, agsize=8192 blks
> = sectsz=512 attr=2, projid32bit=1
> = crc=1 finobt=1, sparse=0
> data = bsize=4096 blocks=32768, imaxpct=25
> = sunit=0 swidth=0 blks
> naming =version 2 bsize=4096 ascii-ci=0 ftype=1
> log =internal log bsize=4096 blocks=552, version=2
> = sectsz=512 sunit=4 blks, lazy-count=1
> realtime =none extsz=4096 blocks=0, rtextents=0
>
> So what's going on here...? It has something to do with how
> calc_stripe_factors() operates, and when that happens vs. the checks.
>
> lsu and lsunit are really specifying the same thing, just in different
> units.
>
> Your patch does fix it up, but I wonder if these tests are in the
> wrong place; with your patch we essentially do this:
>
> 1) Parse lsu value
> 2) Convert lsu to lsunit
> 3) Check lsunit (it's ok)
> 4) Go back and check lsu (it's not ok)
>
> It seems to me that we should recognize either lsu or lsunit
> as the "one true value" and convert the other to it as soon as possible,
> rather than carrying around both, and checking both at various times.
>
> calc_stripe_factors() does do some validity checks; perhaps it should
> check against blocksize multiples as well, so it's all in one place?
> And a comment about what that function is doing (converting su's to
> sunit's, really) would be helpful.
>
> What do you think?
>
> Your patch does resolve the problem but I think it adds a little more
> confusion to already confusing code.
>
I moved both the new check and the lsunit check to
calc_stripe_factor(). We need to check both, though, because the
entire issue stems from how lsunit is computed from lsu: *lsunit =
(int)BTOBBT(lsu);
So if lsu is incorrectly sized, we still get a valid lsunit. It just
might be different from what user thought...
Thanks for pointing it out, though.
Jan
--
Jan Tulak
jtulak@xxxxxxxxxx / jan@xxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
