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

Groups > linux.kernel > #1406079 > unrolled thread

[PATCH 0/4]nbd: fixes for nbd

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4]nbd: fixes for nbd  "Pranay Kr. Srivastava" <pranjas@gmail.com> - 2016-05-24 13:30 +0200
    [PATCH 2/4] fix various coding standard warnings "Pranay Kr. Srivastava" <pranjas@gmail.com> - 2016-05-24 13:30 +0200
    [PATCH 1/4] fix might_sleep warning on socket shutdown. "Pranay Kr. Srivastava" <pranjas@gmail.com> - 2016-05-24 13:30 +0200
      Re: [PATCH 1/4] fix might_sleep warning on socket shutdown. Markus Pargmann <mpa@pengutronix.de> - 2016-05-30 14:10 +0200
    [PATCH 3/4] make nbd device wait for its users. "Pranay Kr. Srivastava" <pranjas@gmail.com> - 2016-05-24 13:30 +0200
      Re: [PATCH 3/4] make nbd device wait for its users. Markus Pargmann <mpa@pengutronix.de> - 2016-05-30 12:50 +0200
    [PATCH 4/4] use device_attr macros for sysfs attribute "Pranay Kr. Srivastava" <pranjas@gmail.com> - 2016-05-24 13:30 +0200
    Re: [PATCH 0/4]nbd: fixes for nbd Pranay Srivastava <pranjas@gmail.com> - 2016-05-30 05:40 +0200
    Re: [PATCH 0/4]nbd: fixes for nbd Markus Pargmann <mpa@pengutronix.de> - 2016-05-30 14:30 +0200

#1406079 — [PATCH 0/4]nbd: fixes for nbd

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)fix various coding standard warnings
   Make shutdown get called in a process context instead, using
   system_wq.

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.

4) use device_attr macros for sysfs attribute
   use DEVICE_ATTR_RO for sysfs pid attribute.

Pranay Kr. Srivastava (4):
  fix might_sleep warning on socket shutdown.
  fix various coding standard warnings
  make nbd device wait for its users.
  use device_attr macros for sysfs attribute

 drivers/block/nbd.c |  150 +++++++++++++++++++++++++++++++++++----------------
 1 file changed, 105 insertions(+), 45 deletions(-)

-- 
1.7.9.5

[toc] | [next] | [standalone]

#1406081 — [PATCH 2/4] fix various coding standard warnings

1 )nbd: fix checkpatch trailing space warning.

2) nbd: fix checkpatch warning use linux/uaccess.h

3) nbd : fix checkpatch pointer declaration warning

4) nbd: fix checkpatch warning no newline after decleration.

5) nbd: fix checkpatch warning no newline after decleration.

6) nbd : fix checkpatch line over 80 char warning

7) nbd: fix checkpatch trailing whitespace warning.

8) nbd: fix checkpatch trailing whitespace warning.

9) nbd : fix checkpatch structure declaration braces on next line warning.

10) nbd : fix checkpatch trailing whitespace warning

11) nbd : fix checkpatch printk warning

12) nbd: fix checkpatch no extra line after decleration warning

13) nbd: fix checkpatch printk warning to pr_info

14) nbd: fix checkpatch no new line after decleration warning

15) nbd: fix checkpatch printk warning to pr_info

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index a5dab48..af86c9b 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -3,7 +3,7 @@
  *
  * Note that you can not swap over this thing, yet. Seems to work but
  * deadlocks sometimes - you can not swap over TCP in general.
- * 
+ *
  * Copyright 1997-2000, 2008 Pavel Machek <pavel@ucw.cz>
  * Parts copyright 2001 Steven Whitehouse <steve@chygwyn.com>
  *
@@ -35,7 +35,7 @@
 #include <linux/types.h>
 #include <linux/debugfs.h>
 
-#include <asm/uaccess.h>
+#include <linux/uaccess.h>
 #include <asm/types.h>
 
 #include <linux/nbd.h>
