On 4/25/22 1:40 PM, J. Bruce Fields wrote:
On Mon, Apr 25, 2022 at 12:42:58PM -0700, dai.ngo@xxxxxxxxxx wrote:
On 4/25/22 11:51 AM, J. Bruce Fields wrote:
On Sat, Apr 23, 2022 at 11:44:09AM -0700, Dai Ngo wrote:
This patch provides courteous server support for delegation only.
Only expired client with delegation but no conflict and no open
or lock state is allowed to be in COURTESY state.
Delegation conflict with COURTESY client is resolved by setting it
to EXPIRABLE, queue work for the laundromat and return delay to
caller. Conflict is resolved when the laudromat runs and expires
the EXIRABLE client while the NFS client retries the OPEN request.
Local thread request that gets conflict is doing the retry in
__break_lease.
Client in COURTESY state is allowed to reconnect and continues to
have access to its state while client in EXPIRABLE state is not.
To prevent 2 clients with the same name are on the conf_name_tree,
nfsd4_setclientid is modified to expire confirmed client in EXPIRABLE
state.
The spinlock cl_cs_lock is used to handle race conditions between
conflict resolver, nfsd_break_deleg_cb, and the COURTESY client
doing reconnect.
find_in_sessionid_hashtbl, find_confirmed_client_by_name and
find_confirmed_client are modified to activate COURTESY client by
setting it to ACTIVE state and skip client in EXPIRABLE state.
Signed-off-by: Dai Ngo <dai.ngo@xxxxxxxxxx>
---
fs/nfsd/nfs4state.c | 129 +++++++++++++++++++++++++++++++++++++++++++++-------
fs/nfsd/nfsd.h | 1 +
fs/nfsd/state.h | 63 +++++++++++++++++++++++++
3 files changed, 177 insertions(+), 16 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 234e852fcdfa..fea5e24e7d94 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -125,6 +125,8 @@ static void free_session(struct nfsd4_session *);
static const struct nfsd4_callback_ops nfsd4_cb_recall_ops;
static const struct nfsd4_callback_ops nfsd4_cb_notify_lock_ops;
+static struct workqueue_struct *laundry_wq;
+
static bool is_session_dead(struct nfsd4_session *ses)
{
return ses->se_flags & NFS4_SESSION_DEAD;
@@ -690,6 +692,14 @@ static unsigned int file_hashval(struct svc_fh *fh)
static struct hlist_head file_hashtbl[FILE_HASH_SIZE];
+static inline void
+run_laundromat(struct nfsd_net *nn, bool wait)
+{
+ mod_delayed_work(laundry_wq, &nn->laundromat_work, 0);
+ if (wait)
+ flush_workqueue(laundry_wq);
+}
Let's keep those two things separate. The "wait" argument isn't
self-documenting when reading the calling code.
we can use enum to spell out the intention.
And the
mod_delayed_work call isn't always needed before flush_workqueue.
ok. I'll keep them separate.
+
static void
__nfs4_file_get_access(struct nfs4_file *fp, u32 access)
{
@@ -1939,6 +1949,11 @@ find_in_sessionid_hashtbl(struct nfs4_sessionid *sessionid, struct net *net,
session = __find_in_sessionid_hashtbl(sessionid, net);
if (!session)
goto out;
+ if (!try_to_activate_client(session->se_client)) {
+ /* client is EXPIRABLE */
+ session = NULL;
+ goto out;
No, we definitely don't want to do this. As I said before, an
"expirable client" should be treated in every way exactly like any
regular active client. Literally the only difference is that the
laundromat can try to expire it.
Do you mean leave the state as EXPIRABLE but allow the callers
to use the client as an ACTIVE client:
The only place we should even be checking whether a client is EXPIRABLE
is in the laundromat thread, in the loop over the client_lru. Even
there, EXPIRABLE being set doesn't mean the client *has* to be expired,
the real decision is left to mark_client_expired_locked(), as it
currently is.
static inline bool try_to_expire_client(struct nfs4_client *clp)
{
bool ret;
spin_lock(&clp->cl_cs_lock);
if (clp->cl_state == NFSD4_EXPIRABLE) {
spin_unlock(&clp->cl_cs_lock);
return false; <<<====== was true
}
ret = NFSD4_COURTESY ==
cmpxchg(&clp->cl_state, NFSD4_COURTESY, NFSD4_EXPIRABLE);
spin_unlock(&clp->cl_cs_lock);
return ret;
}
So, try_to_expire_client(), as I said, should be just
static bool try_to_expire_client(struct nfs4_client *clp)
{
return COURTESY == cmpxchg(clp->cl_state, COURTESY, EXPIRABLE);
}
Except, OK, I did forget that somebody else could have already set
EXPIRABLE, and we still want to tell the caller to retry in that case,
so, oops, let's make it:
return ACTIVE != cmpxchg(clp->cl_state, COURTESY, EXPIRABLE);
So functionally this is the same as what i have in the patch, except this
makes it simpler?
Do we need to make any change in try_to_activate_client to work with
this change in try_to_expire_client?
In other words: set EXPIRABLE if and only if the state is COURTESY, and
then tell the caller to retry the operation if and only if the state was
previously COURTESY or EXPIRABLE.
But we shouldn't need the cl_cs_lock,
The cl_cs_lock is to synchronize between COURTESY client trying to
reconnect (try_to_activate_client) and another thread is trying to
resolve a delegation/share/lock conflict (try_to_expire_client).
So you don't think this is necessary?
-Dai
and certainly shouldn't be failing
lookups of EXPIRABLE clients.
--b.
-Dai
And then all this code becomes unnecessary:
@@ -702,4 +727,42 @@ extern void nfsd4_client_record_remove(struct nfs4_client *clp);
extern int nfsd4_client_record_check(struct nfs4_client *clp);
extern void nfsd4_record_grace_done(struct nfsd_net *nn);
+static inline bool try_to_expire_client(struct nfs4_client *clp)
+{
+ bool ret;
+
+ spin_lock(&clp->cl_cs_lock);
+ if (clp->cl_state == NFSD4_EXPIRABLE) {
+ spin_unlock(&clp->cl_cs_lock);
+ return true;
+ }
+ ret = NFSD4_COURTESY ==
+ cmpxchg(&clp->cl_state, NFSD4_COURTESY, NFSD4_EXPIRABLE);
+ spin_unlock(&clp->cl_cs_lock);
+ return ret;
+}
+
+static inline bool try_to_activate_client(struct nfs4_client *clp)
+{
+ bool ret;
+ struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
+
+ /* sync with laundromat */
+ lockdep_assert_held(&nn->client_lock);
+
+ /* sync with try_to_expire_client */
+ spin_lock(&clp->cl_cs_lock);
+ if (clp->cl_state == NFSD4_ACTIVE) {
+ spin_unlock(&clp->cl_cs_lock);
+ return true;
+ }
+ if (clp->cl_state == NFSD4_EXPIRABLE) {
+ spin_unlock(&clp->cl_cs_lock);
+ return false;
+ }
+ ret = NFSD4_COURTESY ==
+ cmpxchg(&clp->cl_state, NFSD4_COURTESY, NFSD4_ACTIVE);
+ spin_unlock(&clp->cl_cs_lock);
+ return ret;
+}
--b.
