On Mon, Aug 18, 2014 at 12:41:24AM +0800, kbuild test robot wrote:
> tree: git://linux-nfs.org/~bfields/linux.git nfsd-next
> head: da8b8f7602ed3b8105c6247a2844455fec72caaa
> commit: 983c684466e02b21f83c025ea539deee6c0aeac0 [5/23] SUNRPC: get rid of the request wait queue
> config: make ARCH=xtensa allyesconfig
>
> All error/warnings:
>
> net/sunrpc/svc_xprt.c: In function 'svc_recv':
> >> net/sunrpc/svc_xprt.c:794:20: error: 'current_task' undeclared (first use in this function)
> rqstp->rq_task = current_task;
> ^
> net/sunrpc/svc_xprt.c:794:20: note: each undeclared identifier is reported only once for each function it appears in
>
> vim +/current_task +794 net/sunrpc/svc_xprt.c
>
> 788 printk(KERN_ERR
> 789 "svc_recv: service %p, transport not NULL!\n",
> 790 rqstp);
> 791
> 792 /* Make sure the task pointer is set! */
> 793 if (WARN_ON_ONCE(!rqstp->rq_task))
> > 794 rqstp->rq_task = current_task;
I appreciate the effort to fail gracefully here, but maybe this would be
a case for simple BUG_ON()?
I don't think there are normally any important locks held here, and I
can't see any other reason why killing a server thread would bring a box
down.
And the first developer to forget to set rq_task on a newly created
server will get an immediate bug, so this should be obvious enough.
Unless there's fear of a race between starting the thread and setting
rq_task?
--b.
> 795
> 796 err = svc_alloc_arg(rqstp);
> 797 if (err)
>
> ---
> 0-DAY kernel build testing backend Open Source Technology Center
> http://lists.01.org/mailman/listinfo/kbuild Intel Corporation
On Mon, Aug 18, 2014 at 12:18 PM, J. Bruce Fields <[email protected]> wrote:
> On Mon, Aug 18, 2014 at 12:41:24AM +0800, kbuild test robot wrote:
>> tree: git://linux-nfs.org/~bfields/linux.git nfsd-next
>> head: da8b8f7602ed3b8105c6247a2844455fec72caaa
>> commit: 983c684466e02b21f83c025ea539deee6c0aeac0 [5/23] SUNRPC: get rid of the request wait queue
>> config: make ARCH=xtensa allyesconfig
>>
>> All error/warnings:
>>
>> net/sunrpc/svc_xprt.c: In function 'svc_recv':
>> >> net/sunrpc/svc_xprt.c:794:20: error: 'current_task' undeclared (first use in this function)
>> rqstp->rq_task = current_task;
>> ^
>> net/sunrpc/svc_xprt.c:794:20: note: each undeclared identifier is reported only once for each function it appears in
>>
>> vim +/current_task +794 net/sunrpc/svc_xprt.c
>>
>> 788 printk(KERN_ERR
>> 789 "svc_recv: service %p, transport not NULL!\n",
>> 790 rqstp);
>> 791
>> 792 /* Make sure the task pointer is set! */
>> 793 if (WARN_ON_ONCE(!rqstp->rq_task))
>> > 794 rqstp->rq_task = current_task;
>
> I appreciate the effort to fail gracefully here, but maybe this would be
> a case for simple BUG_ON()?
>
> I don't think there are normally any important locks held here, and I
> can't see any other reason why killing a server thread would bring a box
> down.
>
> And the first developer to forget to set rq_task on a newly created
> server will get an immediate bug, so this should be obvious enough.
> Unless there's fear of a race between starting the thread and setting
> rq_task?
>
No, there is no fear of a race. I'm fine with removing the above check
altogether and just letting code Oops if it turns out that we add new
cases that don't set rq_task.
Ultimately, though, I think we need a better interface for starting
RPC server threads; something that combines svc_prepare_thread() and
kthread_run() into a single function call and that can thus set
rqstp->rq_task correctly.
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]
On Wed, Aug 20, 2014 at 06:33:06PM -0400, Trond Myklebust wrote:
> On Mon, Aug 18, 2014 at 12:18 PM, J. Bruce Fields <[email protected]> wrote:
> > On Mon, Aug 18, 2014 at 12:41:24AM +0800, kbuild test robot wrote:
> >> tree: git://linux-nfs.org/~bfields/linux.git nfsd-next
> >> head: da8b8f7602ed3b8105c6247a2844455fec72caaa
> >> commit: 983c684466e02b21f83c025ea539deee6c0aeac0 [5/23] SUNRPC: get rid of the request wait queue
> >> config: make ARCH=xtensa allyesconfig
> >>
> >> All error/warnings:
> >>
> >> net/sunrpc/svc_xprt.c: In function 'svc_recv':
> >> >> net/sunrpc/svc_xprt.c:794:20: error: 'current_task' undeclared (first use in this function)
> >> rqstp->rq_task = current_task;
> >> ^
> >> net/sunrpc/svc_xprt.c:794:20: note: each undeclared identifier is reported only once for each function it appears in
> >>
> >> vim +/current_task +794 net/sunrpc/svc_xprt.c
> >>
> >> 788 printk(KERN_ERR
> >> 789 "svc_recv: service %p, transport not NULL!\n",
> >> 790 rqstp);
> >> 791
> >> 792 /* Make sure the task pointer is set! */
> >> 793 if (WARN_ON_ONCE(!rqstp->rq_task))
> >> > 794 rqstp->rq_task = current_task;
> >
> > I appreciate the effort to fail gracefully here, but maybe this would be
> > a case for simple BUG_ON()?
> >
> > I don't think there are normally any important locks held here, and I
> > can't see any other reason why killing a server thread would bring a box
> > down.
> >
> > And the first developer to forget to set rq_task on a newly created
> > server will get an immediate bug, so this should be obvious enough.
> > Unless there's fear of a race between starting the thread and setting
> > rq_task?
>
> No, there is no fear of a race. I'm fine with removing the above check
> altogether and just letting code Oops if it turns out that we add new
> cases that don't set rq_task.
OK, I'll remove it entirely.
> Ultimately, though, I think we need a better interface for starting
> RPC server threads; something that combines svc_prepare_thread() and
> kthread_run() into a single function call and that can thus set
> rqstp->rq_task correctly.
Sounds good.
--b.