On Thu, Dec 05, 2013 at 10:09:52AM +0800, Kinglong Mee wrote:
> On 12/05/2013 01:10 AM, J. Bruce Fields wrote:
> > On Tue, Dec 03, 2013 at 10:46:47AM +0800, Kinglong Mee wrote:
> >> 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.
> >
> > On second thoughts, why is that actually a problem?
> > setup_callback_client() will fail and you'll get an unnecessary printk
> > from nfsd4_mark_cb_down, I guess.
>
> Yes, that's right. setup_callback_client will failed with －EINVAL.
> This patch just changes the logic, function returns immediately when finding backchannel failed,
> instead of enter setup_callback_client.
>
> > Is that the only problem?
>
> Yes, this path only focus the logic problem.
Apologies, I'm still not sure I understand:
As far as I can tell, the *only* way that your patch changes behavior is
to change the error passed to nfsd4_mark_cb_path down.
Is that right, or am I missing some other problem which this fixes?
--b.
--
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
