VFS: Clean up shared mount flag propagation

PATCH In review Valerie Aurora 4 participants 7 messages Jan 26, 2010 – Jan 29, 2010 VFS

Share & tools

Valerie Aurora

author

2010-01-26 19:21 UTC #0 lore raw

PATCH 1/1 VFS: Clean up shared mount flag propagation

Al, you might have already fixed this as part of your mount flag rewrite, but here's a patch to clean up some problems with shared mount flag propagation that I ran into while working on union mounts. Against 2.6.32.

-VAL

commit 3f436c4466f278900d451494b317f6bb80b4c24b

Author: Valerie Aurora <[email protected]>
Date:   Tue Jan 26 11:09:00 2010 -0800

VFS: Clean up shared mount flag propagation

The handling of mount flags in set_mnt_shared() got a little tangled up during previous cleanups, with the following problems:

* MNT_PNODE_MASK is defined as a literal constant when it should be a bitwise xor of other MNT_* flags
* set_mnt_shared() clears and then sets MNT_SHARED (part of MNT_PNODE_MASK)
* MNT_PNODE_MASK could use a comment in mount.h
* MNT_PNODE_MASK is a terrible name, change to MNT_SHARED_MASK

This patch fixes these problems.

fs/pnode.h +1 -1

index 958665d..adc52ca 100644
@@ -26,7 +26,7 @@
 
 static inline void set_mnt_shared(struct vfsmount *mnt)
 {
-	mnt->mnt_flags &= ~MNT_PNODE_MASK;
+	mnt->mnt_flags &= ~MNT_SHARED_MASK;
 	mnt->mnt_flags |= MNT_SHARED;
 }

include/linux/mount.h +9 -1

index 5d52753..257985f 100644
@@ -34,7 +34,15 @@ struct mnt_namespace;
 
 #define MNT_SHARED	0x1000	/* if the vfsmount is a shared mount */
 #define MNT_UNBINDABLE	0x2000	/* if the vfsmount is a unbindable mount */
