Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1430558 > unrolled thread

[PATCH v3 0/3] nbd: resolve bugs and limitations

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 0/3] nbd: resolve bugs and limitations "Pranay Kr. Srivastava" <pranjas@gmail.com> - 2016-06-24 12:10 +0200
    [PATCH 3/3]nbd: make nbd device wait for its users "Pranay Kr. Srivastava" <pranjas@gmail.com> - 2016-06-24 12:20 +0200
      Re: [Nbd] [PATCH 3/3]nbd: make nbd device wait for its users Eric Blake <eblake@redhat.com> - 2016-06-24 15:50 +0200
        Re: [Nbd] [PATCH 3/3]nbd: make nbd device wait for its users Pranay Srivastava <pranjas@gmail.com> - 2016-06-25 20:00 +0200
      Re: [PATCH 3/3]nbd: make nbd device wait for its users Pranay Srivastava <pranjas@gmail.com> - 2016-06-25 20:10 +0200
      Re: [PATCH 3/3]nbd: make nbd device wait for its users Markus Pargmann <mpa@pengutronix.de> - 2016-06-29 09:10 +0200
        Re: [PATCH 3/3]nbd: make nbd device wait for its users Pranay Srivastava <pranjas@gmail.com> - 2016-06-29 09:20 +0200
    [PATCH v3 1/3]nbd: fix might_sleep warning on socket shutdown "Pranay Kr. Srivastava" <pranjas@gmail.com> - 2016-06-24 12:20 +0200
      Re: [PATCH v3 1/3]nbd: fix might_sleep warning on socket shutdown Pranay Srivastava <pranjas@gmail.com> - 2016-06-28 07:50 +0200
      Re: [PATCH v3 1/3]nbd: fix might_sleep warning on socket shutdown Markus Pargmann <mpa@pengutronix.de> - 2016-06-29 09:20 +0200
    [PATCH v3 2/3]nbd: cleanup nbd_set_socket "Pranay Kr. Srivastava" <pranjas@gmail.com> - 2016-06-24 12:20 +0200

#1430558 — [PATCH v3 0/3] nbd: resolve bugs and limitations

This patch series fixes the following

1) fix might_sleep warning on socket shutdown:
   Fix sock_shutdown to avoid calling kernel_sock_shutdown
   while holding spin_lock.

2) cleanup nbd_set_socket
   Simple fixes to nbd_set_socket.

3) make nbd device wait for its users.
   When a timeout or error occurs then nbd driver simply kills
   the block device. Many filesystem(s) example ext2/ext3 don't
   expect their buffer heads to disappear like that. Fix this
   by making nbd device wait for its users.

   The same work function is used to trigger the kill_bdev as well
   do a sock_shutdown, depending on either a timeout/error occured
   or a disconnect was issued.

   Also avoid scheduling the work_fn in case a timeout for a request
   has already occured.


Pranay Kr. Srivastava (3):
  fix might_sleep warning on socket shutdown
  cleanup nbd_set_socket
  make nbd device wait for its users

 drivers/block/nbd.c | 169 +++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 127 insertions(+), 42 deletions(-)

Changelog:

Rebased all patches above on git://git.pengutronix.de/git/mpa/linux-nbd.git,
commit:7ed71a8704eda7b75fbd0ed73fd0a5b6e469d250

3) make nbd device wait for its users.
   Instead of using kref_sub just use kref_init and under the bd_mutex for
   serializing on open/close.
-- 
1.9.1

[toc] | [next] | [standalone]

#1430560 — [PATCH 3/3]nbd: make nbd device wait for its users

When a timeout occurs or a recv fails, then
instead of abruplty killing nbd block device
wait for it's users to finish.

This is more required when filesystem(s) like
ext2 or ext3 don't expect their buffer heads to
disappear while the filesystem is mounted.

Each open of a nbd device is refcounted, while
the userland program [nbd-client] doing the
NBD_DO_IT ioctl would now wait for any other users
of this device before invalidating the nbd device.

A timedout or a disconnected device, if in use, can't
be used until it has been resetted. The resetting happens
when all tasks having this bdev open closes this bdev.

Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
---
 drivers/block/nbd.c | 124 ++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 96 insertions(+), 28 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 9223b09..0587bbd 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -70,10 +70,13 @@ struct nbd_device {
 #if IS_ENABLED(CONFIG_DEBUG_FS)
 	struct dentry *dbg_dir;
 #endif
+
 	/*
-	*This is specifically for calling sock_shutdown, for now.
-	*/
+	 *This is specifically for calling sock_shutdown, for now.
+	 */
 	struct work_struct ws_shutdown;
+	struct kref users;
+	struct completion user_completion;
 };
 
 #if IS_ENABLED(CONFIG_DEBUG_FS)
@@ -104,6 +107,8 @@ static DEFINE_SPINLOCK(nbd_lock);
  * Shutdown function for nbd_dev work struct.
  */
 static void nbd_ws_func_shutdown(struct work_struct *);
+static void nbd_kref_release(struct kref *);
+static int nbd_size_clear(struct nbd_device *, struct block_device *);
 
 static inline struct device *nbd_to_dev(struct nbd_device *nbd)
 {
@@ -129,7 +134,7 @@ static const char *nbdcmd_to_ascii(int cmd)
 
 static int nbd_size_clear(struct nbd_device *nbd, struct block_device *bdev)
 {
-	bdev->bd_inode->i_size = 0;
+	i_size_write(bdev->bd_inode, 0);
 	set_capacity(nbd->disk, 0);
 	kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE);
 
@@ -141,7 +146,7 @@ static void nbd_size_update(struct nbd_device *nbd, struct block_device *bdev)
 	if (!nbd_is_connected(nbd))
 		return;
 
-	bdev->bd_inode->i_size = nbd->bytesize;
+	i_size_write(bdev->bd_inode, nbd->bytesize);
 	set_capacity(nbd->disk, nbd->bytesize >> 9);
 	kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE);
 }
