7 ms·

I understand that reviewing 27k LoC is daunting and probably not very fun. But unlike most patches that draw a similar response it's not a narrowly useful patc

I understand that reviewing 27k LoC is daunting and probably not very fun. But unlike most patches that draw a similar response it's not a narrowly useful patch that mostly serves the submitter. Proper NTFS support benefits a large proportion of Linux users (the jab from the article that there are more advanced file systems out there seems out of place; there are no signs that windows is about to switch its default FS to something else). Additionally this code has been used in production for years now (e.g. my 2015 router runs the closed source version of this driver in order to support NTFS formatted external drives) so most likely a lot of quality issues have already been found and addressed. So I feel it's a bit unreasonable to respond with so much negativity to this contribution.

AnotherGoodName 6y ago
It's also no big deal from either side. Paragon sent in the patch and it's appreciated. There's a few problems to get this in. Reviewers noted the issues and what would need to be done to get this through. The process to get this in is happening. Split your diff! and Fix your makefile! have to be one of the most benign and common pieces of diff feedback i've seen. I feel that you could make a media story about any submission to the Linux kernel based on there being comments in the review process.
- piscisaureus 6y ago
  Admittedly I didn't actually read the mailing list discussion. It's entirely possible that The Register made up a big drama where there was none.
  - echlebek 6y ago
    There wasn't much drama in the mailing list discussion as I read it. Mostly comments asking "can you make this easier for us to review?".
  - mehrdadn 6y ago
    Not sure if this counts as drama per Linux kernel mailing list standards: https://lore.kernel.org/linux-fsdevel/2911ac5cd20b46e397be506268718d74@paragon-software.com/t/#m34fc84d93a7009caf533ff400f7c763da4cab366 https://lore.kernel.org/linux-fsdevel/2911ac5cd20b46e397be50... > So how exactly do you expect someone to review this monstrosity ?
    - isatty 6y ago
      It’s a legitimate concern - I would not assume malice. How exactly would someone review a 25k loc .patch file?
      - mehrdadn 6y ago
        I didn't mean to imply otherwise. Drama is often not malice and rather due to legitimate concerns on one or both sides.
      - michaelt 6y ago
        It would be tough, no doubt. But it's not like splitting the feature into 100 patches of 250 lines each would make it any quicker to review. Or merging code that was known not to work, as it was only a fraction of what was needed for the functionality.
        wtallis 6y ago
        > But it's not like splitting the feature into 100 patches of 250 lines each would make it any quicker to review. Or merging code that was known not to work, as it was only a fraction of what was needed for the functionality. That would also be rejected, because the kernel maintainers aren't idiots and their standards aren't the stupid arbitrary rules you construe them to be. They generally want big changes to be broken up into logical, sensible chunks that each leave the tree in a usable state, so that git-bisect still works.
        skissane 6y ago
        How do people merge big new filesystems in practice though? Especially one with years of pre-existing out-of-tree development? I guess one could start by merging a skeleton of the filesystem which supports mount/unmount but then returns an IO error on every operation? And then a patch to add directory traversal (you can view the files but not their contents), and then a patch to add file reading, and then a patch to add file writing, and then a patch to add mkdir/rmdir, and then a patch to add rename/delete of regular files. Breaking down an existing filesystem into a sequence of patches like that, no doubt it is doable, but it is going to be a lot of work.
  - chx 6y ago
    I read the discussion. There's no drama at all. Paragon did an unreviewable code dump with intent to maintain and they are warmly welcome in general. David laid out the path to review and probable acceptance https://lore.kernel.org/linux-fsdevel/20200815190642.GZ2026@twin.jikos.cz/ https://lore.kernel.org/linux-fsdevel/20200815190642.GZ2026@... If there was any fuss it's because of the unreviewable nature of the patch but especially by kernel standards the discussion was cordial. In fact, aside from Nikolay's outburst by any standard it was a cordial discussion. (Someone should've gently told him this is no way to welcome newcomers especially newcomers carrying such a gift.) Others noted it needs to pass the existing test suite and that it is close.
