On Tue, Dec 20, 2016 at 2:28 AM, Darrick J. Wong
<darrick.wong@xxxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