@@ -150,11 +155,9 @@ static int nbd_size_set(struct nbd_device *nbd, struct block_device *bdev,
 			int blocksize, int nr_blocks)
 {
 	int ret;
-
 	ret = set_blocksize(bdev, blocksize);
 	if (ret)
 		return ret;
-
 	nbd->blksize = blocksize;
 	nbd->bytesize = (loff_t)blocksize * (loff_t)nr_blocks;
 
@@ -202,14 +205,19 @@ static void nbd_xmit_timeout(unsigned long arg)
 {
 	struct nbd_device *nbd = (struct nbd_device *)arg;
 
+	if (nbd->timedout)
+		return;
+
 	if (list_empty(&nbd->queue_head))
 		return;
+
 	nbd->timedout = true;
-	schedule_work(&nbd->ws_shutdown);
+
 	/*
 	 * Make sure sender thread sees nbd->timedout.
 	 */
 	smp_wmb();
+	schedule_work(&nbd->ws_shutdown);
 	wake_up(&nbd->waiting_wq);
 	dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connection\n");
 }
@@ -476,8 +484,6 @@ static int nbd_thread_recv(struct nbd_device *nbd, struct block_device *bdev)
 		nbd_end_request(nbd, req);
 	}
 
-	nbd_size_clear(nbd, bdev);
-
 	device_remove_file(disk_to_dev(nbd->disk), &dev_attr_pid);
 
 	nbd->task_recv = NULL;
@@ -580,8 +586,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))
@@ -589,11 +595,11 @@ 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);
 
-		nbd_handle_req(nbd, req);
+		/* handle request */
 		if (nbd->timedout) {
 			req->errors++;
 			nbd_end_request(nbd, req);
@@ -654,12 +660,13 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
 	int ret = 0;
 
 	spin_lock(&nbd->sock_lock);
-	if (nbd->sock)
+
+	if (nbd->sock || nbd->timedout)
 		ret = -EBUSY;
 	else
 		nbd->sock = sock;
-	spin_unlock(&nbd->sock_lock);
 
+	spin_unlock(&nbd->sock_lock);
 	return ret;
 }
 
@@ -674,6 +681,7 @@ static void nbd_reset(struct nbd_device *nbd)
 	nbd->flags = 0;
 	nbd->xmit_timeout = 0;
 	INIT_WORK(&nbd->ws_shutdown, nbd_ws_func_shutdown);
+	init_completion(&nbd->user_completion);
 	queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
 	del_timer_sync(&nbd->timeout_timer);
 }
@@ -708,6 +716,9 @@ static void nbd_dev_dbg_close(struct nbd_device *nbd);
 static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		       unsigned int cmd, unsigned long arg)
 {
+	if (nbd->timedout || nbd->disconnect)
+		return -EBUSY;
+
 	switch (cmd) {
 	case NBD_DISCONNECT: {
 		struct request sreq;
@@ -737,7 +748,6 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		nbd_clear_que(nbd);
 		BUG_ON(!list_empty(&nbd->queue_head));
 		BUG_ON(!list_empty(&nbd->waiting_queue));
-		kill_bdev(bdev);
 		return 0;
 
 	case NBD_SET_SOCK: {
@@ -756,7 +766,6 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 
 	case NBD_SET_BLKSIZE: {
 		loff_t bsize = div_s64(nbd->bytesize, arg);
-
 		return nbd_size_set(nbd, bdev, arg, bsize);
 	}
 
@@ -808,22 +817,29 @@ 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;
-
-		nbd_clear_que(nbd);
-		kill_bdev(bdev);
-		nbd_bdev_reset(bdev);
+		sock_shutdown(nbd);
 
 		if (nbd->disconnect) /* user requested, ignore socket errors */
 			error = 0;
 		if (nbd->timedout)
 			error = -ETIMEDOUT;
 
-		nbd_reset(nbd);
+		mutex_lock(&nbd->tx_lock);
+		nbd_clear_que(nbd);
+		nbd->disconnect = true; /* To kill bdev*/
+		mutex_unlock(&nbd->tx_lock);
+		cancel_work_sync(&nbd->ws_shutdown);
+		kref_put(&nbd->users, nbd_kref_release);
+		wait_for_completion(&nbd->user_completion);
 
+		mutex_lock(&bdev->bd_mutex);
+		if (!kref_get_unless_zero(&nbd->users))
+			kref_init(&nbd->users);
+		mutex_unlock(&bdev->bd_mutex);
+
+		mutex_lock(&nbd->tx_lock);
+		nbd_reset(nbd);
 		return error;
 	}
 
@@ -861,19 +877,71 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
 
 	return error;
 }
+static void nbd_kref_release(struct kref *kref_users)
+{
+	struct nbd_device *nbd = container_of(kref_users, struct nbd_device,
+			users);
+	schedule_work(&nbd->ws_shutdown);
+}
+
+static int nbd_open(struct block_device *bdev, fmode_t mode)
+{
+	struct nbd_device *nbd_dev = bdev->bd_disk->private_data;
+
+	if (!kref_get_unless_zero(&nbd_dev->users))
+		kref_init(&nbd_dev->users);
+
+	pr_debug("Opening nbd_dev %s. Active users = %u\n",
+			bdev->bd_disk->disk_name,
+			atomic_read(&nbd_dev->users.refcount)
+		);
+	return 0;
+}
+
+static void nbd_release(struct gendisk *disk, fmode_t mode)
+{
+	struct nbd_device *nbd_dev = disk->private_data;
+
+	kref_put(&nbd_dev->users,  nbd_kref_release);
+
+	pr_debug("Closing nbd_dev %s. Active users = %u\n",
+			disk->disk_name,
+			atomic_read(&nbd_dev->users.refcount)
+		);
+}
 
 static const struct block_device_operations nbd_fops = {
 	.owner =	THIS_MODULE,
 	.ioctl =	nbd_ioctl,
 	.compat_ioctl =	nbd_ioctl,
+	.open =		nbd_open,
+	.release =	nbd_release
 };
 
+
 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);
+							ws_shutdown);
+	struct block_device *bdev = bdget(part_devt(
+						dev_to_part(nbd_to_dev(nbd_dev))
+						)
+					);
+	BUG_ON(!bdev);
+	if (nbd_dev->timedout)
+		sock_shutdown(nbd_dev);
+
+	if (nbd_dev->disconnect) {
+		mutex_lock(&nbd_dev->tx_lock);
+		nbd_dev->task_recv = NULL;
+		nbd_clear_que(nbd_dev);
+		kill_bdev(bdev);
+		nbd_bdev_reset(bdev);
+		mutex_unlock(&nbd_dev->tx_lock);
+		nbd_size_clear(nbd_dev, bdev);
+		complete(&nbd_dev->user_completion);
+	}
+	bdput(bdev);
 }
 
 #if IS_ENABLED(CONFIG_DEBUG_FS)
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1430673 — Re: [Nbd] [PATCH 3/3]nbd: make nbd device wait for its users

