From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752621AbcF2HSa (ORCPT ); Wed, 29 Jun 2016 03:18:30 -0400 Received: from metis.ext.4.pengutronix.de ([92.198.50.35]:53577 "EHLO metis.ext.4.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750790AbcF2HS3 (ORCPT ); Wed, 29 Jun 2016 03:18:29 -0400 From: Markus Pargmann To: "Pranay Kr. Srivastava" Cc: nbd-general@lists.sourceforge.net, linux-kernel@vger.kernel.org, w@uter.be Subject: Re: [PATCH v3 1/3]nbd: fix might_sleep warning on socket shutdown Date: Wed, 29 Jun 2016 09:18:26 +0200 Message-ID: <10712985.x0lCjeaRAP@adelgunde> User-Agent: KMail/4.14.1 (Linux/4.6.0-0.bpo.1-amd64; KDE/4.14.2; x86_64; ; ) In-Reply-To: <1466762976-12648-2-git-send-email-pranjas@gmail.com> References: <1962682.lYRJ5o9hTF@adelgunde> <1466762976-12648-1-git-send-email-pranjas@gmail.com> <1466762976-12648-2-git-send-email-pranjas@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart20963389.vGeMDK8irG"; 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 --nextPart20963389.vGeMDK8irG Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="us-ascii" On Friday 24 June 2016 13:09:34 Pranay Kr. Srivastava 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. >=20 > Reported-by: Mikulas Patocka > Signed-off-by: Markus Pargmann >=20 > Changelog: > Pranay Kr. Srivastava: >=20 > 1) Use spin_lock instead of irq version for sock_shutdown. >=20 > 2) Use system work queue to actually trigger the shutdown of > socket. This solves the issue when kernel_sendmsg is currently > blocked while a timeout occurs. >=20 > Signed-off-by: Pranay Kr. Srivastava > --- > drivers/block/nbd.c | 69 ++++++++++++++++++++++++++++++++++---------= =2D--------- > 1 file changed, 44 insertions(+), 25 deletions(-) >=20 > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c > index 56f7f5d..586d946 100644 > --- a/drivers/block/nbd.c > +++ b/drivers/block/nbd.c > @@ -39,6 +39,7 @@ > #include > =20 > #include > +#include > =20 > struct nbd_device { > =09u32 flags; > @@ -69,6 +70,10 @@ struct nbd_device { > #if IS_ENABLED(CONFIG_DEBUG_FS) > =09struct dentry *dbg_dir; > #endif > +=09/* > +=09*This is specifically for calling sock_shutdown, for now. > +=09*/ Please fix the indentation of this comment. > +=09struct work_struct ws_shutdown; > }; > =20 > #if IS_ENABLED(CONFIG_DEBUG_FS) > @@ -95,6 +100,11 @@ static int max_part; > */ > static DEFINE_SPINLOCK(nbd_lock); > =20 > +/* > + * Shutdown function for nbd_dev work struct. > + */ > +static void nbd_ws_func_shutdown(struct work_struct *); You could as well put the function implementation here. No need for a function signature. > + > static inline struct device *nbd_to_dev(struct nbd_device *nbd) > { > =09return disk_to_dev(nbd->disk); > @@ -172,39 +182,35 @@ static void nbd_end_request(struct nbd_device *= nbd, struct request *req) > */ > static void sock_shutdown(struct nbd_device *nbd) > { > -=09spin_lock_irq(&nbd->sock_lock); > - > -=09if (!nbd->sock) { > -=09=09spin_unlock_irq(&nbd->sock_lock); > -=09=09return; > -=09} > +=09struct socket *sock; > =20 > -=09dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n"); > -=09kernel_sock_shutdown(nbd->sock, SHUT_RDWR); > -=09sockfd_put(nbd->sock); > +=09spin_lock(&nbd->sock_lock); > +=09sock =3D nbd->sock; > =09nbd->sock =3D NULL; > -=09spin_unlock_irq(&nbd->sock_lock); > +=09spin_unlock(&nbd->sock_lock); > + > +=09if (!sock) > +=09=09return; > =20 > =09del_timer(&nbd->timeout_timer); > +=09dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n"); > +=09kernel_sock_shutdown(sock, SHUT_RDWR); > +=09sockfd_put(sock); > } > =20 > static void nbd_xmit_timeout(unsigned long arg) > { > =09struct nbd_device *nbd =3D (struct nbd_device *)arg; > -=09unsigned long flags; > =20 > =09if (list_empty(&nbd->queue_head)) > =09=09return; > - > -=09spin_lock_irqsave(&nbd->sock_lock, flags); > - > =09nbd->timedout =3D true; > - > -=09if (nbd->sock) > -=09=09kernel_sock_shutdown(nbd->sock, SHUT_RDWR); > - > -=09spin_unlock_irqrestore(&nbd->sock_lock, flags); > - > +=09schedule_work(&nbd->ws_shutdown); > +=09/* > +=09 * Make sure sender thread sees nbd->timedout. > +=09 */ > +=09smp_wmb(); > +=09wake_up(&nbd->waiting_wq); > =09dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down con= nection\n"); > } > =20 > @@ -574,8 +580,8 @@ static int nbd_thread_send(void *data) > =09while (!kthread_should_stop() || !list_empty(&nbd->waiting_queue)= ) { > =09=09/* wait for something to do */ > =09=09wait_event_interruptible(nbd->waiting_wq, > -=09=09=09=09=09 kthread_should_stop() || > -=09=09=09=09=09 !list_empty(&nbd->waiting_queue)); > +=09=09=09=09kthread_should_stop() || > +=09=09=09=09!list_empty(&nbd->waiting_queue)); This is unrelated, please remove. > =20 > =09=09/* extract request */ > =09=09if (list_empty(&nbd->waiting_queue)) > @@ -583,12 +589,16 @@ static int nbd_thread_send(void *data) > =20 > =09=09spin_lock_irq(&nbd->queue_lock); > =09=09req =3D list_entry(nbd->waiting_queue.next, struct request, > -=09=09=09=09 queuelist); > +=09=09=09=09queuelist); Unrelated as well. > =09=09list_del_init(&req->queuelist); > =09=09spin_unlock_irq(&nbd->queue_lock); > =20 > -=09=09/* handle request */ Unrelated. > =09=09nbd_handle_req(nbd, req); > +=09=09if (nbd->timedout) { > +=09=09=09req->errors++; > +=09=09=09nbd_end_request(nbd, req); > +=09=09} else > +=09=09=09nbd_handle_req(nbd, req); This does not change anything to avoid the spinlock sock_shutdown issue= . Please split in a separate patch. Also this is already handled in nbd_handle_req(). For !nbd->sock the same code is executed. Otherwise the patch looks good. Best Regards, Markus > =09} > =20 > =09nbd->task_send =3D NULL; > @@ -668,6 +678,7 @@ static void nbd_reset(struct nbd_device *nbd) > =09set_capacity(nbd->disk, 0); > =09nbd->flags =3D 0; > =09nbd->xmit_timeout =3D 0; > +=09INIT_WORK(&nbd->ws_shutdown, nbd_ws_func_shutdown); > =09queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue); > =09del_timer_sync(&nbd->timeout_timer); > } > @@ -802,11 +813,11 @@ static int __nbd_ioctl(struct block_device *bde= v, struct nbd_device *nbd, > =09=09error =3D nbd_thread_recv(nbd, bdev); > =09=09nbd_dev_dbg_close(nbd); > =09=09kthread_stop(thread); > +=09=09sock_shutdown(nbd); > =20 > =09=09mutex_lock(&nbd->tx_lock); > =09=09nbd->task_recv =3D NULL; > =20 > -=09=09sock_shutdown(nbd); > =09=09nbd_clear_que(nbd); > =09=09kill_bdev(bdev); > =09=09nbd_bdev_reset(bdev); > @@ -862,6 +873,14 @@ static const struct block_device_operations nbd_= fops =3D { > =09.compat_ioctl =3D=09nbd_ioctl, > }; > =20 > +static void nbd_ws_func_shutdown(struct work_struct *ws_nbd) > +{ > +=09struct nbd_device *nbd_dev =3D container_of(ws_nbd, struct nbd_de= vice, > +=09=09=09ws_shutdown); > + > +=09sock_shutdown(nbd_dev); > +} > + > #if IS_ENABLED(CONFIG_DEBUG_FS) > =20 > static int nbd_dbg_tasks_show(struct seq_file *s, void *unused) >=20 =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 | --nextPart20963389.vGeMDK8irG 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 iQIcBAABCAAGBQJXc3ZCAAoJENnm3voMNZulFBsP/2Yf/Nlh5f9miY/DqxMYsd1Y irDdQ5XKuAMCaxNCYUl27eDjAWXv2v3H1eekSn+A9e33y33ET2eV4rWvumcPX1sE B3bu2RXs41wRpxCtbCDQ4RYWFfT5jKet+j9VTWGC4NGiaNMeZwn8wjFAv8BmhVKv McHs+TPph4GNt3xbA81g/vpqdBv0dz7x3FT9fh3lCtlweck3ThYu2eJ2eKRGL4ox vl+XMiT5b1fRpmIPDRo5osp0DhUkpl9RhHq3MiZV0gwFdEWhHrKc47YEbLXU7tBf 9s8An2CiCa0IqgFn2Zd0yGIVguKSxxAM/U+S/9F5Jeha5yfBf8HaFZdQSq2SZeFM DqY4AQRi2PWFi93Hnw1USaJKT0VQJru1mZmy1NAlWFVVRKAHPnFY1AfnF8dsKkT0 0W1k9lLNQYpvzcaN0Hq1que4rjN9Mj8LKV2W6CbtBJqIOF4KvFbNdZF4cITOfxT9 tt2maQhyb2im4MnLHhCdh4Y0R9w3fpjNR0lWVt1L32vxBtFldSBpoemZQ79cU+Qa ahri/BaGo3/DObSmOl7N8wBJ3Sf475FbOf28D8KPtNmR6vwCKz0oROcgb21Nksfg 7AUNLrpSNf2yuB/uGx5YIYjyS3I2s2YglDxq1B87zYvyhoK4yN21rYCADZwY7pXu L9Dqt967twqtggDaTt1z =+89z -----END PGP SIGNATURE----- --nextPart20963389.vGeMDK8irG--