[PATCH] namespace.c: cleanup in mark_mounts_for_expiry()

[PATCH] namespace.c: cleanup in mark_mounts_for_expiry()

[Miklos Szeredi]

[ fell in love with that function, now can't let go... ]

This patch simplifies mark_mounts_for_expiry() by using detach_mnt()
instead of duplicating everything it does.

It should be an equivalent transformation except for righting the
dput/mntput order.

Signed-off-by: Miklos Szeredi <miklos@szeredi.hu>

Index: linux/fs/namespace.c
===================================================================
--- linux.orig/fs/namespace.c	2005-05-20 15:44:59.000000000 +0200
+++ linux/fs/namespace.c	2005-05-20 15:45:21.000000000 +0200
@@ -880,24 +880,13 @@ void mark_mounts_for_expiry(struct list_
 		/* check that it is still dead: the count should now be 2 - as
 		 * contributed by the vfsmount parent and the mntget above */
 		if (atomic_read(&mnt->mnt_count) == 2) {
-			struct vfsmount *xdmnt;
-			struct dentry *xdentry;
+			struct nameidata old_nd;
 
 			/* delete from the namespace */
 			list_del_init(&mnt->mnt_list);
-			list_del_init(&mnt->mnt_child);
-			list_del_init(&mnt->mnt_hash);
-			mnt->mnt_mountpoint->d_mounted--;
-
-			xdentry = mnt->mnt_mountpoint;
-			mnt->mnt_mountpoint = mnt->mnt_root;
-			xdmnt = mnt->mnt_parent;
-			mnt->mnt_parent = mnt;
-
+			detach_mnt(mnt, &old_nd);
 			spin_unlock(&vfsmount_lock);
-
-			mntput(xdmnt);
-			dput(xdentry);
+			path_release(&old_nd);
 
 			/* now lay it to rest if this was the last ref on the
 			 * superblock */

Re: [PATCH] namespace.c: cleanup in mark_mounts_for_expiry()

[Al Viro]

On Fri, May 20, 2005 at 03:54:51PM +0200, Miklos Szeredi wrote:
> [ fell in love with that function, now can't let go... ]
> 
> This patch simplifies mark_mounts_for_expiry() by using detach_mnt()
> instead of duplicating everything it does.
> 
> It should be an equivalent transformation except for righting the
> dput/mntput order.

Looks sane.  However, we still have a problem here - just what would
happen if vfsmount is detached while we were grabbing namespace
semaphore?  Refcount alone is not useful here - we might be held by
whoever had detached the vfsmount.  IOW, we should check that it's
still attached (i.e. that mnt->mnt_parent != mnt).  If it's not -
just leave it alone, do mntput() and let whoever holds it deal with
the sucker.  No need to put it back on lists.

Re: [PATCH] namespace.c: cleanup in mark_mounts_for_expiry()

[Miklos Szeredi]

> Looks sane.  However, we still have a problem here - just what would
> happen if vfsmount is detached while we were grabbing namespace
> semaphore?  Refcount alone is not useful here - we might be held by
> whoever had detached the vfsmount.  IOW, we should check that it's
> still attached (i.e. that mnt->mnt_parent != mnt).  If it's not -
> just leave it alone, do mntput() and let whoever holds it deal with
> the sucker.  No need to put it back on lists.

Right.  I'll fix that too.

On a bit unrelated node, in do_unmount() why is that
DQUOT_OFF()/acct_auto_close() thing only called for the base of a tree
being detached, and not for any submounts?  Is that how it's supposed
to work?

Miklos

Re: [PATCH] namespace.c: cleanup in mark_mounts_for_expiry()

[Jan Kara]

> > Looks sane.  However, we still have a problem here - just what would
> > happen if vfsmount is detached while we were grabbing namespace
> > semaphore?  Refcount alone is not useful here - we might be held by
> > whoever had detached the vfsmount.  IOW, we should check that it's
> > still attached (i.e. that mnt->mnt_parent != mnt).  If it's not -
> > just leave it alone, do mntput() and let whoever holds it deal with
> > the sucker.  No need to put it back on lists.
> 
> Right.  I'll fix that too.
> 
> On a bit unrelated node, in do_unmount() why is that
> DQUOT_OFF()/acct_auto_close() thing only called for the base of a tree
> being detached, and not for any submounts?  Is that how it's supposed
> to work?
  I guess the code is there since the good old times when each
filesystem could be mounted at most once and you had to call umount on
it directly ;). I see two possibilites there:
  1) Call DQUOT_OFF() when the last reference to the superblock should
  be dropped. This has a problem that currently quota code holds the
  reference to the vfsmount of the mountpoint it was called on (to
  protect itself against umount). So if you try something like
  mount /home, quotaon /home, mount --bind /home /home2, umount /home,
  it will fail with EBUSY.
  2) Make quota code protect against umount in a different way without
  holding the vfsmount references (any ideas?). Then the above described
  use will work. But I'm not sure it's worth the problems especially
  with userspace tools not being able to see the proper mount options
  and so on.

So personally I'd prefer 1). For the namespace code it means only that
it should call DQUOT_OFF() whenever it intends to drop the last
reference to the superblock (and check for business only after quota
has been turned off).

								Honza
-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs