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

From: Amir Goldstein <amir73il@gmail.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: Jan Kara <jack@suse.com>, "Theodore Ts'o" <tytso@mit.edu>,
	Dave Chinner <david@fromorbit.com>, Chris Mason <clm@fb.com>,
	Boaz Harrosh <boaz@plexistor.com>,
	Jaegeuk Kim <jaegeuk@kernel.org>,
	Ryusuke Konishi <konishi.ryusuke@lab.ntt.co.jp>,
	Mark Fasheh <mfasheh@versity.com>,
	Evgeniy Dushistov <dushistov@mail.ru>,
	Miklos Szeredi <miklos@szeredi.hu>,
	Al Viro <viro@zeniv.linux.org.uk>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [RFC][PATCH 11/11] xfs: use common file type conversion
Date: Tue, 20 Dec 2016 07:20:22 +0200	[thread overview]
Message-ID: <CAOQ4uxj3hpuuADiTv0q6BBvhUvmpi=2mkVdk=1pXO2cCdT05Qg@mail.gmail.com> (raw)
In-Reply-To: <20161220002832.GB7297@birch.djwong.org>

On Tue, Dec 20, 2016 at 2:28 AM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> 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.
>

No way that the common conversion code will change those values
too many fs depend on it. that should be made clear.

> 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.)
>

btrfs has a private flag on bit4 (FT_XTTR) and this implementation
does not prevent btrfs from using the private bit.
If XFS ever wants to write a private bit it will have to not blindly
translate mode to ftype.
The scope of the common implementation should remain only
the 3bits that translate directly to POSIX types.

> 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.
>

As a matter of fact, I would be very much interested to standardize
use the upper 5 bits some can remain private and some can be used
for generic fs purpose. But doing that will require shring a common
library (or at least a spec) between fs offline tools, so that's a
bigger challenge.

> 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.
>

The answer "this is not wanted for XFS" is perfectly valid :)
The common implementation can be used by the small fs, which never
want anymore than the basic ext2 implementation.

However, since the DT_ values and  XFS_DIR3_FT_ values of 0..7
are already carved in stone, as long as comments in both common code
and libxfs code makes that clear, I see no harm in using the common
implementation in xfs, besides the need to yank more code from fs.h to
libxfs, but it's your decision.

Amir.