[Multipart message — attachments visible in raw view] — view raw

On 06/24/2016 04:09 AM, Pranay Kr. Srivastava wrote:
> When a timeout occurs or a recv fails, then
> instead of abruplty killing nbd block device

s/abruplty/abruptly/

> wait for it's users to finish.

s/it's/its/

> 
> This is more required when filesystem(s) like
> ext2 or ext3 don't expect their buffer heads to
> disappear while the filesystem is mounted.
> 
> Each open of a nbd device is refcounted, while
> the userland program [nbd-client] doing the
> NBD_DO_IT ioctl would now wait for any other users
> of this device before invalidating the nbd device.
> 
> A timedout or a disconnected device, if in use, can't
> be used until it has been resetted. The resetting happens

s/resetted/reset/

> when all tasks having this bdev open closes this bdev.
> 
> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
> ---
>  drivers/block/nbd.c | 124 ++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 96 insertions(+), 28 deletions(-)
> 


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[toc] | [prev] | [next] | [standalone]

#1431186 — Re: [Nbd] [PATCH 3/3]nbd: make nbd device wait for its users

Hi Eric,

On Fri, Jun 24, 2016 at 7:12 PM, Eric Blake <eblake@redhat.com> wrote:
> On 06/24/2016 04:09 AM, Pranay Kr. Srivastava wrote:
>> When a timeout occurs or a recv fails, then
>> instead of abruplty killing nbd block device
>
> s/abruplty/abruptly/
>
>> wait for it's users to finish.
>
> s/it's/its/
>
>>
>> This is more required when filesystem(s) like
>> ext2 or ext3 don't expect their buffer heads to
>> disappear while the filesystem is mounted.
>>
>> Each open of a nbd device is refcounted, while
>> the userland program [nbd-client] doing the
>> NBD_DO_IT ioctl would now wait for any other users
>> of this device before invalidating the nbd device.
>>
>> A timedout or a disconnected device, if in use, can't
>> be used until it has been resetted. The resetting happens
>
> s/resetted/reset/
>

Thanks for going through the patch. Can I get some review on the
code as well so I can fix and resend that too, along with fixes of grammatical
errors.

>> when all tasks having this bdev open closes this bdev.
>>
>> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
>> ---
>>  drivers/block/nbd.c | 124 ++++++++++++++++++++++++++++++++++++++++------------
>>  1 file changed, 96 insertions(+), 28 deletions(-)
>>
>
>
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>



-- 
        ---P.K.S

[toc] | [prev] | [next] | [standalone]

#1431190 — Re: [PATCH 3/3]nbd: make nbd device wait for its users

Hi

