On 12/02/2013 10:59 PM, J. Bruce Fields wrote:
> On Wed, Nov 27, 2013 at 06:29:22PM +0800, Kinglong Mee wrote:
>> If finding backchannel failed, nfsd should not enter setup_callback_client.
>>
>> Signed-off-by: Kinglong Mee <kinglongmee@xxxxxxxxx>
>> ---
>> fs/nfsd/nfs4callback.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
>> index 7f05cd1..755d6d6 100644
>> --- a/fs/nfsd/nfs4callback.c
>> +++ b/fs/nfsd/nfs4callback.c
>> @@ -664,7 +664,7 @@ static int setup_callback_client(struct nfs4_client *clp, struct nfs4_cb_conn *c
>> args.authflavor = clp->cl_cred.cr_flavor;
>> clp->cl_cb_ident = conn->cb_ident;
>> } else {
>> - if (!conn->cb_xprt)
>> + if (!conn->cb_xprt || !ses)
>
> It looks to me like ses should be set whenenver conn is. Do you have
> reason to believe the contrary?
No. It just a double check as conn->cb_xprt.
I will drop it in v2 of this path.
>
>> return -EINVAL;
>> clp->cl_cb_conn.cb_xprt = conn->cb_xprt;
>> clp->cl_cb_session = ses;
>> @@ -982,7 +982,7 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb)
>> }
>> spin_unlock(&clp->cl_lock);
>>
>> - err = setup_callback_client(clp, &conn, ses);
>> + err = c ? setup_callback_client(clp, &conn, ses) : -ENOENT;
>
> But, whoops, yes, this looks like a good fix. (Have you hit this
> failure in practice?)
Yes. When destroying the last session from client, nfsd4_probe_callback_sync
will be called in function nfsd4_destroy_session.
As that, callback client will be update in function nfsd4_process_cb_update,
so __nfsd4_find_backchannel always failed with returning NULL. (Ps: Maybe needs
another patch to avoid updating callback connect when destroying the last session ?)
For debugging, add message when finding backchannel failed.
Just testing mount/umount at client, the debug message will be print every umount.
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 7f05cd1..1be4c61 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -979,6 +979,8 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb)
svc_xprt_get(c->cn_xprt);
conn.cb_xprt = c->cn_xprt;
ses = c->cn_session;
+ } else {
+ printk("%s: found connet failed.\n", __func__);
}
spin_unlock(&clp->cl_lock);
> But I think we should probably adopt the sort of
> control flow that's more idiomatic in the kernel:
>
> c = __nfsd4_find_backchannel(clp)
> if (!c)
> goto out_unlock;
> ...
> spin_unlock(&clp->cl_lock);
> err = setup_callback_client(clp, &conn, ses);
> if (err)
> goto out_err;
>
OK, it is really better.
thanks,
Kinglong Mee
> etc.
>
> --b.
>
>> if (err) {
>> nfsd4_mark_cb_down(clp, err);
>> return;
>> --
>> 1.8.4.2
>
--
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
