From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751770AbcF1Fma (ORCPT ); Tue, 28 Jun 2016 01:42:30 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:35001 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750704AbcF1Fm3 (ORCPT ); Tue, 28 Jun 2016 01:42:29 -0400 MIME-Version: 1.0 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> From: Pranay Srivastava Date: Tue, 28 Jun 2016 11:12:26 +0530 Message-ID: Subject: Re: [PATCH v3 1/3]nbd: fix might_sleep warning on socket shutdown To: Markus Pargmann , nbd-general@lists.sourceforge.net, linux-kernel@vger.kernel.org, Wouter Verhelst Cc: "Pranay Kr. Srivastava" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Markus, On Fri, Jun 24, 2016 at 3:39 PM, 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. > > Reported-by: Mikulas Patocka > Signed-off-by: Markus Pargmann > > Changelog: > Pranay Kr. Srivastava: > > 1) Use spin_lock instead of irq version for sock_shutdown. > > 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. > > Signed-off-by: Pranay Kr. Srivastava > --- > drivers/block/nbd.c | 69 ++++++++++++++++++++++++++++++++++------------------- > 1 file changed, 44 insertions(+), 25 deletions(-) > > 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 > > #include > +#include > > struct nbd_device { > u32 flags; > @@ -69,6 +70,10 @@ struct nbd_device { > #if IS_ENABLED(CONFIG_DEBUG_FS) > struct dentry *dbg_dir; > #endif > + /* > + *This is specifically for calling sock_shutdown, for now. > + */ > + struct work_struct ws_shutdown; > }; > > #if IS_ENABLED(CONFIG_DEBUG_FS) > @@ -95,6 +100,11 @@ static int max_part; > */ > static DEFINE_SPINLOCK(nbd_lock); > > +/* > + * Shutdown function for nbd_dev work struct. > + */ > +static void nbd_ws_func_shutdown(struct work_struct *); > + > static inline struct device *nbd_to_dev(struct nbd_device *nbd) > { > return 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) > { > - spin_lock_irq(&nbd->sock_lock); > - > - if (!nbd->sock) { > - spin_unlock_irq(&nbd->sock_lock); > - return; > - } > + struct socket *sock; > > - dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n"); > - kernel_sock_shutdown(nbd->sock, SHUT_RDWR); > - sockfd_put(nbd->sock); > + spin_lock(&nbd->sock_lock); > + sock = nbd->sock; > nbd->sock = NULL; > - spin_unlock_irq(&nbd->sock_lock); > + spin_unlock(&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(sock, SHUT_RDWR); > + sockfd_put(sock); > } > > static void nbd_xmit_timeout(unsigned long arg) > { > struct nbd_device *nbd = (struct nbd_device *)arg; > - unsigned long flags; > > if (list_empty(&nbd->queue_head)) > return; > - > - spin_lock_irqsave(&nbd->sock_lock, flags); > - > nbd->timedout = true; > - > - if (nbd->sock) > - kernel_sock_shutdown(nbd->sock, SHUT_RDWR); > - > - spin_unlock_irqrestore(&nbd->sock_lock, flags); > - > + schedule_work(&nbd->ws_shutdown); > + /* > + * Make sure sender thread sees nbd->timedout. > + */ > + smp_wmb(); > + wake_up(&nbd->waiting_wq); > dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connection\n"); > } > > @@ -574,8 +580,8 @@ static int nbd_thread_send(void *data) > while (!kthread_should_stop() || !list_empty(&nbd->waiting_queue)) { > /* wait for something to do */ > wait_event_interruptible(nbd->waiting_wq, > - kthread_should_stop() || > - !list_empty(&nbd->waiting_queue)); > + kthread_should_stop() || > + !list_empty(&nbd->waiting_queue)); > > /* extract request */ > if (list_empty(&nbd->waiting_queue)) > @@ -583,12 +589,16 @@ static int nbd_thread_send(void *data) > > spin_lock_irq(&nbd->queue_lock); > req = list_entry(nbd->waiting_queue.next, struct request, > - queuelist); > + queuelist); > list_del_init(&req->queuelist); > spin_unlock_irq(&nbd->queue_lock); > > - /* handle request */ > nbd_handle_req(nbd, req); > + if (nbd->timedout) { > + req->errors++; > + nbd_end_request(nbd, req); > + } else > + nbd_handle_req(nbd, req); > } > > nbd->task_send = NULL; > @@ -668,6 +678,7 @@ static void nbd_reset(struct nbd_device *nbd) > set_capacity(nbd->disk, 0); > nbd->flags = 0; > nbd->xmit_timeout = 0; > + INIT_WORK(&nbd->ws_shutdown, nbd_ws_func_shutdown); > queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue); > del_timer_sync(&nbd->timeout_timer); > } > @@ -802,11 +813,11 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd, > error = nbd_thread_recv(nbd, bdev); > nbd_dev_dbg_close(nbd); > kthread_stop(thread); > + sock_shutdown(nbd); > > mutex_lock(&nbd->tx_lock); > nbd->task_recv = NULL; > > - sock_shutdown(nbd); > nbd_clear_que(nbd); > kill_bdev(bdev); > nbd_bdev_reset(bdev); > @@ -862,6 +873,14 @@ static const struct block_device_operations nbd_fops = { > .compat_ioctl = nbd_ioctl, > }; > > +static void nbd_ws_func_shutdown(struct work_struct *ws_nbd) > +{ > + struct nbd_device *nbd_dev = container_of(ws_nbd, struct nbd_device, > + ws_shutdown); > + > + sock_shutdown(nbd_dev); > +} > + > #if IS_ENABLED(CONFIG_DEBUG_FS) > > static int nbd_dbg_tasks_show(struct seq_file *s, void *unused) > -- > 1.9.1 > Do you think this series can be reviewed for 4.7? -- ---P.K.S