From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi0-f66.google.com ([209.85.218.66]:36826 "EHLO mail-oi0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752143AbcLTFUY (ORCPT ); Tue, 20 Dec 2016 00:20:24 -0500 Received: by mail-oi0-f66.google.com with SMTP id u15so22438398oie.3 for ; Mon, 19 Dec 2016 21:20:23 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <20161220002832.GB7297@birch.djwong.org> References: <1482178268-22883-1-git-send-email-amir73il@gmail.com> <1482178268-22883-12-git-send-email-amir73il@gmail.com> <20161220002832.GB7297@birch.djwong.org> From: Amir Goldstein Date: Tue, 20 Dec 2016 07:20:22 +0200 Message-ID: Subject: Re: [RFC][PATCH 11/11] xfs: use common file type conversion To: "Darrick J. Wong" Cc: Jan Kara , "Theodore Ts'o" , Dave Chinner , Chris Mason , Boaz Harrosh , Jaegeuk Kim , Ryusuke Konishi , Mark Fasheh , Evgeniy Dushistov , Miklos Szeredi , Al Viro , linux-fsdevel Content-Type: text/plain; charset=UTF-8 Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue, Dec 20, 2016 at 2:28 AM, Darrick J. Wong 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 >> --- >> 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.