Re: [PATCH 03/34] VFS: Add CL_NO_SLAVE flag to clone_mnt()/copy_tree()

[Valerie Aurora <vaurora@redhat.com>]

On Thu, Sep 16, 2010 at 09:34:01PM -0700, Ram Pai wrote:
> On Thu, Sep 16, 2010 at 05:09:58PM -0700, Ram Pai wrote:
> > On Thu, Sep 16, 2010 at 3:11 PM, Valerie Aurora <vaurora@redhat.com> wrote:
> > 
> > > Passing the CL_NO_SLAVE flag to clone_mnt() causes the clone
> > > to fail if the source mnt is a slave.
> > >
> > > Signed-off-by: Valerie Aurora <vaurora@redhat.com>
> > > ---
> > >  fs/namespace.c |    3 +++
> > >  fs/pnode.h     |    1 +
> > >  2 files changed, 4 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/fs/namespace.c b/fs/namespace.c
> > > index eeb4c22..6956062 100644
> > > --- a/fs/namespace.c
> > > +++ b/fs/namespace.c
> > > @@ -565,6 +565,9 @@ static struct vfsmount *clone_mnt(struct vfsmount *old,
> > > struct dentry *root,
> > >        if ((flag & CL_NO_SHARED) && (IS_MNT_SHARED(old)))
> > >                return ERR_PTR(-EINVAL);
> > >
> > > +       if ((flag & CL_NO_SLAVE) && (IS_MNT_SLAVE(old)))
> > > +               return ERR_PTR(-EINVAL);
> > > +
> > >
> > 
> > 
> > its been a while and my memory may have corroded.  But I dont think this
> > check is needed. Because cloning a 'slave mount' makes the mount a 'private
> > mount' and not a 'slave mount'.
>
> There is one case where a 'slave mount' when cloned can generate a 'slave mount', and
> that is when the 'slave mount' is also a 'shared mount'. So the above check has to
> be
> 
>        if ((flag & CL_NO_SLAVE) && (IS_MNT_SLAVE(old) && IS_MNT_SHARED(old)))
>                return ERR_PTR(-EINVAL);

Hey Ram,

I added this flag for union mounts. Union mounts can't deal with
namespace changes in the read-only layers, so we don't allow union of
read-only mounts that are the target of propagation events (shared or
slave).

We could automatically convert all slave or shared mounts into private
mounts when we clone the mounts, but that would surprise an
administrator who carefully set up their shared or slave read-only
mounts before unioning them.  So instead of silently converting slave
or shared to private, we error out.  Does that make sense?

All that being said, I debated how to do this cleanly and I'm still
not satisfied.  My goal is to both check and clone the proposed
read-only layers in one pass.  Without these flags, I had to do four
passes:

1. Find the "lowest" read-only mount at this mountpoint.
2. Check each mount for read-only, not shared, not slave.
3. Clone the subtree starting at the "lowest" mount.
4. Recheck the cloned tree for rules in #2.

One of the reasons I had to do it this way is that you can't hold
vfsmount_lock while calling copy_tree(), so the mount flags can change
between the first check in #2 and the copy_tree() in #3.  Also
sb->s_flag can change.  One of the problems with the current code is
that it can't deal with cloning existing union mounts, which we need
if we are to make bind mounts work (see do_loopback()).

Anyway, if you have any ideas, I'm all ears.

Thanks for reviewing,

-VAL