* [GIT PULL] dtype handling cleanup for v4.21-rc1 @ 2019-01-02 17:47 Jan Kara 2019-01-08 10:05 ` Jan Kara 0 siblings, 1 reply; 13+ messages in thread From: Jan Kara @ 2019-01-02 17:47 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-fsdevel, linux-ext4, Al Viro 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [GIT PULL] dtype handling cleanup for v4.21-rc1 2019-01-02 17:47 [GIT PULL] dtype handling cleanup for v4.21-rc1 Jan Kara @ 2019-01-08 10:05 ` Jan Kara 2019-01-15 9:24 ` Jan Kara 0 siblings, 1 reply; 13+ messages in thread From: Jan Kara @ 2019-01-08 10:05 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-fsdevel, linux-ext4, Al Viro 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [GIT PULL] dtype handling cleanup for v4.21-rc1 2019-01-08 10:05 ` Jan Kara @ 2019-01-15 9:24 ` Jan Kara 2019-01-15 17:36 ` Linus Torvalds 0 siblings, 1 reply; 13+ messages in thread From: Jan Kara @ 2019-01-15 9:24 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-fsdevel, linux-ext4, Al Viro 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [GIT PULL] dtype handling cleanup for v4.21-rc1 2019-01-15 9:24 ` Jan Kara @ 2019-01-15 17:36 ` Linus Torvalds 2019-01-15 18:01 ` Matthew Wilcox 2019-01-16 6:22 ` Amir Goldstein 0 siblings, 2 replies; 13+ messages in thread From: Linus Torvalds @ 2019-01-15 17:36 UTC (permalink / raw) To: Jan Kara; +Cc: linux-fsdevel, linux-ext4, Al Viro 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [GIT PULL] dtype handling cleanup for v4.21-rc1 2019-01-15 17:36 ` Linus Torvalds @ 2019-01-15 18:01 ` Matthew Wilcox 2019-01-15 19:25 ` Linus Torvalds 2019-01-16 6:22 ` Amir Goldstein 1 sibling, 1 reply; 13+ messages in thread From: Matthew Wilcox @ 2019-01-15 18:01 UTC (permalink / raw) To: Linus Torvalds; +Cc: Jan Kara, linux-fsdevel, linux-ext4, Al Viro 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/ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [GIT PULL] dtype handling cleanup for v4.21-rc1 2019-01-15 18:01 ` Matthew Wilcox @ 2019-01-15 19:25 ` Linus Torvalds 2019-01-16 12:07 ` Jan Kara 0 siblings, 1 reply; 13+ messages in thread From: Linus Torvalds @ 2019-01-15 19:25 UTC (permalink / raw) To: Matthew Wilcox; +Cc: Jan Kara, linux-fsdevel, linux-ext4, Al Viro 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [GIT PULL] dtype handling cleanup for v4.21-rc1 2019-01-15 19:25 ` Linus Torvalds @ 2019-01-16 12:07 ` Jan Kara 2019-01-16 16:34 ` Phillip Potter 0 siblings, 1 reply; 13+ messages in thread From: Jan Kara @ 2019-01-16 12:07 UTC (permalink / raw) To: Linus Torvalds Cc: Matthew Wilcox, Jan Kara, linux-fsdevel, linux-ext4, Al Viro, Phillip Potter, Amir Goldstein [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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [GIT PULL] dtype handling cleanup for v4.21-rc1 2019-01-16 12:07 ` Jan Kara @ 2019-01-16 16:34 ` Phillip Potter 2019-01-16 16:51 ` Jan Kara 0 siblings, 1 reply; 13+ messages in thread From: Phillip Potter @ 2019-01-16 16:34 UTC (permalink / raw) To: Jan Kara Cc: Linus Torvalds, cc: Matthew Wilcox, linux-fsdevel, linux-ext4, Al Viro, Amir Goldstein 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [GIT PULL] dtype handling cleanup for v4.21-rc1 2019-01-16 16:34 ` Phillip Potter @ 2019-01-16 16:51 ` Jan Kara 2019-01-16 16:56 ` Amir Goldstein 0 siblings, 1 reply; 13+ messages in thread From: Jan Kara @ 2019-01-16 16:51 UTC (permalink / raw) To: Phillip Potter Cc: Jan Kara, Linus Torvalds, cc: Matthew Wilcox, linux-fsdevel, linux-ext4, Al Viro, Amir Goldstein 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [GIT PULL] dtype handling cleanup for v4.21-rc1 2019-01-16 16:51 ` Jan Kara @ 2019-01-16 16:56 ` Amir Goldstein 2019-01-17 9:35 ` Jan Kara 0 siblings, 1 reply; 13+ messages in thread From: Amir Goldstein @ 2019-01-16 16:56 UTC (permalink / raw) To: Jan Kara Cc: Phillip Potter, Linus Torvalds, cc: Matthew Wilcox, linux-fsdevel, Ext4, Al Viro 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [GIT PULL] dtype handling cleanup for v4.21-rc1 2019-01-16 16:56 ` Amir Goldstein @ 2019-01-17 9:35 ` Jan Kara 2019-01-17 11:17 ` Amir Goldstein 0 siblings, 1 reply; 13+ messages in thread From: Jan Kara @ 2019-01-17 9:35 UTC (permalink / raw) To: Amir Goldstein Cc: Jan Kara, Phillip Potter, Linus Torvalds, cc: Matthew Wilcox, linux-fsdevel, Ext4, Al Viro 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [GIT PULL] dtype handling cleanup for v4.21-rc1 2019-01-17 9:35 ` Jan Kara @ 2019-01-17 11:17 ` Amir Goldstein 0 siblings, 0 replies; 13+ messages in thread From: Amir Goldstein @ 2019-01-17 11:17 UTC (permalink / raw) To: Phillip Potter Cc: Jan Kara, Linus Torvalds, cc: Matthew Wilcox, linux-fsdevel, Ext4, Al Viro 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [GIT PULL] dtype handling cleanup for v4.21-rc1 2019-01-15 17:36 ` Linus Torvalds 2019-01-15 18:01 ` Matthew Wilcox @ 2019-01-16 6:22 ` Amir Goldstein 1 sibling, 0 replies; 13+ messages in thread From: Amir Goldstein @ 2019-01-16 6:22 UTC (permalink / raw) To: Linus Torvalds; +Cc: Jan Kara, linux-fsdevel, Ext4, Al Viro 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-01-17 11:17 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-02 17:47 [GIT PULL] dtype handling cleanup for v4.21-rc1 Jan Kara 2019-01-08 10:05 ` Jan Kara 2019-01-15 9:24 ` Jan Kara 2019-01-15 17:36 ` Linus Torvalds 2019-01-15 18:01 ` Matthew Wilcox 2019-01-15 19:25 ` Linus Torvalds 2019-01-16 12:07 ` Jan Kara 2019-01-16 16:34 ` Phillip Potter 2019-01-16 16:51 ` Jan Kara 2019-01-16 16:56 ` Amir Goldstein 2019-01-17 9:35 ` Jan Kara 2019-01-17 11:17 ` Amir Goldstein 2019-01-16 6:22 ` Amir Goldstein
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.