Re: [2.6 patch] fs/jbd/journal.c: cleanups
On Mon, Feb 18, 2008 at 08:12:29AM +0100, Ingo Molnar wrote: Nack. I don't object to un-exporting journal_update_superblock(), because that is pretty internal, but the other functions are intended specifically for use by code outside of JBD. For example, the journal checksum patch for ext3/4 uses journal_set_features() to turn on features in the JBD superblock. Similarly, for 64-bit support in ext4 uses journal_set_features() to set a 64-bit feature flag in the journal superblock. that's an invalid excuse for the benefit of out-of-tree forks: reality is that you can export those functions in the journal checksum patch just fine. So you cannot 'nack' a sensible patch on that ground and no maintainer does it on that ground. Once you get your stuff upstream, you can re-add the export. I'm going to NACK it as well. This kind of code churn where we make symbols static only to make them non-static again in an existing ext4 tree is exactly the sort of needless code churn that makes patches start to conflict and where we need different patches depending on whether it is intended for -mm or linux-next or mainline. I think we really have gotten WAY to doctrinaire on the if there are no in-tree users, it MUST be static. This is exactly the sort of mindless rules that cause the patch conflicts that have been causing us so much pain and grief. In this case, it is an existing symbol which is already non-static, and for which we have code in a development tree that will be using it. In the r/o bind case, it is the insistence that you can't push an existing patch to expose a new interface that must be used later in the r/o bind patchset and which sweeps across all trees changing stuff that causes pain and grief. In both cases, if we expand in-tree development users to include known development trees that are intended for mainline, it makes all of our lives MUCH easier. - Ted - To unsubscribe from this list: send the line unsubscribe linux-ext4 in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [2.6 patch] fs/jbd/journal.c: cleanups
On Mon, Feb 18, 2008 at 06:49:36AM -0500, Theodore Tso wrote: On Mon, Feb 18, 2008 at 08:12:29AM +0100, Ingo Molnar wrote: Nack. I don't object to un-exporting journal_update_superblock(), because that is pretty internal, but the other functions are intended specifically for use by code outside of JBD. For example, the journal checksum patch for ext3/4 uses journal_set_features() to turn on features in the JBD superblock. Similarly, for 64-bit support in ext4 uses journal_set_features() to set a 64-bit feature flag in the journal superblock. that's an invalid excuse for the benefit of out-of-tree forks: reality is that you can export those functions in the journal checksum patch just fine. So you cannot 'nack' a sensible patch on that ground and no maintainer does it on that ground. Once you get your stuff upstream, you can re-add the export. I'm going to NACK it as well. This kind of code churn where we make symbols static only to make them non-static again in an existing ext4 tree is exactly the sort of needless code churn that makes patches start to conflict and where we need different patches depending on whether it is intended for -mm or linux-next or mainline. I think we really have gotten WAY to doctrinaire on the if there are no in-tree users, it MUST be static. This is exactly the sort of mindless rules that cause the patch conflicts that have been causing us so much pain and grief. In this case, it is an existing symbol which is already non-static, and for which we have code in a development tree that will be using it. In the r/o bind case, it is the insistence that you can't push an existing patch to expose a new interface that must be used later in the r/o bind patchset and which sweeps across all trees changing stuff that causes pain and grief. In both cases, if we expand in-tree development users to include known development trees that are intended for mainline, it makes all of our lives MUCH easier. The following was meant 100% seriously: This patch has been sent on: - 16 May 2006 - 1 May 2006 - 23 Apr 2006 If me resending this old patch collides with something finally getting a user this part of my patch shouldn't be applied now (but you might get it again in 6 months if it's still unused...). But generally such conflicts would become visible if known development trees that are intended for mainline were in -mm. - Ted cu Adrian -- Is there not promise of rain? Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. Only a promise, Lao Er said. Pearl S. Buck - Dragon Seed - To unsubscribe from this list: send the line unsubscribe linux-ext4 in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [2.6 patch] fs/jbd/journal.c: cleanups
* Theodore Tso [EMAIL PROTECTED] wrote: I'm going to NACK it as well. This kind of code churn where we make symbols static only to make them non-static again in an existing ext4 tree is exactly the sort of needless code churn that makes patches start to conflict and where we need different patches depending on whether it is intended for -mm or linux-next or mainline. Resolving a reject due to a line of code difference is about 10 seconds of your or time - and you two already spent 10-100 times more time on just fighting that line of code - which is weird. Yes, naked exports are sometimes OK (and i recently defended and NACK-ed an export removal in one such case), but the reason given here, out of tree forks are very much not such a case. Example: recently i got some scheduler stuff not included in time - and had to remove an unnecessary export. Stupid me for not having planned it right and i deserved looking stupid in the git log. This time you did not get the journal checksum patch into 2.6.25 by time: tough luck, it shows that you suck at getting stuff done in time sometimes. So please deal with it like most other subsystem maintainers do and stop complaining about code churn - nobody but you changes the ext3 codebase, it's one of the codebases least affected by general kernel flux, it's an ultimate leaf subsystem. In fact, we only had 5300 lines of code flux in ext3 in the past ~3 years: $ git-log -p -M v2.6.12.. fs/ext3/ | diffstat | tail -1 45 files changed, 3205 insertions(+), 2043 deletions(-) with its 16 KLOC's that's a churn rate of ~10% per year. [and 9% out of that was ext3's own feature churn, nothing externally inflicted] As a comparison, the core kernel had a churn of 182,000 lines [and this excludes renames] in the past 3 years: $ git-log -p -M v2.6.12.. kernel/ | diffstat | tail -1 284 files changed, 96256 insertions(+), 45277 deletions(-) and with it being a 91 KLOC codebase that's a _51%_ churn rate per year. Or take a look at the networking code, with a ~40% 3-year churn rate in half a million lines of code. _That_ is a high rate of change - but the networking people have learned how to deal with it and Linux has become the best OS on the planet in terms of networking technology. The whole kernel has 8406491 LOC at the moment, in the past 3 years it had a churn of 9214017 lines of code (excluding renames), which averages to a churn rate of ~35% per year. Ingo - To unsubscribe from this list: send the line unsubscribe linux-ext4 in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [2.6 patch] fs/jbd/journal.c: cleanups
On Mon, Feb 18, 2008 at 03:12:09PM +0200, Adrian Bunk wrote: If me resending this old patch collides with something finally getting a user this part of my patch shouldn't be applied now (but you might get it again in 6 months if it's still unused...). But generally such conflicts would become visible if known development trees that are intended for mainline were in -mm. It *has* been in -mm, except for periods when akpm has dropped it due to conflicts due to the must have an in-tree user doctrinaire attitude due to a conflict with the r/o bind patch. Did you actually try to do a compile test, or only made sure the patch would apply? The patch won't collide at application time, but it would when you compile it - Ted - To unsubscribe from this list: send the line unsubscribe linux-ext4 in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [2.6 patch] fs/jbd/journal.c: cleanups
So please deal with it like most other subsystem maintainers do and stop complaining about code churn - nobody but you changes the ext3 codebase, it's one of the codebases least affected by general kernel flux, it's an ultimate leaf subsystem. Right, sorry. I misread the filename; I thought this was against fs/jbd2, instead of fs/jbd. - Ted - To unsubscribe from this list: send the line unsubscribe linux-ext4 in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [2.6 patch] fs/jbd/journal.c: cleanups
* Adrian Bunk [EMAIL PROTECTED] wrote: The following was meant 100% seriously: This patch has been sent on: - 16 May 2006 - 1 May 2006 - 23 Apr 2006 ouch ... i guess this explains what static code metrics already suggest: $ code-quality fs/ext3/ fs/ext4/ errors lines of code errors/KLOC fs/ext3/408 16055 25.4 fs/ext4/394 25169 15.6 compared to: $ code-quality kernel/ errors lines of code errors/KLOC kernel/ 739 98759 7.4 and i guess our hope is now in: $ code-quality fs/btrfs/ errors lines of code errors/KLOC fs/btrfs/ 240 20556 11.6 (which hopefully will be a project done in a much more Linux-ish manner: open participation, the easy taking of patches, an inclusive, newbie-friendly contribution atmosphere, etc.) Ingo ps. http://redhat.com/~mingo/x86.git/code-quality - To unsubscribe from this list: send the line unsubscribe linux-ext4 in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [2.6 patch] fs/jbd/journal.c: cleanups
* Theodore Tso [EMAIL PROTECTED] wrote: On Mon, Feb 18, 2008 at 02:31:40PM +0100, Ingo Molnar wrote: i guess this explains what static code metrics already suggest: Am I right in assuming that code-quality is just a program which runs checkpatch.pl and measures the number of warnings and calls them errors? no, it picks the checkpatch.pl errors and calls them errors. The script ignores checkpatch.pl warnings. (although even the checkpatch.pl warnings tend to be correct in like 90% of the cases - YMMV) but yes, i'd agree with you if you said this was an arbitrary metric that does not map to the functional qualities of the code in a provable way. (other than the fact that any of these style errors are valid reasons to reject new code that is being offered into the kernel. I.e. if the ext4 codebase was submitted as a new, unknown filesystem right now, it would probably have a hard time getting accepted with all those silly style problems in it. Why should old subsystems have a lower threshold to meet than newer subsystems? Isnt that unfair? ) And fact is that i've yet to see really crappy code with an excellent checkpatch score, and really good code with a very poor checkpatch score. So i think you have to accept that there's _some_ correlation. And i believe this comes from the simple fact that mental carelessness is contagious: lack of attention to details on the thinking level subsconsciously slips over into the style space as well. It's hard (impossible) to measure true code quality, but the style space is interrelated with the true quality space and is easier to measure. I just had a look at the errors and warnings that checkpatch --file emits on fs/ext3/*.c, and if i was maintaining those files i'd be fixing over 50% of them - because they are just style inconsistencies often _within the same function_ which would be constant distraction when adding new code. In fact, the more critical a piece of code, the more strategic it is to users, the less acceptable it is IMO to have style noise and similar visual distractions in it. One unusual indentation or spacing, although silly to even mention in isolation, _can_ trick the human eye into glossing over just one critical piece of information to find or avoid a bug. And as a bonus, consistent source code style is also an attractive aesthetic experience to any first-time visitor of your subsystem. BYMMV. Ingo - To unsubscribe from this list: send the line unsubscribe linux-ext4 in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