@@ -43,7 +43,7 @@
 
 struct nbd_device {
 	u32 flags;
-	struct socket * sock;	/* If == NULL, device is not ready, yet	*/
+	struct socket *sock;	/* If == NULL, device is not ready, yet	*/
 	int magic;
 
 	spinlock_t queue_lock;
@@ -261,6 +261,7 @@ static inline int sock_send_bvec(struct nbd_device *nbd, struct bio_vec *bvec,
 {
 	int result;
 	void *kaddr = kmap(bvec->bv_page);
+
 	result = sock_xmit(nbd, 1, kaddr + bvec->bv_offset,
 			   bvec->bv_len, flags);
 	kunmap(bvec->bv_page);
@@ -358,6 +359,7 @@ static inline int sock_recv_bvec(struct nbd_device *nbd, struct bio_vec *bvec)
 {
 	int result;
 	void *kaddr = kmap(bvec->bv_page);
+
 	result = sock_xmit(nbd, 0, kaddr + bvec->bv_offset, bvec->bv_len,
 			MSG_WAITALL);
 	kunmap(bvec->bv_page);
@@ -600,8 +602,8 @@ static int nbd_thread_send(void *data)
 }
 
 /*
- * We always wait for result of write, for now. It would be nice to make it optional
- * in future
+ * We always wait for result of write, for now. It would be nice to make it
+ * optional in future
  * if ((rq_data_dir(req) == WRITE) && (nbd->flags & NBD_WRITE_NOCHK))
  *   { printk( "Warning: Ignoring result!\n"); nbd_end_request( req ); }
  */
@@ -610,7 +612,7 @@ static void nbd_request_handler(struct request_queue *q)
 		__releases(q->queue_lock) __acquires(q->queue_lock)
 {
 	struct request *req;
-	
+
 	while ((req = blk_fetch_request(q)) != NULL) {
 		struct nbd_device *nbd;
 
@@ -729,7 +731,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		nbd_send_req(nbd, &sreq);
 		return 0;
 	}
- 
+
 	case NBD_CLEAR_SOCK:
 		sock_shutdown(nbd);
 		nbd_clear_que(nbd);
@@ -856,8 +858,7 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
 	return error;
 }
 
-static const struct block_device_operations nbd_fops =
-{
+static const struct block_device_operations nbd_fops = {
 	.owner =	THIS_MODULE,
 	.ioctl =	nbd_ioctl,
 	.compat_ioctl =	nbd_ioctl,
@@ -1000,7 +1001,7 @@ static void nbd_dbg_close(void)
 #endif
 
 /*
- * And here should be modules and kernel interface 
+ * And here should be modules and kernel interface
  *  (Just smiley confuses emacs :-)
  */
 
@@ -1013,7 +1014,7 @@ static int __init nbd_init(void)
 	BUILD_BUG_ON(sizeof(struct nbd_request) != 28);
 
 	if (max_part < 0) {
-		printk(KERN_ERR "nbd: max_part must be >= 0\n");
+		pr_err("nbd: max_part must be >= 0\n");
 		return -EINVAL;
 	}
 
@@ -1044,6 +1045,7 @@ static int __init nbd_init(void)
 
 	for (i = 0; i < nbds_max; i++) {
 		struct gendisk *disk = alloc_disk(1 << part_shift);
+
 		if (!disk)
 			goto out;
 		nbd_dev[i].disk = disk;
@@ -1074,12 +1076,13 @@ static int __init nbd_init(void)
 		goto out;
 	}
 
-	printk(KERN_INFO "nbd: registered device at major %d\n", NBD_MAJOR);
+	pr_info("nbd: registered device at major %d\n", NBD_MAJOR);
 
 	nbd_dbg_init();
 
 	for (i = 0; i < nbds_max; i++) {
 		struct gendisk *disk = nbd_dev[i].disk;
+
 		nbd_dev[i].magic = NBD_MAGIC;
 		INIT_LIST_HEAD(&nbd_dev[i].waiting_queue);
 		spin_lock_init(&nbd_dev[i].queue_lock);
@@ -1127,7 +1130,7 @@ static void __exit nbd_cleanup(void)
 	}
 	unregister_blkdev(NBD_MAJOR, "nbd");
 	kfree(nbd_dev);
-	printk(KERN_INFO "nbd: unregistered device at major %d\n", NBD_MAJOR);
+	pr_info("nbd: unregistered device at major %d\n", NBD_MAJOR);
 }
 
 module_init(nbd_init);
-- 
1.7.9.5

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

#1406084 — [PATCH 1/4] 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 |   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 <asm/types.h>
 
 #include <linux/nbd.h>
+#include <linux/workqueue.h>
 
 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;
 	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;
 };
 
 #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 *);
 
 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);
+
+	sock_shutdown(nbd_dev);
+}
+
 #if IS_ENABLED(CONFIG_DEBUG_FS)
 
 static int nbd_dbg_tasks_show(struct seq_file *s, void *unused)
-- 
1.7.9.5

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

#1409034 — Re: [PATCH 1/4] fix might_sleep warning on socket shutdown.

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

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

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

#1406085 — [PATCH 3/4] 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.

Use a kref for users using this. The device will
be released for kref count of 2, not less or more.

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index af86c9b..59db890 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -71,6 +71,8 @@ struct nbd_device {
 	struct dentry *dbg_dir;
 #endif
 	struct work_struct ws_nbd;
+	struct kref users;
+	struct completion user_completion;
 };
 
 #if IS_ENABLED(CONFIG_DEBUG_FS)
@@ -674,6 +676,7 @@ static void nbd_reset(struct nbd_device *nbd)
 	nbd->flags = 0;
 	nbd->xmit_timeout = 0;
 	INIT_WORK(&nbd->ws_nbd, nbd_work_func);
+	init_completion(&nbd->user_completion);
 	queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
 	del_timer_sync(&nbd->timeout_timer);
 }
