Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1399839 > unrolled thread
| Started by | Markus Pargmann <mpa@pengutronix.de> |
|---|---|
| First post | 2016-05-12 11:50 +0200 |
| Last post | 2016-05-12 17:10 +0200 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH] nbd: Move socket shutdown out of spinlock Markus Pargmann <mpa@pengutronix.de> - 2016-05-12 11:50 +0200
Re: [PATCH] nbd: Move socket shutdown out of spinlock Pranay Srivastava <pranjas@gmail.com> - 2016-05-12 13:20 +0200
Re: [PATCH] nbd: Move socket shutdown out of spinlock Markus Pargmann <mpa@pengutronix.de> - 2016-05-12 14:50 +0200
Re: [PATCH] nbd: Move socket shutdown out of spinlock Pranay Srivastava <pranjas@gmail.com> - 2016-05-12 17:10 +0200
| From | Markus Pargmann <mpa@pengutronix.de> |
|---|---|
| Date | 2016-05-12 11:50 +0200 |
| Subject | [PATCH] nbd: Move socket shutdown out of spinlock |
| Message-ID | <rxVpN-2Qy-19@gated-at.bofh.it> |
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@twibright.com>
Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
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)
--
2.8.0.rc3
[toc] | [next] | [standalone]
| From | Pranay Srivastava <pranjas@gmail.com> |
|---|---|
| Date | 2016-05-12 13:20 +0200 |
| Message-ID | <rxWOS-4u5-25@gated-at.bofh.it> |
| In reply to | #1399839 |
Hi Markus,
On Thu, May 12, 2016 at 3:13 PM, Markus Pargmann <mpa@pengutronix.de> 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@twibright.com>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
> 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.
Can you have a look at that again.
> --
> 2.8.0.rc3
>
--
---P.K.S
[toc] | [prev] | [next] | [standalone]
| From | Markus Pargmann <mpa@pengutronix.de> |
|---|---|
| Date | 2016-05-12 14:50 +0200 |
| Message-ID | <rxYdY-5WB-13@gated-at.bofh.it> |
| In reply to | #1399930 |
[Multipart message — attachments visible in raw view] — view raw
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@pengutronix.de> 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@twibright.com>
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > ---
> > 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);
}
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 |
[toc] | [prev] | [next] | [standalone]
| From | Pranay Srivastava <pranjas@gmail.com> |
|---|---|
| Date | 2016-05-12 17:10 +0200 |
| Message-ID | <ry0ps-8tw-15@gated-at.bofh.it> |
| In reply to | #1400036 |
On Thu, May 12, 2016 at 6:13 PM, Markus Pargmann <mpa@pengutronix.de> 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@pengutronix.de> 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@twibright.com>
>> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
>> > ---
>> > 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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web