From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753641AbcESGW3 (ORCPT ); Thu, 19 May 2016 02:22:29 -0400 Received: from metis.ext.4.pengutronix.de ([92.198.50.35]:46444 "EHLO metis.ext.4.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752153AbcESGWR (ORCPT ); Thu, 19 May 2016 02:22:17 -0400 Date: Thu, 19 May 2016 08:22:13 +0200 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 Message-ID: <20160519062213.GD19642@pengutronix.de> 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; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="1sNVjLsmu1MXqwQ/" Content-Disposition: inline In-Reply-To: <1462954726-11825-2-git-send-email-pranjas@gmail.com> X-Sent-From: Pengutronix Hildesheim X-URL: http://www.pengutronix.de/ X-IRC: #ptxdist @freenode X-Accept-Language: de,en X-Accept-Content-Type: text/plain X-Uptime: 08:06:48 up 18 days, 15:08, 112 users, load average: 0.83, 0.64, 0.47 User-Agent: Mutt/1.5.24 (2015-08-30) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::7 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 --1sNVjLsmu1MXqwQ/ Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Wed, May 11, 2016 at 11:18:29AM +0300, 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 What happens if a send blocks? >=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. I can't see why we need a mutex instead of a spinlock? >=20 > Signed-off-by: Pranay Kr. Srivastava > --- > drivers/block/nbd.c | 65 ++++++++++++++++++++++++++++++++---------------= ------ > 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 { > int blksize; > loff_t bytesize; > int xmit_timeout; > - bool timedout; > + atomic_t timedout; > bool disconnect; /* a disconnect has been requested by user */ > =20 > struct timer_list timeout_timer; > /* protects initialization and shutdown of the socket */ > - spinlock_t sock_lock; > + struct mutex sock_lock; > struct task_struct *task_recv; > struct task_struct *task_send; > =20 > @@ -172,10 +172,9 @@ static void nbd_end_request(struct nbd_device *nbd, = struct request *req) > */ > static void sock_shutdown(struct nbd_device *nbd) > { > - spin_lock_irq(&nbd->sock_lock); > - > + mutex_lock(&nbd->sock_lock); > if (!nbd->sock) { > - spin_unlock_irq(&nbd->sock_lock); > + mutex_unlock(&nbd->sock_lock); > return; > } > =20 > @@ -183,27 +182,19 @@ static void sock_shutdown(struct nbd_device *nbd) > kernel_sock_shutdown(nbd->sock, SHUT_RDWR); > sockfd_put(nbd->sock); > nbd->sock =3D NULL; > - spin_unlock_irq(&nbd->sock_lock); > - > + mutex_unlock(&nbd->sock_lock); > del_timer(&nbd->timeout_timer); > } > =20 > static void nbd_xmit_timeout(unsigned long arg) > { > struct nbd_device *nbd =3D (struct nbd_device *)arg; > - unsigned long flags; > =20 > if (list_empty(&nbd->queue_head)) > return; > =20 > - spin_lock_irqsave(&nbd->sock_lock, flags); > - > - nbd->timedout =3D true; > - > - if (nbd->sock) > - kernel_sock_shutdown(nbd->sock, SHUT_RDWR); > - > - spin_unlock_irqrestore(&nbd->sock_lock, flags); > + atomic_inc(&nbd->timedout); > + wake_up(&nbd->waiting_wq); > =20 > dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connectio= n\n"); > } > @@ -579,7 +570,27 @@ static int nbd_thread_send(void *data) > /* wait for something to do */ > wait_event_interruptible(nbd->waiting_wq, > kthread_should_stop() || > - !list_empty(&nbd->waiting_queue)); > + !list_empty(&nbd->waiting_queue) || > + atomic_read(&nbd->timedout)); > + > + if (atomic_read(&nbd->timedout)) { > + mutex_lock(&nbd->sock_lock); > + if (nbd->sock) { > + struct request sreq; > + > + blk_rq_init(NULL, &sreq); > + sreq.cmd_type =3D REQ_TYPE_DRV_PRIV; > + mutex_lock(&nbd->tx_lock); > + nbd->disconnect =3D true; > + nbd_send_req(nbd, &sreq); > + mutex_unlock(&nbd->tx_lock); > + dev_err(disk_to_dev(nbd->disk), > + "Device Timeout occured.Shutting down" > + " socket."); > + } > + mutex_unlock(&nbd->sock_lock); > + sock_shutdown(nbd); Why are you trying to send something on a connection that timed out (nbd_send_req())? And afterwards you execute a socket shutdown so in most timeout cases this won't reach the server and we risk a blocking send on a timedout connection. Regards, Markus > + } > =20 > /* extract request */ > if (list_empty(&nbd->waiting_queue)) > @@ -592,7 +603,11 @@ static int nbd_thread_send(void *data) > spin_unlock_irq(&nbd->queue_lock); > =20 > /* handle request */ > - nbd_handle_req(nbd, req); > + if (atomic_read(&nbd->timedout)) { > + req->errors++; > + nbd_end_request(nbd, req); > + } else > + nbd_handle_req(nbd, req); > } > =20 > nbd->task_send =3D NULL; > @@ -647,7 +662,7 @@ static int nbd_set_socket(struct nbd_device *nbd, str= uct socket *sock) > { > int ret =3D 0; > =20 > - spin_lock_irq(&nbd->sock_lock); > + mutex_lock(&nbd->sock_lock); > =20 > if (nbd->sock) { > ret =3D -EBUSY; > @@ -657,7 +672,7 @@ static int nbd_set_socket(struct nbd_device *nbd, str= uct socket *sock) > nbd->sock =3D sock; > =20 > out: > - spin_unlock_irq(&nbd->sock_lock); > + mutex_unlock(&nbd->sock_lock); > =20 > return ret; > } > @@ -666,7 +681,7 @@ out: > static void nbd_reset(struct nbd_device *nbd) > { > nbd->disconnect =3D false; > - nbd->timedout =3D false; > + atomic_set(&nbd->timedout, 0); > nbd->blksize =3D 1024; > nbd->bytesize =3D 0; > set_capacity(nbd->disk, 0); > @@ -803,17 +818,15 @@ static int __nbd_ioctl(struct block_device *bdev, s= truct nbd_device *nbd, > error =3D nbd_thread_recv(nbd, bdev); > nbd_dev_dbg_close(nbd); > kthread_stop(thread); > - > - mutex_lock(&nbd->tx_lock); > - > sock_shutdown(nbd); > + mutex_lock(&nbd->tx_lock); > nbd_clear_que(nbd); > kill_bdev(bdev); > nbd_bdev_reset(bdev); > =20 > if (nbd->disconnect) /* user requested, ignore socket errors */ > error =3D 0; > - if (nbd->timedout) > + if (atomic_read(&nbd->timedout)) > error =3D -ETIMEDOUT; > =20 > nbd_reset(nbd); > @@ -1075,7 +1088,7 @@ static int __init nbd_init(void) > nbd_dev[i].magic =3D NBD_MAGIC; > INIT_LIST_HEAD(&nbd_dev[i].waiting_queue); > spin_lock_init(&nbd_dev[i].queue_lock); > - spin_lock_init(&nbd_dev[i].sock_lock); > + mutex_init(&nbd_dev[i].sock_lock); > INIT_LIST_HEAD(&nbd_dev[i].queue_head); > mutex_init(&nbd_dev[i].tx_lock); > init_timer(&nbd_dev[i].timeout_timer); > --=20 > 2.6.2 >=20 >=20 --=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-5555 | --1sNVjLsmu1MXqwQ/ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXPVuUAAoJEEpcgKtcEGQQi7AP+wTG1leKYnhkwJIGUO8rzEC3 SVe+SOvc+n1o1jy/4rpmyqU9oSuIhQMFTTUh+1AXjZ3/SlIAxtFJ/otYJNyw3Yp5 A9M1UGd1+SnIB1TmHqX4QCIgdSnmuPJHaQwxIBVkAB5EZPh5NpI9dAvunHx5tG+f sZePH1XUdEM5JNjkUbzAX0qbSl8HtJX6t/L0HBb+Ys+YKRHphny8aVZlyHtNw/I8 RJXAz6U73Ua0v0gcvs+eaIIfrFQVOZQUfsEuH5o0+dD7+AnCpKJUO5AHdsBdx755 d4pgf9EE+HvghnLlFX2VXVVOwdPCKqKnispyecQ/DGf9JvBcJtuZqZZKcFOH9uqk 1mA2zQRVJv7T2PHxYEQe2BtV60nCTnUj2rkwztdIN2LTSECR8XjPCxNPJOkFwglp fxc5djYyeCW10GBF2IXHyGTa1cpkffnHKhMOGvzaWZmKmuj7/LJKCvsCeeK7TN4F J4N6UOVzRUbDTn5ui+hp6FV/DBehLyAjczRaeaemJ3wv9n7/zC5ogqmJc5cAcT1X EcP/ybh/rCvwI5hXdfo1vFKsj38rkcqSQ1JeYPrNju5yb/WOq5DwqqFOji3kYFbA pFcM6wxtLn6tcchY+SkcxrJ2bnY100eMuvt0yxPRFYXRsAzRV6uo+fYYkcPZ242x UBZBmb7ElhFuat6tO0cO =8Mqz -----END PGP SIGNATURE----- --1sNVjLsmu1MXqwQ/--