@@ -807,6 +810,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		kthread_stop(thread);
 
 		sock_shutdown(nbd);
+		wait_for_completion(&nbd->user_completion);
 		mutex_lock(&nbd->tx_lock);
 		nbd_clear_que(nbd);
 		kill_bdev(bdev);
@@ -858,12 +862,58 @@ 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);
+	pr_debug("Releasing kref [%s]\n", __FUNCTION__);
+	complete(&nbd->user_completion);
+
+}
+
+static int nbd_open(struct block_device *bdev, fmode_t mode)
+{
+	struct nbd_device *nbd_dev = bdev->bd_disk->private_data;
+
+	kref_get(&nbd_dev->users);
+	pr_debug("Opening nbd_dev %s. Active users = %u\n",
+			bdev->bd_disk->disk_name,
+			atomic_read(&nbd_dev->users.refcount) - 1);
+	return 0;
+}
+
+static void nbd_release(struct gendisk *disk, fmode_t mode)
+{
+	struct nbd_device *nbd_dev = disk->private_data;
+	/*
+	*kref_init initializes ref count to 1, so we
+	*we check for refcount to be 2 for a final put.
+	*
+	*kref needs to be re-initialized just here as the
+	*other process holding it must see the ref count as 2.
+	*/
+	kref_put(&nbd_dev->users,  nbd_kref_release);
+
+	if (atomic_read(&nbd_dev->users.refcount) == 2) {
+		kref_sub(&nbd_dev->users, 2, nbd_kref_release);
+		kref_init(&nbd_dev->users);
+		kref_get(&nbd_dev->users);
+	}
+
+	pr_debug("Closing nbd_dev %s. Active users = %u\n",
+			disk->disk_name,
+			atomic_read(&nbd_dev->users.refcount) - 1);
+}
+
 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_work_func(struct work_struct *ws_nbd)
 {
 	struct nbd_device *nbd_dev = container_of(ws_nbd, struct nbd_device,
@@ -1098,6 +1148,7 @@ static int __init nbd_init(void)
 		disk->first_minor = i << part_shift;
 		disk->fops = &nbd_fops;
 		disk->private_data = &nbd_dev[i];
+		kref_init(&nbd_dev[i].users);
 		sprintf(disk->disk_name, "nbd%d", i);
 		nbd_reset(&nbd_dev[i]);
 		add_disk(disk);
-- 
1.7.9.5

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

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

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

Hi,

sorry I couldn't fit the review into last week.

On Tuesday 24 May 2016 14:26:27 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.
> 
> Use a kref for users using this. The device will
> be released for kref count of 2, not less or more.
> 
> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
> ---
>  drivers/block/nbd.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index af86c9b..59db890 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -71,6 +71,8 @@ struct nbd_device {
>  	struct dentry *dbg_dir;
>  #endif
>  	struct work_struct ws_nbd;
> +	struct kref users;
> +	struct completion user_completion;
>  };
>  
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -674,6 +676,7 @@ static void nbd_reset(struct nbd_device *nbd)
>  	nbd->flags = 0;
>  	nbd->xmit_timeout = 0;
>  	INIT_WORK(&nbd->ws_nbd, nbd_work_func);
> +	init_completion(&nbd->user_completion);
>  	queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
>  	del_timer_sync(&nbd->timeout_timer);
>  }
> @@ -807,6 +810,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>  		kthread_stop(thread);
>  
>  		sock_shutdown(nbd);
> +		wait_for_completion(&nbd->user_completion);
>  		mutex_lock(&nbd->tx_lock);
>  		nbd_clear_que(nbd);
>  		kill_bdev(bdev);
> @@ -858,12 +862,58 @@ 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);
> +	pr_debug("Releasing kref [%s]\n", __FUNCTION__);
> +	complete(&nbd->user_completion);
> +
> +}
> +
> +static int nbd_open(struct block_device *bdev, fmode_t mode)
> +{
> +	struct nbd_device *nbd_dev = bdev->bd_disk->private_data;
> +
> +	kref_get(&nbd_dev->users);
> +	pr_debug("Opening nbd_dev %s. Active users = %u\n",
> +			bdev->bd_disk->disk_name,
> +			atomic_read(&nbd_dev->users.refcount) - 1);
> +	return 0;
> +}
> +
> +static void nbd_release(struct gendisk *disk, fmode_t mode)
> +{
> +	struct nbd_device *nbd_dev = disk->private_data;
> +	/*
> +	*kref_init initializes ref count to 1, so we
> +	*we check for refcount to be 2 for a final put.
> +	*
> +	*kref needs to be re-initialized just here as the
> +	*other process holding it must see the ref count as 2.
> +	*/
> +	kref_put(&nbd_dev->users,  nbd_kref_release);
> +
> +	if (atomic_read(&nbd_dev->users.refcount) == 2) {
> +		kref_sub(&nbd_dev->users, 2, nbd_kref_release);
> +		kref_init(&nbd_dev->users);
> +		kref_get(&nbd_dev->users);

Reading the refcount directly seems not to be as it supposed to be.

Why don't you put a kref_init() and kref_put() call into NBD_DO_IT? This
way you don't have to work around the property that kref_init() starts
with a refcount of 1 but you can use it.

For example:
	NBD_DO_IT:
		kref_init()
		...
		kref_put()

	nbd_thread_recv() and nbd_thread_send():
		kref_get()
		...
		kref_put()

	In nbd_open() you could use kref_get_unless_zero() to avoid
	opening a not connected device.

	nbd_release() would then be a very simple kref_put() without
	checking for 2 and so on.

Also there are some checkpatch issues with this patch.

Best Regards,

Markus

> +	}
> +
> +	pr_debug("Closing nbd_dev %s. Active users = %u\n",
> +			disk->disk_name,
> +			atomic_read(&nbd_dev->users.refcount) - 1);
> +}
> +
>  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_work_func(struct work_struct *ws_nbd)
>  {
>  	struct nbd_device *nbd_dev = container_of(ws_nbd, struct nbd_device,
> @@ -1098,6 +1148,7 @@ static int __init nbd_init(void)
>  		disk->first_minor = i << part_shift;
>  		disk->fops = &nbd_fops;
>  		disk->private_data = &nbd_dev[i];
> +		kref_init(&nbd_dev[i].users);
>  		sprintf(disk->disk_name, "nbd%d", i);
>  		nbd_reset(&nbd_dev[i]);
>  		add_disk(disk);
> 

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

#1406087 — [PATCH 4/4] use device_attr macros for sysfs attribute

This patch changes the pid sysfs device attribute to use
DEVICE_ATTR_* macro.

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 59db890..d6ab9c9 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -436,10 +436,8 @@ static ssize_t pid_show(struct device *dev,
 	return sprintf(buf, "%d\n", task_pid_nr(nbd->task_recv));
 }
 
-static struct device_attribute pid_attr = {
-	.attr = { .name = "pid", .mode = S_IRUGO},
-	.show = pid_show,
-};
+
+static DEVICE_ATTR_RO(pid);
 
 static int nbd_thread_recv(struct nbd_device *nbd, struct block_device *bdev)
 {
@@ -452,7 +450,7 @@ static int nbd_thread_recv(struct nbd_device *nbd, struct block_device *bdev)
 
 	nbd->task_recv = current;
 
-	ret = device_create_file(disk_to_dev(nbd->disk), &pid_attr);
+	ret = device_create_file(disk_to_dev(nbd->disk), &dev_attr_pid);
 	if (ret) {
 		dev_err(disk_to_dev(nbd->disk), "device_create_file failed!\n");
 
@@ -475,7 +473,7 @@ static int nbd_thread_recv(struct nbd_device *nbd, struct block_device *bdev)
 
 	nbd_size_clear(nbd, bdev);
 
-	device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
+	device_remove_file(disk_to_dev(nbd->disk), &dev_attr_pid);
 
 	nbd->task_recv = NULL;
 
-- 
1.7.9.5

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

#1408748

Hi

On Tue, May 24, 2016 at 4:56 PM, Pranay Kr. Srivastava
<pranjas@gmail.com> wrote:
> 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)fix various coding standard warnings
>    Make shutdown get called in a process context instead, using
>    system_wq.
>
> 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.
>
> 4) use device_attr macros for sysfs attribute
>    use DEVICE_ATTR_RO for sysfs pid attribute.
>
> Pranay Kr. Srivastava (4):
>   fix might_sleep warning on socket shutdown.
>   fix various coding standard warnings
>   make nbd device wait for its users.
>   use device_attr macros for sysfs attribute
>
>  drivers/block/nbd.c |  150 +++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 105 insertions(+), 45 deletions(-)
>
> --
> 1.7.9.5
>

Can anyone review this?

-- 
        ---P.K.S

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

#1409057

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

Hi,

On Tuesday 24 May 2016 14:26:24 Pranay Kr. Srivastava wrote:
> 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)fix various coding standard warnings
>    Make shutdown get called in a process context instead, using
>    system_wq.
> 
> 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.
> 
> 4) use device_attr macros for sysfs attribute
>    use DEVICE_ATTR_RO for sysfs pid attribute.

Thanks, I applied patches 2 and 4. For the next version please add a
"nbd: " prefix to the other patches.

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 |

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web