The first five are some bugfix and update,
the last nine are some cleanups.
Kinglong Mee (14):
nfsd: Add layouts checking for state resources
nfsd: Add missing gen_confirm nfsd4_setclientid()
nfsd: Fix memory leak of so_owner.data in nfs4_stateowner
nfsd: Fix a memory leak of struct file_lock
nfsd: Use check_stateid_generation() for generation checking
nfsd: Drop duplicate locks_init_lock()
nfsd: Remove unneeded values in nfsd4_open()
nfsd: Drop duplicate checking of seqid in nfsd4_create_session()
nfsd: Remove nfs4_set_claim_prev()
nfsd: Remove unused values in nfs4_setlease()
nfsd: Remove duplicate checking of nfsd_net in nfs4_laundromat()
nfsd: Remove macro LOFF_OVERFLOW
nfsd: Use lk_new_xxx instead of v.new.xxx for nfs4_lockowner
nfsd: Remove unused clientid arguments from find_lockowner_str{_locked}
fs/nfsd/nfs4layouts.c | 7 +++--
fs/nfsd/nfs4proc.c | 4 +--
fs/nfsd/nfs4state.c | 74 ++++++++++++++++++++++-----------------------------
fs/nfsd/state.h | 1 +
4 files changed, 37 insertions(+), 49 deletions(-)
--
2.4.3
Layout is a state resource, nfsd should check it too.
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4state.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 61dfb33..e0a4556 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2241,6 +2241,7 @@ static bool client_has_state(struct nfs4_client *clp)
* Also note we should probably be using this in 4.0 case too.
*/
return !list_empty(&clp->cl_openowners)
+ || !list_empty(&clp->cl_lo_states)
|| !list_empty(&clp->cl_delegations)
|| !list_empty(&clp->cl_sessions);
}
@@ -4257,8 +4258,9 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
goto out;
clp = cstate->clp;
status = nfserr_cb_path_down;
- if (!list_empty(&clp->cl_delegations)
- && clp->cl_cb_state != NFSD4_CB_UP)
+ if ((!list_empty(&clp->cl_delegations)
+ || !list_empty(&clp->cl_lo_states))
+ && clp->cl_cb_state != NFSD4_CB_UP)
goto out;
status = nfs_ok;
out:
--
2.4.3
Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock"
have moved gen_confirm() to gen_clid().
After it, setclientid will return a bad reply with all zero confirms
after copy_clid().
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4state.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e0a4556..b1f84fc 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
unconf = find_unconfirmed_client_by_name(&clname, nn);
if (unconf)
unhash_client_locked(unconf);
- if (conf && same_verf(&conf->cl_verifier, &clverifier))
+ if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
/* case 1: probable callback update */
copy_clid(new, conf);
- else /* case 4 (new client) or cases 2, 3 (client reboot): */
+ gen_confirm(new, nn);
+ } else /* case 4 (new client) or cases 2, 3 (client reboot): */
gen_clid(new, nn);
new->cl_minorversion = 0;
gen_callback(new, setclid, rqstp);
--
2.4.3
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4state.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b1f84fc..e5e14fa 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3316,8 +3316,10 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
if (ret == NULL) {
hash_openowner(oo, clp, strhashval);
ret = oo;
- } else
+ } else {
+ kfree(oo->oo_owner.so_owner.data);
nfs4_free_openowner(&oo->oo_owner);
+ }
spin_unlock(&clp->cl_lock);
return ret;
}
@@ -5217,8 +5219,10 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
list_add(&lo->lo_owner.so_strhash,
&clp->cl_ownerstr_hashtbl[strhashval]);
ret = lo;
- } else
- nfs4_free_lockowner(&lo->lo_owner);
+ } else {
+ kfree(lo->lo_owner.so_owner.data);
+ nfs4_free_openowner(&lo->lo_owner);
+ }
spin_unlock(&clp->cl_lock);
return ret;
}
--
2.4.3
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4state.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e5e14fa..998166d 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3938,6 +3938,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
if (!filp) {
/* We should always have a readable file here */
WARN_ON_ONCE(1);
+ locks_free_lock(fl);
return -EBADF;
}
fl->fl_file = filp;
--
2.4.3
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4layouts.c | 5 ++---
fs/nfsd/nfs4state.c | 2 +-
fs/nfsd/state.h | 1 +
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
index 6904213..5c0f1d6 100644
--- a/fs/nfsd/nfs4layouts.c
+++ b/fs/nfsd/nfs4layouts.c
@@ -263,9 +263,8 @@ nfsd4_preprocess_layout_stateid(struct svc_rqst *rqstp,
goto out;
} else {
ls = container_of(stid, struct nfs4_layout_stateid, ls_stid);
-
- status = nfserr_bad_stateid;
- if (stateid->si_generation > stid->sc_stateid.si_generation)
+ status = check_stateid_generation(stateid, &stid->sc_stateid, 1);
+ if (status)
goto out_put_stid;
if (layout_type != ls->ls_layout_type)
goto out_put_stid;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 998166d..2edfedc 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4474,7 +4474,7 @@ static bool stateid_generation_after(stateid_t *a, stateid_t *b)
return (s32)(a->si_generation - b->si_generation) > 0;
}
-static __be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)
+__be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)
{
/*
* When sessions are used the stateid generation number is ignored
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 4874ce5..18d015d 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -589,6 +589,7 @@ extern __be32 nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
__be32 nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
stateid_t *stateid, unsigned char typemask,
struct nfs4_stid **s, struct nfsd_net *nn);
+__be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session);
struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl,
struct kmem_cache *slab);
void nfs4_unhash_stid(struct nfs4_stid *s);
--
2.4.3
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4layouts.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
index 5c0f1d6..c5af4ccb 100644
--- a/fs/nfsd/nfs4layouts.c
+++ b/fs/nfsd/nfs4layouts.c
@@ -162,7 +162,7 @@ nfsd4_layout_setlease(struct nfs4_layout_stateid *ls)
fl = locks_alloc_lock();
if (!fl)
return -ENOMEM;
- locks_init_lock(fl);
+
fl->fl_lmops = &nfsd4_layouts_lm_ops;
fl->fl_flags = FL_LAYOUT;
fl->fl_type = F_RDLCK;
--
2.4.3
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4proc.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 90cfda7..5d99111 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -362,7 +362,6 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
{
__be32 status;
struct svc_fh *resfh = NULL;
- struct nfsd4_compoundres *resp;
struct net *net = SVC_NET(rqstp);
struct nfsd_net *nn = net_generic(net, nfsd_net_id);
@@ -389,8 +388,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
copy_clientid(&open->op_clientid, cstate->session);
/* check seqid for replay. set nfs4_owner */
- resp = rqstp->rq_resp;
- status = nfsd4_process_open1(&resp->cstate, open, nn);
+ status = nfsd4_process_open1(cstate, open, nn);
if (status == nfserr_replay_me) {
struct nfs4_replay *rp = &open->op_openowner->oo_owner.so_replay;
fh_put(&cstate->current_fh);
--
2.4.3
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4state.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2edfedc..c94d1ef 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2548,11 +2548,9 @@ nfsd4_create_session(struct svc_rqst *rqstp,
goto out_free_conn;
cs_slot = &conf->cl_cs_slot;
status = check_slot_seqid(cr_ses->seqid, cs_slot->sl_seqid, 0);
- if (status == nfserr_replay_cache) {
- status = nfsd4_replay_create_session(cr_ses, cs_slot);
- goto out_free_conn;
- } else if (cr_ses->seqid != cs_slot->sl_seqid + 1) {
- status = nfserr_seq_misordered;
+ if (status) {
+ if (status == nfserr_replay_cache)
+ status = nfsd4_replay_create_session(cr_ses, cs_slot);
goto out_free_conn;
}
} else if (unconf) {
--
2.4.3
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4state.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c94d1ef..f3c38e3 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3887,12 +3887,6 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp, struct svc_fh *c
return status;
}
-static void
-nfs4_set_claim_prev(struct nfsd4_open *open, bool has_session)
-{
- open->op_openowner->oo_flags |= NFS4_OO_CONFIRMED;
-}
-
/* Should we give out recallable state?: */
static bool nfsd4_cb_channel_good(struct nfs4_client *clp)
{
@@ -4212,7 +4206,7 @@ out:
if (fp)
put_nfs4_file(fp);
if (status == 0 && open->op_claim_type == NFS4_OPEN_CLAIM_PREVIOUS)
- nfs4_set_claim_prev(open, nfsd4_has_session(&resp->cstate));
+ open->op_openowner->oo_flags |= NFS4_OO_CONFIRMED;
/*
* To finish the open response, we just need to set the rflags.
*/
--
2.4.3
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4state.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 8205542..d342769 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4335,8 +4335,6 @@ nfs4_laundromat(struct nfsd_net *nn)
spin_lock(&state_lock);
list_for_each_safe(pos, next, &nn->del_recall_lru) {
dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
- if (net_generic(dp->dl_stid.sc_client->net, nfsd_net_id) != nn)
- continue;
if (time_after((unsigned long)dp->dl_time, (unsigned long)cutoff)) {
t = dp->dl_time - cutoff;
new_timeo = min(new_timeo, t);
--
2.4.3
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4state.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d342769..13df582 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5040,9 +5040,6 @@ out:
return status;
}
-
-#define LOFF_OVERFLOW(start, len) ((u64)(len) > ~(u64)(start))
-
static inline u64
end_offset(u64 start, u64 len)
{
@@ -5295,8 +5292,8 @@ find_or_create_lock_stateid(struct nfs4_lockowner *lo, struct nfs4_file *fi,
static int
check_lock_length(u64 offset, u64 length)
{
- return ((length == 0) || ((length != NFS4_MAX_UINT64) &&
- LOFF_OVERFLOW(offset, length)));
+ return ((length == 0) || ((length != NFS4_MAX_UINT64) &&
+ (length > ~offset)));
}
static void get_lock_access(struct nfs4_ol_stateid *lock_stp, u32 access)
--
2.4.3
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4state.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f3c38e3..8205542 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3919,7 +3919,7 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_file *fp, int flag)
static int nfs4_setlease(struct nfs4_delegation *dp)
{
struct nfs4_file *fp = dp->dl_stid.sc_file;
- struct file_lock *fl, *ret;
+ struct file_lock *fl;
struct file *filp;
int status = 0;
@@ -3934,7 +3934,6 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
return -EBADF;
}
fl->fl_file = filp;
- ret = fl;
status = vfs_setlease(filp, fl->fl_type, &fl, NULL);
if (fl)
locks_free_lock(fl);
--
2.4.3
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4state.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 13df582..2704270 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5322,9 +5322,9 @@ lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
struct nfs4_lockowner *lo;
unsigned int strhashval;
- lo = find_lockowner_str(&cl->cl_clientid, &lock->v.new.owner, cl);
+ lo = find_lockowner_str(&cl->cl_clientid, &lock->lk_new_owner, cl);
if (!lo) {
- strhashval = ownerstr_hashval(&lock->v.new.owner);
+ strhashval = ownerstr_hashval(&lock->lk_new_owner);
lo = alloc_init_lock_stateowner(strhashval, cl, ost, lock);
if (lo == NULL)
return nfserr_jukebox;
@@ -5385,7 +5385,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
if (lock->lk_is_new) {
if (nfsd4_has_session(cstate))
/* See rfc 5661 18.10.3: given clientid is ignored: */
- memcpy(&lock->v.new.clientid,
+ memcpy(&lock->lk_new_clientid,
&cstate->session->se_client->cl_clientid,
sizeof(clientid_t));
@@ -5403,7 +5403,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
open_sop = openowner(open_stp->st_stateowner);
status = nfserr_bad_stateid;
if (!same_clid(&open_sop->oo_owner.so_client->cl_clientid,
- &lock->v.new.clientid))
+ &lock->lk_new_clientid))
goto out;
status = lookup_or_create_lock_state(cstate, open_stp, lock,
&lock_stp, &new);
--
2.4.3
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4state.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2704270..b6c134d 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5131,8 +5131,7 @@ nevermind:
}
static struct nfs4_lockowner *
-find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
- struct nfs4_client *clp)
+find_lockowner_str_locked(struct nfs4_client *clp, struct xdr_netobj *owner)
{
unsigned int strhashval = ownerstr_hashval(owner);
struct nfs4_stateowner *so;
@@ -5150,13 +5149,12 @@ find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
}
static struct nfs4_lockowner *
-find_lockowner_str(clientid_t *clid, struct xdr_netobj *owner,
- struct nfs4_client *clp)
+find_lockowner_str(struct nfs4_client *clp, struct xdr_netobj *owner)
{
struct nfs4_lockowner *lo;
spin_lock(&clp->cl_lock);
- lo = find_lockowner_str_locked(clid, owner, clp);
+ lo = find_lockowner_str_locked(clp, owner);
spin_unlock(&clp->cl_lock);
return lo;
}
@@ -5200,8 +5198,7 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
lo->lo_owner.so_seqid = lock->lk_new_lock_seqid;
lo->lo_owner.so_ops = &lockowner_ops;
spin_lock(&clp->cl_lock);
- ret = find_lockowner_str_locked(&clp->cl_clientid,
- &lock->lk_new_owner, clp);
+ ret = find_lockowner_str_locked(clp, &lock->lk_new_owner);
if (ret == NULL) {
list_add(&lo->lo_owner.so_strhash,
&clp->cl_ownerstr_hashtbl[strhashval]);
@@ -5322,7 +5319,7 @@ lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
struct nfs4_lockowner *lo;
unsigned int strhashval;
- lo = find_lockowner_str(&cl->cl_clientid, &lock->lk_new_owner, cl);
+ lo = find_lockowner_str(cl, &lock->lk_new_owner);
if (!lo) {
strhashval = ownerstr_hashval(&lock->lk_new_owner);
lo = alloc_init_lock_stateowner(strhashval, cl, ost, lock);
@@ -5597,8 +5594,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
goto out;
}
- lo = find_lockowner_str(&lockt->lt_clientid, &lockt->lt_owner,
- cstate->clp);
+ lo = find_lockowner_str(cstate->clp, &lockt->lt_owner);
if (lo)
file_lock->fl_owner = (fl_owner_t)lo;
file_lock->fl_pid = current->tgid;
--
2.4.3
On Mon, Jul 13, 2015 at 05:29:01PM +0800, Kinglong Mee wrote:
> Layout is a state resource, nfsd should check it too.
>
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 61dfb33..e0a4556 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2241,6 +2241,7 @@ static bool client_has_state(struct nfs4_client *clp)
> * Also note we should probably be using this in 4.0 case too.
> */
> return !list_empty(&clp->cl_openowners)
> + || !list_empty(&clp->cl_lo_states)
> || !list_empty(&clp->cl_delegations)
> || !list_empty(&clp->cl_sessions);
> }
> @@ -4257,8 +4258,9 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> goto out;
> clp = cstate->clp;
> status = nfserr_cb_path_down;
> - if (!list_empty(&clp->cl_delegations)
> - && clp->cl_cb_state != NFSD4_CB_UP)
> + if ((!list_empty(&clp->cl_delegations)
> + || !list_empty(&clp->cl_lo_states))
nfsd4_renew is NFSv4.0 only, so I don't think this part is necessary.
That said, it's harmless--why don't we just call client_has_state() here
instead?
--b.
> + && clp->cl_cb_state != NFSD4_CB_UP)
> goto out;
> status = nfs_ok;
> out:
> --
> 2.4.3
On Mon, Jul 13, 2015 at 05:29:41PM +0800, Kinglong Mee wrote:
> Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock"
> have moved gen_confirm() to gen_clid().
This means the statement in that earlier commit is wrong:
With this, there's no need to keep two counters as they'd always
be in sync anyway, so just use the clientid_counter for both.
Looks to me like this may need a separate counter to eliminate the
possibibility of returning the same confirm twice for a one clientid?
--b.
>
> After it, setclientid will return a bad reply with all zero confirms
> after copy_clid().
>
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index e0a4556..b1f84fc 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> unconf = find_unconfirmed_client_by_name(&clname, nn);
> if (unconf)
> unhash_client_locked(unconf);
> - if (conf && same_verf(&conf->cl_verifier, &clverifier))
> + if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
> /* case 1: probable callback update */
> copy_clid(new, conf);
> - else /* case 4 (new client) or cases 2, 3 (client reboot): */
> + gen_confirm(new, nn);
> + } else /* case 4 (new client) or cases 2, 3 (client reboot): */
> gen_clid(new, nn);
> new->cl_minorversion = 0;
> gen_callback(new, setclid, rqstp);
> --
> 2.4.3
On Wed, Jul 15, 2015 at 04:47:48PM -0400, J. Bruce Fields wrote:
> On Mon, Jul 13, 2015 at 05:29:41PM +0800, Kinglong Mee wrote:
> > Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock"
> > have moved gen_confirm() to gen_clid().
>
> This means the statement in that earlier commit is wrong:
>
>
> With this, there's no need to keep two counters as they'd always
> be in sync anyway, so just use the clientid_counter for both.
>
> Looks to me like this may need a separate counter to eliminate the
> possibibility of returning the same confirm twice for a one clientid?
(but frankly I can never completely review changes to the
setclientid/setclientid_confirm behavior without rereading RFC 7530
16.33.5 every time, which is a slog. Might help to contrive a pynfs
test derived from that text which tests for this particular behavior.)
>
> --b.
>
> >
> > After it, setclientid will return a bad reply with all zero confirms
> > after copy_clid().
> >
> > Signed-off-by: Kinglong Mee <[email protected]>
> > ---
> > fs/nfsd/nfs4state.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index e0a4556..b1f84fc 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > unconf = find_unconfirmed_client_by_name(&clname, nn);
> > if (unconf)
> > unhash_client_locked(unconf);
> > - if (conf && same_verf(&conf->cl_verifier, &clverifier))
> > + if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
> > /* case 1: probable callback update */
> > copy_clid(new, conf);
> > - else /* case 4 (new client) or cases 2, 3 (client reboot): */
> > + gen_confirm(new, nn);
> > + } else /* case 4 (new client) or cases 2, 3 (client reboot): */
> > gen_clid(new, nn);
> > new->cl_minorversion = 0;
> > gen_callback(new, setclid, rqstp);
> > --
> > 2.4.3
Good catch, but could we make a common nfs4_free_stateowner() helper
called both from here and nfs4_put_stateowner() so we only have to do
the kfree() in that one place?
--b.
On Mon, Jul 13, 2015 at 05:30:21PM +0800, Kinglong Mee wrote:
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index b1f84fc..e5e14fa 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3316,8 +3316,10 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
> if (ret == NULL) {
> hash_openowner(oo, clp, strhashval);
> ret = oo;
> - } else
> + } else {
> + kfree(oo->oo_owner.so_owner.data);
> nfs4_free_openowner(&oo->oo_owner);
> + }
> spin_unlock(&clp->cl_lock);
> return ret;
> }
> @@ -5217,8 +5219,10 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
> list_add(&lo->lo_owner.so_strhash,
> &clp->cl_ownerstr_hashtbl[strhashval]);
> ret = lo;
> - } else
> - nfs4_free_lockowner(&lo->lo_owner);
> + } else {
> + kfree(lo->lo_owner.so_owner.data);
> + nfs4_free_openowner(&lo->lo_owner);
> + }
> spin_unlock(&clp->cl_lock);
> return ret;
> }
> --
> 2.4.3
Thanks, applying.--b.
On Mon, Jul 13, 2015 at 05:30:51PM +0800, Kinglong Mee wrote:
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index e5e14fa..998166d 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3938,6 +3938,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
> if (!filp) {
> /* We should always have a readable file here */
> WARN_ON_ONCE(1);
> + locks_free_lock(fl);
> return -EBADF;
> }
> fl->fl_file = filp;
> --
> 2.4.3
On 7/15/2015 23:03, J. Bruce Fields wrote:
> On Mon, Jul 13, 2015 at 05:29:01PM +0800, Kinglong Mee wrote:
>> Layout is a state resource, nfsd should check it too.
>>
>> Signed-off-by: Kinglong Mee <[email protected]>
>> ---
>> fs/nfsd/nfs4state.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 61dfb33..e0a4556 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -2241,6 +2241,7 @@ static bool client_has_state(struct nfs4_client *clp)
>> * Also note we should probably be using this in 4.0 case too.
>> */
>> return !list_empty(&clp->cl_openowners)
>> + || !list_empty(&clp->cl_lo_states)
>> || !list_empty(&clp->cl_delegations)
>> || !list_empty(&clp->cl_sessions);
>> }
>> @@ -4257,8 +4258,9 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> goto out;
>> clp = cstate->clp;
>> status = nfserr_cb_path_down;
>> - if (!list_empty(&clp->cl_delegations)
>> - && clp->cl_cb_state != NFSD4_CB_UP)
>> + if ((!list_empty(&clp->cl_delegations)
>> + || !list_empty(&clp->cl_lo_states))
>
> nfsd4_renew is NFSv4.0 only, so I don't think this part is necessary.
>
> That said, it's harmless--why don't we just call client_has_state() here
> instead?
I don't think so.
NFSv4.0's callback path is used for delegation recalling only,
without delegation, callback path is useless.
Also, describing in rfc3530,
When the client holds delegations, it needs to use RENEW to detect
when the server has determined that the callback path is down. When
the server has made such a determination, only the RENEW operation
will renew the lease on delegations. If the server determines the
callback path is down, it returns NFS4ERR_CB_PATH_DOWN. Even though
it returns NFS4ERR_CB_PATH_DOWN, the server MUST renew the lease on
the record locks and share reservations that the client has
established on the server.
I will drop the second modify in version 2.
thanks,
Kinglong Mee
Layout is a state resource, nfsd should check it too.
v2,
drop unneeded updating in nfsd4_renew()
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4state.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 61dfb33..00a0f8f 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2241,6 +2241,7 @@ static bool client_has_state(struct nfs4_client *clp)
* Also note we should probably be using this in 4.0 case too.
*/
return !list_empty(&clp->cl_openowners)
+ || !list_empty(&clp->cl_lo_states)
|| !list_empty(&clp->cl_delegations)
|| !list_empty(&clp->cl_sessions);
}
--
2.4.3
On 7/16/2015 04:49, J. Bruce Fields wrote:
> On Wed, Jul 15, 2015 at 04:47:48PM -0400, J. Bruce Fields wrote:
>> On Mon, Jul 13, 2015 at 05:29:41PM +0800, Kinglong Mee wrote:
>>> Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock"
>>> have moved gen_confirm() to gen_clid().
>>
>> This means the statement in that earlier commit is wrong:
>>
>>
>> With this, there's no need to keep two counters as they'd always
>> be in sync anyway, so just use the clientid_counter for both.
>>
>> Looks to me like this may need a separate counter to eliminate the
>> possibibility of returning the same confirm twice for a one clientid?
Yes, nfsd will generate same confirm for one clientid in one second.
verf[0] = (__force __be32)jiffies;
verf[1] = (__force __be32)nn->clientid_counter;
for case 1: probable callback update, the new unconf client needs
a different confirm.
Rereading rfc7530,
x be the value of the client.id subfield of the SETCLIENTID4args
structure.
v be the value of the client.verifier subfield of the
SETCLIENTID4args structure.
c be the value of the client ID field returned in the
SETCLIENTID4resok structure.
k represent the value combination of the callback and callback_ident
fields of the SETCLIENTID4args structure.
s be the setclientid_confirm value returned in the SETCLIENTID4resok
structure.
{ v, x, c, k, s } be a quintuple for a client record. A client
record is confirmed if there has been a SETCLIENTID_CONFIRM
operation to confirm it. Otherwise, it is unconfirmed. An
unconfirmed record is established by a SETCLIENTID call.
... /* case 1: probable callback update */ ...
o The server checks if it has recorded a confirmed record for { v,
x, c, l, s }, where l may or may not equal k. If so, and since
the id verifier v of the request matches that which is confirmed
and recorded, the server treats this as a probable callback
information update and records an unconfirmed { v, x, c, k, t }
and leaves the confirmed { v, x, c, l, s } in place, such that
t != s. It does not matter whether k equals l or not. Any
pre-existing unconfirmed { v, x, c, *, * } is removed.
The server returns { c, t }. It is indeed returning the old
clientid4 value c, because the client apparently only wants to
update callback value k to value l. It's possible this request is
one from the Byzantine router that has stale callback information,
but this is not a problem. The callback information update is
only confirmed if followed up by a SETCLIENTID_CONFIRM { c, t }.
The server awaits confirmation of k via SETCLIENTID_CONFIRM
{ c, t }.
The server does NOT remove client (lock/share/delegation) state
for x.
>
> (but frankly I can never completely review changes to the
> setclientid/setclientid_confirm behavior without rereading RFC 7530
> 16.33.5 every time, which is a slog. Might help to contrive a pynfs
> test derived from that text which tests for this particular behavior.)
>
Make sense.
I will make it later.
thanks,
Kinglong Mee
>>
>> --b.
>>
>>>
>>> After it, setclientid will return a bad reply with all zero confirms
>>> after copy_clid().
>>>
>>> Signed-off-by: Kinglong Mee <[email protected]>
>>> ---
>>> fs/nfsd/nfs4state.c | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index e0a4556..b1f84fc 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>> unconf = find_unconfirmed_client_by_name(&clname, nn);
>>> if (unconf)
>>> unhash_client_locked(unconf);
>>> - if (conf && same_verf(&conf->cl_verifier, &clverifier))
>>> + if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
>>> /* case 1: probable callback update */
>>> copy_clid(new, conf);
>>> - else /* case 4 (new client) or cases 2, 3 (client reboot): */
>>> + gen_confirm(new, nn);
>>> + } else /* case 4 (new client) or cases 2, 3 (client reboot): */
>>> gen_clid(new, nn);
>>> new->cl_minorversion = 0;
>>> gen_callback(new, setclid, rqstp);
>>> --
>>> 2.4.3
>
On 7/16/2015 11:36, Kinglong Mee wrote:
> On 7/16/2015 04:49, J. Bruce Fields wrote:
>> On Wed, Jul 15, 2015 at 04:47:48PM -0400, J. Bruce Fields wrote:
>>> On Mon, Jul 13, 2015 at 05:29:41PM +0800, Kinglong Mee wrote:
>>>> Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock"
>>>> have moved gen_confirm() to gen_clid().
>>>
>>> This means the statement in that earlier commit is wrong:
>>>
>>>
>>> With this, there's no need to keep two counters as they'd always
>>> be in sync anyway, so just use the clientid_counter for both.
>>>
>>> Looks to me like this may need a separate counter to eliminate the
>>> possibibility of returning the same confirm twice for a one clientid?
>
> Yes, nfsd will generate same confirm for one clientid in one second.
>
> verf[0] = (__force __be32)jiffies;
> verf[1] = (__force __be32)nn->clientid_counter;
>
> for case 1: probable callback update, the new unconf client needs
> a different confirm.
Ignore this patch, and just revert commit 294ac32e99
"nfsd: protect clid and verifier generation with client_lock"
is a better solve.
thanks,
Kinglong Mee
>
> Rereading rfc7530,
> x be the value of the client.id subfield of the SETCLIENTID4args
> structure.
>
> v be the value of the client.verifier subfield of the
> SETCLIENTID4args structure.
>
> c be the value of the client ID field returned in the
> SETCLIENTID4resok structure.
>
> k represent the value combination of the callback and callback_ident
> fields of the SETCLIENTID4args structure.
>
> s be the setclientid_confirm value returned in the SETCLIENTID4resok
> structure.
>
> { v, x, c, k, s } be a quintuple for a client record. A client
> record is confirmed if there has been a SETCLIENTID_CONFIRM
> operation to confirm it. Otherwise, it is unconfirmed. An
> unconfirmed record is established by a SETCLIENTID call.
>
> ... /* case 1: probable callback update */ ...
>
> o The server checks if it has recorded a confirmed record for { v,
> x, c, l, s }, where l may or may not equal k. If so, and since
> the id verifier v of the request matches that which is confirmed
> and recorded, the server treats this as a probable callback
> information update and records an unconfirmed { v, x, c, k, t }
> and leaves the confirmed { v, x, c, l, s } in place, such that
> t != s. It does not matter whether k equals l or not. Any
> pre-existing unconfirmed { v, x, c, *, * } is removed.
>
> The server returns { c, t }. It is indeed returning the old
> clientid4 value c, because the client apparently only wants to
> update callback value k to value l. It's possible this request is
> one from the Byzantine router that has stale callback information,
> but this is not a problem. The callback information update is
> only confirmed if followed up by a SETCLIENTID_CONFIRM { c, t }.
>
> The server awaits confirmation of k via SETCLIENTID_CONFIRM
> { c, t }.
>
> The server does NOT remove client (lock/share/delegation) state
> for x.
>
>>
>> (but frankly I can never completely review changes to the
>> setclientid/setclientid_confirm behavior without rereading RFC 7530
>> 16.33.5 every time, which is a slog. Might help to contrive a pynfs
>> test derived from that text which tests for this particular behavior.)
>>
>
> Make sense.
> I will make it later.
>
> thanks,
> Kinglong Mee
>
>
>>>
>>> --b.
>>>
>>>>
>>>> After it, setclientid will return a bad reply with all zero confirms
>>>> after copy_clid().
>>>>
>>>> Signed-off-by: Kinglong Mee <[email protected]>
>>>> ---
>>>> fs/nfsd/nfs4state.c | 5 +++--
>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>> index e0a4556..b1f84fc 100644
>>>> --- a/fs/nfsd/nfs4state.c
>>>> +++ b/fs/nfsd/nfs4state.c
>>>> @@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>> unconf = find_unconfirmed_client_by_name(&clname, nn);
>>>> if (unconf)
>>>> unhash_client_locked(unconf);
>>>> - if (conf && same_verf(&conf->cl_verifier, &clverifier))
>>>> + if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
>>>> /* case 1: probable callback update */
>>>> copy_clid(new, conf);
>>>> - else /* case 4 (new client) or cases 2, 3 (client reboot): */
>>>> + gen_confirm(new, nn);
>>>> + } else /* case 4 (new client) or cases 2, 3 (client reboot): */
>>>> gen_clid(new, nn);
>>>> new->cl_minorversion = 0;
>>>> gen_callback(new, setclid, rqstp);
>>>> --
>>>> 2.4.3
>>
>
v2, new helper nfs4_free_stateowner for freeing so_owner.data and sop
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4state.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 00a0f8f..3c24c72 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -990,6 +990,12 @@ release_all_access(struct nfs4_ol_stateid *stp)
}
}
+static inline void nfs4_free_stateowner(struct nfs4_stateowner *sop)
+{
+ kfree(sop->so_owner.data);
+ sop->so_ops->so_free(sop);
+}
+
static void nfs4_put_stateowner(struct nfs4_stateowner *sop)
{
struct nfs4_client *clp = sop->so_client;
@@ -1000,8 +1006,7 @@ static void nfs4_put_stateowner(struct nfs4_stateowner *sop)
return;
sop->so_ops->so_unhash(sop);
spin_unlock(&clp->cl_lock);
- kfree(sop->so_owner.data);
- sop->so_ops->so_free(sop);
+ nfs4_free_stateowner(sop);
}
static void unhash_ol_stateid(struct nfs4_ol_stateid *stp)
@@ -3316,7 +3321,8 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
hash_openowner(oo, clp, strhashval);
ret = oo;
} else
- nfs4_free_openowner(&oo->oo_owner);
+ nfs4_free_stateowner(&oo->oo_owner);
+
spin_unlock(&clp->cl_lock);
return ret;
}
@@ -5216,7 +5222,8 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
&clp->cl_ownerstr_hashtbl[strhashval]);
ret = lo;
} else
- nfs4_free_lockowner(&lo->lo_owner);
+ nfs4_free_stateowner(&lo->lo_owner);
+
spin_unlock(&clp->cl_lock);
return ret;
}
--
2.4.3
On Thu, Jul 16, 2015 at 10:30:37AM +0800, Kinglong Mee wrote:
> On 7/15/2015 23:03, J. Bruce Fields wrote:
> > On Mon, Jul 13, 2015 at 05:29:01PM +0800, Kinglong Mee wrote:
> >> Layout is a state resource, nfsd should check it too.
> >>
> >> Signed-off-by: Kinglong Mee <[email protected]>
> >> ---
> >> fs/nfsd/nfs4state.c | 6 ++++--
> >> 1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >> index 61dfb33..e0a4556 100644
> >> --- a/fs/nfsd/nfs4state.c
> >> +++ b/fs/nfsd/nfs4state.c
> >> @@ -2241,6 +2241,7 @@ static bool client_has_state(struct nfs4_client *clp)
> >> * Also note we should probably be using this in 4.0 case too.
> >> */
> >> return !list_empty(&clp->cl_openowners)
> >> + || !list_empty(&clp->cl_lo_states)
> >> || !list_empty(&clp->cl_delegations)
> >> || !list_empty(&clp->cl_sessions);
> >> }
> >> @@ -4257,8 +4258,9 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >> goto out;
> >> clp = cstate->clp;
> >> status = nfserr_cb_path_down;
> >> - if (!list_empty(&clp->cl_delegations)
> >> - && clp->cl_cb_state != NFSD4_CB_UP)
> >> + if ((!list_empty(&clp->cl_delegations)
> >> + || !list_empty(&clp->cl_lo_states))
> >
> > nfsd4_renew is NFSv4.0 only, so I don't think this part is necessary.
> >
> > That said, it's harmless--why don't we just call client_has_state() here
> > instead?
>
> I don't think so.
>
> NFSv4.0's callback path is used for delegation recalling only,
> without delegation, callback path is useless.
>
> Also, describing in rfc3530,
>
> When the client holds delegations, it needs to use RENEW to detect
> when the server has determined that the callback path is down. When
> the server has made such a determination, only the RENEW operation
> will renew the lease on delegations. If the server determines the
> callback path is down, it returns NFS4ERR_CB_PATH_DOWN. Even though
> it returns NFS4ERR_CB_PATH_DOWN, the server MUST renew the lease on
> the record locks and share reservations that the client has
> established on the server.
>
> I will drop the second modify in version 2.
Oh, you're right, the cl_openowners check would be wrong for the renew
case.
Applying v2, thanks.--b.
>
> thanks,
> Kinglong Mee
On Thu, Jul 16, 2015 at 11:50:55AM +0800, Kinglong Mee wrote:
> On 7/16/2015 11:36, Kinglong Mee wrote:
> > On 7/16/2015 04:49, J. Bruce Fields wrote:
> >> On Wed, Jul 15, 2015 at 04:47:48PM -0400, J. Bruce Fields wrote:
> >>> On Mon, Jul 13, 2015 at 05:29:41PM +0800, Kinglong Mee wrote:
> >>>> Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock"
> >>>> have moved gen_confirm() to gen_clid().
> >>>
> >>> This means the statement in that earlier commit is wrong:
> >>>
> >>>
> >>> With this, there's no need to keep two counters as they'd always
> >>> be in sync anyway, so just use the clientid_counter for both.
> >>>
> >>> Looks to me like this may need a separate counter to eliminate the
> >>> possibibility of returning the same confirm twice for a one clientid?
> >
> > Yes, nfsd will generate same confirm for one clientid in one second.
> >
> > verf[0] = (__force __be32)jiffies;
> > verf[1] = (__force __be32)nn->clientid_counter;
> >
> > for case 1: probable callback update, the new unconf client needs
> > a different confirm.
>
> Ignore this patch, and just revert commit 294ac32e99
> "nfsd: protect clid and verifier generation with client_lock"
> is a better solve.
We can't revert that completely, it does fix a real locking bug at
least, I think.
I'd agree to reinstating a separate counter for the verifier. That
verifier probably also needs to be per-network namespace to make the
per-network-namespace locking correct.
--b.
>
> thanks,
> Kinglong Mee
>
> >
> > Rereading rfc7530,
> > x be the value of the client.id subfield of the SETCLIENTID4args
> > structure.
> >
> > v be the value of the client.verifier subfield of the
> > SETCLIENTID4args structure.
> >
> > c be the value of the client ID field returned in the
> > SETCLIENTID4resok structure.
> >
> > k represent the value combination of the callback and callback_ident
> > fields of the SETCLIENTID4args structure.
> >
> > s be the setclientid_confirm value returned in the SETCLIENTID4resok
> > structure.
> >
> > { v, x, c, k, s } be a quintuple for a client record. A client
> > record is confirmed if there has been a SETCLIENTID_CONFIRM
> > operation to confirm it. Otherwise, it is unconfirmed. An
> > unconfirmed record is established by a SETCLIENTID call.
> >
> > ... /* case 1: probable callback update */ ...
> >
> > o The server checks if it has recorded a confirmed record for { v,
> > x, c, l, s }, where l may or may not equal k. If so, and since
> > the id verifier v of the request matches that which is confirmed
> > and recorded, the server treats this as a probable callback
> > information update and records an unconfirmed { v, x, c, k, t }
> > and leaves the confirmed { v, x, c, l, s } in place, such that
> > t != s. It does not matter whether k equals l or not. Any
> > pre-existing unconfirmed { v, x, c, *, * } is removed.
> >
> > The server returns { c, t }. It is indeed returning the old
> > clientid4 value c, because the client apparently only wants to
> > update callback value k to value l. It's possible this request is
> > one from the Byzantine router that has stale callback information,
> > but this is not a problem. The callback information update is
> > only confirmed if followed up by a SETCLIENTID_CONFIRM { c, t }.
> >
> > The server awaits confirmation of k via SETCLIENTID_CONFIRM
> > { c, t }.
> >
> > The server does NOT remove client (lock/share/delegation) state
> > for x.
> >
> >>
> >> (but frankly I can never completely review changes to the
> >> setclientid/setclientid_confirm behavior without rereading RFC 7530
> >> 16.33.5 every time, which is a slog. Might help to contrive a pynfs
> >> test derived from that text which tests for this particular behavior.)
> >>
> >
> > Make sense.
> > I will make it later.
> >
> > thanks,
> > Kinglong Mee
> >
> >
> >>>
> >>> --b.
> >>>
> >>>>
> >>>> After it, setclientid will return a bad reply with all zero confirms
> >>>> after copy_clid().
> >>>>
> >>>> Signed-off-by: Kinglong Mee <[email protected]>
> >>>> ---
> >>>> fs/nfsd/nfs4state.c | 5 +++--
> >>>> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >>>> index e0a4556..b1f84fc 100644
> >>>> --- a/fs/nfsd/nfs4state.c
> >>>> +++ b/fs/nfsd/nfs4state.c
> >>>> @@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >>>> unconf = find_unconfirmed_client_by_name(&clname, nn);
> >>>> if (unconf)
> >>>> unhash_client_locked(unconf);
> >>>> - if (conf && same_verf(&conf->cl_verifier, &clverifier))
> >>>> + if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
> >>>> /* case 1: probable callback update */
> >>>> copy_clid(new, conf);
> >>>> - else /* case 4 (new client) or cases 2, 3 (client reboot): */
> >>>> + gen_confirm(new, nn);
> >>>> + } else /* case 4 (new client) or cases 2, 3 (client reboot): */
> >>>> gen_clid(new, nn);
> >>>> new->cl_minorversion = 0;
> >>>> gen_callback(new, setclid, rqstp);
> >>>> --
> >>>> 2.4.3
> >>
> >
On Thu, Jul 16, 2015 at 12:05:07PM +0800, Kinglong Mee wrote:
> v2, new helper nfs4_free_stateowner for freeing so_owner.data and sop
Got it, thanks.--b.
>
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 00a0f8f..3c24c72 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -990,6 +990,12 @@ release_all_access(struct nfs4_ol_stateid *stp)
> }
> }
>
> +static inline void nfs4_free_stateowner(struct nfs4_stateowner *sop)
> +{
> + kfree(sop->so_owner.data);
> + sop->so_ops->so_free(sop);
> +}
> +
> static void nfs4_put_stateowner(struct nfs4_stateowner *sop)
> {
> struct nfs4_client *clp = sop->so_client;
> @@ -1000,8 +1006,7 @@ static void nfs4_put_stateowner(struct nfs4_stateowner *sop)
> return;
> sop->so_ops->so_unhash(sop);
> spin_unlock(&clp->cl_lock);
> - kfree(sop->so_owner.data);
> - sop->so_ops->so_free(sop);
> + nfs4_free_stateowner(sop);
> }
>
> static void unhash_ol_stateid(struct nfs4_ol_stateid *stp)
> @@ -3316,7 +3321,8 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
> hash_openowner(oo, clp, strhashval);
> ret = oo;
> } else
> - nfs4_free_openowner(&oo->oo_owner);
> + nfs4_free_stateowner(&oo->oo_owner);
> +
> spin_unlock(&clp->cl_lock);
> return ret;
> }
> @@ -5216,7 +5222,8 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
> &clp->cl_ownerstr_hashtbl[strhashval]);
> ret = lo;
> } else
> - nfs4_free_lockowner(&lo->lo_owner);
> + nfs4_free_stateowner(&lo->lo_owner);
> +
> spin_unlock(&clp->cl_lock);
> return ret;
> }
> --
> 2.4.3
On Fri, 17 Jul 2015 11:58:46 -0400
"J. Bruce Fields" <[email protected]> wrote:
> On Thu, Jul 16, 2015 at 11:50:55AM +0800, Kinglong Mee wrote:
> > On 7/16/2015 11:36, Kinglong Mee wrote:
> > > On 7/16/2015 04:49, J. Bruce Fields wrote:
> > >> On Wed, Jul 15, 2015 at 04:47:48PM -0400, J. Bruce Fields wrote:
> > >>> On Mon, Jul 13, 2015 at 05:29:41PM +0800, Kinglong Mee wrote:
> > >>>> Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock"
> > >>>> have moved gen_confirm() to gen_clid().
> > >>>
> > >>> This means the statement in that earlier commit is wrong:
> > >>>
> > >>>
> > >>> With this, there's no need to keep two counters as they'd always
> > >>> be in sync anyway, so just use the clientid_counter for both.
> > >>>
> > >>> Looks to me like this may need a separate counter to eliminate the
> > >>> possibibility of returning the same confirm twice for a one clientid?
> > >
> > > Yes, nfsd will generate same confirm for one clientid in one second.
> > >
> > > verf[0] = (__force __be32)jiffies;
> > > verf[1] = (__force __be32)nn->clientid_counter;
> > >
> > > for case 1: probable callback update, the new unconf client needs
> > > a different confirm.
> >
> > Ignore this patch, and just revert commit 294ac32e99
> > "nfsd: protect clid and verifier generation with client_lock"
> > is a better solve.
>
> We can't revert that completely, it does fix a real locking bug at
> least, I think.
>
> I'd agree to reinstating a separate counter for the verifier. That
> verifier probably also needs to be per-network namespace to make the
> per-network-namespace locking correct.
>
> --b.
>
Sorry, just getting caught up on this. At the time I was just
"following the code" and not necessarily paying much attention to the
spec.
Yes, I agree -- a separate counter sounds like the right fix for now,
in conjunction with Kinglong's patch (or something like it).
> >
> > thanks,
> > Kinglong Mee
> >
> > >
> > > Rereading rfc7530,
> > > x be the value of the client.id subfield of the SETCLIENTID4args
> > > structure.
> > >
> > > v be the value of the client.verifier subfield of the
> > > SETCLIENTID4args structure.
> > >
> > > c be the value of the client ID field returned in the
> > > SETCLIENTID4resok structure.
> > >
> > > k represent the value combination of the callback and callback_ident
> > > fields of the SETCLIENTID4args structure.
> > >
> > > s be the setclientid_confirm value returned in the SETCLIENTID4resok
> > > structure.
> > >
> > > { v, x, c, k, s } be a quintuple for a client record. A client
> > > record is confirmed if there has been a SETCLIENTID_CONFIRM
> > > operation to confirm it. Otherwise, it is unconfirmed. An
> > > unconfirmed record is established by a SETCLIENTID call.
> > >
> > > ... /* case 1: probable callback update */ ...
> > >
> > > o The server checks if it has recorded a confirmed record for { v,
> > > x, c, l, s }, where l may or may not equal k. If so, and since
> > > the id verifier v of the request matches that which is confirmed
> > > and recorded, the server treats this as a probable callback
> > > information update and records an unconfirmed { v, x, c, k, t }
> > > and leaves the confirmed { v, x, c, l, s } in place, such that
> > > t != s. It does not matter whether k equals l or not. Any
> > > pre-existing unconfirmed { v, x, c, *, * } is removed.
> > >
> > > The server returns { c, t }. It is indeed returning the old
> > > clientid4 value c, because the client apparently only wants to
> > > update callback value k to value l. It's possible this request is
> > > one from the Byzantine router that has stale callback information,
> > > but this is not a problem. The callback information update is
> > > only confirmed if followed up by a SETCLIENTID_CONFIRM { c, t }.
> > >
> > > The server awaits confirmation of k via SETCLIENTID_CONFIRM
> > > { c, t }.
> > >
> > > The server does NOT remove client (lock/share/delegation) state
> > > for x.
> > >
> > >>
> > >> (but frankly I can never completely review changes to the
> > >> setclientid/setclientid_confirm behavior without rereading RFC 7530
> > >> 16.33.5 every time, which is a slog. Might help to contrive a pynfs
> > >> test derived from that text which tests for this particular behavior.)
> > >>
> > >
> > > Make sense.
> > > I will make it later.
> > >
> > > thanks,
> > > Kinglong Mee
> > >
> > >
> > >>>
> > >>> --b.
> > >>>
> > >>>>
> > >>>> After it, setclientid will return a bad reply with all zero confirms
> > >>>> after copy_clid().
> > >>>>
> > >>>> Signed-off-by: Kinglong Mee <[email protected]>
> > >>>> ---
> > >>>> fs/nfsd/nfs4state.c | 5 +++--
> > >>>> 1 file changed, 3 insertions(+), 2 deletions(-)
> > >>>>
> > >>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > >>>> index e0a4556..b1f84fc 100644
> > >>>> --- a/fs/nfsd/nfs4state.c
> > >>>> +++ b/fs/nfsd/nfs4state.c
> > >>>> @@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > >>>> unconf = find_unconfirmed_client_by_name(&clname, nn);
> > >>>> if (unconf)
> > >>>> unhash_client_locked(unconf);
> > >>>> - if (conf && same_verf(&conf->cl_verifier, &clverifier))
> > >>>> + if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
> > >>>> /* case 1: probable callback update */
> > >>>> copy_clid(new, conf);
> > >>>> - else /* case 4 (new client) or cases 2, 3 (client reboot): */
> > >>>> + gen_confirm(new, nn);
> > >>>> + } else /* case 4 (new client) or cases 2, 3 (client reboot): */
> > >>>> gen_clid(new, nn);
> > >>>> new->cl_minorversion = 0;
> > >>>> gen_callback(new, setclid, rqstp);
> > >>>> --
> > >>>> 2.4.3
> > >>
> > >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jeff Layton <[email protected]>
If using clientid_counter, gen_confirm will generate same verifier
in one second.
A new counter for client confirm verifier make sure gen_confirm
generating different each calling for the same clientid.
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/netns.h | 1 +
fs/nfsd/nfs4state.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index ea6749a..d8b16c2 100644
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -110,6 +110,7 @@ struct nfsd_net {
unsigned int max_connections;
u32 clientid_counter;
+ u32 clverifier_counter;
struct svc_serv *nfsd_serv;
};
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 61dfb33..5a64757e 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1894,7 +1894,7 @@ static void gen_confirm(struct nfs4_client *clp, struct nfsd_net *nn)
* __force to keep sparse happy
*/
verf[0] = (__force __be32)get_seconds();
- verf[1] = (__force __be32)nn->clientid_counter;
+ verf[1] = (__force __be32)nn->clverifier_counter++;
memcpy(clp->cl_confirm.data, verf, sizeof(clp->cl_confirm.data));
}
--
2.4.3
On Sat, 18 Jul 2015 07:33:31 +0800
Kinglong Mee <[email protected]> wrote:
> If using clientid_counter, gen_confirm will generate same verifier
> in one second.
>
> A new counter for client confirm verifier make sure gen_confirm
> generating different each calling for the same clientid.
>
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> fs/nfsd/netns.h | 1 +
> fs/nfsd/nfs4state.c | 2 +-
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> index ea6749a..d8b16c2 100644
> --- a/fs/nfsd/netns.h
> +++ b/fs/nfsd/netns.h
> @@ -110,6 +110,7 @@ struct nfsd_net {
> unsigned int max_connections;
>
> u32 clientid_counter;
> + u32 clverifier_counter;
>
> struct svc_serv *nfsd_serv;
> };
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 61dfb33..5a64757e 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1894,7 +1894,7 @@ static void gen_confirm(struct nfs4_client *clp, struct nfsd_net *nn)
> * __force to keep sparse happy
> */
> verf[0] = (__force __be32)get_seconds();
> - verf[1] = (__force __be32)nn->clientid_counter;
> + verf[1] = (__force __be32)nn->clverifier_counter++;
> memcpy(clp->cl_confirm.data, verf, sizeof(clp->cl_confirm.data));
> }
>
Reviewed-by: Jeff Layton <[email protected]>
On Sat, Jul 18, 2015 at 08:16:08AM -0400, Jeff Layton wrote:
> On Sat, 18 Jul 2015 07:33:31 +0800
> Kinglong Mee <[email protected]> wrote:
>
> > If using clientid_counter, gen_confirm will generate same verifier
> > in one second.
> >
> > A new counter for client confirm verifier make sure gen_confirm
> > generating different each calling for the same clientid.
> >
> > Signed-off-by: Kinglong Mee <[email protected]>
> > ---
> > fs/nfsd/netns.h | 1 +
> > fs/nfsd/nfs4state.c | 2 +-
> > 2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> > index ea6749a..d8b16c2 100644
> > --- a/fs/nfsd/netns.h
> > +++ b/fs/nfsd/netns.h
> > @@ -110,6 +110,7 @@ struct nfsd_net {
> > unsigned int max_connections;
> >
> > u32 clientid_counter;
> > + u32 clverifier_counter;
> >
> > struct svc_serv *nfsd_serv;
> > };
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 61dfb33..5a64757e 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -1894,7 +1894,7 @@ static void gen_confirm(struct nfs4_client *clp, struct nfsd_net *nn)
> > * __force to keep sparse happy
> > */
> > verf[0] = (__force __be32)get_seconds();
> > - verf[1] = (__force __be32)nn->clientid_counter;
> > + verf[1] = (__force __be32)nn->clverifier_counter++;
> > memcpy(clp->cl_confirm.data, verf, sizeof(clp->cl_confirm.data));
> > }
> >
>
> Reviewed-by: Jeff Layton <[email protected]>
Thanks.--b.
I don't think layout stateid handling is the same as handling of other
stateid's. See
https://tools.ietf.org/html/rfc5661#section-12.5.3
I haven't reviewed it in detail yet.
--b.
On Mon, Jul 13, 2015 at 05:31:21PM +0800, Kinglong Mee wrote:
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> fs/nfsd/nfs4layouts.c | 5 ++---
> fs/nfsd/nfs4state.c | 2 +-
> fs/nfsd/state.h | 1 +
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
> index 6904213..5c0f1d6 100644
> --- a/fs/nfsd/nfs4layouts.c
> +++ b/fs/nfsd/nfs4layouts.c
> @@ -263,9 +263,8 @@ nfsd4_preprocess_layout_stateid(struct svc_rqst *rqstp,
> goto out;
> } else {
> ls = container_of(stid, struct nfs4_layout_stateid, ls_stid);
> -
> - status = nfserr_bad_stateid;
> - if (stateid->si_generation > stid->sc_stateid.si_generation)
> + status = check_stateid_generation(stateid, &stid->sc_stateid, 1);
> + if (status)
> goto out_put_stid;
> if (layout_type != ls->ls_layout_type)
> goto out_put_stid;
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 998166d..2edfedc 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4474,7 +4474,7 @@ static bool stateid_generation_after(stateid_t *a, stateid_t *b)
> return (s32)(a->si_generation - b->si_generation) > 0;
> }
>
> -static __be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)
> +__be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)
> {
> /*
> * When sessions are used the stateid generation number is ignored
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 4874ce5..18d015d 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -589,6 +589,7 @@ extern __be32 nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
> __be32 nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
> stateid_t *stateid, unsigned char typemask,
> struct nfs4_stid **s, struct nfsd_net *nn);
> +__be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session);
> struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl,
> struct kmem_cache *slab);
> void nfs4_unhash_stid(struct nfs4_stid *s);
> --
> 2.4.3
OK.--b.
On Mon, Jul 13, 2015 at 05:31:42PM +0800, Kinglong Mee wrote:
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> fs/nfsd/nfs4layouts.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
> index 5c0f1d6..c5af4ccb 100644
> --- a/fs/nfsd/nfs4layouts.c
> +++ b/fs/nfsd/nfs4layouts.c
> @@ -162,7 +162,7 @@ nfsd4_layout_setlease(struct nfs4_layout_stateid *ls)
> fl = locks_alloc_lock();
> if (!fl)
> return -ENOMEM;
> - locks_init_lock(fl);
> +
> fl->fl_lmops = &nfsd4_layouts_lm_ops;
> fl->fl_flags = FL_LAYOUT;
> fl->fl_type = F_RDLCK;
> --
> 2.4.3
OK, applied the rest of these.--b.
On Mon, Jul 13, 2015 at 05:35:37PM +0800, Kinglong Mee wrote:
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 2704270..b6c134d 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5131,8 +5131,7 @@ nevermind:
> }
>
> static struct nfs4_lockowner *
> -find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
> - struct nfs4_client *clp)
> +find_lockowner_str_locked(struct nfs4_client *clp, struct xdr_netobj *owner)
> {
> unsigned int strhashval = ownerstr_hashval(owner);
> struct nfs4_stateowner *so;
> @@ -5150,13 +5149,12 @@ find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
> }
>
> static struct nfs4_lockowner *
> -find_lockowner_str(clientid_t *clid, struct xdr_netobj *owner,
> - struct nfs4_client *clp)
> +find_lockowner_str(struct nfs4_client *clp, struct xdr_netobj *owner)
> {
> struct nfs4_lockowner *lo;
>
> spin_lock(&clp->cl_lock);
> - lo = find_lockowner_str_locked(clid, owner, clp);
> + lo = find_lockowner_str_locked(clp, owner);
> spin_unlock(&clp->cl_lock);
> return lo;
> }
> @@ -5200,8 +5198,7 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
> lo->lo_owner.so_seqid = lock->lk_new_lock_seqid;
> lo->lo_owner.so_ops = &lockowner_ops;
> spin_lock(&clp->cl_lock);
> - ret = find_lockowner_str_locked(&clp->cl_clientid,
> - &lock->lk_new_owner, clp);
> + ret = find_lockowner_str_locked(clp, &lock->lk_new_owner);
> if (ret == NULL) {
> list_add(&lo->lo_owner.so_strhash,
> &clp->cl_ownerstr_hashtbl[strhashval]);
> @@ -5322,7 +5319,7 @@ lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
> struct nfs4_lockowner *lo;
> unsigned int strhashval;
>
> - lo = find_lockowner_str(&cl->cl_clientid, &lock->lk_new_owner, cl);
> + lo = find_lockowner_str(cl, &lock->lk_new_owner);
> if (!lo) {
> strhashval = ownerstr_hashval(&lock->lk_new_owner);
> lo = alloc_init_lock_stateowner(strhashval, cl, ost, lock);
> @@ -5597,8 +5594,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> goto out;
> }
>
> - lo = find_lockowner_str(&lockt->lt_clientid, &lockt->lt_owner,
> - cstate->clp);
> + lo = find_lockowner_str(cstate->clp, &lockt->lt_owner);
> if (lo)
> file_lock->fl_owner = (fl_owner_t)lo;
> file_lock->fl_pid = current->tgid;
> --
> 2.4.3
On 7/23/2015 02:22, J. Bruce Fields wrote:
> I don't think layout stateid handling is the same as handling of other
> stateid's. See
>
> https://tools.ietf.org/html/rfc5661#section-12.5.3
Yes,
"Given the design goal of pNFS to provide parallelism, the layout
stateid differs from other stateid types in that the client is
expected to send LAYOUTGET and LAYOUTRETURN operations in parallel."
Please ignore this patch.
thanks,
Kinglong Mee
>
> I haven't reviewed it in detail yet.
>
> --b.
>
> On Mon, Jul 13, 2015 at 05:31:21PM +0800, Kinglong Mee wrote:
>> Signed-off-by: Kinglong Mee <[email protected]>
>> ---
>> fs/nfsd/nfs4layouts.c | 5 ++---
>> fs/nfsd/nfs4state.c | 2 +-
>> fs/nfsd/state.h | 1 +
>> 3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
>> index 6904213..5c0f1d6 100644
>> --- a/fs/nfsd/nfs4layouts.c
>> +++ b/fs/nfsd/nfs4layouts.c
>> @@ -263,9 +263,8 @@ nfsd4_preprocess_layout_stateid(struct svc_rqst *rqstp,
>> goto out;
>> } else {
>> ls = container_of(stid, struct nfs4_layout_stateid, ls_stid);
>> -
>> - status = nfserr_bad_stateid;
>> - if (stateid->si_generation > stid->sc_stateid.si_generation)
>> + status = check_stateid_generation(stateid, &stid->sc_stateid, 1);
>> + if (status)
>> goto out_put_stid;
>> if (layout_type != ls->ls_layout_type)
>> goto out_put_stid;
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 998166d..2edfedc 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -4474,7 +4474,7 @@ static bool stateid_generation_after(stateid_t *a, stateid_t *b)
>> return (s32)(a->si_generation - b->si_generation) > 0;
>> }
>>
>> -static __be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)
>> +__be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)
>> {
>> /*
>> * When sessions are used the stateid generation number is ignored
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index 4874ce5..18d015d 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -589,6 +589,7 @@ extern __be32 nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
>> __be32 nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>> stateid_t *stateid, unsigned char typemask,
>> struct nfs4_stid **s, struct nfsd_net *nn);
>> +__be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session);
>> struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl,
>> struct kmem_cache *slab);
>> void nfs4_unhash_stid(struct nfs4_stid *s);
>> --
>> 2.4.3
>
This one needs be applied with
"nfsd: New counter for generating client confirm verifier"
that is only update client confirm verifier without fix this problem.
thanks,
Kinglong Mee
On 7/13/2015 17:29, Kinglong Mee wrote:
> Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock"
> have moved gen_confirm() to gen_clid().
>
> After it, setclientid will return a bad reply with all zero confirms
> after copy_clid().
>
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index e0a4556..b1f84fc 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> unconf = find_unconfirmed_client_by_name(&clname, nn);
> if (unconf)
> unhash_client_locked(unconf);
> - if (conf && same_verf(&conf->cl_verifier, &clverifier))
> + if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
> /* case 1: probable callback update */
> copy_clid(new, conf);
> - else /* case 4 (new client) or cases 2, 3 (client reboot): */
> + gen_confirm(new, nn);
> + } else /* case 4 (new client) or cases 2, 3 (client reboot): */
> gen_clid(new, nn);
> new->cl_minorversion = 0;
> gen_callback(new, setclid, rqstp);
>
On Thu, Jul 23, 2015 at 09:16:27AM +0800, Kinglong Mee wrote:
> This one needs be applied with
> "nfsd: New counter for generating client confirm verifier"
> that is only update client confirm verifier without fix this problem.
Oh, right, I forgot the original patch after the discussion; applying
"nfsd: Add missing gen_confirm...".
--b.
>
> thanks,
> Kinglong Mee
>
> On 7/13/2015 17:29, Kinglong Mee wrote:
> > Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock"
> > have moved gen_confirm() to gen_clid().
> >
> > After it, setclientid will return a bad reply with all zero confirms
> > after copy_clid().
> >
> > Signed-off-by: Kinglong Mee <[email protected]>
> > ---
> > fs/nfsd/nfs4state.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index e0a4556..b1f84fc 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > unconf = find_unconfirmed_client_by_name(&clname, nn);
> > if (unconf)
> > unhash_client_locked(unconf);
> > - if (conf && same_verf(&conf->cl_verifier, &clverifier))
> > + if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
> > /* case 1: probable callback update */
> > copy_clid(new, conf);
> > - else /* case 4 (new client) or cases 2, 3 (client reboot): */
> > + gen_confirm(new, nn);
> > + } else /* case 4 (new client) or cases 2, 3 (client reboot): */
> > gen_clid(new, nn);
> > new->cl_minorversion = 0;
> > gen_callback(new, setclid, rqstp);
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html