[PATCH] VFS: Clean up shared mount flag propagation

[PATCH] VFS: Clean up shared mount flag propagation

[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 <vaurora@redhat.com>
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.

diff --git a/fs/pnode.h b/fs/pnode.h
index 958665d..adc52ca 100644
--- a/fs/pnode.h
+++ b/fs/pnode.h
@@ -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;
 }
 
diff --git a/include/linux/mount.h b/include/linux/mount.h
index 5d52753..257985f 100644
--- a/include/linux/mount.h
+++ b/include/linux/mount.h
@@ -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;

Re: [PATCH] VFS: Clean up shared mount flag propagation

[Al Viro]

On Tue, Jan 26, 2010 at 02:20:47PM -0500, Valerie Aurora wrote:
> 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.

Re: [PATCH] VFS: Clean up shared mount flag propagation

[Miklos Szeredi]

On Tue, 26 Jan 2010, Al Viro wrote:
> On Tue, Jan 26, 2010 at 02:20:47PM -0500, Valerie Aurora wrote:
> > 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

Re: [PATCH] VFS: Clean up shared mount flag propagation

[Al Viro]

On Fri, Jan 29, 2010 at 09:40:17AM +0100, Miklos Szeredi wrote:
> On Tue, 26 Jan 2010, Al Viro wrote:
> > On Tue, Jan 26, 2010 at 02:20:47PM -0500, Valerie Aurora wrote:
> > > 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().

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

Re: [PATCH] VFS: Clean up shared mount flag propagation

[Valerie Aurora]

On Fri, Jan 29, 2010 at 09:40:17AM +0100, Miklos Szeredi wrote:
> On Tue, 26 Jan 2010, Al Viro wrote:
> > On Tue, Jan 26, 2010 at 02:20:47PM -0500, Valerie Aurora wrote:
> > > 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().

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

Re: [PATCH] VFS: Clean up shared mount flag propagation

[Ram Pai]

On Tue, 2010-01-26 at 14:20 -0500, Valerie Aurora wrote:
> 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 <vaurora@redhat.com>
> 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.
> 
> diff --git a/fs/pnode.h b/fs/pnode.h
> index 958665d..adc52ca 100644
> --- a/fs/pnode.h
> +++ b/fs/pnode.h
> @@ -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; 





> diff --git a/include/linux/mount.h b/include/linux/mount.h
> index 5d52753..257985f 100644
> --- a/include/linux/mount.h
> +++ b/include/linux/mount.h
> @@ -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.

> +/*
> + * 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)

RP
> +
> 
>  struct vfsmount {
>  	struct list_head mnt_hash;
-- 
Ram Pai
System X Device-Driver Enablement Lead
Linux Technology Center
Beaverton OR-97006
503-5783752 t/l 7753752
linuxram@us.ibm.com

Re: [PATCH] VFS: Clean up shared mount flag propagation

[Valerie Aurora]

On Fri, Jan 29, 2010 at 09:52:17AM -0800, Ram Pai wrote:
> On Tue, 2010-01-26 at 14:20 -0500, Valerie Aurora wrote:
> > 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 <vaurora@redhat.com>
> > 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.
> > 
> > diff --git a/fs/pnode.h b/fs/pnode.h
> > index 958665d..adc52ca 100644
> > --- a/fs/pnode.h
> > +++ b/fs/pnode.h
> > @@ -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?

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.

> 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.

> > diff --git a/include/linux/mount.h b/include/linux/mount.h
> > index 5d52753..257985f 100644
> > --- a/include/linux/mount.h
> > +++ b/include/linux/mount.h
> > @@ -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.

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)

> > +/*
> > + * 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)

Agree with this.

-VAL