From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751870AbcFNJuO (ORCPT ); Tue, 14 Jun 2016 05:50:14 -0400 Received: from mail-wm0-f65.google.com ([74.125.82.65]:35279 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751291AbcFNJuK (ORCPT ); Tue, 14 Jun 2016 05:50:10 -0400 MIME-Version: 1.0 In-Reply-To: <1962682.lYRJ5o9hTF@adelgunde> References: <3898019.JRqDjBPssX@adelgunde> <1464863101-16805-1-git-send-email-pranjas@gmail.com> <1464863101-16805-2-git-send-email-pranjas@gmail.com> <1962682.lYRJ5o9hTF@adelgunde> From: Pranay Srivastava Date: Tue, 14 Jun 2016 15:20:08 +0530 Message-ID: Subject: Re: [PATCH v2 1/5] nbd: fix might_sleep warning on socket shutdown. To: Markus Pargmann Cc: nbd-general@lists.sourceforge.net, linux-kernel@vger.kernel.org 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 On Tue, Jun 14, 2016 at 2:22 PM, Markus Pargmann wrote: > Hi, > > On Thursday 02 June 2016 13:24:57 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 > > This looks better. Some smaller things inline. Also this patch does not > apply on my tree anymore. Can you please rebase it onto: > http://git.pengutronix.de/?p=mpa/linux-nbd.git;a=shortlog;h=refs/heads/master > Ok will do. >> --- >> drivers/block/nbd.c | 65 ++++++++++++++++++++++++++++++++++------------------- >> 1 file changed, 42 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c >> index 31e73a7..0339d40 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(); > > I am not sure that we need this memory barrier here. But as it is just > the timeout path it probably won't hurt. > >> + wake_up(&nbd->waiting_wq); >> dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connection\n"); >> } >> >> @@ -592,7 +598,11 @@ static int nbd_thread_send(void *data) >> 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; >> @@ -672,6 +682,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); >> } >> @@ -804,15 +815,15 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd, >> 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); >> >> if (nbd->disconnect) /* user requested, ignore socket errors */ >> error = 0; >> + > > Random newline here. Ok, will take care of this. > > Best Regards, > > Markus > >> if (nbd->timedout) >> error = -ETIMEDOUT; >> >> @@ -863,6 +874,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) >> > > -- > 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 | -- ---P.K.S