On Wed, Jan 16, 2019 at 6:01 AM Matthew Wilcox [off-list ref] wrote:

The ext2/ext4 patches don't show much improvement.  The other patches show
more:

 fs/nilfs2/dir.c                    | 52 ++++++++++--------------------
 include/uapi/linux/nilfs2_ondisk.h |  1 +
 2 files changed, 18 insertions(+), 35 deletions(-)

(for example).

UFS ends up benefiting the most.  You can see the whole diffstat here:

https://lore.kernel.org/lkml/20181023201952.GA15676@pathfinder/ (local)

Well, even with _all_ the filesystems converted, you actually have
more lines added than removed by this "cleanup".

Sharing code just isn't a win here.

That said, it's not really the number of lines per se that make me
question this, I think that's really more of a symptom than the root
cause. The root cause for the newly adde lines is that this whole
approach requires that all the numbers are in sync, but then they have
different *names*.

Honestly, my gut feel is that I should not pull this in this form.

I have a suggestion: if people want to do this, and actually share the
transformation, then the filesystems that use this common code should
simply *NOT* have their own private names for the enumerations. They
should actually use those standard names.

So if the patch for ext2 (for example) were to entirely get rid of the
whole EXT2_FT_DIR define entirely, and ext2 would just use the actual
FT_DIR define, than I'd be ok with it. At that point you don't add a
pointless and expensive abstraction. At that point you say "ext2 uses
the standard values, so ext2 can just use the standard #defines
directly".

See my argument?

I think it is completely disgsting to have stuff like this:

+ BUILD_BUG_ON(EXT2_FT_UNKNOWN != FT_UNKNOWN);
+ BUILD_BUG_ON(EXT2_FT_REG_FILE != FT_REG_FILE);
+ BUILD_BUG_ON(EXT2_FT_DIR != FT_DIR);
+ BUILD_BUG_ON(EXT2_FT_CHRDEV != FT_CHRDEV);
+ BUILD_BUG_ON(EXT2_FT_BLKDEV != FT_BLKDEV);
+ BUILD_BUG_ON(EXT2_FT_FIFO != FT_FIFO);
+ BUILD_BUG_ON(EXT2_FT_SOCK != FT_SOCK);
+ BUILD_BUG_ON(EXT2_FT_SYMLINK != FT_SYMLINK);
+ BUILD_BUG_ON(EXT2_FT_MAX != FT_MAX);

the above is just *garbage*.

If you fundamentally need the values to be the same, then you simply
shouldn't have two different set of #defines.

Get rid of the EXT2_FT_xyz enumeration entirely, and the whole
craziness goes away.

We'd see a lot more improvement in line count if Philip weren't quite
so paranoid about checking FOOFS_FT_* == FT_* at build time; eg for btrfs:

Exact same issue.

So the more I look at this, the less I like it.

But if people are actually willing to use *truly* shared code, instead
of using their own values and then having the crazy "they need to
match", then it would be a different issue. As it is, I think the
patch series adds complexity rather than helping anything.

More complexity and more lines of code? There is absolutely zero upside.

               Linus