geofft 6y ago
There were two specific concerns in the initial review that I think were reasonable: 1) The Linux kernel already has an in-kernel read-only NTFS driver. What should be done about it? (There are a number of reasonable options here, including just getting rid of it and replacing it with Paragon's, but that requires at least some buy-in from the maintainers of the existing driver.) 2) The patch didn't actually build, which was a one-line Makefile fix, but raised some concern about how it wax tested/how the patch was generated.

AnotherGoodName 6y ago

It's also no big deal from either side. Paragon sent in the patch and it's appreciated. There's a few problems to get this in. Reviewers noted the issues and what would need to be done to get this through. The process to get this in is happening. Split your diff! and Fix your makefile! have to be one of the most benign and common pieces of diff feedback i've seen. I feel that you could make a media story about any submission to the Linux kernel based on there being comments in the review process.

piscisaureus 6y ago
Admittedly I didn't actually read the mailing list discussion. It's entirely possible that The Register made up a big drama where there was none.
- echlebek 6y ago
  There wasn't much drama in the mailing list discussion as I read it. Mostly comments asking "can you make this easier for us to review?".
- mehrdadn 6y ago
  Not sure if this counts as drama per Linux kernel mailing list standards: https://lore.kernel.org/linux-fsdevel/2911ac5cd20b46e397be506268718d74@paragon-software.com/t/#m34fc84d93a7009caf533ff400f7c763da4cab366 https://lore.kernel.org/linux-fsdevel/2911ac5cd20b46e397be50... > So how exactly do you expect someone to review this monstrosity ?
  - isatty 6y ago
    It’s a legitimate concern - I would not assume malice. How exactly would someone review a 25k loc .patch file?
    - mehrdadn 6y ago
      I didn't mean to imply otherwise. Drama is often not malice and rather due to legitimate concerns on one or both sides.
    - michaelt 6y ago
      It would be tough, no doubt. But it's not like splitting the feature into 100 patches of 250 lines each would make it any quicker to review. Or merging code that was known not to work, as it was only a fraction of what was needed for the functionality.
      - wtallis 6y ago
        > But it's not like splitting the feature into 100 patches of 250 lines each would make it any quicker to review. Or merging code that was known not to work, as it was only a fraction of what was needed for the functionality. That would also be rejected, because the kernel maintainers aren't idiots and their standards aren't the stupid arbitrary rules you construe them to be. They generally want big changes to be broken up into logical, sensible chunks that each leave the tree in a usable state, so that git-bisect still works.
        skissane 6y ago
        How do people merge big new filesystems in practice though? Especially one with years of pre-existing out-of-tree development? I guess one could start by merging a skeleton of the filesystem which supports mount/unmount but then returns an IO error on every operation? And then a patch to add directory traversal (you can view the files but not their contents), and then a patch to add file reading, and then a patch to add file writing, and then a patch to add mkdir/rmdir, and then a patch to add rename/delete of regular files. Breaking down an existing filesystem into a sequence of patches like that, no doubt it is doable, but it is going to be a lot of work.
- chx 6y ago
  I read the discussion. There's no drama at all. Paragon did an unreviewable code dump with intent to maintain and they are warmly welcome in general. David laid out the path to review and probable acceptance https://lore.kernel.org/linux-fsdevel/20200815190642.GZ2026@twin.jikos.cz/ https://lore.kernel.org/linux-fsdevel/20200815190642.GZ2026@... If there was any fuss it's because of the unreviewable nature of the patch but especially by kernel standards the discussion was cordial. In fact, aside from Nikolay's outburst by any standard it was a cordial discussion. (Someone should've gently told him this is no way to welcome newcomers especially newcomers carrying such a gift.) Others noted it needs to pass the existing test suite and that it is close.

geofft 6y ago

There were two specific concerns in the initial review that I think were reasonable: 1) The Linux kernel already has an in-kernel read-only NTFS driver. What should be done about it? (There are a number of reasonable options here, including just getting rid of it and replacing it with Paragon's, but that requires at least some buy-in from the maintainers of the existing driver.) 2) The patch didn't actually build, which was a one-line Makefile fix, but raised some concern about how it wax tested/how the patch was generated.