On Thu, May 12, 2016 at 6:13 PM, Markus Pargmann <[email protected]> wrote: > Hi, > > On Thursday 12 May 2016 16:42:31 Pranay Srivastava wrote: >> Hi Markus, >> >> >> On Thu, May 12, 2016 at 3:13 PM, Markus Pargmann <[email protected]> wrote: >> > spinlocked ranges should be small and not contain calls into huge >> > subfunctions. Fix my mistake and just get the pointer to the socket >> > instead of doing everything with spinlock held. >> > >> > Reported-by: Mikulas Patocka <[email protected]> >> > Signed-off-by: Markus Pargmann <[email protected]> >> > --- >> > drivers/block/nbd.c | 18 ++++++++++-------- >> > 1 file changed, 10 insertions(+), 8 deletions(-) >> > >> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c >> > index 0b892eed06a0..157bf3da876e 100644 >> > --- a/drivers/block/nbd.c >> > +++ b/drivers/block/nbd.c >> > @@ -173,20 +173,22 @@ static void nbd_end_request(struct nbd_device *nbd, >> > struct request *req) >> > */ >> > static void sock_shutdown(struct nbd_device *nbd) >> > { >> > + struct socket *sock; >> > + >> > spin_lock_irq(&nbd->sock_lock); >> > + sock = nbd->sock; >> > + nbd->sock = NULL; >> > + spin_unlock_irq(&nbd->sock_lock); >> > >> > - if (!nbd->sock) { >> > - spin_unlock_irq(&nbd->sock_lock); >> > + if (!sock) >> > return; >> > - } >> > + >> > + del_timer(&nbd->timeout_timer); >> > >> > dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n"); >> > - kernel_sock_shutdown(nbd->sock, SHUT_RDWR); >> > - sockfd_put(nbd->sock); >> > - nbd->sock = NULL; >> > - spin_unlock_irq(&nbd->sock_lock); >> > + kernel_sock_shutdown(sock, SHUT_RDWR); >> > + sockfd_put(sock); >> > >> > - del_timer(&nbd->timeout_timer); >> > } >> > >> > static void nbd_xmit_timeout(unsigned long arg) >> >> I was concerned about nbd_xmit_timeout as well. There's also a call to >> kernel_sock_shutdown, >> while holding the spin_lock in the timeout. The above is ok for >> sock_shutdown but some kind of change >> is also required in nbd_xmit_timeout as well. My patch addressed both these. > > Oh I see thanks. Seems there is some duplicate code in > nbd_xmit_timeout and sock_shutdown. I think nbd_xmit_timeout could > perhaps be simplified? > > static void nbd_xmit_timeout(unsigned long arg) > { > struct nbd_device *nbd = (struct nbd_device *)arg; > > if (list_empty(&nbd->queue_head)) > return; > > nbd->timedout = true; > dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down > connection\n"); > > sock_shutdown(nbd);
Even then, the timeout is non-process context so there will still be a might_sleep warning. So sock_shutdown itself can be simplified, as you posted in earlier patch, but I don't think sock_shutdown should be called from non-process context[?]. > } > > Thanks, > > Markus > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -- ---P.K.S
