On Wed, 16 Jan 2019 at 12:07, Jan Kara <jack@xxxxxxx> wrote: > > [Added Phillip and Amir to CC (authors)] > > On Wed 16-01-19 07:25:01, Linus Torvalds wrote: > > 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". > > OK, I'm fine with that. We just have to have a big fat warning at FT_ > definitions that these are on-disk values for several filesystems and thus > cannot ever change. As Amir mentioned in another email, the original > motivation for this is that quite a few filesystems copy-pasted ext2 code > and that is slightly buggy. So I still do think there's value in this > cleanup excercise. > > > 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. > > OK, understood. Phillip, could you please rework the patches as Linus > suggests? Thanks! > > Honza > -- > Jan Kara <jack@xxxxxxxx> > SUSE Labs, CR Dear Jan, I am happy to rework the patches, all fair comment. Slight problem being my computer is in a box right now as I've just moved house. I will get this done in the next few days if that's ok? Regards, Phil
