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

From: Theodore Ts'o <tytso@mit.edu>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Dave Chinner <david@fromorbit.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 17:56:05 -0500	[thread overview]
Message-ID: <20161221225605.qcgpwkyrugwmjnza@thunk.org> (raw)
In-Reply-To: <CAOQ4uxjnUNQWiwx_rExePs_S_eTZWbVkqY1UgBUzbehKn4LxWQ@mail.gmail.com>

On Wed, Dec 21, 2016 at 06:37:42PM +0200, Amir Goldstein wrote:
> 
> Right. never claimed that saving data segment space is the issue
> here. To be completely honest, I started looking at ways to optimize
> whiteout iteration and was appalled to see all that code copy&paste,
> so went on an OCD cleanup spree.
> 
> So it is really a lot more about clumsiness of the current code then
> about saving actual lines on code or space in data segment.
> 
> Perhaps I should have settled with defining this common section:

> +#define DT_WHT         14

What are you trying to accomplish here?  Unless we actually encoding
DT_WHT into the on-disk format, it's not really going to buy you much.

And if you are going to encode this into the on-disk format, each file
system is going to have to set some kind of flag in the superblock to
indicate whether it's doing this new thing or not.  And this is
*precisely* why Dave and Darrick are objecting --- if we are going to
make any kind of file system encoding change, the file system has to
know about it.

> Note that all those file system copy&pasted a potential bug.
> 
> This is an array of size 15:
> 
> static unsigned char ext2_type_by_mode[S_IFMT >> S_SHIFT] = {
> 
> and it is always addressed this way:
> 
> ext2_type_by_mode[(inode->i_mode & S_IFMT)>>S_SHIFT];
> 
> Which *can* try to address ext2_type_by_mode[15]
> 
> Now, can you say for certain that you can never get a malformed i_mode
> with ((inode->i_mode & S_IFMT) == S_IFMT) either from disk or from
> some code calling into VFS functions, which do not sanity check the
> file type of the mode argument.

It's not a problem and not a bug, because we only assign the file type
when the inode is created, and at that point, the i_mode is set by the
*kernel*, and not by the on-disk encoding.

And BTW this is why DT_WHT makes no sense.  We the DT encodings come
from the directory entry, and *never* come from the inode i_mode read
from disk --- since at the time when we do the readdir(2), the inode
may not have been read into memory.  So we can't add DT_WHT unless we
also change the on-disk encoding of the directory entry on a file
system by file system basis.

Cheers,

						- Ted