[lustre-devel] [PATCH 14/24] lustre: llite: Always do lookup on ENOENT in open

[James Simmons]

From: Patrick Farrell <pfarrell at whamcloud.com>

When there is no valid dentry found for a file we want to
open, we perform a full lookup, which goes to the server
and looks up the file by name. When we find an existing
dentry in cache *but the file is not open on the node*, we
do not do a full lookup.  We move directly to opening the
file.

When we open files, we use the FID of the file.  The
problem occurs when a new file is renamed *over* the file
we were trying to open.  This removes the FID we are
trying to open, but the file *name* userspace called open()
on is still present.  In this case, we will return ENOENT,
even though there is a file matching the name used in the
open() call.

The solution is when we get an ENOENT on open (indicating
our open raced with an unlink), we always send ESTALE back
to the VFS, which restarts the open and forces a lookup to
the server (by forcing Lustre to consider the dentry
invalid, see comments in ll_intent_file_open and code in
ll_revalidate_dentry).

This causes a lookup by name, which will correctly handle
the rename, allowing the open to proceed normally.

This should only generate extra retries in the case where a
positive dentry exists on the client but the file has been
removed on the server, ie, open racing with unlink.

This should hopefully be rare.

WC-bug-id: https://jira.whamcloud.com/browse/LU-14949
lustre-commit: 72c1f7095203cc1ba ("LU-14949 llite: Always do lookup on ENOENT in open")
Signed-off-by: Patrick Farrell <pfarrell at whamcloud.com>
Signed-off-by: Oleg Drokin <green at whamcloud.com>
Reviewed-on: https://review.whamcloud.com/44675
Reviewed-by: Hongchao Zhang <hongchao at whamcloud.com>
Reviewed-by: Andreas Dilger <adilger at whamcloud.com>
Signed-off-by: James Simmons <jsimmons at infradead.org>
---
 fs/lustre/include/obd_support.h |  1 +
 fs/lustre/llite/file.c          | 23 +++++++++++++++--------
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/fs/lustre/include/obd_support.h b/fs/lustre/include/obd_support.h
index 1e8cebf..540e1e0 100644
--- a/fs/lustre/include/obd_support.h
+++ b/fs/lustre/include/obd_support.h
@@ -483,6 +483,7 @@
 #define OBD_FAIL_LLITE_CREATE_FILE_PAUSE2		0x1416
 #define OBD_FAIL_LLITE_RACE_MOUNT			0x1417
 #define OBD_FAIL_LLITE_PAGE_ALLOC			0x1418
+#define OBD_FAIL_LLITE_OPEN_DELAY			0x1419
 
 #define OBD_FAIL_FID_INDIR				0x1501
 #define OBD_FAIL_FID_INLMA				0x1502
diff --git a/fs/lustre/llite/file.c b/fs/lustre/llite/file.c
index aa5c662..10450ce 100644
--- a/fs/lustre/llite/file.c
+++ b/fs/lustre/llite/file.c
@@ -639,6 +639,8 @@ static int ll_intent_file_open(struct dentry *de, void *lmm, int lmmsize,
 	op_data->op_data = lmm;
 	op_data->op_data_size = lmmsize;
 
+	OBD_FAIL_TIMEOUT(OBD_FAIL_LLITE_OPEN_DELAY, cfs_fail_val);
+
 	rc = md_intent_lock(sbi->ll_md_exp, op_data, itp, &req,
 			    &ll_md_blocking_ast, 0);
 	kfree(name);
@@ -692,15 +694,20 @@ static int ll_intent_file_open(struct dentry *de, void *lmm, int lmmsize,
 	ptlrpc_req_finished(req);
 	ll_intent_drop_lock(itp);
 
-	/*
-	 * We did open by fid, but by the time we got to the server,
-	 * the object disappeared. If this is a create, we cannot really
-	 * tell the userspace that the file it was trying to create
-	 * does not exist. Instead let's return -ESTALE, and the VFS will
-	 * retry the create with LOOKUP_REVAL that we are going to catch
-	 * in ll_revalidate_dentry() and use lookup then.
+	/* We did open by fid, but by the time we got to the server, the object
+	 * disappeared.  This is possible if the object was unlinked, but it's
+	 * also possible if the object was unlinked by a rename.  In the case
+	 * of an object renamed over our existing one, we can't fail this open.
+	 * O_CREAT also goes through this path if we had an existing dentry,
+	 * and it's obviously wrong to return ENOENT for O_CREAT.
+	 *
+	 * Instead let's return -ESTALE, and the VFS will retry the open with
+	 * LOOKUP_REVAL, which we catch in ll_revalidate_dentry and fail to
+	 * revalidate, causing a lookup.  This causes extra lookups in the case
+	 * where we had a dentry in cache but the file is being unlinked and we
+	 * lose the race with unlink, but this should be very rare.
 	 */
-	if (rc == -ENOENT && itp->it_op & IT_CREAT)
+	if (rc == -ENOENT)
 		rc = -ESTALE;
 
 	return rc;
-- 
1.8.3.1