From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752450AbcELJku (ORCPT ); Thu, 12 May 2016 05:40:50 -0400 Received: from metis.ext.4.pengutronix.de ([92.198.50.35]:33994 "EHLO metis.ext.4.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751512AbcELJks (ORCPT ); Thu, 12 May 2016 05:40:48 -0400 From: Markus Pargmann To: "Pranay Kr. Srivastava" Cc: nbd-general@lists.sourceforge.net, linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org Subject: Re: [PATCH v4 01/18] nbd: Fix might_sleep warning on xmit timeout Date: Thu, 12 May 2016 11:40:45 +0200 Message-ID: <2919757.NIRCIcnYi2@adelgunde> User-Agent: KMail/4.14.1 (Linux/4.5.0-0.bpo.1-amd64; KDE/4.14.2; x86_64; ; ) In-Reply-To: <1462954726-11825-2-git-send-email-pranjas@gmail.com> References: <1462954726-11825-1-git-send-email-pranjas@gmail.com> <1462954726-11825-2-git-send-email-pranjas@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart1942644.RaNOEFrAHm"; 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 --nextPart1942644.RaNOEFrAHm Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="us-ascii" Hi, On Wednesday 11 May 2016 11:18:29 Pranay Kr. Srivastava wrote: > This patch fixes the warning generated when a timeout occurs > on the request and socket is closed from a non-sleep context > by >=20 > 1. Moving the socket closing on a timeout to nbd_thread_send >=20 > 2. Make sock lock to be a mutex instead of a spin lock, since > nbd_xmit_timeout doesn't need to hold it anymore. This patch seems quite big and complicated. Isn't the main issue that a= socket shutdown is called from within a spinlock? When the issue got reported by Mikulas Patocka I created a patch but forgot to send it, sorry. It is a bit simpler by simpling moving the socket shutdown out of the spinlock. I will send it as reply. Please have a look. Thanks, Markus >=20 > Signed-off-by: Pranay Kr. Srivastava > --- > drivers/block/nbd.c | 65 ++++++++++++++++++++++++++++++++-----------= =2D--------- > 1 file changed, 39 insertions(+), 26 deletions(-) >=20 > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c > index 31e73a7..c79bcd7 100644 > --- a/drivers/block/nbd.c > +++ b/drivers/block/nbd.c > @@ -57,12 +57,12 @@ struct nbd_device { > =09int blksize; > =09loff_t bytesize; > =09int xmit_timeout; > -=09bool timedout; > +=09atomic_t timedout; > =09bool disconnect; /* a disconnect has been requested by user */ > =20 > =09struct timer_list timeout_timer; > =09/* protects initialization and shutdown of the socket */ > -=09spinlock_t sock_lock; > +=09struct mutex sock_lock; > =09struct task_struct *task_recv; > =09struct task_struct *task_send; > =20 > @@ -172,10 +172,9 @@ static void nbd_end_request(struct nbd_device *n= bd, struct request *req) > */ > static void sock_shutdown(struct nbd_device *nbd) > { > -=09spin_lock_irq(&nbd->sock_lock); > - > +=09mutex_lock(&nbd->sock_lock); > =09if (!nbd->sock) { > -=09=09spin_unlock_irq(&nbd->sock_lock); > +=09=09mutex_unlock(&nbd->sock_lock); > =09=09return; > =09} > =20 > @@ -183,27 +182,19 @@ static void sock_shutdown(struct nbd_device *nb= d) > =09kernel_sock_shutdown(nbd->sock, SHUT_RDWR); > =09sockfd_put(nbd->sock); > =09nbd->sock =3D NULL; > -=09spin_unlock_irq(&nbd->sock_lock); > - > +=09mutex_unlock(&nbd->sock_lock); > =09del_timer(&nbd->timeout_timer); > } > =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; > =20 > -=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); > +=09atomic_inc(&nbd->timedout); > +=09wake_up(&nbd->waiting_wq); > =20 > =09dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down con= nection\n"); > } > @@ -579,7 +570,27 @@ static int nbd_thread_send(void *data) > =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=09=09 !list_empty(&nbd->waiting_queue) || > +=09=09=09=09=09 atomic_read(&nbd->timedout)); > + > +=09=09if (atomic_read(&nbd->timedout)) { > +=09=09=09mutex_lock(&nbd->sock_lock); > +=09=09=09if (nbd->sock) { > +=09=09=09=09struct request sreq; > + > +=09=09=09=09blk_rq_init(NULL, &sreq); > +=09=09=09=09sreq.cmd_type =3D REQ_TYPE_DRV_PRIV; > +=09=09=09=09mutex_lock(&nbd->tx_lock); > +=09=09=09=09nbd->disconnect =3D true; > +=09=09=09=09nbd_send_req(nbd, &sreq); > +=09=09=09=09mutex_unlock(&nbd->tx_lock); > +=09=09=09=09dev_err(disk_to_dev(nbd->disk), > +=09=09=09=09=09"Device Timeout occured.Shutting down" > +=09=09=09=09=09" socket."); > +=09=09=09} > +=09=09=09mutex_unlock(&nbd->sock_lock); > +=09=09=09sock_shutdown(nbd); > +=09=09} > =20 > =09=09/* extract request */ > =09=09if (list_empty(&nbd->waiting_queue)) > @@ -592,7 +603,11 @@ static int nbd_thread_send(void *data) > =09=09spin_unlock_irq(&nbd->queue_lock); > =20 > =09=09/* handle request */ > -=09=09nbd_handle_req(nbd, req); > +=09=09if (atomic_read(&nbd->timedout)) { > +=09=09=09req->errors++; > +=09=09=09nbd_end_request(nbd, req); > +=09=09} else > +=09=09=09nbd_handle_req(nbd, req); > =09} > =20 > =09nbd->task_send =3D NULL; > @@ -647,7 +662,7 @@ static int nbd_set_socket(struct nbd_device *nbd,= struct socket *sock) > { > =09int ret =3D 0; > =20 > -=09spin_lock_irq(&nbd->sock_lock); > +=09mutex_lock(&nbd->sock_lock); > =20 > =09if (nbd->sock) { > =09=09ret =3D -EBUSY; > @@ -657,7 +672,7 @@ static int nbd_set_socket(struct nbd_device *nbd,= struct socket *sock) > =09nbd->sock =3D sock; > =20 > out: > -=09spin_unlock_irq(&nbd->sock_lock); > +=09mutex_unlock(&nbd->sock_lock); > =20 > =09return ret; > } > @@ -666,7 +681,7 @@ out: > static void nbd_reset(struct nbd_device *nbd) > { > =09nbd->disconnect =3D false; > -=09nbd->timedout =3D false; > +=09atomic_set(&nbd->timedout, 0); > =09nbd->blksize =3D 1024; > =09nbd->bytesize =3D 0; > =09set_capacity(nbd->disk, 0); > @@ -803,17 +818,15 @@ 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=09mutex_lock(&nbd->tx_lock); > - > =09=09sock_shutdown(nbd); > +=09=09mutex_lock(&nbd->tx_lock); > =09=09nbd_clear_que(nbd); > =09=09kill_bdev(bdev); > =09=09nbd_bdev_reset(bdev); > =20 > =09=09if (nbd->disconnect) /* user requested, ignore socket errors *= / > =09=09=09error =3D 0; > -=09=09if (nbd->timedout) > +=09=09if (atomic_read(&nbd->timedout)) > =09=09=09error =3D -ETIMEDOUT; > =20 > =09=09nbd_reset(nbd); > @@ -1075,7 +1088,7 @@ static int __init nbd_init(void) > =09=09nbd_dev[i].magic =3D NBD_MAGIC; > =09=09INIT_LIST_HEAD(&nbd_dev[i].waiting_queue); > =09=09spin_lock_init(&nbd_dev[i].queue_lock); > -=09=09spin_lock_init(&nbd_dev[i].sock_lock); > +=09=09mutex_init(&nbd_dev[i].sock_lock); > =09=09INIT_LIST_HEAD(&nbd_dev[i].queue_head); > =09=09mutex_init(&nbd_dev[i].tx_lock); > =09=09init_timer(&nbd_dev[i].timeout_timer); >=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 | --nextPart1942644.RaNOEFrAHm 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 iQIcBAABCAAGBQJXNE+dAAoJEEpcgKtcEGQQuKAP/ReeBHp8FnvZXqAmTfLrKuLG yWObFHYPTNWRo8sQjFQFV/XGeb5DRY2GTi7LhotHpeekZh6IdUKIMI/scDnRokrb 2kaZQhni+u+0VZaW+S05lhmjNrg/d2cNkJzVTh8Q4hKeDtzTpOSCUNaUU8wqdIEk E6PAc/sFvWjdG4lA9OHJfVWGzv2OvxZiTrnOYKflbDOF4imWjyEOxWwtBbv8mUJw mNIUZb7qDfc8FOJbRfdcyiVbT/Qm9yUbsQV5ywkR8dh8+h4yzzIwpOEgfvXD/j6k 9m0foX161NzCXZ0SURypYdl7/LTr8Zvd3iCXV/qAbne4d0uE9wmO024K4iUXEIDc SAzRwGQ3Pm7ZymvNNXRm88oP1jz/bcx0FalPtPZrb793KoTbGtmqPCAlmCupCWBt YMrVsBQfJbeLRwLMZMOjydKA2/nw+9qOO4LC922bvKc/YWzYjypxXtOowq+Vlvy8 e+Ir482L5dL9Yp9HV7ifYCMPW64r753MDQrEcLJRFhjOfGsyqcUZcZb6iH6tuyvY eq/7AXXCd2Sw76x5ujSwibCKBzizjHK8Z/5shroAMk2AI1KWnoa/r0hEo8Y4BuCl 2V/KUcDAC9QX5jQUgdlzKTgg/liXUVxek/d/nw+KgsmlUBiIVoa8qXsRvnUzY0Yq DHH1qgzdL9oeP3rA097z =OFlq -----END PGP SIGNATURE----- --nextPart1942644.RaNOEFrAHm--