On Thu, Sep 19, 2013 at 04:05:26PM -0500, Mark Tinguely wrote:
> Add directory inode type feature to mkfs.xfs.
>
> In sb v4, "-n ftype=1" turns on the feature. The feature is still
> automatically turned on for sb v5.
Ok, so you named it "ftype" here. that's what needs to be in the
xfs_info output, and mkfs output....
> Signed-off-by: Mark Tinguely <tinguely@xxxxxxx>
> ---
> man/man8/mkfs.xfs.8 | 7 +++++++
> man/man8/mkfs.xfs.8 | 10 ++++++++++
> mkfs/xfs_mkfs.c | 40 +++++++++++++++++++++++++++-------------
> mkfs/xfs_mkfs.h | 4 +++-
> 3 files changed, 40 insertions(+), 14 deletions(-)
>
> Index: b/man/man8/mkfs.xfs.8
> ===================================================================
> --- a/man/man8/mkfs.xfs.8
> +++ b/man/man8/mkfs.xfs.8
> @@ -517,6 +517,16 @@ option enables ASCII only case-insensiti
> are stored in directories using the case they were created with.
> .IP
> Note: Version 1 directories are not supported.
> +.TP
> +.BI ftype= value
> +Version 4 superblock supports the inode type stored in the directory feature.
It's a very brief description of the feature. Nobody is really going
to understand it from that. You need to mention that allows the
type of object a directory entry points to to be stored in the
directory structure so that inodes don't have to be read to traverse
the filesystem or determine the type of a directory entry. And that
the inforamtion is returned to readdir(3) and getdents(2), so users
should see the those man poages for how to access the
information....
> +.I value
> +can be either 0 or 1.
> +With 0 meaning not supported (default) and 1 meaning supported.
The value is either 0 or 1, with 1 signifiying that filetype
information will be stored in the directory structure. The default
value is 0.
> +.IP
> +Version 5 superblocks automatically support this feature and this
> +setting will be ignored.
Users don't know what a "version 5 superblock" means, really. So
what this should say is something like this:
"When CRCs are enabled via -m crc=1, the ftype functionality is
always enabled. This feature can not be turned off for such
filesystem configurations."
As it is, trying to turn off ftype on a crc enabled filesystem
should throw an error, not be ignored.
> loginternal = 1;
> logversion = 2;
> logagno = logblocks = rtblocks = rtextblocks = 0;
> - Nflag = nlflag = nsflag = nvflag = nci = 0;
> - dirblocklog = dirblocksize = 0;
> + Nflag = niflag = nlflag = nsflag = nvflag = nci = 0;
> + dirftype = dirblocklog = dirblocksize = 0;
two flags?
Also, can you put them on their own line for initialisation rather
than add more to the existing multi-variable assignments..
> dirversion = XFS_DFL_DIR_VERSION;
> qflag = 0;
> imaxpct = inodelog = inopblock = isize = 0;
> @@ -1533,6 +1537,14 @@ main(
> }
> nvflag = 1;
> break;
> + case N_FTYPE:
> + if (!value || *value == '\0')
> + reqval('n', nopts, N_FTYPE);
> + if (niflag)
> + respec('n', nopts, N_FTYPE);
> + dirftype = atoi(value);
> + niflag = 1;
> + break;
So, niflag indicates that the cli option has been set. Where does
that get used? what does the "i" in niflag mean? Wouldn't
"nftype_flag" be a better name?
And then if it is set, it should be rejected if crcs_enabled is also
set, dumping the usage information...
> default:
> unknown('n', value);
> }
> @@ -2434,6 +2446,14 @@ _("size %s specified for log subvolume i
> }
> validate_log_size(logblocks, blocklog, min_logblocks);
>
> + /*
> + * dirent filetype field always enabled on v5 superblocks
> + */
> + if (crcs_enabled) {
> + sbp->sb_features_incompat = XFS_SB_FEAT_INCOMPAT_FTYPE;
> + dirftype = 1;
> + }
So dirftype is set for crc enabled filesystems....
> +
> if (!qflag || Nflag) {
> printf(_(
> "meta-data=%-22s isize=%-6d agcount=%lld, agsize=%lld blks\n"
> @@ -2441,7 +2461,7 @@ _("size %s specified for log subvolume i
> " =%-22s crc=%u\n"
> "data =%-22s bsize=%-6u blocks=%llu, imaxpct=%u\n"
> " =%-22s sunit=%-6u swidth=%u blks\n"
> - "naming =version %-14u bsize=%-6u ascii-ci=%d\n"
> + "naming =version %-14u bsize=%-6u ascii-ci=%d ftype=%d\n"
Yup, you named it "ftype" here....
> "log =%-22s bsize=%-6d blocks=%lld, version=%d\n"
> " =%-22s sectsz=%-5u sunit=%d blks, lazy-count=%d\n"
> "realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
> @@ -2450,7 +2470,7 @@ _("size %s specified for log subvolume i
> "", crcs_enabled,
> "", blocksize, (long long)dblocks, imaxpct,
> "", dsunit, dswidth,
> - dirversion, dirblocksize, nci,
> + dirversion, dirblocksize, nci, dirftype,
dirftype is used here for output...
> logfile, 1 << blocklog, (long long)logblocks,
> logversion, "", lsectorsize, lsunit, lazy_sb_counters,
> rtfile, rtextblocks << blocklog,
> @@ -2512,8 +2532,9 @@ _("size %s specified for log subvolume i
> sbp->sb_logsectlog = 0;
> sbp->sb_logsectsize = 0;
> }
> +
> sbp->sb_features2 = XFS_SB_VERSION2_MKFS(crcs_enabled, lazy_sb_counters,
> - attrversion == 2, !projid16bit, 0);
> + attrversion == 2, !projid16bit, 0, dirftype);
and for setting the v4 feature bit.
Hmmm, that maybe a problem - on v5 filesystems that's setting both
the v4 feature and the v5 feature bit. I don't think that is the
right thing to do here. It might be fine for an incompat v5 feature
bit, but if this was a read-only compat feature bit then the v4
feature bit would prevent the v5 feature bit from working correctly.
Hence for new "dual v4/v5 feature bit features", only the relevant
feature bit for the filesystem version should be set.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
