Re: [Nbd] [PATCH] nbd: Move socket shutdown out of spinlock

[Pranay Srivastava <pranjas@...17...>]

On Thu, May 12, 2016 at 6:13 PM, Markus Pargmann <mpa@...1897...> 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 <mpa@...1897...> 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 <mikulas@...2380...>
>> > Signed-off-by: Markus Pargmann <mpa@...1897...>
>> > ---
>> >  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