Re: [PATCH 5/5] exportfs: fix quadratic behavior in filehandle lookup

["J. Bruce Fields" <bfields@redhat.com>]

On Wed, Oct 16, 2013 at 01:04:07AM -0700, Christoph Hellwig wrote:
> On Tue, Oct 15, 2013 at 04:39:33PM -0400, J. Bruce Fields wrote:
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > We shouldn't really have to re-lookup the parents on each pass through.
> > This should make behavior linear instead of quadratic as a function of
> > directory depth.
> 
> This looks valid after going through the nasty cases like a
> cross-directory rename.  But I'd really prefer the changelog to be way
> more detailed.
> 
> Also a few untested fixups below:
> 	- document that reconnect_one can only be called for directories
> 	- make sure we always properly drop the parent dentry on
> 	  reconnect_one failure
> 	- warn then the tmp dentry is not the same as the one we started
> 	  with.  Given that we deal with directories this should not
> 	  happen
> 	- add/update a few comments.
> 	- tidy up the reconnect_path loop so the reconnect case shares
> 	  more logic with the dget_parent case
> 
> 
> Index: linux/fs/exportfs/expfs.c
> ===================================================================
> --- linux.orig/fs/exportfs/expfs.c	2013-10-16 09:58:44.544119517 +0200
> +++ linux/fs/exportfs/expfs.c	2013-10-16 10:04:19.176127470 +0200
> @@ -104,38 +104,30 @@ static void clear_disconnected(struct de
>  }
>  
>  /*
> - * Return the parent directory on success.
> + * Reconnect a directory dentry with its parent.
>   *
> - * If it races with a rename or unlink, assume the other operation
> - * connected it, but return NULL to indicate the caller should check
> - * this just to make sure the filesystem isn't nuts.
> + * Return the parent directory when successfully reconnecting, NULL if it
> + * raced with another VFS operation, in which case it got connected or
> + * the whole file handle will become stale.
>   *
>   * Otherwise return an error.
>   */
> -static struct dentry *reconnect_one(struct vfsmount *mnt, struct dentry *dentry, char *nbuf)
> +static struct dentry *reconnect_one(struct vfsmount *mnt,
> +		struct dentry *dentry, char *nbuf)
>  {
>  	struct dentry *parent;
>  	struct dentry *tmp;
>  	int err;
> -	/*
> -	 * Getting the parent can't be supported generically, the
> -	 * locking is too icky.
> -	 *
> -	 * If it can't be done, we just return EACCES.  If you were
> -	 * depending on the dcache finding the parent for you, you lose
> -	 * if there's a reboot or inodes get flushed.
> -	 */
> -	parent = ERR_PTR(-EACCES);
>  
> +	parent = ERR_PTR(-EACCES);
>  	mutex_lock(&dentry->d_inode->i_mutex);
>  	if (mnt->mnt_sb->s_export_op->get_parent)
>  		parent = mnt->mnt_sb->s_export_op->get_parent(dentry);
>  	mutex_unlock(&dentry->d_inode->i_mutex);
>  
>  	if (IS_ERR(parent)) {
> -		err = PTR_ERR(parent);
>  		dprintk("%s: get_parent of %ld failed, err %d\n",
> -			__func__, dentry->d_inode->i_ino, err);
> +			__func__, dentry->d_inode->i_ino, PTR_ERR(parent));
>  		return parent;
>  	}
>  
> @@ -143,26 +135,45 @@ static struct dentry *reconnect_one(stru
>  		dentry->d_inode->i_ino, parent->d_inode->i_ino);
>  	err = exportfs_get_name(mnt, parent, nbuf, dentry);
>  	if (err) {
> +		/*
> +		 * Someone must have renamed our entry into another parent,
> +		 * in which case it has been reconnected by the rename.
> +		 *
> +		 * Or someone removed it entirely, in which case the file
> +		 * handle has become stale.
> +		 */
>  		if (err == -ENOENT)
> -			 return NULL;
> -		dput(parent);
> -		return ERR_PTR(err);
> +			goto out_reconnected;
> +		goto out_err;
>  	}
>  	dprintk("%s: found name: %s\n", __func__, nbuf);
>  	mutex_lock(&parent->d_inode->i_mutex);
>  	tmp = lookup_one_len(nbuf, parent, strlen(nbuf));
>  	mutex_unlock(&parent->d_inode->i_mutex);
>  	if (IS_ERR(tmp)) {
> -		err = PTR_ERR(tmp);
> -		dprintk("%s: lookup failed: %d\n",
> -			__func__, err);
> -		return NULL;
> +		dprintk("%s: lookup failed: %d\n", __func__, PTR_ERR(tmp));
> +		goto out_reconnected;
> +	}
> +	if (WARN_ON(tmp != dentry)) {
> +		dprintk("%s: got different dentry while reconnecting.\n",
> +			__func__);
> +		dput(tmp);
> +		err = -ESTALE;
> +		goto out_err;

Wait, no, this isn't right.  This isn't a multiple-directory-alias case.

It's just another "out_reconnected" case.  Somebody renamed or removed
between get_name and lookup_one_len and lookup_one_len() and now
whatever lookup_one_len() finds could be something totally different.

--b.

>  	}
> -	/* Note tmp != dentry is possible at this point, but that's OK */
>  	dput(tmp);
> -	if (IS_ROOT(dentry))
> -		return ERR_PTR(-ESTALE);
> +	if (IS_ROOT(dentry)) {
> +		err = -ESTALE;
> +		goto out_err;
> +	}
>  	return parent;
> +
> +out_err:
> +	dput(parent);
> +	return ERR_PTR(err);
> +out_reconnected:
> +	dput(parent);
> +	return NULL;
>  }
>  
>  /*
> @@ -178,21 +189,18 @@ reconnect_path(struct vfsmount *mnt, str
>  	dentry = dget(target_dir);
>  
>  	while (dentry->d_flags & DCACHE_DISCONNECTED) {
> -		if (dentry == mnt->mnt_sb->s_root) {
> -			WARN_ON_ONCE(1);
> -			break;
> -		}
> -		if (!IS_ROOT(dentry)) {
> +		BUG_ON(dentry == mnt->mnt_sb->s_root);
> +
> +		if (IS_ROOT(dentry)) {
> +			/*
> +			 * We have hit the top of a disconnected path, try to
> +			 * find parent and connect.
> +			 */
> +			parent = reconnect_one(mnt, dentry, nbuf);
> +		} else {
>  			parent = dget_parent(dentry);
> -			dput(dentry);
> -			dentry = parent;
> -			continue;
>  		}
> -		/*
> -		 * We have hit the top of a disconnected path, try to
> -		 * find parent and connect.
> -		 */
> -		parent = reconnect_one(mnt, dentry, nbuf);
> +
>  		dput(dentry);
>  		if (!parent)
>  			break;
> @@ -200,6 +208,7 @@ reconnect_path(struct vfsmount *mnt, str
>  			return PTR_ERR(parent);
>  		dentry = parent;
>  	}
> +
>  	/*
>  	 * Should be unnecessary, but let's be careful:
>  	 */