On Fri, Jun 24, 2016 at 3:39 PM, Pranay Kr. Srivastava
<pranjas@gmail.com> wrote:
> When a timeout occurs or a recv fails, then
> instead of abruplty killing nbd block device
> wait for it's users to finish.
>
> This is more required when filesystem(s) like
> ext2 or ext3 don't expect their buffer heads to
> disappear while the filesystem is mounted.
>
> Each open of a nbd device is refcounted, while
> the userland program [nbd-client] doing the
> NBD_DO_IT ioctl would now wait for any other users
> of this device before invalidating the nbd device.
>
> A timedout or a disconnected device, if in use, can't
> be used until it has been resetted. The resetting happens
> when all tasks having this bdev open closes this bdev.
>
> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
> ---
>  drivers/block/nbd.c | 124 ++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 96 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 9223b09..0587bbd 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -70,10 +70,13 @@ struct nbd_device {
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>         struct dentry *dbg_dir;
>  #endif
> +
>         /*
> -       *This is specifically for calling sock_shutdown, for now.
> -       */
> +        *This is specifically for calling sock_shutdown, for now.
> +        */
>         struct work_struct ws_shutdown;
> +       struct kref users;
> +       struct completion user_completion;
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -104,6 +107,8 @@ static DEFINE_SPINLOCK(nbd_lock);
>   * Shutdown function for nbd_dev work struct.
>   */
>  static void nbd_ws_func_shutdown(struct work_struct *);
> +static void nbd_kref_release(struct kref *);
> +static int nbd_size_clear(struct nbd_device *, struct block_device *);
>
>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>  {
> @@ -129,7 +134,7 @@ static const char *nbdcmd_to_ascii(int cmd)
>
>  static int nbd_size_clear(struct nbd_device *nbd, struct block_device *bdev)
>  {
> -       bdev->bd_inode->i_size = 0;
> +       i_size_write(bdev->bd_inode, 0);
>         set_capacity(nbd->disk, 0);
>         kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE);
>
> @@ -141,7 +146,7 @@ static void nbd_size_update(struct nbd_device *nbd, struct block_device *bdev)
>         if (!nbd_is_connected(nbd))
>                 return;
>
> -       bdev->bd_inode->i_size = nbd->bytesize;
> +       i_size_write(bdev->bd_inode, nbd->bytesize);
>         set_capacity(nbd->disk, nbd->bytesize >> 9);
>         kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE);
>  }
> @@ -150,11 +155,9 @@ static int nbd_size_set(struct nbd_device *nbd, struct block_device *bdev,
>                         int blocksize, int nr_blocks)
>  {
>         int ret;
> -
>         ret = set_blocksize(bdev, blocksize);
>         if (ret)
>                 return ret;
> -
>         nbd->blksize = blocksize;
>         nbd->bytesize = (loff_t)blocksize * (loff_t)nr_blocks;
>
> @@ -202,14 +205,19 @@ static void nbd_xmit_timeout(unsigned long arg)
>  {
>         struct nbd_device *nbd = (struct nbd_device *)arg;
>
> +       if (nbd->timedout)
> +               return;
> +
>         if (list_empty(&nbd->queue_head))
>                 return;
> +
>         nbd->timedout = true;
> -       schedule_work(&nbd->ws_shutdown);
> +
>         /*
>          * Make sure sender thread sees nbd->timedout.
>          */
>         smp_wmb();
> +       schedule_work(&nbd->ws_shutdown);
>         wake_up(&nbd->waiting_wq);
>         dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connection\n");
>  }
> @@ -476,8 +484,6 @@ static int nbd_thread_recv(struct nbd_device *nbd, struct block_device *bdev)
>                 nbd_end_request(nbd, req);
>         }
>
> -       nbd_size_clear(nbd, bdev);
> -
>         device_remove_file(disk_to_dev(nbd->disk), &dev_attr_pid);
>
>         nbd->task_recv = NULL;
> @@ -580,8 +586,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))
> @@ -589,11 +595,11 @@ 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);
>
> -               nbd_handle_req(nbd, req);
> +               /* handle request */
>                 if (nbd->timedout) {
>                         req->errors++;
>                         nbd_end_request(nbd, req);
> @@ -654,12 +660,13 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
>         int ret = 0;
>
>         spin_lock(&nbd->sock_lock);
> -       if (nbd->sock)
> +
> +       if (nbd->sock || nbd->timedout)
>                 ret = -EBUSY;
>         else
>                 nbd->sock = sock;
> -       spin_unlock(&nbd->sock_lock);
>
> +       spin_unlock(&nbd->sock_lock);
>         return ret;
>  }
>
> @@ -674,6 +681,7 @@ static void nbd_reset(struct nbd_device *nbd)
>         nbd->flags = 0;
>         nbd->xmit_timeout = 0;
>         INIT_WORK(&nbd->ws_shutdown, nbd_ws_func_shutdown);
> +       init_completion(&nbd->user_completion);
>         queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
>         del_timer_sync(&nbd->timeout_timer);
>  }
> @@ -708,6 +716,9 @@ static void nbd_dev_dbg_close(struct nbd_device *nbd);
>  static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>                        unsigned int cmd, unsigned long arg)
>  {
> +       if (nbd->timedout || nbd->disconnect)
> +               return -EBUSY;
> +
>         switch (cmd) {
>         case NBD_DISCONNECT: {
>                 struct request sreq;
> @@ -737,7 +748,6 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>                 nbd_clear_que(nbd);
>                 BUG_ON(!list_empty(&nbd->queue_head));
>                 BUG_ON(!list_empty(&nbd->waiting_queue));
> -               kill_bdev(bdev);
>                 return 0;
>
>         case NBD_SET_SOCK: {
> @@ -756,7 +766,6 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>
>         case NBD_SET_BLKSIZE: {
>                 loff_t bsize = div_s64(nbd->bytesize, arg);
> -
>                 return nbd_size_set(nbd, bdev, arg, bsize);
>         }
>
> @@ -808,22 +817,29 @@ 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;
> -
> -               nbd_clear_que(nbd);
> -               kill_bdev(bdev);
> -               nbd_bdev_reset(bdev);
> +               sock_shutdown(nbd);
>
>                 if (nbd->disconnect) /* user requested, ignore socket errors */
>                         error = 0;
>                 if (nbd->timedout)
>                         error = -ETIMEDOUT;
>
> -               nbd_reset(nbd);
> +               mutex_lock(&nbd->tx_lock);
> +               nbd_clear_que(nbd);
> +               nbd->disconnect = true; /* To kill bdev*/
> +               mutex_unlock(&nbd->tx_lock);
> +               cancel_work_sync(&nbd->ws_shutdown);
> +               kref_put(&nbd->users, nbd_kref_release);
> +               wait_for_completion(&nbd->user_completion);
>
> +               mutex_lock(&bdev->bd_mutex);
> +               if (!kref_get_unless_zero(&nbd->users))
> +                       kref_init(&nbd->users);
> +               mutex_unlock(&bdev->bd_mutex);
> +
> +               mutex_lock(&nbd->tx_lock);
> +               nbd_reset(nbd);
>                 return error;
>         }
>
> @@ -861,19 +877,71 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
>
>         return error;
>  }
> +static void nbd_kref_release(struct kref *kref_users)
> +{
> +       struct nbd_device *nbd = container_of(kref_users, struct nbd_device,
> +                       users);
> +       schedule_work(&nbd->ws_shutdown);
> +}
> +
> +static int nbd_open(struct block_device *bdev, fmode_t mode)
> +{
> +       struct nbd_device *nbd_dev = bdev->bd_disk->private_data;
> +
> +       if (!kref_get_unless_zero(&nbd_dev->users))
> +               kref_init(&nbd_dev->users);
> +
> +       pr_debug("Opening nbd_dev %s. Active users = %u\n",
> +                       bdev->bd_disk->disk_name,
> +                       atomic_read(&nbd_dev->users.refcount)
> +               );
> +       return 0;
> +}
> +
> +static void nbd_release(struct gendisk *disk, fmode_t mode)
> +{
> +       struct nbd_device *nbd_dev = disk->private_data;
> +
> +       kref_put(&nbd_dev->users,  nbd_kref_release);
> +
> +       pr_debug("Closing nbd_dev %s. Active users = %u\n",
> +                       disk->disk_name,
> +                       atomic_read(&nbd_dev->users.refcount)
> +               );
> +}
>
>  static const struct block_device_operations nbd_fops = {
>         .owner =        THIS_MODULE,
>         .ioctl =        nbd_ioctl,
>         .compat_ioctl = nbd_ioctl,
> +       .open =         nbd_open,
> +       .release =      nbd_release
>  };
>
> +
>  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);
> +                                                       ws_shutdown);
> +       struct block_device *bdev = bdget(part_devt(
> +                                               dev_to_part(nbd_to_dev(nbd_dev))
> +                                               )
> +                                       );
> +       BUG_ON(!bdev);
> +       if (nbd_dev->timedout)
> +               sock_shutdown(nbd_dev);
> +
> +       if (nbd_dev->disconnect) {
> +               mutex_lock(&nbd_dev->tx_lock);
> +               nbd_dev->task_recv = NULL;
> +               nbd_clear_que(nbd_dev);
> +               kill_bdev(bdev);
> +               nbd_bdev_reset(bdev);
> +               mutex_unlock(&nbd_dev->tx_lock);
> +               nbd_size_clear(nbd_dev, bdev);
> +               complete(&nbd_dev->user_completion);
> +       }
> +       bdput(bdev);
>  }
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> --
> 1.9.1
>

This is supposed to be v3 3/3. Perhaps I gave an incorrect message id
while sending
Should I resend the series afresh [Markus?].

