From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752990AbcELMn3 (ORCPT ); Thu, 12 May 2016 08:43:29 -0400 Received: from metis.ext.4.pengutronix.de ([92.198.50.35]:38520 "EHLO metis.ext.4.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752691AbcELMn1 (ORCPT ); Thu, 12 May 2016 08:43:27 -0400 From: Markus Pargmann To: Pranay Srivastava Cc: nbd-general@lists.sourceforge.net, linux-kernel@vger.kernel.org, Greg KH Subject: Re: [PATCH] nbd: Move socket shutdown out of spinlock Date: Thu, 12 May 2016 14:43:17 +0200 Message-ID: <81303581.gVSuzjqYUx@adelgunde> User-Agent: KMail/4.14.1 (Linux/4.5.0-0.bpo.1-amd64; KDE/4.14.2; x86_64; ; ) In-Reply-To: References: <2919757.NIRCIcnYi2@adelgunde> <1463046195-30536-1-git-send-email-mpa@pengutronix.de> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart1835594.uZUb4u4LcQ"; micalg="pgp-sha256"; protocol="application/pgp-signature" X-SA-Exim-Connect-IP: 2001:67c:670:100:a61f:72ff:fe68:75ba X-SA-Exim-Mail-From: mpa@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --nextPart1835594.uZUb4u4LcQ Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="us-ascii" Hi, On Thursday 12 May 2016 16:42:31 Pranay Srivastava wrote: > Hi Markus, >=20 >=20 > On Thu, May 12, 2016 at 3:13 PM, Markus Pargmann = 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 > > Signed-off-by: Markus Pargmann > > --- > > 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 =3D nbd->sock; > > + nbd->sock =3D 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 =3D 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) >=20 > I was concerned about nbd_xmit_timeout as well. There's also a call t= o > 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? =09static void nbd_xmit_timeout(unsigned long arg) =09{ =09=09struct nbd_device *nbd =3D (struct nbd_device *)arg; =09=09if (list_empty(&nbd->queue_head)) =09=09=09return; =09=09nbd->timedout =3D true; =09=09dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down con= nection\n"); =09=09sock_shutdown(nbd); =09} Thanks, Markus =2D-=20 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-555= 5 | --nextPart1835594.uZUb4u4LcQ Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJXNHprAAoJEEpcgKtcEGQQ9qUP/3uq9RBn/VjKMEApdTTGwJIU dzy59ErxX7juBAXPwJ+ExEyVs9jtFMgsRZfJ8477z3ScQfryaLwugIuNBUCGcMVH Gt0ytK/arfvPUP2xFQwQLZbSyzhjL9ZpIh9PKThyPNtpnfhaLz2KeSQXM3K9+9a4 7LuHXyZq5aQ+NA2/jsmCHOX4sha0Ttwj6BxAk6FitBvriLGOX8sbzBmmOD49HqdD 0YmJ/VlLRheveov5oBokfqASFMhvM6GPpsyDz2N3Q3zKikQKB4c+LwuaeHHFqgjL 9vhgv3kUsil4SXjJxiT5MhfAwavm2ZDwYhgA+b7uIvk0GwBd6eABGDus0JQTr4Pf jthMNIxIQOCjWqGCiVvhPozhWw5OBie0nglghDKunOFDboYv23nPLc5XM9sAuPDj 0sj0u9E70zks8Bo+GruPc3B3VAIzDFRzCXXYRFenfrWWMHVYt0ovsuIkjl16Ep9n kP1+RGOPFnd3KYt4Br4z9EkQg8Y8u8iE0PyOoZiRK/wHnj/QmLUUaCqcv2G9VK3e D7y7idUNHu2tngFEOkhybavA3GjAE3qFNOpn85wTsoPSh6N723Av3i5LT4eC2h7t QTHBRlZz30o5qz0wRkiF7kroKxc6L6BtYffRxPu4vLZFcgJ6BA3xN8GCHvkpEIGN Fv6MBsaJPOnePE1mk7eG =ZZVB -----END PGP SIGNATURE----- --nextPart1835594.uZUb4u4LcQ--