xfs
[Top] [All Lists]

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

To: Mark Tinguely <tinguely@xxxxxxx>
Subject: Re: [PATCH 3/3] xfsprog: add mkfs.xfs sb v4 support for dirent filetype field
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 20 Sep 2013 09:15:17 +1000
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130919211539.216012307@xxxxxxx>
References: <20130919211523.407741285@xxxxxxx> <20130919211539.216012307@xxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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

<Prev in Thread] Current Thread [Next in Thread>