Re: [PATCH v5] mnt: Tuck mounts under others instead of creating shadow/side mounts.

[ebiederm@xmission.com (Eric W. Biederman)]

Ram Pai <linuxram@us.ibm.com> writes:

>> @@ -359,12 +373,24 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
>> 
>>  	for (m = propagation_next(parent, parent); m;
>>  	     		m = propagation_next(m, parent)) {
>> -		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
>> -		if (child && list_empty(&child->mnt_mounts) &&
>> -		    (ret = do_refcount_check(child, 1)))
>> -			break;
>> +		int count = 1;
>> +		child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
>> +		if (!child)
>> +			continue;
>> +
>> +		/* Is there exactly one mount on the child that covers
>> +		 * it completely whose reference should be ignored?
>> +		 */
>> +		topper = find_topper(child);
>
> This is tricky. I understand it is trying to identify the case where a
> mount got tucked-in because of propagation.  But this will not
> distinguish the case where a mount got over-mounted genuinely, not because of
> propagation, but because of explicit user action.
>
>
> example:
>
> case 1: (explicit user action)
> 	B is a slave of A
> 	mount something on A/a , it will propagate to B/a
> 	and than mount something on B/a
>
> case 2: (tucked mount)
> 	B is a slave of A
> 	mount something on B/a
> 	and than mount something on A/a
>
> Both case 1 and case 2 lead to the same mount configuration.
>
>
> 	  however 'umount A/a' in case 1 should fail.
> 	  and 'umount A/a' in case 2 should pass.
>
> Right? in other words, umounts of 'tucked mounts' should pass(case 2).
> 	whereas umounts of mounts on which overmounts exist should
> 		fail.(case 1)

Looking at your example.  I agree that case 1 will fail today.
However my actual expectation would be for both mount configurations
to behave the same.  In both cases something has been explicitly mounted
on B/a and something has propagated to B/a.  In both cases the mount
on top is what was explicitly mounted, and the mount below is what was
propagated to B/a.

I don't see why the order of operations should matter.

> maybe we need a flag to identify tucked mounts?

To preserve our exact current semantics yes.

The mount configurations that are delibearately constructed that I am
aware of are comparatively simple.  I don't think anyone has even taken
advantage of the shadow/side mounts at this point.  I made a reasonable
effort to find out and no one was even aware they existed.  Much less
what they were.  And certainly no one I talked to could find code that
used them.

So I think we are fine with a very modest semantic change here.
Especially one that appears to make the semantics more consistent and
predictable.

I also expect the checkpoint/restart folks will appreciate the change
as by giving them options it will make it easier to reconstruct
complicated mount trees.

Eric


>> +
>> +		if (do_refcount_check(child, count))
>> +			return 1;
>>  	}
>> -	return ret;
>> +	return 0;
>>  }
>> 
>>  /*
>> @@ -381,7 +407,7 @@ void propagate_mount_unlock(struct mount *mnt)
>> 
>>  	for (m = propagation_next(parent, parent); m;
>>  			m = propagation_next(m, parent)) {
>> -		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
>> +		child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
>>  		if (child)
>>  			child->mnt.mnt_flags &= ~MNT_LOCKED;
>>  	}
>> @@ -399,9 +425,11 @@ static void mark_umount_candidates(struct mount *mnt)
>> 
>>  	for (m = propagation_next(parent, parent); m;
>>  			m = propagation_next(m, parent)) {
>> -		struct mount *child = __lookup_mnt_last(&m->mnt,
>> +		struct mount *child = __lookup_mnt(&m->mnt,
>>  						mnt->mnt_mountpoint);
>> -		if (child && (!IS_MNT_LOCKED(child) || IS_MNT_MARKED(m))) {
>> +		if (!child || (child->mnt.mnt_flags & MNT_UMOUNT))
>> +			continue;
>> +		if (!IS_MNT_LOCKED(child) || IS_MNT_MARKED(m)) {
>>  			SET_MNT_MARK(child);
>>  		}
>>  	}
>> @@ -420,8 +448,8 @@ static void __propagate_umount(struct mount *mnt)
>> 
>>  	for (m = propagation_next(parent, parent); m;
>>  			m = propagation_next(m, parent)) {
>> -
>> -		struct mount *child = __lookup_mnt_last(&m->mnt,
>> +		struct mount *topper;
>> +		struct mount *child = __lookup_mnt(&m->mnt,
>>  						mnt->mnt_mountpoint);
>>  		/*
>>  		 * umount the child only if the child has no children
>> @@ -430,6 +458,15 @@ static void __propagate_umount(struct mount *mnt)
>>  		if (!child || !IS_MNT_MARKED(child))
>>  			continue;
>>  		CLEAR_MNT_MARK(child);
>> +
>> +		/* If there is exactly one mount covering all of child
>> +		 * replace child with that mount.
>> +		 */
>> +		topper = find_topper(child);
>> +		if (topper)
>> +			mnt_change_mountpoint(child->mnt_parent, child->mnt_mp,
>> +					      topper);
>> +
>>  		if (list_empty(&child->mnt_mounts)) {
>>  			list_del_init(&child->mnt_child);
>>  			child->mnt.mnt_flags |= MNT_UMOUNT;
>> diff --git a/fs/pnode.h b/fs/pnode.h
>> index 550f5a8b4fcf..dc87e65becd2 100644
>> --- a/fs/pnode.h
>> +++ b/fs/pnode.h
>> @@ -49,6 +49,8 @@ int get_dominating_id(struct mount *mnt, const struct path *root);
>>  unsigned int mnt_get_count(struct mount *mnt);
>>  void mnt_set_mountpoint(struct mount *, struct mountpoint *,
>>  			struct mount *);
>> +void mnt_change_mountpoint(struct mount *parent, struct mountpoint *mp,
>> +			   struct mount *mnt);
>>  struct mount *copy_tree(struct mount *, struct dentry *, int);
>>  bool is_path_reachable(struct mount *, struct dentry *,
>>  			 const struct path *root);
>> -- 
>> 2.10.1