linux-ext4 - Re: [GIT PULL] dtype handling cleanup for v4.21-rc1

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 16 Jan 2019 07:25:01 +1200
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     Jan Kara <jack@...e.cz>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-ext4@...r.kernel.org, Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [GIT PULL] dtype handling cleanup for v4.21-rc1

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