2008-09-25 21:38:34

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] NLM: Revert parts of commit 24e36663

On Wed, Sep 24, 2008 at 02:50:36PM -0400, Chuck Lever wrote:
> statd and some servers always need lockd to listen on UDP, so always
> start both lockd listener sockets.
>
> This probably leaks an svc_serv if the TCP listener can't be created,
> but it will do for now.
>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
>
> I currently have this workaround lurking in my git repo. Is there some kind
> of fix for this issue planned for 2.6.28?

Not that I know of.

> This is only a reminder, not a suggested patch.

Any chance you could generate a patch?

--b.

>
> fs/lockd/svc.c | 31 +++++++++++++------------------
> 1 files changed, 13 insertions(+), 18 deletions(-)
>
> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> index a7b604c..36b6d03 100644
> --- a/fs/lockd/svc.c
> +++ b/fs/lockd/svc.c
> @@ -213,25 +213,23 @@ lockd(void *vrqstp)
> }
>
> /*
> - * Make any sockets that are needed but not present.
> - * If nlm_udpport or nlm_tcpport were set as module
> - * options, make those sockets unconditionally
> + * Create UDP and TCP listeners for lockd. Even if we only
> + * have TCP NFS mounts and TCP NFSDs, some services (such
> + * as rpc.statd) still require UDP.
> */
> -static int make_socks(struct svc_serv *serv, const unsigned short proto)
> +static int make_socks(struct svc_serv *serv)
> {
> static int warned;
> struct svc_xprt *xprt;
> int err = 0;
>
> - if (proto == IPPROTO_UDP || nlm_udpport) {
> - xprt = svc_find_xprt(serv, "udp", 0, 0);
> - if (!xprt)
> - err = svc_create_xprt(serv, "udp", nlm_udpport,
> - SVC_SOCK_DEFAULTS);
> - else
> - svc_xprt_put(xprt);
> - }
> - if (err >= 0 && (proto == IPPROTO_TCP || nlm_tcpport)) {
> + xprt = svc_find_xprt(serv, "udp", 0, 0);
> + if (!xprt)
> + err = svc_create_xprt(serv, "udp", nlm_udpport,
> + SVC_SOCK_DEFAULTS);
> + else
> + svc_xprt_put(xprt);
> + if (err >= 0) {
> xprt = svc_find_xprt(serv, "tcp", 0, 0);
> if (!xprt)
> err = svc_create_xprt(serv, "tcp", nlm_tcpport,
> @@ -260,11 +258,8 @@ int lockd_up(const unsigned short proto)
> /*
> * Check whether we're already up and running.
> */
> - if (nlmsvc_rqst) {
> - if (proto)
> - error = make_socks(nlmsvc_rqst->rq_server, proto);
> + if (nlmsvc_rqst)
> goto out;
> - }
>
> /*
> * Sanity check: if there's no pid,
> @@ -281,7 +276,7 @@ int lockd_up(const unsigned short proto)
> goto out;
> }
>
> - if ((error = make_socks(serv, proto)) < 0)
> + if ((error = make_socks(serv)) < 0)
> goto destroy_and_out;
>
> /*
>


2008-09-26 17:54:18

by Chuck Lever

[permalink] [raw]
Subject: Re: [PATCH] NLM: Revert parts of commit 24e36663

On Sep 25, 2008, at Sep 25, 2008, 5:38 PM, J. Bruce Fields wrote:
> On Wed, Sep 24, 2008 at 02:50:36PM -0400, Chuck Lever wrote:
>> statd and some servers always need lockd to listen on UDP, so always
>> start both lockd listener sockets.
>>
>> This probably leaks an svc_serv if the TCP listener can't be created,
>> but it will do for now.
>>
>> Signed-off-by: Chuck Lever <[email protected]>
>> ---
>>
>> I currently have this workaround lurking in my git repo. Is there
>> some kind
>> of fix for this issue planned for 2.6.28?
>
> Not that I know of.
>
>> This is only a reminder, not a suggested patch.
>
> Any chance you could generate a patch?

I can, but no guarantee it will be in time for the 2.6.28 merge window.

Would we consider this a regression fix? If so, we could post it
after the merge window closes. It would give a little more time to do
some testing.

>> fs/lockd/svc.c | 31 +++++++++++++------------------
>> 1 files changed, 13 insertions(+), 18 deletions(-)
>>
>> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
>> index a7b604c..36b6d03 100644
>> --- a/fs/lockd/svc.c
>> +++ b/fs/lockd/svc.c
>> @@ -213,25 +213,23 @@ lockd(void *vrqstp)
>> }
>>
>> /*
>> - * Make any sockets that are needed but not present.
>> - * If nlm_udpport or nlm_tcpport were set as module
>> - * options, make those sockets unconditionally
>> + * Create UDP and TCP listeners for lockd. Even if we only
>> + * have TCP NFS mounts and TCP NFSDs, some services (such
>> + * as rpc.statd) still require UDP.
>> */
>> -static int make_socks(struct svc_serv *serv, const unsigned short
>> proto)
>> +static int make_socks(struct svc_serv *serv)
>> {
>> static int warned;
>> struct svc_xprt *xprt;
>> int err = 0;
>>
>> - if (proto == IPPROTO_UDP || nlm_udpport) {
>> - xprt = svc_find_xprt(serv, "udp", 0, 0);
>> - if (!xprt)
>> - err = svc_create_xprt(serv, "udp", nlm_udpport,
>> - SVC_SOCK_DEFAULTS);
>> - else
>> - svc_xprt_put(xprt);
>> - }
>> - if (err >= 0 && (proto == IPPROTO_TCP || nlm_tcpport)) {
>> + xprt = svc_find_xprt(serv, "udp", 0, 0);
>> + if (!xprt)
>> + err = svc_create_xprt(serv, "udp", nlm_udpport,
>> + SVC_SOCK_DEFAULTS);
>> + else
>> + svc_xprt_put(xprt);
>> + if (err >= 0) {
>> xprt = svc_find_xprt(serv, "tcp", 0, 0);
>> if (!xprt)
>> err = svc_create_xprt(serv, "tcp", nlm_tcpport,
>> @@ -260,11 +258,8 @@ int lockd_up(const unsigned short proto)
>> /*
>> * Check whether we're already up and running.
>> */
>> - if (nlmsvc_rqst) {
>> - if (proto)
>> - error = make_socks(nlmsvc_rqst->rq_server, proto);
>> + if (nlmsvc_rqst)
>> goto out;
>> - }
>>
>> /*
>> * Sanity check: if there's no pid,
>> @@ -281,7 +276,7 @@ int lockd_up(const unsigned short proto)
>> goto out;
>> }
>>
>> - if ((error = make_socks(serv, proto)) < 0)
>> + if ((error = make_socks(serv)) < 0)
>> goto destroy_and_out;
>>
>> /*
>>

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com





2008-09-26 18:45:57

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] NLM: Revert parts of commit 24e36663

On Fri, Sep 26, 2008 at 01:53:58PM -0400, Chuck Lever wrote:
> On Sep 25, 2008, at Sep 25, 2008, 5:38 PM, J. Bruce Fields wrote:
>> On Wed, Sep 24, 2008 at 02:50:36PM -0400, Chuck Lever wrote:
>>> statd and some servers always need lockd to listen on UDP, so always
>>> start both lockd listener sockets.
>>>
>>> This probably leaks an svc_serv if the TCP listener can't be created,
>>> but it will do for now.
>>>
>>> Signed-off-by: Chuck Lever <[email protected]>
>>> ---
>>>
>>> I currently have this workaround lurking in my git repo. Is there
>>> some kind
>>> of fix for this issue planned for 2.6.28?
>>
>> Not that I know of.
>>
>>> This is only a reminder, not a suggested patch.
>>
>> Any chance you could generate a patch?
>
> I can, but no guarantee it will be in time for the 2.6.28 merge window.
>
> Would we consider this a regression fix?

I suppose so. (Are there any bug reports? If so, could we get URL's
for them into the commit message?)

> If so, we could post it after
> the merge window closes. It would give a little more time to do some
> testing.

That sound suspiciously like "since this is a high priority, it will be
accepted later, so we can make it a lower priority." Help!

But anyway, yes, we might have more time.

--b.

2008-09-26 18:50:04

by Myklebust, Trond

[permalink] [raw]
Subject: Re: [PATCH] NLM: Revert parts of commit 24e36663

On Fri, 2008-09-26 at 14:45 -0400, J. Bruce Fields wrote:
> > If so, we could post it after
> > the merge window closes. It would give a little more time to do some
> > testing.
>
> That sound suspiciously like "since this is a high priority, it will be
> accepted later, so we can make it a lower priority." Help!
>
> But anyway, yes, we might have more time.

At the Kernel Summit Linus said he wants this kind of fix to go into the
merge window rather than in the -rc series if possible. His position was
that he'd rather revert a bad fix instead of delaying the testing in the
mainline kernel...

Trond

--
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
http://www.netapp.com

2008-09-26 18:51:42

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] NLM: Revert parts of commit 24e36663

On Fri, Sep 26, 2008 at 02:49:35PM -0400, Trond Myklebust wrote:
> On Fri, 2008-09-26 at 14:45 -0400, J. Bruce Fields wrote:
> > > If so, we could post it after
> > > the merge window closes. It would give a little more time to do some
> > > testing.
> >
> > That sound suspiciously like "since this is a high priority, it will be
> > accepted later, so we can make it a lower priority." Help!
> >
> > But anyway, yes, we might have more time.
>
> At the Kernel Summit Linus said he wants this kind of fix to go into the
> merge window rather than in the -rc series if possible. His position was
> that he'd rather revert a bad fix instead of delaying the testing in the
> mainline kernel...

Makes sense to me.

--b.

2008-09-26 19:04:28

by Chuck Lever

[permalink] [raw]
Subject: Re: [PATCH] NLM: Revert parts of commit 24e36663

On Sep 26, 2008, at Sep 26, 2008, 2:49 PM, Trond Myklebust wrote:
> On Fri, 2008-09-26 at 14:45 -0400, J. Bruce Fields wrote:
>>> If so, we could post it after
>>> the merge window closes. It would give a little more time to do
>>> some
>>> testing.
>>
>> That sound suspiciously like "since this is a high priority, it
>> will be
>> accepted later, so we can make it a lower priority." Help!

No, more like "I have way too much to do in the next week because I'm
out two days, and am required to finish an ethics course before the
30th." I don't think there's much to do to get this in shape, I just
don't want to make promises I can't keep.

>> But anyway, yes, we might have more time.
>
> At the Kernel Summit Linus said he wants this kind of fix to go into
> the
> merge window rather than in the -rc series if possible. His position
> was
> that he'd rather revert a bad fix instead of delaying the testing in
> the
> mainline kernel...

So in light of Linus' stated position, it would be helpful for me (and
maybe others on the list) to have a good feel for when you guys are
planning to push your merge-window changes to Linus before each window
opens.

At the very least, since many of us do not follow LKML, would someone
commit to reflecting the LKML merge announcements to linux-nfs? Or is
there a kernel announcements list we can follow (or that we can
subscribe linux-nfs to)?

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

2008-09-26 19:11:33

by Myklebust, Trond

[permalink] [raw]
Subject: Re: [PATCH] NLM: Revert parts of commit 24e36663

On Fri, 2008-09-26 at 15:04 -0400, Chuck Lever wrote:
>
> So in light of Linus' stated position, it would be helpful for me (and
> maybe others on the list) to have a good feel for when you guys are
> planning to push your merge-window changes to Linus before each window
> opens.
>
> At the very least, since many of us do not follow LKML, would someone
> commit to reflecting the LKML merge announcements to linux-nfs? Or is
> there a kernel announcements list we can follow (or that we can
> subscribe linux-nfs to)?

There are no announcements. The merge window starts as soon as 2.6.27 is
released, and lasts for 2 weeks. There has been some talk about
shortening it to 1 week, but that hasn't been fully decided yet.

IOW: Assume as soon as you see the 2.6.27 announcement, that we're
preparing the merge.

--
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
http://www.netapp.com

2008-09-26 19:44:24

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] NLM: Revert parts of commit 24e36663

On Fri, Sep 26, 2008 at 03:11:30PM -0400, Trond Myklebust wrote:
> On Fri, 2008-09-26 at 15:04 -0400, Chuck Lever wrote:
> >
> > So in light of Linus' stated position, it would be helpful for me (and
> > maybe others on the list) to have a good feel for when you guys are
> > planning to push your merge-window changes to Linus before each window
> > opens.
> >
> > At the very least, since many of us do not follow LKML, would someone
> > commit to reflecting the LKML merge announcements to linux-nfs? Or is
> > there a kernel announcements list we can follow (or that we can
> > subscribe linux-nfs to)?
>
> There are no announcements. The merge window starts as soon as 2.6.27 is
> released, and lasts for 2 weeks. There has been some talk about
> shortening it to 1 week, but that hasn't been fully decided yet.
>
> IOW: Assume as soon as you see the 2.6.27 announcement, that we're
> preparing the merge.

Yeah. Unfortunately I don't know of a simple way to get just the
release announcements. I guess lwn.net's rss feed would get them with a
little less noise than lkml.

We should probably restart sending those "what's in my tree for 2.6.x"
messages with the tentative shortlog for the pull. I think I did it a
couple times and then got lazy....

--b.

2008-09-26 19:55:13

by Chuck Lever

[permalink] [raw]
Subject: Re: [PATCH] NLM: Revert parts of commit 24e36663

On Sep 26, 2008, at Sep 26, 2008, 3:44 PM, J. Bruce Fields wrote:
> On Fri, Sep 26, 2008 at 03:11:30PM -0400, Trond Myklebust wrote:
>> On Fri, 2008-09-26 at 15:04 -0400, Chuck Lever wrote:
>>>
>>> So in light of Linus' stated position, it would be helpful for me
>>> (and
>>> maybe others on the list) to have a good feel for when you guys are
>>> planning to push your merge-window changes to Linus before each
>>> window
>>> opens.
>>>
>>> At the very least, since many of us do not follow LKML, would
>>> someone
>>> commit to reflecting the LKML merge announcements to linux-nfs?
>>> Or is
>>> there a kernel announcements list we can follow (or that we can
>>> subscribe linux-nfs to)?
>>
>> There are no announcements. The merge window starts as soon as
>> 2.6.27 is
>> released, and lasts for 2 weeks. There has been some talk about
>> shortening it to 1 week, but that hasn't been fully decided yet.
>>
>> IOW: Assume as soon as you see the 2.6.27 announcement, that we're
>> preparing the merge.
>
> Yeah. Unfortunately I don't know of a simple way to get just the
> release announcements. I guess lwn.net's rss feed would get them
> with a
> little less noise than lkml.
>
> We should probably restart sending those "what's in my tree for 2.6.x"
> messages with the tentative shortlog for the pull. I think I did it a
> couple times and then got lazy....

The whole system makes it absolutely impossible to plan features and
bug fixes.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

2008-09-26 19:59:26

by Myklebust, Trond

[permalink] [raw]
Subject: Re: [PATCH] NLM: Revert parts of commit 24e36663

On Fri, 2008-09-26 at 15:54 -0400, Chuck Lever wrote:
> The whole system makes it absolutely impossible to plan features and
> bug fixes.

The only thing is makes impossible is planning features for a
_particular_ kernel revision. You shouldn't be doing that anyway...

--
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
http://www.netapp.com

2008-09-26 21:11:44

by Chuck Lever

[permalink] [raw]
Subject: Re: [PATCH] NLM: Revert parts of commit 24e36663

On Sep 26, 2008, at Sep 26, 2008, 3:57 PM, Trond Myklebust wrote:
> On Fri, 2008-09-26 at 15:54 -0400, Chuck Lever wrote:
>> The whole system makes it absolutely impossible to plan features and
>> bug fixes.
>
> The only thing it makes impossible is planning features for a
> _particular_ kernel revision.

I find it impossible to plan for a particular _year_ (or even two).

The problem is _you_, the subsystem maintainer, have a merge window
too. I find it challenging to align my patch submissions with two
sliding merge windows that open and close at arbitrary times without
any warning whatever. And now I have to worry about Bruce's merge
window as well....

Plus, the submission guidelines keep changing. FreeBSD has an amazing
guide to kernel development rules and conventions posted on the web.
I wish Linux had the same.

If I know in advance when you (and Bruce) are taking new features or
bug fixes for each kernel release, that would be helpful.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

2008-09-26 22:22:40

by Myklebust, Trond

[permalink] [raw]
Subject: Re: [PATCH] NLM: Revert parts of commit 24e36663

On Fri, 2008-09-26 at 17:11 -0400, Chuck Lever wrote:
> If I know in advance when you (and Bruce) are taking new features or
> bug fixes for each kernel release, that would be helpful.

That won't happen.

Send it when it is _ready_. That is the way everyone else works when
doing Linux kernel development...

If it needs to go into a particular kernel release, either because it is
a bugfix for a known problem, or because other patches depend upon it,
then make a special case for that.

--
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
http://www.netapp.com

2008-09-26 23:31:10

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] NLM: Revert parts of commit 24e36663

On Fri, Sep 26, 2008 at 05:11:25PM -0400, Chuck Lever wrote:
> On Sep 26, 2008, at Sep 26, 2008, 3:57 PM, Trond Myklebust wrote:
>> On Fri, 2008-09-26 at 15:54 -0400, Chuck Lever wrote:
>>> The whole system makes it absolutely impossible to plan features and
>>> bug fixes.
>>
>> The only thing it makes impossible is planning features for a
>> _particular_ kernel revision.
>
> I find it impossible to plan for a particular _year_ (or even two).
>
> The problem is _you_, the subsystem maintainer, have a merge window too.
> I find it challenging to align my patch submissions with two sliding
> merge windows that open and close at arbitrary times without any warning
> whatever. And now I have to worry about Bruce's merge window as well....

Please just send them in whenever they're ready--I mean to take patches
at any time, and then decide separately whether they should be queued up
for the current version, the next one, and/or the previous (stable)
version. There are inevitably delays (especially with vacations and
such), but normally I'll try to at least respond with an estimate if
it's going to take longer than a couple days.

> Plus, the submission guidelines keep changing. FreeBSD has an amazing
> guide to kernel development rules and conventions posted on the web. I
> wish Linux had the same.

Do you have a pointer to it, and/or suggestions how it could be used to
improve existing linux documentation?

There's Documentation/SubmittingPatches and stuff. My main gripe with
Documentation/ is that people contribute to it, but there's no
maintainer to say "no" or to keep things organized, so it tends to get
large and messy over time.

In any case lkml would be a better forum for this.

> If I know in advance when you (and Bruce) are taking new features or bug
> fixes for each kernel release, that would be helpful.

I'd like to say the answer is "any time", barring occasional
interruptions.

Or if the problem is that you've got stuff that you're basically done
with, but that you know you *might* get the chance to take one more look
at if you had another week--I think it'd be better if people just sent
stuff in when they think it's good enough, and if we later find a
problem we'll fix it.

--b.