On Tue, Oct 15, 2013 at 04:39:33PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@xxxxxxxxxx>
>
> 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;
}
- /* 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:
*/
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
