If finding backchannel failed, nfsd should not enter setup_callback_client.
Signed-off-by: Kinglong Mee <[email protected]>
---
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)
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;
if (err) {
nfsd4_mark_cb_down(clp, err);
return;
--
1.8.4.2
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 <[email protected]>
> >>>> ---
> >>>> 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.
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 <[email protected]>
> ---
> 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?
> 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?) 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;
etc.
--b.
> if (err) {
> nfsd4_mark_cb_down(clp, err);
> return;
> --
> 1.8.4.2
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 <[email protected]>
>>>> ---
>>>> 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.
thanks,
Kinglong Mee
>> (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
>>>
>>
>
2013/12/7 J. Bruce Fields <[email protected]>:
> 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 <[email protected]>
>> >>>> ---
>> >>>> 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?
Sorry for the not clearly explan.
there are two behavior of this patch.
the first is as you said, change the error number.
the second is the logic, return immediately when finding session
failed, not enter setup_callback_client.
thanks,
Kinglong Mee
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 <[email protected]>
> >> ---
> >> 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. Is that the only problem?
--b.
> (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
> >
>
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 <[email protected]>
>> ---
>> 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
>
Finding backchannel failed, nfsd should not enter setup_callback_client.
v2. drop ses checking in setup_callback_client and make code more clearer.
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4callback.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 7f05cd1..44d33ce 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -975,11 +975,15 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb)
clear_bit(NFSD4_CLIENT_CB_UPDATE, &clp->cl_flags);
memcpy(&conn, &cb->cb_clp->cl_cb_conn, sizeof(struct nfs4_cb_conn));
c = __nfsd4_find_backchannel(clp);
- if (c) {
- svc_xprt_get(c->cn_xprt);
- conn.cb_xprt = c->cn_xprt;
- ses = c->cn_session;
+ if (!c) {
+ spin_unlock(&clp->cl_lock);
+ nfsd4_mark_cb_down(clp, -ENOTCONN);
+ return;
}
+
+ svc_xprt_get(c->cn_xprt);
+ conn.cb_xprt = c->cn_xprt;
+ ses = c->cn_session;
spin_unlock(&clp->cl_lock);
err = setup_callback_client(clp, &conn, ses);
--
1.8.4.2