Re: [announce] vfs-scale git tree update

[Ian Kent <raven@themaw.net>]

On Thu, 2011-01-13 at 10:23 +0800, Ian Kent wrote:
> On Wed, 2011-01-12 at 14:11 -0600, Alex Elder wrote:
> > On Wed, 2011-01-12 at 12:15 +0800, Ian Kent wrote:
> > > On Wed, 2011-01-12 at 11:59 +0800, Ian Kent wrote:
> > > > On Tue, 2011-01-11 at 11:57 -0600, Alex Elder wrote:
> > > > > On Tue, 2011-01-11 at 08:51 -0800, Linus Torvalds wrote:
> > > > > > On Tue, Jan 11, 2011 at 8:34 AM, Alex Elder <aelder@sgi.com> wrote:
> > > > > > >
> > > > > > > FYI, when using this code, as merged by Linus, I hit the
> > > > > > > BUG_ON() at the beginning of d_set_d_op() when it's called
> > > > > > > by autofs4_dir_mkdir().  I managed to work around it by
> > > > > > > just commenting out those BUG_ON() calls but it's something
> > > > > > > that ought to get addressed properly.
> > > > > > 
> > > > > > Yeah, removing the BUG_ON() isn't the right thing to do - it means
> > > > > > that autofs4 is obviously setting the dentry ops twice for the same
> > > > > > dentry.
> > > > > > 
> > > > > > Possibly the thing could be relaxed to allow setting the _same_ d_op
> > > > > > pointer, ie do something like
> > > > > > 
> > > > > >    if (dentry->d_op == op)
> > > > > >       return;
> > > > > > 
> > > > > > at the top of that function. But looking at it, I don't think that
> > > > > > fixes the autofs4 issue.
> > > > > 
> > > > > That's easy enough, but it seems everybody else ensures
> > > > > this gets done just once per dentry, and it would be nice
> > > > > to preserve that "tightness" if possible.
> > > > > 
> > > > > > The fact that autofs4 does "d_add()" before it sets the d_ops (or
> > > > > > other dentry state, for that matter) looks a bit scary. To me that
> > > > > > smells like it might get a  dentry lookup hit before it's actually
> > > > > > fully done.
> > > > > 
> > > > > Agreed.
> > > > 
> > > > Isn't the parent i_mutex held during mkdir()?
> > > > Still the order can be changed, of course.
> > > > 
> > > > > 
> > > > > > Does it make any difference if you move the various d_add() calls down
> > > > > > to the end of the functions to when the "dentry" has really been
> > > > > > instantiated?
> > > > > 
> > > > > Looking at it quickly, I don't think that would matter for
> > > > > the case at hand.  I.e., that might be safer but it doesn't
> > > > > address the fact that these fields are getting initialized
> > > > > multiple times.
> > > > 
> > > > Yeah, a hangover from changes done over time.
> > > > Not setting the dentry op in ->lookup() should fix this.
> > > 
> > > Could you try this patch please.
> > 
> > OK, sorry for the delay.  I tried the patch.  I applied
> > it against 4162cf64973df51fc885825bc9ca4d055891c49f,
> > which is the linus/master branch I had on hand.  This
> > time I got a different failure due to a null pointer
> > dereference.  Console capture below.  I can log
> > in still but the boot sequence never got to the
> > login prompt on the console as it normally does.
> 
> 
> Sorry, that is rather an obvious mistake on my part.
> There's a call to d_op->d_revalidate() just below where the dentry
> operations are set. I'm tempted to just call the revalidate function
> directly since it is always called anyway. But let me check we've done
> in the our current of tree autofs patch series first.

Ha, there is no revalidate in the current not yet merged autofs.
So I think we just need to work around it.

Try this and see if it resolves the issue.

autofs4 - set dentry op in ->lookup() only

From: Ian Kent <raven@themaw.net>

With the introduction of the vfs-scale patch series setting dentry
operations more than once (or changing them) triggers a BUG_ON().
Since the two dentry operations used are the same, just set the
operations in ->lookup() and remove the set in ->symlink() and
->mkdir().

