On Wed, Jan 16, 2019 at 6:01 AM Matthew Wilcox <willy@xxxxxxxxxxxxx> 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/ 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
