Re: [RFC][PATCH 11/11] xfs: use common file type conversion

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Mon, Dec 19, 2016 at 10:11:08PM +0200, Amir Goldstein wrote:
> deduplicate the xfs file type conversion implementation.
> 
> xfs readdir code may expose DT_WHT type to user if that
> type was set on disk, but xfs code never set a file type
> of WHT on disk.
> 
> If it is acceptable to expose to user DT_UNKNOWN in case
> WHT type somehow got to disk, then xfs_dir3_filetype_table
> could also be replaced with the common fs_dtype() helper.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_da_format.h |  5 +++--
>  fs/xfs/libxfs/xfs_dir2.c      | 17 -----------------
>  fs/xfs/libxfs/xfs_dir2.h      |  6 ------
>  fs/xfs/xfs_iops.c             |  2 +-
>  4 files changed, 4 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
> index 9a492a9..c66c26f 100644
> --- a/fs/xfs/libxfs/xfs_da_format.h
> +++ b/fs/xfs/libxfs/xfs_da_format.h
> @@ -169,8 +169,9 @@ struct xfs_da3_icnode_hdr {
>  
>  /*
>   * Dirents in version 3 directories have a file type field. Additions to this
> - * list are an on-disk format change, requiring feature bits. Valid values
> - * are as follows:
> + * list are an on-disk format change, requiring feature bits.
> + * Values 0..7 should match common file type values in file_type.h.
> + * Valid values are as follows:

They have to stay in sync... but there's nothing here to enforce this.

Originally XFS takes the mode value passed in by the VFS and converts
that to an XFS_DIR3_FT_* equivalent, which is passed around internally
before being written to disk.  On the way up (readdir), the _DIR3_FT_
values are converted to their DT_* equivalents before being passed back
to the other VFS directory iterator functions.

With this patch applied, XFS no longer gets to write its own ftype
values to disk -- all that is now opaque in the VFS.  Effectively, we
lose control of that part of our own disk format.  You can subtly change
the range of fs_umode_to_ftype() and that'll get written to disk.

Normally we evaluate creating new feature flags when the on-disk format
changes, to try to minimize user problems.  These problems could arise
in xfs_dir3_get_dtype() which still relies on converting XFS_DIR3_FT_
values in to DT_ values, or with old versions of xfsprogs getting very
confused when it encounters a new value.

The proposed FT_* space also seems inflexible to the VFS -- the upper
5 bits are reserved for private use and the lower 3 bits are all in use,
which means that the VFS cannot later add more type codes bits unless
we can find bits that *nobody* else is using.  (The same theoretically
applies in reverse to XFS but as we don't have any private type codes,
it's not an issue yet.)

Furthermore, xfsprogs uses (roughly) the same libxfs code as the kernel.
Any code hoisted out of the kernel libxfs into the VFS must still be
provided for in xfsprogs so that we can build new xfsprogs on old kernel
headers.  If the hoist is into linux/fs.h, as is the case here, then
xfsprogs must retain a private version of the definitions, grow a bunch
of m4/autoconf macros to check for the presence of the linux/fs.h
versions, and wire up the private versions if linux/fs.h doesn't provide
one.  We also have to make sure that anything added to the VFS version
gets added to the xfsprogs version.

Note: We did this all this work a short time ago so that ext4 and XFS
could present the same FSGETXATTR ioctl to user programs (major benefit)
but it was kind of a pain to get right.  I don't see an upside to ceding
control of this part of the disk format to the VFS.

--D

>   */
>  #define XFS_DIR3_FT_UNKNOWN		0
>  #define XFS_DIR3_FT_REG_FILE		1
> diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
> index c58d72c..645a542 100644
> --- a/fs/xfs/libxfs/xfs_dir2.c
> +++ b/fs/xfs/libxfs/xfs_dir2.c
> @@ -36,23 +36,6 @@
>  struct xfs_name xfs_name_dotdot = { (unsigned char *)"..", 2, XFS_DIR3_FT_DIR };
>  
>  /*
> - * @mode, if set, indicates that the type field needs to be set up.
> - * This uses the transformation from file mode to DT_* as defined in linux/fs.h
> - * for file type specification. This will be propagated into the directory
> - * structure if appropriate for the given operation and filesystem config.
> - */
> -const unsigned char xfs_mode_to_ftype[S_IFMT >> S_SHIFT] = {
> -	[0]			= XFS_DIR3_FT_UNKNOWN,
> -	[S_IFREG >> S_SHIFT]    = XFS_DIR3_FT_REG_FILE,
> -	[S_IFDIR >> S_SHIFT]    = XFS_DIR3_FT_DIR,
> -	[S_IFCHR >> S_SHIFT]    = XFS_DIR3_FT_CHRDEV,
> -	[S_IFBLK >> S_SHIFT]    = XFS_DIR3_FT_BLKDEV,
> -	[S_IFIFO >> S_SHIFT]    = XFS_DIR3_FT_FIFO,
> -	[S_IFSOCK >> S_SHIFT]   = XFS_DIR3_FT_SOCK,
> -	[S_IFLNK >> S_SHIFT]    = XFS_DIR3_FT_SYMLINK,
> -};
> -
> -/*
>   * ASCII case-insensitive (ie. A-Z) support for directories that was
>   * used in IRIX.
>   */
> diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
> index 0197590..f9b9b50 100644
> --- a/fs/xfs/libxfs/xfs_dir2.h
> +++ b/fs/xfs/libxfs/xfs_dir2.h
> @@ -32,12 +32,6 @@ struct xfs_dir2_data_unused;
>  extern struct xfs_name	xfs_name_dotdot;
>  
>  /*
> - * directory filetype conversion tables.
> - */
> -#define S_SHIFT 12
> -extern const unsigned char xfs_mode_to_ftype[];
> -
> -/*
>   * directory operations vector for encode/decode routines
>   */
>  struct xfs_dir_ops {
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 308bebb..c122827 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -103,7 +103,7 @@ xfs_dentry_to_name(
>  {
>  	namep->name = dentry->d_name.name;
>  	namep->len = dentry->d_name.len;
> -	namep->type = xfs_mode_to_ftype[(mode & S_IFMT) >> S_SHIFT];
> +	namep->type = fs_umode_to_ftype(mode);
>  }
>  
>  STATIC void
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html