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

[Amir Goldstein <amir73il@gmail.com>]

On Wed, Dec 21, 2016 at 5:06 PM, Theodore Ts'o <tytso@mit.edu> wrote:
...

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

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 S_DT_SHIFT     12
+#define S_DT(mode)     (((mode) & S_IFMT) >> S_DT_SHIFT)
+#define S_DT_MASK      (S_IFMT >> S_DT_SHIFT)
+
+#define DT_UNKNOWN     0
+#define DT_FIFO                S_DT(S_IFIFO) /* 1 */
+#define DT_CHR         S_DT(S_IFCHR) /* 2 */
+#define DT_DIR         S_DT(S_IFDIR) /* 4 */
+#define DT_BLK         S_DT(S_IFBLK) /* 6 */
+#define DT_REG         S_DT(S_IFREG) /* 8 */
+#define DT_LNK         S_DT(S_IFLNK) /* 10 */
+#define DT_SOCK                S_DT(S_IFSOCK) /* 12 */
+#define DT_WHT         14
+
+#define DT_MAX         (S_DT_MASK + 1) /* 16 */

and using S_DT(mode) and DT_MAX in place of all the many places in the
code that open code the >> S_SHIFT.

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.

Regardless, setting an array size of 15 is just as buggy as setting an
array size of DT_SOCK+1 (13) because no syscall sets a larger value
for the type bits.


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

You are absolutely right. Not sure if you replied after seeing
v2 of this patch, but I did not use any fs provided complex conversion
functions.

I left the XFS code mostly as it was (same complexity) and sorted
it out to use some common defines and helpers in a way that
seem cleaner to me. At least that minor bug has been addressed.

I could further simplify the patch to use the S_DT(mode) macro
instead of the fs_umode_to_ftype(mode) helper.
If I do that, then the small snippet above that defines S_DT()
and DT_MAX is the only code that needs to be carried from
linux/fs.h to xfsprogs for building libxfs (same goes for e2fsprogs).

That's would be a technical code cleanup that does not mess
with on-disk format definitions.
I will send out an ext4 sample patch for you to consider.

Thanks for the feedback,
Amir.