-- 
        ---P.K.S

[toc] | [prev] | [next] | [standalone]

#1433495 — Re: [PATCH 3/3]nbd: make nbd device wait for its users

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Friday 24 June 2016 13:09:36 Pranay Kr. Srivastava wrote:
> When a timeout occurs or a recv fails, then
> instead of abruplty killing nbd block device
> wait for it's users to finish.
> 
> This is more required when filesystem(s) like
> ext2 or ext3 don't expect their buffer heads to
> disappear while the filesystem is mounted.
> 
> Each open of a nbd device is refcounted, while
> the userland program [nbd-client] doing the
> NBD_DO_IT ioctl would now wait for any other users
> of this device before invalidating the nbd device.
> 
> A timedout or a disconnected device, if in use, can't
> be used until it has been resetted. The resetting happens
> when all tasks having this bdev open closes this bdev.

Sorry, but this patch is unreadable. You are changing so many unrelated
whitespaces, lines, comments (that you introduced yourself in a previous
patch) and unrelated code. Please keep only the things that are
necessary for this single patch. Everything else can go into different
patches. Also it would be good to run checkpatch sometimes.

Also using your own atomic implementation instead of kref would be good
perhaps. Although I thought kref would work at the beginning but it
seems not to.

Best Regards,

Markus

> 
> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
> ---
>  drivers/block/nbd.c | 124 ++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 96 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 9223b09..0587bbd 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -70,10 +70,13 @@ struct nbd_device {
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>  	struct dentry *dbg_dir;
>  #endif
> +
>  	/*
> -	*This is specifically for calling sock_shutdown, for now.
> -	*/
> +	 *This is specifically for calling sock_shutdown, for now.
> +	 */
>  	struct work_struct ws_shutdown;
> +	struct kref users;
> +	struct completion user_completion;
>  };
>  
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -104,6 +107,8 @@ static DEFINE_SPINLOCK(nbd_lock);
>   * Shutdown function for nbd_dev work struct.
>   */
>  static void nbd_ws_func_shutdown(struct work_struct *);
> +static void nbd_kref_release(struct kref *);
> +static int nbd_size_clear(struct nbd_device *, struct block_device *);
>  
>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>  {
> @@ -129,7 +134,7 @@ static const char *nbdcmd_to_ascii(int cmd)
>  
>  static int nbd_size_clear(struct nbd_device *nbd, struct block_device *bdev)
>  {
> -	bdev->bd_inode->i_size = 0;
> +	i_size_write(bdev->bd_inode, 0);
>  	set_capacity(nbd->disk, 0);
>  	kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE);
>  
> @@ -141,7 +146,7 @@ static void nbd_size_update(struct nbd_device *nbd, struct block_device *bdev)
>  	if (!nbd_is_connected(nbd))
>  		return;
>  
> -	bdev->bd_inode->i_size = nbd->bytesize;
> +	i_size_write(bdev->bd_inode, nbd->bytesize);
>  	set_capacity(nbd->disk, nbd->bytesize >> 9);
>  	kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE);
>  }
> @@ -150,11 +155,9 @@ static int nbd_size_set(struct nbd_device *nbd, struct block_device *bdev,
>  			int blocksize, int nr_blocks)
>  {
>  	int ret;
> -
>  	ret = set_blocksize(bdev, blocksize);
>  	if (ret)
>  		return ret;
> -
>  	nbd->blksize = blocksize;
>  	nbd->bytesize = (loff_t)blocksize * (loff_t)nr_blocks;
>  
> @@ -202,14 +205,19 @@ static void nbd_xmit_timeout(unsigned long arg)
>  {
>  	struct nbd_device *nbd = (struct nbd_device *)arg;
>  
> +	if (nbd->timedout)
> +		return;
> +
>  	if (list_empty(&nbd->queue_head))
>  		return;
> +
>  	nbd->timedout = true;
> -	schedule_work(&nbd->ws_shutdown);
> +
>  	/*
>  	 * Make sure sender thread sees nbd->timedout.
>  	 */
>  	smp_wmb();
> +	schedule_work(&nbd->ws_shutdown);
>  	wake_up(&nbd->waiting_wq);
>  	dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connection\n");
>  }
> @@ -476,8 +484,6 @@ static int nbd_thread_recv(struct nbd_device *nbd, struct block_device *bdev)
>  		nbd_end_request(nbd, req);
>  	}
>  
> -	nbd_size_clear(nbd, bdev);
> -
>  	device_remove_file(disk_to_dev(nbd->disk), &dev_attr_pid);
>  
>  	nbd->task_recv = NULL;
> @@ -580,8 +586,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))
> @@ -589,11 +595,11 @@ 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);
>  
> -		nbd_handle_req(nbd, req);
> +		/* handle request */
>  		if (nbd->timedout) {
>  			req->errors++;
>  			nbd_end_request(nbd, req);
> @@ -654,12 +660,13 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
>  	int ret = 0;
>  
>  	spin_lock(&nbd->sock_lock);
> -	if (nbd->sock)
> +
> +	if (nbd->sock || nbd->timedout)
>  		ret = -EBUSY;
>  	else
>  		nbd->sock = sock;
> -	spin_unlock(&nbd->sock_lock);
>  
> +	spin_unlock(&nbd->sock_lock);
>  	return ret;
>  }
>  
> @@ -674,6 +681,7 @@ static void nbd_reset(struct nbd_device *nbd)
>  	nbd->flags = 0;
>  	nbd->xmit_timeout = 0;
>  	INIT_WORK(&nbd->ws_shutdown, nbd_ws_func_shutdown);
> +	init_completion(&nbd->user_completion);
>  	queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
>  	del_timer_sync(&nbd->timeout_timer);
>  }
> @@ -708,6 +716,9 @@ static void nbd_dev_dbg_close(struct nbd_device *nbd);
>  static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>  		       unsigned int cmd, unsigned long arg)
>  {
> +	if (nbd->timedout || nbd->disconnect)
> +		return -EBUSY;
> +
>  	switch (cmd) {
>  	case NBD_DISCONNECT: {
>  		struct request sreq;
> @@ -737,7 +748,6 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>  		nbd_clear_que(nbd);
>  		BUG_ON(!list_empty(&nbd->queue_head));
>  		BUG_ON(!list_empty(&nbd->waiting_queue));
> -		kill_bdev(bdev);
>  		return 0;
>  
>  	case NBD_SET_SOCK: {
> @@ -756,7 +766,6 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>  
>  	case NBD_SET_BLKSIZE: {
>  		loff_t bsize = div_s64(nbd->bytesize, arg);
> -
>  		return nbd_size_set(nbd, bdev, arg, bsize);
>  	}
>  
> @@ -808,22 +817,29 @@ 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;
> -
> -		nbd_clear_que(nbd);
> -		kill_bdev(bdev);
> -		nbd_bdev_reset(bdev);
> +		sock_shutdown(nbd);
>  
>  		if (nbd->disconnect) /* user requested, ignore socket errors */
>  			error = 0;
>  		if (nbd->timedout)
>  			error = -ETIMEDOUT;
>  
> -		nbd_reset(nbd);
> +		mutex_lock(&nbd->tx_lock);
> +		nbd_clear_que(nbd);
> +		nbd->disconnect = true; /* To kill bdev*/
> +		mutex_unlock(&nbd->tx_lock);
> +		cancel_work_sync(&nbd->ws_shutdown);
> +		kref_put(&nbd->users, nbd_kref_release);
> +		wait_for_completion(&nbd->user_completion);
>  
> +		mutex_lock(&bdev->bd_mutex);
> +		if (!kref_get_unless_zero(&nbd->users))
> +			kref_init(&nbd->users);
> +		mutex_unlock(&bdev->bd_mutex);
> +
> +		mutex_lock(&nbd->tx_lock);
> +		nbd_reset(nbd);
>  		return error;
>  	}
>  
> @@ -861,19 +877,71 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
>  
>  	return error;
>  }
> +static void nbd_kref_release(struct kref *kref_users)
> +{
> +	struct nbd_device *nbd = container_of(kref_users, struct nbd_device,
> +			users);
> +	schedule_work(&nbd->ws_shutdown);
> +}
> +
> +static int nbd_open(struct block_device *bdev, fmode_t mode)
> +{
> +	struct nbd_device *nbd_dev = bdev->bd_disk->private_data;
> +
> +	if (!kref_get_unless_zero(&nbd_dev->users))
> +		kref_init(&nbd_dev->users);
> +
> +	pr_debug("Opening nbd_dev %s. Active users = %u\n",
> +			bdev->bd_disk->disk_name,
> +			atomic_read(&nbd_dev->users.refcount)
> +		);
> +	return 0;
> +}
> +
> +static void nbd_release(struct gendisk *disk, fmode_t mode)
> +{
> +	struct nbd_device *nbd_dev = disk->private_data;
> +
> +	kref_put(&nbd_dev->users,  nbd_kref_release);
> +
> +	pr_debug("Closing nbd_dev %s. Active users = %u\n",
> +			disk->disk_name,
> +			atomic_read(&nbd_dev->users.refcount)
> +		);
> +}
>  
>  static const struct block_device_operations nbd_fops = {
>  	.owner =	THIS_MODULE,
>  	.ioctl =	nbd_ioctl,
>  	.compat_ioctl =	nbd_ioctl,
> +	.open =		nbd_open,
> +	.release =	nbd_release
>  };
>  
> +
>  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);
> +							ws_shutdown);
> +	struct block_device *bdev = bdget(part_devt(
> +						dev_to_part(nbd_to_dev(nbd_dev))
> +						)
> +					);
> +	BUG_ON(!bdev);
> +	if (nbd_dev->timedout)
> +		sock_shutdown(nbd_dev);
> +
> +	if (nbd_dev->disconnect) {
> +		mutex_lock(&nbd_dev->tx_lock);
> +		nbd_dev->task_recv = NULL;
> +		nbd_clear_que(nbd_dev);
> +		kill_bdev(bdev);
> +		nbd_bdev_reset(bdev);
> +		mutex_unlock(&nbd_dev->tx_lock);
> +		nbd_size_clear(nbd_dev, bdev);
> +		complete(&nbd_dev->user_completion);
> +	}
> +	bdput(bdev);
>  }
>  
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> 

