Re: [PATCH 6/6] xfstests: Add mkfs input validation tests

[Dave Chinner <david@fromorbit.com>]

On Mon, Jul 18, 2016 at 02:33:29PM +0200, Jan Tulak wrote:
> On Mon, Jul 18, 2016 at 1:54 PM, Jan Tulak <jtulak@redhat.com> wrote:
> > On Mon, Jul 18, 2016 at 1:47 PM, Eryu Guan <eguan@redhat.com> wrote:
> >> On Mon, Jul 18, 2016 at 01:29:47PM +0200, Jan Tulak wrote:
> >>> On Mon, Jul 18, 2016 at 1:30 AM, Dave Chinner <david@fromorbit.com> wrote:
> >>> > On Sat, Jul 16, 2016 at 05:33:58PM +0800, Eryu Guan wrote:
> >>> >> On Thu, Jul 14, 2016 at 02:43:34PM +0200, Jan Tulak wrote:
> >>> >> > +do_mkfs_fail -l lazy-count=1garbage $SCRATCH_DEV
> >>> >> > +do_mkfs_fail -l lazy-count=2 $SCRATCH_DEV
> >>> >> > +do_mkfs_fail -l lazy-count=0 -m crc=1 $SCRATCH_DEV
> >>> >> > +do_mkfs_fail -l version=1 -m crc=1 $SCRATCH_DEV
> >>> >>
> >>> >> This test fails in my DAX testing, where SCRATCH_DEV is ramdisk. The
> >>> >> mkfs itself should fail, but it passed. Log version 2 was used
> >>> >> automatically, instead of prompting "V2 logs always enabled for CRC
> >>> >> enabled filesytems"
> >>> >>
> >>> >> [root@dhcp-66-86-11 xfstests]# mkfs -t xfs -f -l version=1 -m crc=1 /dev/ram0
> >>> >> meta-data=/dev/ram0              isize=512    agcount=1, agsize=4096 blks
> >>> >>          =                       sectsz=4096  attr=2, projid32bit=1
> >>> >>          =                       crc=1        finobt=1, sparse=0
> >>> >> data     =                       bsize=4096   blocks=4096, imaxpct=25
> >>> >>          =                       sunit=0      swidth=0 blks
> >>> >> naming   =version 2              bsize=4096   ascii-ci=0 ftype=1
> >>> >> log      =internal log           bsize=4096   blocks=1605, version=2
> >>> >>          =                       sectsz=4096  sunit=1 blks, lazy-count=1
> >>> >> realtime =none                   extsz=4096   blocks=0, rtextents=0
> >>> >>
> >>> >> Is it a mkfs.xfs bug or the test case should handle the special case?
> >>> >
> >>> > Looks like it might be a side effect of using a 4k sector size. v1
> >>> > logs only supported 512 byte sectors, so it's entirely possible that
> >>> > the sector size is silently overriding the log version
> >>> > specification. Probably should be fixed in mkfs.
> >>> >
> >>> >
> >>>
> >>> I tried to duplicate this, but in my config it didn't failed - how did
> >>> you create the ramdisk?
> >>
> >> I think you need to test on a 4k sector size disk. I use scsi_debug to
> >> simulate physical 4k sector disk to reproduce this:
> >>
> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe -r scsi_debug
> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe scsi_debug dev_size_mb=128 physblk_exp=3
> >> [root@dhcp-66-86-11 xfsprogs-dev]# blockdev --getbsz --getpbsz --getss /dev/sdc
> >> 4096
> >> 4096
> >> 512
> >> [root@dhcp-66-86-11 xfsprogs-dev]# mkfs -t xfs -l version=1 -m crc=1 /dev/sdc

So this is an invalid filesystem configuration. It should be
detected as such during command line parsing and rejected before we
get anywhere near checking topology constraints. In mkfs
terms, it's a conflicting option set.

> And the culprit is in mkfs, some forty lines before the crc & log version check:
> 
> 2026 ⇥       } else if (lsectorsize > XFS_MIN_SECTORSIZE && !lsu && !lsunit) {
> 2027 ⇥       ⇥       lsu = blocksize;
> 2028 ⇥       ⇥       sb_feat.log_version = 2;
> 2029 ⇥       }
> 
> The possible solutions I can think of are:

None of which really appeal because, IMO, they are trying to solve
the wrong problem.

The whole point of moving to table based command line option parsing
is that we can encode these sorts of conflicts into the option
table. The conflict resolution in the option table is currently not
complete - it can only encode and detect conflicts within a
suboption type, but not across suboption types (e.g. within -d
suboptions, but not between -d and -l suboptions).

This is simply because I never got as far as implementing this level
of conflict encoding/resolution. In essence, the conflict array
needs to define the sub option type, the suboption that is
in conflict and the value that it conflicts against. Hence the
conflicts table can then encode such things as "version 1 logs are
invalid for CRC enabled filesystems" and vice versa.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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