[Fwd: [PATCH 3/5] NFS: Abstract out namespace initialisation [try #2]]

[Fwd: [PATCH 3/5] NFS: Abstract out namespace initialisation [try #2]]

[Sam Vilain]

The attached patch abstracts out the namespace initialisation so that 
temporary namespaces can be set up elsewhere.

Signed-Off-By: David Howells <dhowells@redhat.com>
Acked-By: Sam Vilain <sam.vilain@catalyst.net.nz>
---
David,

This looks sane to me, thought I'd just quickly ack it as I'm also doing 
work in this area... it seems a lot of what you're doing is cleaning up 
the boundaries between VFS and FS - did you get a chance to review the 
patch Herbert Pötzl sent to the list about the permission() cleanup?

  fs/namespace.c            |    8 +-------
  include/linux/namespace.h |   15 +++++++++++++++
  2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 51d3ebc..0194538 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1688,13 +1688,7 @@ static void __init init_mount_tree(void)
  	namespace = kmalloc(sizeof(*namespace), GFP_KERNEL);
  	if (!namespace)
  		panic("Can't allocate initial namespace");
-	atomic_set(&namespace->count, 1);
-	INIT_LIST_HEAD(&namespace->list);
-	init_waitqueue_head(&namespace->poll);
-	namespace->event = 0;
-	list_add(&mnt->mnt_list, &namespace->list);
-	namespace->root = mnt;
-	mnt->mnt_namespace = namespace;
+	init_namespace(namespace, mnt);

  	init_task.namespace = namespace;
  	read_lock(&tasklist_lock);
diff --git a/include/linux/namespace.h b/include/linux/namespace.h
index 3abc8e3..ea6fd62 100644
--- a/include/linux/namespace.h
+++ b/include/linux/namespace.h
@@ -17,6 +17,21 @@ extern int copy_namespace(int, struct ta
  extern void __put_namespace(struct namespace *namespace);
  extern struct namespace *dup_namespace(struct task_struct *, struct 
fs_struct *);

+static inline void init_namespace(struct namespace *namespace,
+				  struct vfsmount *mnt)
+{
+	atomic_set(&namespace->count, 1);
+	INIT_LIST_HEAD(&namespace->list);
+	init_waitqueue_head(&namespace->poll);
+	namespace->event = 0;
+	namespace->root = mnt;
+
+	if (mnt) {
+		list_add(&mnt->mnt_list, &namespace->list);
+		mnt->mnt_namespace = namespace;
+	}
+}
+
  static inline void put_namespace(struct namespace *namespace)
  {
  	if (atomic_dec_and_lock(&namespace->count, &vfsmount_lock))

Re: [Fwd: [PATCH 3/5] NFS: Abstract out namespace initialisation [try #2]]

[Christoph Hellwig]

On Thu, Mar 02, 2006 at 10:37:03AM +1300, Sam Vilain wrote:
> The attached patch abstracts out the namespace initialisation so that 
> temporary namespaces can be set up elsewhere.

Definitily shouldb't be inline.  And until you have a second caller
it's utterly pointless.  So NACK for now.

Re: [Fwd: [PATCH 3/5] NFS: Abstract out namespace initialisation [try #2]]

[David Howells]

Christoph Hellwig <hch@infradead.org> wrote:

> > The attached patch abstracts out the namespace initialisation so that 
> > temporary namespaces can be set up elsewhere.
> 
> Definitily shouldb't be inline.  And until you have a second caller
> it's utterly pointless.  So NACK for now.

I presume from that you didn't notice that the second caller was in patch 5/5?

David

Re: [Fwd: [PATCH 3/5] NFS: Abstract out namespace initialisation [try #2]]

[Sam Vilain]

David Howells wrote:
> Christoph Hellwig <hch@infradead.org> wrote:
>>>The attached patch abstracts out the namespace initialisation so that 
>>>temporary namespaces can be set up elsewhere.
>>Definitily shouldb't be inline.  And until you have a second caller
>>it's utterly pointless.  So NACK for now.
> I presume from that you didn't notice that the second caller was in patch 5/5?

AIUI, each patch must stand on its own in every regard.  I guess you 
need to make it inline in the later patch - or not at all given the 
marginal speed difference vs. core size increase.

Sam.

Re: [Fwd: [PATCH 3/5] NFS: Abstract out namespace initialisation [try #2]]

[David Howells]

Sam Vilain <sam@vilain.net> wrote:

> AIUI, each patch must stand on its own in every regard.  I guess you 
> need to make it inline in the later patch - or not at all given the 
> marginal speed difference vs. core size increase.

No. It has to be permissable to make a series of patches that depend one upon
another for at least three reasons:

 (1) Patches can be unmanageably large in one lump, so splitting them up is a
     sensible option, even through the individual patches won't work or even
     compile independently.

 (2) It may make sense to place linked changes to two logically separate units
     in two separate patches, for instance I'm changing the core kernel to add
     an extra argument to get_sb() and the get_sb_*() convenience functions in
     one patch and then supplying another patch to change all the filesystems.

     This makes it much easier for a reviewer to see what's going on. They know
     the patches are interdependent, but they can see the main core of the
     changes separated out from the massively repetative but basically less
     interesting changes that are a side effect of the main change.

 (3) A series of patches may form a set of logical steps (for instance my
     patches 1-2 are the first step and patches 3-5 the second). It may be (and
     it is in my case) that each step will build and run, provided all the
     previous steps are applied; but that a step won't build or run without the
     preceding steps.

Remember: one of the main reasons for splitting patches is to make it easier
for other people to appreciate just how sublimely terrific your work is:-)

David

Re: [Fwd: [PATCH 3/5] NFS: Abstract out namespace initialisation [try #2]]

[Sam Vilain]

David Howells wrote:

>>AIUI, each patch must stand on its own in every regard.  I guess you 
>>need to make it inline in the later patch - or not at all given the 
>>marginal speed difference vs. core size increase.
>>    
>>
>
>No. It has to be permissable to make a series of patches that depend one upon
>another for at least three reasons:
>
> (1) Patches can be unmanageably large in one lump, so splitting them up is a
>     sensible option, even through the individual patches won't work or even
>     compile independently.
>
> (2) It may make sense to place linked changes to two logically separate units
>     in two separate patches, for instance I'm changing the core kernel to add
>     an extra argument to get_sb() and the get_sb_*() convenience functions in
>     one patch and then supplying another patch to change all the filesystems.
>
>     This makes it much easier for a reviewer to see what's going on. They know
>     the patches are interdependent, but they can see the main core of the
>     changes separated out from the massively repetative but basically less
>     interesting changes that are a side effect of the main change.
>
> (3) A series of patches may form a set of logical steps (for instance my
>     patches 1-2 are the first step and patches 3-5 the second). It may be (and
>     it is in my case) that each step will build and run, provided all the
>     previous steps are applied; but that a step won't build or run without the
>     preceding steps.
>
>Remember: one of the main reasons for splitting patches is to make it easier
>for other people to appreciate just how sublimely terrific your work is:-)
>  
>

Interesting.  I've just seen patches slammed by subsystem maintainers 
before for doing things "the wrong way around" within a patchset.

I don't remember seeing this covered in TPP, am I missing having read a 
guide document or is this grey area?

Sam.

Re: [Fwd: [PATCH 3/5] NFS: Abstract out namespace initialisation [try #2]]

[Andrew Morton]

Sam Vilain <sam@vilain.net> wrote:
>
>  >Remember: one of the main reasons for splitting patches is to make it easier
>  >for other people to appreciate just how sublimely terrific your work is:-)
>  >  
>  >
> 
>  Interesting.  I've just seen patches slammed by subsystem maintainers 
>  before for doing things "the wrong way around" within a patchset.
> 
>  I don't remember seeing this covered in TPP, am I missing having read a 
>  guide document or is this grey area?

I just updated it.

--- tpp.txt	2006-03-04 16:32:28.000000000 -0800
+++ tpp2.txt	2006-03-04 16:33:10.000000000 -0800
@@ -1,7 +1,7 @@
 
 The perfect patch.
 akpm@osdl.org
-Updated 12 Jan 2006
+Updated 4 March 2006
 
 The latest version of this document may be found at
 http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt
@@ -93,8 +93,8 @@
    patch should contain a standalone changelog.  This implies that you need a
    patch management system which maintains changelogs.  See below.
 
-e) Add a Signed-off-by: line, as per the Documentation/SubmittingPatches
-   file in the kernel tree.
+e) Add a Signed-off-by: line, as per section 11 of the
+   Documentation/SubmittingPatches file in the kernel tree.
 
    Signed-off-by: implies that you had some part in the developent of the
    patch, or that you handled it and passed it on to another developer for
@@ -174,8 +174,49 @@
 	done
 
 
-6: Overall
-=========
+6: Patch series
+===============
+
+a) When sending a series of patches, number them in the Subject:s thusly:
+
+	[patch 1/10] ext2: block allocation: frob the globnozzle
+	[patch 2/10] ext2: block allocation: wash the pizza
+	etc
+
+b) Some people like to introduce a patch series with an introductory email
+   which doesn't actually carry a patch, such as:
+
+	[patch 0/10] ext2: block allocation changes
+
+   Please don't do this.  There is no facility in the git tree to carry
+   changelog-only changesets such as this (or at least, we don't do that) so
+   the information in the introductory email will be lost.
+
+   So I end up copying and pasting your nice introduction into the
+   changelog for the first patch, so it gets into git.  I'll follow it with
+   the text
+
+	This patch:
+
+   and then I'll include the changelog for the first patch of the series.
+
+   It would be preferred if the patch originators were to do this.
+
+c) Try very hard to ensure that the kernel builds and runs correctly at
+   every step of the patch series.  This requirement exists because of
+   `git-bisect'.  If someone is doing a bisection search for a kernel bug and
+   they land upon your won't-compile point partway through the exercise, they
+   will be unhappy.
+
+d) If your patch series includes non-runtime-affecting things such as
+   cleanups, whitespace fixes, file renames, moving functions around, etc then
+   this work should be done in the initial patches in the series.  The
+   functional changes should come later in the series.
+
+   This is mainly so that reversion of problematic changes becomes simpler.
+
+7: Overall
+==========
 
 a) Avoid MIME and attachements if possible.  Make sure that your email
    client does not wordwrap your patch.  Make sure that your email client does

Re: [Fwd: [PATCH 3/5] NFS: Abstract out namespace initialisation [try #2]]

[J. Bruce Fields]

On Thu, Mar 02, 2006 at 09:12:23PM +0000, David Howells wrote:
> No. It has to be permissable to make a series of patches that depend one upon
> another for at least three reasons:
> 
>  (1) Patches can be unmanageably large in one lump, so splitting them up is a
>      sensible option, even through the individual patches won't work or even
>      compile independently.

That breaks git-bisect.

>  (2) It may make sense to place linked changes to two logically separate units
>      in two separate patches, for instance I'm changing the core kernel to add
>      an extra argument to get_sb() and the get_sb_*() convenience functions in
>      one patch and then supplying another patch to change all the filesystems.
> 
>      This makes it much easier for a reviewer to see what's going on. They know
>      the patches are interdependent, but they can see the main core of the
>      changes separated out from the massively repetative but basically less
>      interesting changes that are a side effect of the main change.

It's also much easier to read a series of patches if each patch depends
only on the previous patches.  Then if I want to verify that they don't
break anything, I just need to read them through in order and verify
that each one is correct.

If earlier patches depend on later patches, then I may not be able to
verify correctness until I've read and understand the whole series,
which defeats somewhat the purpose of splitting up the patches.  Though
the above example wouldn't really be a problem.

And of course it seems rather silly to complain about splitting out a
function before adding the new caller.

--b.

Re: [Fwd: [PATCH 3/5] NFS: Abstract out namespace initialisation [try #2]]

[Sam Vilain]

Sam Vilain wrote:
> The attached patch abstracts out the namespace initialisation so that 
> temporary namespaces can be set up elsewhere.
> 
> Signed-Off-By: David Howells <dhowells@redhat.com>
> Acked-By: Sam Vilain <sam.vilain@catalyst.net.nz>

Sorry, folks, forgot the From: line in my Acks.  These patches are 
David's work, not mine :)

Sam.