-- 
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 |

[toc] | [prev] | [next] | [standalone]

#1433507 — Re: [PATCH 3/3]nbd: make nbd device wait for its users

Hi

On Wed, Jun 29, 2016 at 12:36 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
> Hi,
>
> On Friday 24 June 2016 13:09:36 Pranay Kr. Srivastava wrote:
>> When a timeout occurs or a recv fails, then
>> instead of abruplty killing nbd block device
>> wait for it's users to finish.
>>
>> This is more required when filesystem(s) like
>> ext2 or ext3 don't expect their buffer heads to
>> disappear while the filesystem is mounted.
>>
>> Each open of a nbd device is refcounted, while
>> the userland program [nbd-client] doing the
>> NBD_DO_IT ioctl would now wait for any other users
>> of this device before invalidating the nbd device.
>>
>> A timedout or a disconnected device, if in use, can't
>> be used until it has been resetted. The resetting happens
>> when all tasks having this bdev open closes this bdev.
>
> Sorry, but this patch is unreadable. You are changing so many unrelated
> whitespaces, lines, comments (that you introduced yourself in a previous
> patch) and unrelated code. Please keep only the things that are

Yes you are right.

> necessary for this single patch. Everything else can go into different
> patches. Also it would be good to run checkpatch sometimes.
>

> Also using your own atomic implementation instead of kref would be good
> perhaps. Although I thought kref would work at the beginning but it
> seems not to.

I was able to make it work and it turned out to be simpler.
Do you think you can go over this patch again just for the kref part?
Should I resend this patch again?

