This patch contains the following cleanups:
- make the following needlessly global function static:
- journal_check_used_features()
- remove the following unused EXPORT_SYMBOL's:
- journal_set_features
- journal_update_superblock
Signed-off-by: Adrian Bunk <[email protected]>
---
This patch has been sent on:
- 16 May 2006
- 1 May 2006
- 23 Apr 2006
fs/jbd/journal.c | 9 ++++-----
include/linux/jbd.h | 2 --
2 files changed, 4 insertions(+), 7 deletions(-)
4b48b0ddcc5e710bbc1e89e219db69000225ee28 diff --git a/fs/jbd/journal.c b/fs/jbd/journal.c
index 3943a89..b230288 100644
--- a/fs/jbd/journal.c
+++ b/fs/jbd/journal.c
@@ -62,13 +62,10 @@ EXPORT_SYMBOL(journal_revoke);
EXPORT_SYMBOL(journal_init_dev);
EXPORT_SYMBOL(journal_init_inode);
EXPORT_SYMBOL(journal_update_format);
-EXPORT_SYMBOL(journal_check_used_features);
EXPORT_SYMBOL(journal_check_available_features);
-EXPORT_SYMBOL(journal_set_features);
EXPORT_SYMBOL(journal_create);
EXPORT_SYMBOL(journal_load);
EXPORT_SYMBOL(journal_destroy);
-EXPORT_SYMBOL(journal_update_superblock);
EXPORT_SYMBOL(journal_abort);
EXPORT_SYMBOL(journal_errno);
EXPORT_SYMBOL(journal_ack_err);
@@ -1174,8 +1171,10 @@ void journal_destroy(journal_t *journal)
* features. Return true (non-zero) if it does.
**/
-int journal_check_used_features (journal_t *journal, unsigned long compat,
- unsigned long ro, unsigned long incompat)
+static int journal_check_used_features(journal_t *journal,
+ unsigned long compat,
+ unsigned long ro,
+ unsigned long incompat)
{
journal_superblock_t *sb;
diff --git a/include/linux/jbd.h b/include/linux/jbd.h
index b18fd3b..e8f81ab 100644
--- a/include/linux/jbd.h
+++ b/include/linux/jbd.h
@@ -909,8 +909,6 @@ extern journal_t * journal_init_dev(struct block_device *bdev,
int start, int len, int bsize);
extern journal_t * journal_init_inode (struct inode *);
extern int journal_update_format (journal_t *);
-extern int journal_check_used_features
- (journal_t *, unsigned long, unsigned long, unsigned long);
extern int journal_check_available_features
(journal_t *, unsigned long, unsigned long, unsigned long);
extern int journal_set_features
On Feb 17, 2008 10:19 +0200, Adrian Bunk wrote:
> This patch contains the following cleanups:
> - make the following needlessly global function static:
> - journal_check_used_features()
> - remove the following unused EXPORT_SYMBOL's:
> - journal_set_features
> - journal_update_superblock
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.
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
* Andreas Dilger <[email protected]> wrote:
> > This patch contains the following cleanups:
> > - make the following needlessly global function static:
> > - journal_check_used_features()
> > - remove the following unused EXPORT_SYMBOL's:
> > - journal_set_features
> > - journal_update_superblock
>
> 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.
Ingo
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
* 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
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
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
> 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
* 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
* Theodore Tso <[email protected]> wrote:
> > 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.
Btw., if people have problems with rejects from trivial patches, the
'mpatch' tool does wonders in resolving the easy ones:
http://oss.oracle.com/~mason/mpatch/
when you get a .rej file, first check whether mpatch can resolve it
conflict-free:
mpatch -d kernel/somefile.c.rej
if there are no conflicts then:
mpatch -a kernel/somefile.c.rej
quilt refresh, review the result and it's done in the majority of cases.
Ingo
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?
- Ted
* 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
On Mon, Feb 18, 2008 at 08:28:58AM -0500, Theodore Tso wrote:
> 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.
OK, that's bad luck.
> 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....
Yes, I do test compilations of all of my patches.
But I don't have an overview of all development trees for all
subsystems.
> - 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
On Mon, Feb 18, 2008 at 12:04:39AM -0700, Andreas Dilger wrote:
> On Feb 17, 2008 10:19 +0200, Adrian Bunk wrote:
> > This patch contains the following cleanups:
> > - make the following needlessly global function static:
> > - journal_check_used_features()
> > - remove the following unused EXPORT_SYMBOL's:
> > - journal_set_features
> > - journal_update_superblock
>
> 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.
>...
A patch that only unexports journal_update_superblock is below.
> Cheers, Andreas
cu
Adrian
<-- snip -->
This patch removes the unused EXPORT_SYMBOL(journal_update_superblock).
Signed-off-by: Adrian Bunk <[email protected]>
---
66cbc7390d66cdeb83a3f81d7ec1ea71d6f6e2c8 foobar
diff --git a/fs/jbd/journal.c b/fs/jbd/journal.c
index 3943a89..5c2ebb7 100644
--- a/fs/jbd/journal.c
+++ b/fs/jbd/journal.c
@@ -68,7 +68,6 @@ EXPORT_SYMBOL(journal_set_features);
EXPORT_SYMBOL(journal_create);
EXPORT_SYMBOL(journal_load);
EXPORT_SYMBOL(journal_destroy);
-EXPORT_SYMBOL(journal_update_superblock);
EXPORT_SYMBOL(journal_abort);
EXPORT_SYMBOL(journal_errno);
EXPORT_SYMBOL(journal_ack_err);