Hi, On Tuesday 24 May 2016 14:26:25 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 | 62 +++++++++++++++++++++++++++++---------------------- > 1 file changed, 35 insertions(+), 27 deletions(-) > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c > index 08afbc7..a5dab48 100644 > --- a/drivers/block/nbd.c > +++ b/drivers/block/nbd.c > @@ -39,6 +39,7 @@ > #include > > #include > +#include > > struct nbd_device { > u32 flags; > @@ -57,7 +58,7 @@ struct nbd_device { > int blksize; > loff_t bytesize; > int xmit_timeout; > - bool timedout; > + atomic_t timedout; Why are you using atomic here? It seems you are just counting the occurences of timeouts with this but never actually use that number. > bool disconnect; /* a disconnect has been requested by user */ > > struct timer_list timeout_timer; > @@ -69,6 +70,7 @@ struct nbd_device { > #if IS_ENABLED(CONFIG_DEBUG_FS) > struct dentry *dbg_dir; > #endif > + struct work_struct ws_nbd; Can you rename this so that it is clear that this is a worker struct specifically for the socket shutdown? > }; > > #if IS_ENABLED(CONFIG_DEBUG_FS) > @@ -94,6 +96,7 @@ static int max_part; > * Thanks go to Jens Axboe and Al Viro for their LKML emails explaining this! > */ > static DEFINE_SPINLOCK(nbd_lock); > +static void nbd_work_func(struct work_struct *); Same here. nbd_work_func() doesn't give any hint about the real purpose of the function. > > static inline struct device *nbd_to_dev(struct nbd_device *nbd) > { > @@ -172,39 +175,31 @@ 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); > - > + atomic_inc(&nbd->timedout); > + schedule_work(&nbd->ws_nbd); > + wake_up(&nbd->waiting_wq); > dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connection\n"); > } > > @@ -592,7 +587,11 @@ static int nbd_thread_send(void *data) > spin_unlock_irq(&nbd->queue_lock); > > /* 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); > } > > nbd->task_send = NULL; > @@ -666,12 +665,13 @@ out: > static void nbd_reset(struct nbd_device *nbd) > { > nbd->disconnect = false; > - nbd->timedout = false; > + atomic_set(&nbd->timedout, 0); > nbd->blksize = 1024; > nbd->bytesize = 0; > set_capacity(nbd->disk, 0); > nbd->flags = 0; > nbd->xmit_timeout = 0; > + INIT_WORK(&nbd->ws_nbd, nbd_work_func); > queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue); > del_timer_sync(&nbd->timeout_timer); > } > @@ -804,16 +804,16 @@ 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; > - if (nbd->timedout) > + > + if (atomic_read(&nbd->timedout)) > error = -ETIMEDOUT; > > nbd_reset(nbd); > @@ -863,6 +863,14 @@ static const struct block_device_operations nbd_fops = > .compat_ioctl = nbd_ioctl, > }; > > +static void nbd_work_func(struct work_struct *ws_nbd) > +{ > + struct nbd_device *nbd_dev = container_of(ws_nbd, struct nbd_device, > + ws_nbd); Please align line breaks on the opening bracket. Best Regards, Markus -- 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 |