>
> Best Regards,
>
> Markus
>
>>
>> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
>> ---
>>  drivers/block/nbd.c | 124 ++++++++++++++++++++++++++++++++++++++++------------
>>  1 file changed, 96 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> index 9223b09..0587bbd 100644
>> --- a/drivers/block/nbd.c
>> +++ b/drivers/block/nbd.c
>> @@ -70,10 +70,13 @@ struct nbd_device {
>>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>>       struct dentry *dbg_dir;
>>  #endif
>> +
>>       /*
>> -     *This is specifically for calling sock_shutdown, for now.
>> -     */
>> +      *This is specifically for calling sock_shutdown, for now.
>> +      */
>>       struct work_struct ws_shutdown;
>> +     struct kref users;
>> +     struct completion user_completion;
>>  };
>>
>>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> @@ -104,6 +107,8 @@ static DEFINE_SPINLOCK(nbd_lock);
>>   * Shutdown function for nbd_dev work struct.
>>   */
>>  static void nbd_ws_func_shutdown(struct work_struct *);
>> +static void nbd_kref_release(struct kref *);
>> +static int nbd_size_clear(struct nbd_device *, struct block_device *);
>>
>>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>>  {
>> @@ -129,7 +134,7 @@ static const char *nbdcmd_to_ascii(int cmd)
>>
>>  static int nbd_size_clear(struct nbd_device *nbd, struct block_device *bdev)
>>  {
>> -     bdev->bd_inode->i_size = 0;
>> +     i_size_write(bdev->bd_inode, 0);
>>       set_capacity(nbd->disk, 0);
>>       kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE);
>>
>> @@ -141,7 +146,7 @@ static void nbd_size_update(struct nbd_device *nbd, struct block_device *bdev)
>>       if (!nbd_is_connected(nbd))
>>               return;
>>
>> -     bdev->bd_inode->i_size = nbd->bytesize;
>> +     i_size_write(bdev->bd_inode, nbd->bytesize);
>>       set_capacity(nbd->disk, nbd->bytesize >> 9);
>>       kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE);
>>  }
>> @@ -150,11 +155,9 @@ static int nbd_size_set(struct nbd_device *nbd, struct block_device *bdev,
>>                       int blocksize, int nr_blocks)
>>  {
>>       int ret;
>> -
>>       ret = set_blocksize(bdev, blocksize);
>>       if (ret)
>>               return ret;
>> -
>>       nbd->blksize = blocksize;
>>       nbd->bytesize = (loff_t)blocksize * (loff_t)nr_blocks;
>>
>> @@ -202,14 +205,19 @@ static void nbd_xmit_timeout(unsigned long arg)
>>  {
>>       struct nbd_device *nbd = (struct nbd_device *)arg;
>>
>> +     if (nbd->timedout)
>> +             return;
>> +
>>       if (list_empty(&nbd->queue_head))
>>               return;
>> +
>>       nbd->timedout = true;
>> -     schedule_work(&nbd->ws_shutdown);
>> +
>>       /*
>>        * Make sure sender thread sees nbd->timedout.
>>        */
>>       smp_wmb();
>> +     schedule_work(&nbd->ws_shutdown);
>>       wake_up(&nbd->waiting_wq);
>>       dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connection\n");
>>  }
>> @@ -476,8 +484,6 @@ static int nbd_thread_recv(struct nbd_device *nbd, struct block_device *bdev)
>>               nbd_end_request(nbd, req);
>>       }
>>
>> -     nbd_size_clear(nbd, bdev);
>> -
>>       device_remove_file(disk_to_dev(nbd->disk), &dev_attr_pid);
>>
>>       nbd->task_recv = NULL;
>> @@ -580,8 +586,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))
>> @@ -589,11 +595,11 @@ 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);
>>
>> -             nbd_handle_req(nbd, req);
>> +             /* handle request */
>>               if (nbd->timedout) {
>>                       req->errors++;
>>                       nbd_end_request(nbd, req);
>> @@ -654,12 +660,13 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
>>       int ret = 0;
>>
>>       spin_lock(&nbd->sock_lock);
>> -     if (nbd->sock)
>> +
>> +     if (nbd->sock || nbd->timedout)
>>               ret = -EBUSY;
>>       else
>>               nbd->sock = sock;
>> -     spin_unlock(&nbd->sock_lock);
>>
>> +     spin_unlock(&nbd->sock_lock);
>>       return ret;
>>  }
>>
>> @@ -674,6 +681,7 @@ static void nbd_reset(struct nbd_device *nbd)
>>       nbd->flags = 0;
>>       nbd->xmit_timeout = 0;
>>       INIT_WORK(&nbd->ws_shutdown, nbd_ws_func_shutdown);
>> +     init_completion(&nbd->user_completion);
>>       queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
>>       del_timer_sync(&nbd->timeout_timer);
>>  }
>> @@ -708,6 +716,9 @@ static void nbd_dev_dbg_close(struct nbd_device *nbd);
>>  static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>>                      unsigned int cmd, unsigned long arg)
>>  {
>> +     if (nbd->timedout || nbd->disconnect)
>> +             return -EBUSY;
>> +
>>       switch (cmd) {
>>       case NBD_DISCONNECT: {
>>               struct request sreq;
>> @@ -737,7 +748,6 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>>               nbd_clear_que(nbd);
>>               BUG_ON(!list_empty(&nbd->queue_head));
>>               BUG_ON(!list_empty(&nbd->waiting_queue));
>> -             kill_bdev(bdev);
>>               return 0;
>>
>>       case NBD_SET_SOCK: {
>> @@ -756,7 +766,6 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>>
>>       case NBD_SET_BLKSIZE: {
>>               loff_t bsize = div_s64(nbd->bytesize, arg);
>> -
>>               return nbd_size_set(nbd, bdev, arg, bsize);
>>       }
>>
>> @@ -808,22 +817,29 @@ 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;
>> -
>> -             nbd_clear_que(nbd);
>> -             kill_bdev(bdev);
>> -             nbd_bdev_reset(bdev);
>> +             sock_shutdown(nbd);
>>
>>               if (nbd->disconnect) /* user requested, ignore socket errors */
>>                       error = 0;
>>               if (nbd->timedout)
>>                       error = -ETIMEDOUT;
>>
>> -             nbd_reset(nbd);
>> +             mutex_lock(&nbd->tx_lock);
>> +             nbd_clear_que(nbd);
>> +             nbd->disconnect = true; /* To kill bdev*/
>> +             mutex_unlock(&nbd->tx_lock);
>> +             cancel_work_sync(&nbd->ws_shutdown);
>> +             kref_put(&nbd->users, nbd_kref_release);
>> +             wait_for_completion(&nbd->user_completion);
>>
>> +             mutex_lock(&bdev->bd_mutex);
>> +             if (!kref_get_unless_zero(&nbd->users))
>> +                     kref_init(&nbd->users);
>> +             mutex_unlock(&bdev->bd_mutex);
>> +
>> +             mutex_lock(&nbd->tx_lock);
>> +             nbd_reset(nbd);
>>               return error;
>>       }
>>
>> @@ -861,19 +877,71 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
>>
>>       return error;
>>  }
>> +static void nbd_kref_release(struct kref *kref_users)
>> +{
>> +     struct nbd_device *nbd = container_of(kref_users, struct nbd_device,
>> +                     users);
>> +     schedule_work(&nbd->ws_shutdown);
>> +}
>> +
>> +static int nbd_open(struct block_device *bdev, fmode_t mode)
>> +{
>> +     struct nbd_device *nbd_dev = bdev->bd_disk->private_data;
>> +
>> +     if (!kref_get_unless_zero(&nbd_dev->users))
>> +             kref_init(&nbd_dev->users);
>> +
>> +     pr_debug("Opening nbd_dev %s. Active users = %u\n",
>> +                     bdev->bd_disk->disk_name,
>> +                     atomic_read(&nbd_dev->users.refcount)
>> +             );
>> +     return 0;
>> +}
>> +
>> +static void nbd_release(struct gendisk *disk, fmode_t mode)
>> +{
>> +     struct nbd_device *nbd_dev = disk->private_data;
>> +
>> +     kref_put(&nbd_dev->users,  nbd_kref_release);
>> +
>> +     pr_debug("Closing nbd_dev %s. Active users = %u\n",
>> +                     disk->disk_name,
>> +                     atomic_read(&nbd_dev->users.refcount)
>> +             );
>> +}
>>
>>  static const struct block_device_operations nbd_fops = {
>>       .owner =        THIS_MODULE,
>>       .ioctl =        nbd_ioctl,
>>       .compat_ioctl = nbd_ioctl,
>> +     .open =         nbd_open,
>> +     .release =      nbd_release
>>  };
>>
>> +
>>  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);
>> +                                                     ws_shutdown);
>> +     struct block_device *bdev = bdget(part_devt(
>> +                                             dev_to_part(nbd_to_dev(nbd_dev))
>> +                                             )
>> +                                     );
>> +     BUG_ON(!bdev);
>> +     if (nbd_dev->timedout)
>> +             sock_shutdown(nbd_dev);
>> +
>> +     if (nbd_dev->disconnect) {
>> +             mutex_lock(&nbd_dev->tx_lock);
>> +             nbd_dev->task_recv = NULL;
>> +             nbd_clear_que(nbd_dev);
>> +             kill_bdev(bdev);
>> +             nbd_bdev_reset(bdev);
>> +             mutex_unlock(&nbd_dev->tx_lock);
>> +             nbd_size_clear(nbd_dev, bdev);
>> +             complete(&nbd_dev->user_completion);
>> +     }
>> +     bdput(bdev);
>>  }
>>
>>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>>
>
> --
> 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

