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@xxxxxxxxxx>
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
