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

From: Theodore Ts'o <tytso@mit.edu>
To: Dave Chinner <david@fromorbit.com>
Cc: Amir Goldstein <amir73il@gmail.com>,
	"Darrick J. Wong" <darrick.wong@oracle.com>,
	Jan Kara <jack@suse.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: Wed, 21 Dec 2016 10:06:19 -0500	[thread overview]
Message-ID: <20161221150619.hkgsqwcdzkqlum4v@thunk.org> (raw)
In-Reply-To: <20161221052317.GA4758@dastard>

On Wed, Dec 21, 2016 at 04:23:17PM +1100, Dave Chinner wrote:
> It's a philoophical and architectural issue. We currently have a
> distinct separation between generic functionality and per-filesystem
> specific definitions. The on-disk definitions are owned by the
> filesystem not the generic code, and the generic code /never/ sees
> what the filesystem stores on disk. THe VFS is supposed to be
> completely independent of what the filesystems store on disk - it's
> an abstract concept - so that it can morph into whatever is required
> to support the functionality the different filesystem provides.

I had the same concerns as Dave and Darrick --- which is that it just
"feels" wrong to define file system encoding semantics in generic
code.  So it's really much more of a taste issue than anything else.
I'll note that we've already started taking some steps down this
slippery path with the definition of whiteout --- where we define
WHITEOUT_MODE and WHITEOUT_DEV to be 0, and both the generic code and
the file system code need to agree that the on-disk encoding of a
whiteout is an inode with mode S_IFCHR | WHITEOUT_MODE, and with the
device number set to WHITEOUT_DEV.

(Why we define WHITEOUT_MODE to be zero when everyone has to *know*
and use the same on-disk encoding of S_IFCHR | WhiTEOUT_MODE doesn't
make any sense to me; it would be much simpler to #define
WHITEOUT_MODE to be S_IFCHR since if it ever changed, all file on-disk
encodings of overlay file systems would go *boom*.)

The fact that the three bit encoding of the directory file types is
fully defined, and can *not* ever be extended without breaking things
means that the chances that it would be a problem is much less.  So I
don't object quite as much as Dave and Darrick --- but I'll also point
out the benefits are quite small.  All we are saving is 16 bytes per
file system compiled into the kernel.

So it's a lot of discussion / changes for very little gain.

> The way we normally handle this is a method callout of some kind
> into the filesystem to do the filesystem specific function, and
> if there are multiple filesystems that do the same thing, they use a
> common function. So that part of the patchset - providing common
> helpers to inode mode/filesytem d_type conversion - is fine.

The common helpers are inline functions that do an array lookup.  If
we provide some other more generic way of letting the file systems
provide conversion functions, at that point we're not really saving
anything, and just adding complexity for no real benefit.

Which perhaps is my biggest argument against it; making this be
generic adds complexity where it's not really buying us anything
(except for 16 bytes of data segment).  But whether we think it's a
bad idea due to a slippery slope argument or due to a complexity
argument, it's clear that the gains are marginal, and while I
personally am willing compromise a little on the slippery slope
argument if the gains are large enough --- for example, the fs/crypto
code is certainly adding some commonality in ways that affect on-disk
encodings, in this particular case, the wins just don't seem high
enough.

Cheers,

						- Ted