LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Theodore Tso <tytso@MIT.EDU>
To: Ingo Molnar <mingo@elte.hu>
Cc: Andreas Dilger <adilger@sun.com>, Adrian Bunk <bunk@kernel.org>,
	sct@redhat.com, akpm@linux-foundation.org,
	linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [2.6 patch] fs/jbd/journal.c: cleanups
Date: Mon, 18 Feb 2008 06:49:36 -0500	[thread overview]
Message-ID: <20080218114936.GM8905@mit.edu> (raw)
In-Reply-To: <20080218071229.GA1459@elte.hu>

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

  reply	other threads:[~2008-02-18 11:51 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-17  8:19 Adrian Bunk
2008-02-18  7:04 ` Andreas Dilger
2008-02-18  7:12   ` Ingo Molnar
2008-02-18 11:49     ` Theodore Tso [this message]
2008-02-18 12:57       ` Ingo Molnar
2008-02-18 13:31         ` Theodore Tso
2008-02-18 13:55           ` Ingo Molnar
2008-02-18 13:12       ` Adrian Bunk
2008-02-18 13:28         ` Theodore Tso
2008-02-27 19:39           ` Adrian Bunk
2008-02-18 13:31         ` Ingo Molnar
2008-02-18 15:11           ` Theodore Tso
2008-02-18 16:22             ` Ingo Molnar
2008-02-27 21:20   ` [2.6 patch] unexport journal_update_superblock Adrian Bunk

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080218114936.GM8905@mit.edu \
    --to=tytso@mit.edu \
    --cc=adilger@sun.com \
    --cc=akpm@linux-foundation.org \
    --cc=bunk@kernel.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@elte.hu \
    --cc=sct@redhat.com \
    --subject='Re: [2.6 patch] fs/jbd/journal.c: cleanups' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).