Re: [PATCH 3/3] xfsprog: add mkfs.xfs sb v4 support for dirent filetype field

[Dave Chinner <david@fromorbit.com>]

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@sgi.com>
> ---
> 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@fromorbit.com

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