Also, move the d_add() in ->symlink() and ->mkdir() to the end of
the function.
---

 fs/autofs4/root.c |   40 +++++++---------------------------------
 1 files changed, 7 insertions(+), 33 deletions(-)


diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index 651e4ef..0897706 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -419,12 +419,6 @@ void autofs4_dentry_release(struct dentry *de)
 	}
 }
 
-/* For dentries of directories in the root dir */
-static const struct dentry_operations autofs4_root_dentry_operations = {
-	.d_revalidate	= autofs4_revalidate,
-	.d_release	= autofs4_dentry_release,
-};
-
 /* For other dentries */
 static const struct dentry_operations autofs4_dentry_operations = {
 	.d_revalidate	= autofs4_revalidate,
@@ -568,19 +562,6 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
 		ino = autofs4_dentry_ino(dentry);
 	} else {
 		/*
-		 * Mark the dentry incomplete but don't hash it. We do this
-		 * to serialize our inode creation operations (symlink and
-		 * mkdir) which prevents deadlock during the callback to
-		 * the daemon. Subsequent user space lookups for the same
-		 * dentry are placed on the wait queue while the daemon
-		 * itself is allowed passage unresticted so the create
-		 * operation itself can then hash the dentry. Finally,
-		 * we check for the hashed dentry and return the newly
-		 * hashed dentry.
-		 */
-		d_set_d_op(dentry, &autofs4_root_dentry_operations);
-
-		/*
 		 * And we need to ensure that the same dentry is used for
 		 * all following lookup calls until it is hashed so that
 		 * the dentry flags are persistent throughout the request.
@@ -589,6 +570,8 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
 		if (!ino)
 			return ERR_PTR(-ENOMEM);
 
+		d_set_d_op(dentry, &autofs4_dentry_operations);
+
 		dentry->d_fsdata = ino;
 		ino->dentry = dentry;
 
@@ -714,12 +697,6 @@ static int autofs4_dir_symlink(struct inode *dir,
 			kfree(ino);
 		return -ENOMEM;
 	}
-	d_add(dentry, inode);
-
-	if (dir == dir->i_sb->s_root->d_inode)
-		d_set_d_op(dentry, &autofs4_root_dentry_operations);
-	else
-		d_set_d_op(dentry, &autofs4_dentry_operations);
 
 	dentry->d_fsdata = ino;
 	ino->dentry = dget(dentry);
@@ -732,6 +709,8 @@ static int autofs4_dir_symlink(struct inode *dir,
 	ino->u.symlink = cp;
 	dir->i_mtime = CURRENT_TIME;
 
+	d_add(dentry, inode);
+
 	return 0;
 }
 
@@ -849,12 +828,6 @@ static int autofs4_dir_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 			kfree(ino);
 		return -ENOMEM;
 	}
-	d_add(dentry, inode);
-
-	if (dir == dir->i_sb->s_root->d_inode)
-		d_set_d_op(dentry, &autofs4_root_dentry_operations);
-	else
-		d_set_d_op(dentry, &autofs4_dentry_operations);
 
 	dentry->d_fsdata = ino;
 	ino->dentry = dget(dentry);
@@ -866,6 +839,8 @@ static int autofs4_dir_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 	inc_nlink(dir);
 	dir->i_mtime = CURRENT_TIME;
 
+	d_add(dentry, inode);
+
 	return 0;
 }
 
@@ -944,8 +919,7 @@ static inline int autofs4_ask_umount(struct vfsmount *mnt, int __user *p)
 int is_autofs4_dentry(struct dentry *dentry)
 {
 	return dentry && dentry->d_inode &&
-		(dentry->d_op == &autofs4_root_dentry_operations ||
-		 dentry->d_op == &autofs4_dentry_operations) &&
+		(dentry->d_op == &autofs4_dentry_operations) &&
 		dentry->d_fsdata != NULL;
 }