[GIT PULL] dtype handling cleanup for v4.21-rc1

[GIT PULL] dtype handling cleanup for v4.21-rc1

[Jan Kara]

  Hello Linus,

  could you please pull from

git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git dtype_for_v4.21-rc1

to get a patch to provide generic functions for dtype handling and use those
for ext2. Other filesystems (ext4, btrfs, ocfs2, ...) can then merge the
cleanup patches through their trees.

I've taken the generic functions patch through my tree since Al didn't respond
and it seems non-controversial enough. But I'm sending it separately from my
standard fs fixes batch to mark something unusual is going on (which is also
why I'm sending it so late in the merge window because I forgot about this
topic branch and found out only now).

Top of the tree is 9d6e1fe4e091. The full shortlog is:

Phillip Potter (2):
      fs: common implementation of file type
      ext2: use common file type conversion

The diffstat is

 MAINTAINERS              |   1 +
 fs/Makefile              |   3 +-
 fs/ext2/dir.c            |  49 +++++++++-------------
 fs/fs_types.c            | 105 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h       |  17 +-------
 include/linux/fs_types.h |  73 ++++++++++++++++++++++++++++++++
 6 files changed, 202 insertions(+), 46 deletions(-)
 create mode 100644 fs/fs_types.c
 create mode 100644 include/linux/fs_types.h

							Thanks
								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Jan Kara]

Hello Linus,

On Wed 02-01-19 18:47:36, Jan Kara wrote:
>   could you please pull from
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git dtype_for_v4.21-rc1
> 
> to get a patch to provide generic functions for dtype handling and use those
> for ext2. Other filesystems (ext4, btrfs, ocfs2, ...) can then merge the
> cleanup patches through their trees.

I didn't get any reply WRT this pull request and you didn't pull yet. I
guess you just wanted to have a deeper look and didn't get to it yet? Or do
you have some problem with the patches?

								Honza

> 
> I've taken the generic functions patch through my tree since Al didn't respond
> and it seems non-controversial enough. But I'm sending it separately from my
> standard fs fixes batch to mark something unusual is going on (which is also
> why I'm sending it so late in the merge window because I forgot about this
> topic branch and found out only now).
> 
> Top of the tree is 9d6e1fe4e091. The full shortlog is:
> 
> Phillip Potter (2):
>       fs: common implementation of file type
>       ext2: use common file type conversion
> 
> The diffstat is
> 
>  MAINTAINERS              |   1 +
>  fs/Makefile              |   3 +-
>  fs/ext2/dir.c            |  49 +++++++++-------------
>  fs/fs_types.c            | 105 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fs.h       |  17 +-------
>  include/linux/fs_types.h |  73 ++++++++++++++++++++++++++++++++
>  6 files changed, 202 insertions(+), 46 deletions(-)
>  create mode 100644 fs/fs_types.c
>  create mode 100644 include/linux/fs_types.h
> 
> 							Thanks
> 								Honza
> 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Jan Kara]

Hello Linus,

On Tue 08-01-19 11:05:00, Jan Kara wrote:
> On Wed 02-01-19 18:47:36, Jan Kara wrote:
> >   could you please pull from
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git dtype_for_v4.21-rc1
> > 
> > to get a patch to provide generic functions for dtype handling and use those
> > for ext2. Other filesystems (ext4, btrfs, ocfs2, ...) can then merge the
> > cleanup patches through their trees.
> 
> I didn't get any reply WRT this pull request and you didn't pull yet. I
> guess you just wanted to have a deeper look and didn't get to it yet? Or do
> you have some problem with the patches?

What has happened to this pull request? It may be too late for this to be
merged now but I'd like to understand why it was not merged or rejected...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Linus Torvalds]

On Tue, Jan 15, 2019 at 9:24 PM Jan Kara <jack@suse.cz> wrote:
>
> What has happened to this pull request? It may be too late for this to be
> merged now but I'd like to understand why it was not merged or rejected...

Sorry, initially I left if for later consideration after rc1, and then
I just forgot about it.

I didn't see much point to the cleanup when it actually adds lots of
lines and no actual advantage. The whole dentry type translation
really is fs-specific and it might just happen to be shared. But why
share it if it only adds complexity and unnecessary abstraction?

               Linus

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

[Matthew Wilcox]

On Wed, Jan 16, 2019 at 05:36:06AM +1200, Linus Torvalds wrote:
> On Tue, Jan 15, 2019 at 9:24 PM Jan Kara <jack@suse.cz> wrote:
> >
> > What has happened to this pull request? It may be too late for this to be
> > merged now but I'd like to understand why it was not merged or rejected...
> 
> Sorry, initially I left if for later consideration after rc1, and then
> I just forgot about it.
> 
> I didn't see much point to the cleanup when it actually adds lots of
> lines and no actual advantage. The whole dentry type translation
> really is fs-specific and it might just happen to be shared. But why
> share it if it only adds complexity and unnecessary abstraction?

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/

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:

https://lore.kernel.org/lkml/20181023211728.GA16584@pathfinder/

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

[Linus Torvalds]

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

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

[Jan Kara]

[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@infradead.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".

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@suse.com>
SUSE Labs, CR

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

[Phillip Potter]

On Wed, 16 Jan 2019 at 12:07, Jan Kara <jack@suse.cz> 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@infradead.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".
>
> 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@suse.com>
> 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

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

[Jan Kara]

On Wed 16-01-19 16:34:36, Phillip Potter wrote:
> 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?

Sure, no problem. Patches won't make it in this release cycle so we have
like three weeks to get them ready for the next merge window.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Amir Goldstein]

On Wed, Jan 16, 2019 at 6:51 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 16-01-19 16:34:36, Phillip Potter wrote:
> > 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?
>
> Sure, no problem. Patches won't make it in this release cycle so we have
> like three weeks to get them ready for the next merge window.
>

Also, and Jan will correct me if I am wrong.
I think that reworking only the common and ext2 patches would
be sufficient for this cycle.

You can prepare the rest of the patches for next cycle after
the common patch has landed.

Thanks,
Amir.

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

[Jan Kara]

On Wed 16-01-19 18:56:17, Amir Goldstein wrote:
> On Wed, Jan 16, 2019 at 6:51 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 16-01-19 16:34:36, Phillip Potter wrote:
> > > 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?
> >
> > Sure, no problem. Patches won't make it in this release cycle so we have
> > like three weeks to get them ready for the next merge window.
> >
> 
> Also, and Jan will correct me if I am wrong.
> I think that reworking only the common and ext2 patches would
> be sufficient for this cycle.

Yes, that's also true.

> You can prepare the rest of the patches for next cycle after
> the common patch has landed.

Correct, since the plan is for these to land through other maintainers'
trees. But still it would be nice if we had them earlier just to see of
other maintainers don't seriously oppose the idea. To that end it is
probably good to reference the discussion with Linus in the cover letter
and the email with common infrastructure, explaining why converting to
common defines instead of keeping filesystem specific ones was done.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Amir Goldstein]

On Thu, Jan 17, 2019 at 11:35 AM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 16-01-19 18:56:17, Amir Goldstein wrote:
> > On Wed, Jan 16, 2019 at 6:51 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Wed 16-01-19 16:34:36, Phillip Potter wrote:
> > > > 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?
> > >
> > > Sure, no problem. Patches won't make it in this release cycle so we have
> > > like three weeks to get them ready for the next merge window.
> > >
> >
> > Also, and Jan will correct me if I am wrong.
> > I think that reworking only the common and ext2 patches would
> > be sufficient for this cycle.
>
> Yes, that's also true.
>
> > You can prepare the rest of the patches for next cycle after
> > the common patch has landed.
>
> Correct, since the plan is for these to land through other maintainers'
> trees. But still it would be nice if we had them earlier just to see of
> other maintainers don't seriously oppose the idea. To that end it is
> probably good to reference the discussion with Linus in the cover letter
> and the email with common infrastructure, explaining why converting to
> common defines instead of keeping filesystem specific ones was done.
>

Phillip,

While I see no problem with Linus' proposal to get rid of EXT2_FT_*
completely. That may be practical for EXOFS_FT_*, F2FS_FT_*, ...
But some fs maintainers may prefer to define (not re-define) the
fs specific constants, e.g.:

#define EXT4_FT_DIR FT_DIR

If for no other reason, it will result in slightly less code churn.

However, note that you may NOT get rid of defines in uapi headers:
BTRFS_FT_*, NILFS_FT_*
and defining them to the common constant will require exporting
common constants to uapi headers.
That is one mess you do not want to get yourself into.

So I'm afraid for btrfs/nilfs, the "disgusting" BUILD_BUG_ON()
statements in the only practical outlet if they want to use the common
conversions code.

If you prepare the patch series with several options as described
above (choose one option per fs), then later maintainers can use
either option when applying the individual patches.

Thanks,
Amir.

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

[Amir Goldstein]

On Wed, Jan 16, 2019 at 4:56 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Tue, Jan 15, 2019 at 9:24 PM Jan Kara <jack@suse.cz> wrote:
> >
> > What has happened to this pull request? It may be too late for this to be
> > merged now but I'd like to understand why it was not merged or rejected...
>
> Sorry, initially I left if for later consideration after rc1, and then
> I just forgot about it.
>
> I didn't see much point to the cleanup when it actually adds lots of
> lines and no actual advantage. The whole dentry type translation
> really is fs-specific and it might just happen to be shared. But why
> share it if it only adds complexity and unnecessary abstraction?
>

Linus,

I guess some of the reasoning got lost from the original patch series
to the pull request.

The pull request fails to mention this is an ext2 (very very old) bug fix.

Jan has agreed to carry these 2 patches first to ease to herding all
other filesystem patches in the series:
https://marc.info/?l=linux-fsdevel&m=154060161813861&w=2
https://marc.info/?l=linux-fsdevel&m=154289087825040&w=2

The overall cleanup reduces ~100LOC and was acked by many filesystem
maintainers as a welcome improvement (not as added complexity)
Instead of fixing a potential out-of-bounds access bug that was copy&pasted
from ext2 to many other filesystems, the approach taken was to create a single
correct implementation and use it in all those filesystems.

It is true, that this is filesystem specific code and new filesystem on-disk
formats is not bound to use the same values. But the on-disk values used
for all those filesystems formats are not going to change for eternity, so there
is no meat on the fs-specific argument. The conversion is common de-facto
forever for code that wants to read inodes from existing drives.

If you pull these 2 patches now, other patches could be applied by
fs maintainers for next cycle.

Thanks,
Amir.