-#define MNT_PNODE_MASK	0x3000	/* propagation flag mask */
+/*
+ * MNT_SHARED_MASK is the set of flags that should be cleared when a
+ * mount becomes shared.  Currently, this is only the flag that says a
+ * mount cannot be bind mounted, since this is how we create a mount
+ * that shares events with another mount.  If you add a new MNT_*
+ * flag, consider how it interacts with shared mounts.
+ */
+#define MNT_SHARED_MASK	(MNT_UNBINDABLE)
+
 
 struct vfsmount {
 	struct list_head mnt_hash;

Al Viro

maintainer

2010-01-26 19:53 UTC #1 lore raw

Quoted 4 lines · Valerie Aurora

Al, you might have already fixed this as part of your mount flag rewrite, but here's a patch to clean up some problems with shared mount flag propagation that I ran into while working on union mounts. Against 2.6.32.

Umm... OK by me; I'll put that into for-next on tonight's push.

Miklos Szeredi

2010-01-29 09:11 UTC #2 lore raw

Quoted 7 lines · Al Viro

Quoted 4 lines · Valerie Aurora
Al, you might have already fixed this as part of your mount flag rewrite, but here's a patch to clean up some problems with shared mount flag propagation that I ran into while working on union mounts. Against 2.6.32.
Umm... OK by me; I'll put that into for-next on tonight's push.

The patch as committed to "untested" is wrong, MNT_SHARED_MASK needs to be MNT_UNBINDABLE | MNT_SHARED, because of the code in do_remount().

Thanks,
Miklos

Al Viro

maintainer

2010-01-29 13:36 UTC #3 lore raw

Quoted 7 lines · Miklos Szeredi

Quoted 6 lines · Al Viro
Quoted 4 lines · Valerie Aurora
Al, you might have already fixed this as part of your mount flag rewrite, but here's a patch to clean up some problems with shared mount flag propagation that I ran into while working on union mounts. Against 2.6.32.

Quoted 5 lines

Quoted 1 line
Umm... OK by me; I'll put that into for-next on tonight's push.
The patch as committed to "untested" is wrong, MNT_SHARED_MASK needs to be MNT_UNBINDABLE | MNT_SHARED, because of the code in do_remount().

D'oh... Sorry, my fault. Fixed and pushed (I prefer separate #define for that one, actually). Should propagate from hera shortly...

Valerie Aurora

author

2010-01-29 18:26 UTC #4 lore raw

In reply to Miklos Szeredi

Quoted 7 lines · Miklos Szeredi

Quoted 6 lines · Al Viro
Quoted 4 lines · Valerie Aurora
Al, you might have already fixed this as part of your mount flag rewrite, but here's a patch to clean up some problems with shared mount flag propagation that I ran into while working on union mounts. Against 2.6.32.

Quoted 5 lines

Quoted 1 line
Umm... OK by me; I'll put that into for-next on tonight's push.
The patch as committed to "untested" is wrong, MNT_SHARED_MASK needs to be MNT_UNBINDABLE | MNT_SHARED, because of the code in do_remount().

My bad - I made the patch against 2.6.32, which doesn't have this code. Without this, the only use of this mask was in set_mnt_shared(), which immediately set it again. :)

-VAL

Ram Pai

2010-01-29 17:52 UTC #5 lore raw

In reply to Valerie Aurora

Quoted 29 lines · Valerie Aurora

Al, you might have already fixed this as part of your mount flag rewrite, but here's a patch to clean up some problems with shared mount flag propagation that I ran into while working on union mounts. Against 2.6.32.
-VAL
commit 3f436c4466f278900d451494b317f6bb80b4c24b
Author: Valerie Aurora <[email protected]>
Date:   Tue Jan 26 11:09:00 2010 -0800
VFS: Clean up shared mount flag propagation
The handling of mount flags in set_mnt_shared() got a little tangled up during previous cleanups, with the following problems:
* MNT_PNODE_MASK is defined as a literal constant when it should be a bitwise xor of other MNT_* flags
* set_mnt_shared() clears and then sets MNT_SHARED (part of MNT_PNODE_MASK)
* MNT_PNODE_MASK could use a comment in mount.h
* MNT_PNODE_MASK is a terrible name, change to MNT_SHARED_MASK
This patch fixes these problems.
fs/pnode.h +0 -0
index 958665d..adc52ca 100644

fs/pnode.h line 32

Quoted 8 lines

fs/pnode.h +1 -1

@@ -26,7 +26,7 @@
 
 static inline void set_mnt_shared(struct vfsmount *mnt)
 {
-	mnt->mnt_flags &= ~MNT_PNODE_MASK;
+	mnt->mnt_flags &= ~MNT_SHARED_MASK;
 	mnt->mnt_flags |= MNT_SHARED;
 }

Is resetting the MNT_SHARED bit and setting it back, causing a issue?

If so, why not just do

  -      mnt->mnt_flags &=  ~MNT_PNODE_MASK;
  +      mnt->mnt_flags &=  ~MNT_UNBINDABLE;

include/linux/mount.h struct mnt_namespace; · line 37

Quoted 9 lines

include/linux/mount.h +0 -1

index 5d52753..257985f 100644
@@ -34,7 +34,15 @@ struct mnt_namespace;
 
 #define MNT_SHARED	0x1000	/* if the vfsmount is a shared mount */
 #define MNT_UNBINDABLE	0x2000	/* if the vfsmount is a unbindable mount */
-#define MNT_PNODE_MASK	0x3000	/* propagation flag mask */

I prefer to leave the name MNT_PNODE_MASK as is, because it captures the notion of propagation v/s the notion of just being shared.

include/linux/mount.h struct mnt_namespace; · line 44

Quoted 8 lines

include/linux/mount.h +8 -0

+/*
+ * MNT_SHARED_MASK is the set of flags that should be cleared when a
+ * mount becomes shared.  Currently, this is only the flag that says a
+ * mount cannot be bind mounted, since this is how we create a mount
+ * that shares events with another mount.  If you add a new MNT_*
+ * flag, consider how it interacts with shared mounts.
+ */
+#define MNT_SHARED_MASK	(MNT_UNBINDABLE)

and make this
+#define MNT_SHARED_MASK (MNT_UNBINDABLE | MNT_SHARED)

Quoted 4 lines

include/linux/mount.h +1 -0

+
 
 struct vfsmount {
 	struct list_head mnt_hash;

Signature · 6 lines

Ram Pai
System X Device-Driver Enablement Lead
Linux Technology Center
Beaverton OR-97006
503-5783752 t/l 7753752
[email protected]

Valerie Aurora

author

2010-01-29 18:40 UTC #6 lore raw

Quoted 31 lines · Ram Pai

Quoted 30 lines · Valerie Aurora
Al, you might have already fixed this as part of your mount flag rewrite, but here's a patch to clean up some problems with shared mount flag propagation that I ran into while working on union mounts. Against 2.6.32.
-VAL
commit 3f436c4466f278900d451494b317f6bb80b4c24b
Author: Valerie Aurora <[email protected]>
Date:   Tue Jan 26 11:09:00 2010 -0800
VFS: Clean up shared mount flag propagation
The handling of mount flags in set_mnt_shared() got a little tangled up during previous cleanups, with the following problems:
* MNT_PNODE_MASK is defined as a literal constant when it should be a bitwise xor of other MNT_* flags
* set_mnt_shared() clears and then sets MNT_SHARED (part of MNT_PNODE_MASK)
* MNT_PNODE_MASK could use a comment in mount.h
* MNT_PNODE_MASK is a terrible name, change to MNT_SHARED_MASK
This patch fixes these problems.
fs/pnode.h +0 -0
index 958665d..adc52ca 100644
@@ -26,7 +26,7 @@
 

Quoted 9 lines

fs/pnode.h line 32
Quoted 7 lines
fs/pnode.h +1 -1
 static inline void set_mnt_shared(struct vfsmount *mnt)
 {
-	mnt->mnt_flags &= ~MNT_PNODE_MASK;
+	mnt->mnt_flags &= ~MNT_SHARED_MASK;
 	mnt->mnt_flags |= MNT_SHARED;
 }
 
Is resetting the MNT_SHARED bit and setting it back, causing a issue?

What was confusing is defining MNT_PNODE_MASK to remove the MNT_SHARED bit, and then having exactly one use in the source base which then immediately set it back. That looks like a bug. However, it looks like the real bug was not using MNT_PNODE_MASK to save these bits in do_remount(), fixed in commit 7b43a79f32c0a05e7562043af98e25c05c89b18e.

Quoted 3 lines

If so, why not just do

  -      mnt->mnt_flags &=  ~MNT_PNODE_MASK;
  +      mnt->mnt_flags &=  ~MNT_UNBINDABLE;

I think this is much clearer and easier to understand.

Quoted 6 lines

Quoted 6 lines
include/linux/mount.h +0 -0
index 5d52753..257985f 100644
@@ -34,7 +34,15 @@ struct mnt_namespace;
 

Quoted 6 lines

include/linux/mount.h struct mnt_namespace; · line 37
Quoted 3 lines
include/linux/mount.h +0 -1
 #define MNT_SHARED	0x1000	/* if the vfsmount is a shared mount */
 #define MNT_UNBINDABLE	0x2000	/* if the vfsmount is a unbindable mount */
-#define MNT_PNODE_MASK	0x3000	/* propagation flag mask */
I prefer to leave the name MNT_PNODE_MASK as is, because it captures the notion of propagation v/s the notion of just being shared.

I understand that this name is obvious to you as the author of this code, but it doesn't convey any useful information to me. :) Perhaps:

/*
* We propogate most mnt flag changes to peer mounts, but not the
* flags that specify whether or not propogation occurs. */
#define MNT_PROPOGATION_MASK (MNT_UNBINDABLE | MNT_SHARED)

Quoted 4 lines

include/linux/mount.h +4 -0

+/*
+ * MNT_SHARED_MASK is the set of flags that should be cleared when a
+ * mount becomes shared.  Currently, this is only the flag that says a
+ * mount cannot be bind mounted, since this is how we create a mount

Quoted 8 lines

include/linux/mount.h struct mnt_namespace; · line 44
Quoted 4 lines
include/linux/mount.h +4 -0
+ * that shares events with another mount.  If you add a new MNT_*
+ * flag, consider how it interacts with shared mounts.
+ */
+#define MNT_SHARED_MASK	(MNT_UNBINDABLE)
and make this
+#define MNT_SHARED_MASK (MNT_UNBINDABLE | MNT_SHARED)

Agree with this.

-VAL