[toc] | [prev] | [next] | [standalone]

#1430561 — [PATCH v3 1/3]nbd: fix might_sleep warning on socket shutdown

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 <mikulas@twibright.com>
Signed-off-by: Markus Pargmann <mpa@pengutronix.de>

Changelog:
Pranay Kr. Srivastava<pranjas@gmail.com>:

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 <pranjas@gmail.com>
---
 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 <asm/types.h>
 
 #include <linux/nbd.h>
+#include <linux/workqueue.h>
 
 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

[toc] | [prev] | [next] | [standalone]

#1432493 — Re: [PATCH v3 1/3]nbd: fix might_sleep warning on socket shutdown

Hi Markus,

On Fri, Jun 24, 2016 at 3:39 PM, Pranay Kr. Srivastava
<pranjas@gmail.com> 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 <mikulas@twibright.com>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
>
> Changelog:
> Pranay Kr. Srivastava<pranjas@gmail.com>:
>
> 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 <pranjas@gmail.com>
> ---
>  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 <asm/types.h>
>
>  #include <linux/nbd.h>
> +#include <linux/workqueue.h>
>
>  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

[toc] | [prev] | [next] | [standalone]

#1433501 — Re: [PATCH v3 1/3]nbd: fix might_sleep warning on socket shutdown

[Multipart message — attachments visible in raw view] — view raw

On Friday 24 June 2016 13:09:34 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 <mikulas@twibright.com>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> 
> Changelog:
> Pranay Kr. Srivastava<pranjas@gmail.com>:
> 
> 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 <pranjas@gmail.com>
> ---
>  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 <asm/types.h>
>  
>  #include <linux/nbd.h>
> +#include <linux/workqueue.h>
>  
>  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.
> +	*/

Please fix the indentation of this comment.

> +	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 *);

You could as well put the function implementation here. No need for a
function signature.

> +
>  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));

This is unrelated, please remove.

>  
>  		/* 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);

Unrelated as well.

>  		list_del_init(&req->queuelist);
>  		spin_unlock_irq(&nbd->queue_lock);
>  
> -		/* handle request */

Unrelated.

>  		nbd_handle_req(nbd, req);
> +		if (nbd->timedout) {
> +			req->errors++;
> +			nbd_end_request(nbd, req);
> +		} else
> +			nbd_handle_req(nbd, req);

This does not change anything to avoid the spinlock sock_shutdown issue.
Please split in a separate patch.

Also this is already handled in nbd_handle_req(). For !nbd->sock the
same code is executed.


Otherwise the patch looks good.

Best Regards,

Markus

>  	}
>  
>  	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)
> 

-- 
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 |

[toc] | [prev] | [next] | [standalone]

#1430564 — [PATCH v3 2/3]nbd: cleanup nbd_set_socket

This patch
1) uses spin_lock instead of irq version.

2) removes the goto statement in case a socket
   is already assigned with simple if-else statement.

Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
---
 drivers/block/nbd.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 586d946..9223b09 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -653,17 +653,12 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
 {
 	int ret = 0;
 
-	spin_lock_irq(&nbd->sock_lock);
-
-	if (nbd->sock) {
+	spin_lock(&nbd->sock_lock);
+	if (nbd->sock)
 		ret = -EBUSY;
-		goto out;
-	}
-
-	nbd->sock = sock;
-
-out:
-	spin_unlock_irq(&nbd->sock_lock);
+	else
+		nbd->sock = sock;
+	spin_unlock(&nbd->sock_lock);
 
 	return ret;
 }
-- 
1.9.1

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web