[PATCH v4 00/18] nbd: fixes for might_sleep warning, checkpatch warning and device wait.

[PATCH v4 00/18] nbd: fixes for might_sleep warning, checkpatch warning and device wait.

[Pranay Kr. Srivastava]

Changes in v4
	- make nbd device wait for users to release
	  device instead of hard reset on a timeout
	  error as not all filesystems expect their
	  block device to vanish under them.

Changes in v3
	- Split checkpatch changes into single patch per change.

Changes in v2
	- add checkpatch changes in a single patch.

Changes in v1
	- fix might_sleep warning on xmit_timeout.



Pranay Kr. Srivastava (18):
  nbd: Fix might_sleep warning on xmit timeout
  nbd: fix checkpatch trailing space warning.
  nbd: fix checkpatch warning use linux/uaccess.h
  nbd : fix checkpatch pointer declaration warning
  nbd: fix checkpatch warning no newline after decleration.
  nbd: fix checkpatch warning no newline after decleration.
  nbd: fix checkpatch split string warning.
  nbd : fix checkpatch line over 80 char warning
  nbd: fix checkpatch trailing whitespace warning.
  nbd: fix checkpatch trailing whitespace warning.
  nbd : fix checkpatch structure declaration braces on next line
    warning.
  nbd : fix checkpatch trailing whitespace warning
  nbd : fix checkpatch printk warning
  nbd: fix checkpatch no extra line after decleration warning
  nbd: fix checkpatch printk warning to pr_info
  nbd: fix checkpatch no new line after decleration warning
  nbd: fix checkpatch printk warning to pr_info
  make nbd device wait for its users in case of timeout

 drivers/block/nbd.c      | 131 +++++++++++++++++++++++++++++++++--------------
 include/uapi/linux/nbd.h |   1 +
 2 files changed, 94 insertions(+), 38 deletions(-)

-- 
2.6.2

[PATCH v4 01/18] nbd: Fix might_sleep warning on xmit timeout

[Pranay Kr. Srivastava]

This patch fixes the warning generated when a timeout occurs
on the request and socket is closed from a non-sleep context
by

1. Moving the socket closing on a timeout to nbd_thread_send

2. Make sock lock to be a mutex instead of a spin lock, since
   nbd_xmit_timeout doesn't need to hold it anymore.

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 31e73a7..c79bcd7 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -57,12 +57,12 @@ 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;
 	/* protects initialization and shutdown of the socket */
-	spinlock_t sock_lock;
+	struct mutex sock_lock;
 	struct task_struct *task_recv;
 	struct task_struct *task_send;
 
@@ -172,10 +172,9 @@ 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);
-
+	mutex_lock(&nbd->sock_lock);
 	if (!nbd->sock) {
-		spin_unlock_irq(&nbd->sock_lock);
+		mutex_unlock(&nbd->sock_lock);
 		return;
 	}
 
@@ -183,27 +182,19 @@ static void sock_shutdown(struct nbd_device *nbd)
 	kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
 	sockfd_put(nbd->sock);
 	nbd->sock = NULL;
-	spin_unlock_irq(&nbd->sock_lock);
-
+	mutex_unlock(&nbd->sock_lock);
 	del_timer(&nbd->timeout_timer);
 }
 
 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);
+	wake_up(&nbd->waiting_wq);
 
 	dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connection\n");
 }
@@ -579,7 +570,27 @@ static int nbd_thread_send(void *data)
 		/* wait for something to do */
 		wait_event_interruptible(nbd->waiting_wq,
 					 kthread_should_stop() ||
-					 !list_empty(&nbd->waiting_queue));
+					 !list_empty(&nbd->waiting_queue) ||
+					 atomic_read(&nbd->timedout));
+
+		if (atomic_read(&nbd->timedout)) {
+			mutex_lock(&nbd->sock_lock);
+			if (nbd->sock) {
+				struct request sreq;
+
+				blk_rq_init(NULL, &sreq);
+				sreq.cmd_type = REQ_TYPE_DRV_PRIV;
+				mutex_lock(&nbd->tx_lock);
+				nbd->disconnect = true;
+				nbd_send_req(nbd, &sreq);
+				mutex_unlock(&nbd->tx_lock);
+				dev_err(disk_to_dev(nbd->disk),
+					"Device Timeout occured.Shutting down"
+					" socket.");
+			}
+			mutex_unlock(&nbd->sock_lock);
+			sock_shutdown(nbd);
+		}
 
 		/* extract request */
 		if (list_empty(&nbd->waiting_queue))
@@ -592,7 +603,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;
@@ -647,7 +662,7 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
 {
 	int ret = 0;
 
-	spin_lock_irq(&nbd->sock_lock);
+	mutex_lock(&nbd->sock_lock);
 
 	if (nbd->sock) {
 		ret = -EBUSY;
@@ -657,7 +672,7 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
 	nbd->sock = sock;
 
 out:
-	spin_unlock_irq(&nbd->sock_lock);
+	mutex_unlock(&nbd->sock_lock);
 
 	return ret;
 }
@@ -666,7 +681,7 @@ 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);
@@ -803,17 +818,15 @@ 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);
-
-		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);
@@ -1075,7 +1088,7 @@ static int __init nbd_init(void)
 		nbd_dev[i].magic = NBD_MAGIC;
 		INIT_LIST_HEAD(&nbd_dev[i].waiting_queue);
 		spin_lock_init(&nbd_dev[i].queue_lock);
-		spin_lock_init(&nbd_dev[i].sock_lock);
+		mutex_init(&nbd_dev[i].sock_lock);
 		INIT_LIST_HEAD(&nbd_dev[i].queue_head);
 		mutex_init(&nbd_dev[i].tx_lock);
 		init_timer(&nbd_dev[i].timeout_timer);
-- 
2.6.2

Re: [PATCH v4 01/18] nbd: Fix might_sleep warning on xmit timeout

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 6168 bytes --]

Hi,

On Wednesday 11 May 2016 11:18:29 Pranay Kr. Srivastava wrote:
> This patch fixes the warning generated when a timeout occurs
> on the request and socket is closed from a non-sleep context
> by
> 
> 1. Moving the socket closing on a timeout to nbd_thread_send
> 
> 2. Make sock lock to be a mutex instead of a spin lock, since
>    nbd_xmit_timeout doesn't need to hold it anymore.

This patch seems quite big and complicated. Isn't the main issue that a
socket shutdown is called from within a spinlock?

When the issue got reported by Mikulas Patocka I created a patch but
forgot to send it, sorry. It is a bit simpler by simpling moving the
socket shutdown out of the spinlock. I will send it as reply. Please
have a look.

Thanks,

Markus

> 
> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
> ---
>  drivers/block/nbd.c | 65 ++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 39 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 31e73a7..c79bcd7 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -57,12 +57,12 @@ 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;
>  	/* protects initialization and shutdown of the socket */
> -	spinlock_t sock_lock;
> +	struct mutex sock_lock;
>  	struct task_struct *task_recv;
>  	struct task_struct *task_send;
>  
> @@ -172,10 +172,9 @@ 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);
> -
> +	mutex_lock(&nbd->sock_lock);
>  	if (!nbd->sock) {
> -		spin_unlock_irq(&nbd->sock_lock);
> +		mutex_unlock(&nbd->sock_lock);
>  		return;
>  	}
>  
> @@ -183,27 +182,19 @@ static void sock_shutdown(struct nbd_device *nbd)
>  	kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
>  	sockfd_put(nbd->sock);
>  	nbd->sock = NULL;
> -	spin_unlock_irq(&nbd->sock_lock);
> -
> +	mutex_unlock(&nbd->sock_lock);
>  	del_timer(&nbd->timeout_timer);
>  }
>  
>  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);
> +	wake_up(&nbd->waiting_wq);
>  
>  	dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connection\n");
>  }
> @@ -579,7 +570,27 @@ static int nbd_thread_send(void *data)
>  		/* wait for something to do */
>  		wait_event_interruptible(nbd->waiting_wq,
>  					 kthread_should_stop() ||
> -					 !list_empty(&nbd->waiting_queue));
> +					 !list_empty(&nbd->waiting_queue) ||
> +					 atomic_read(&nbd->timedout));
> +
> +		if (atomic_read(&nbd->timedout)) {
> +			mutex_lock(&nbd->sock_lock);
> +			if (nbd->sock) {
> +				struct request sreq;
> +
> +				blk_rq_init(NULL, &sreq);
> +				sreq.cmd_type = REQ_TYPE_DRV_PRIV;
> +				mutex_lock(&nbd->tx_lock);
> +				nbd->disconnect = true;
> +				nbd_send_req(nbd, &sreq);
> +				mutex_unlock(&nbd->tx_lock);
> +				dev_err(disk_to_dev(nbd->disk),
> +					"Device Timeout occured.Shutting down"
> +					" socket.");
> +			}
> +			mutex_unlock(&nbd->sock_lock);
> +			sock_shutdown(nbd);
> +		}
>  
>  		/* extract request */
>  		if (list_empty(&nbd->waiting_queue))
> @@ -592,7 +603,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;
> @@ -647,7 +662,7 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
>  {
>  	int ret = 0;
>  
> -	spin_lock_irq(&nbd->sock_lock);
> +	mutex_lock(&nbd->sock_lock);
>  
>  	if (nbd->sock) {
>  		ret = -EBUSY;
> @@ -657,7 +672,7 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
>  	nbd->sock = sock;
>  
>  out:
> -	spin_unlock_irq(&nbd->sock_lock);
> +	mutex_unlock(&nbd->sock_lock);
>  
>  	return ret;
>  }
> @@ -666,7 +681,7 @@ 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);
> @@ -803,17 +818,15 @@ 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);
> -
> -		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);
> @@ -1075,7 +1088,7 @@ static int __init nbd_init(void)
>  		nbd_dev[i].magic = NBD_MAGIC;
>  		INIT_LIST_HEAD(&nbd_dev[i].waiting_queue);
>  		spin_lock_init(&nbd_dev[i].queue_lock);
> -		spin_lock_init(&nbd_dev[i].sock_lock);
> +		mutex_init(&nbd_dev[i].sock_lock);
>  		INIT_LIST_HEAD(&nbd_dev[i].queue_head);
>  		mutex_init(&nbd_dev[i].tx_lock);
>  		init_timer(&nbd_dev[i].timeout_timer);
> 

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH] nbd: Move socket shutdown out of spinlock

[Markus Pargmann]

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>
---
 drivers/block/nbd.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 0b892eed06a0..157bf3da876e 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -173,20 +173,22 @@ static void nbd_end_request(struct nbd_device *nbd, struct request *req)
  */
 static void sock_shutdown(struct nbd_device *nbd)
 {
+	struct socket *sock;
+
 	spin_lock_irq(&nbd->sock_lock);
+	sock = nbd->sock;
+	nbd->sock = NULL;
+	spin_unlock_irq(&nbd->sock_lock);
 
-	if (!nbd->sock) {
-		spin_unlock_irq(&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(nbd->sock, SHUT_RDWR);
-	sockfd_put(nbd->sock);
-	nbd->sock = NULL;
-	spin_unlock_irq(&nbd->sock_lock);
+	kernel_sock_shutdown(sock, SHUT_RDWR);
+	sockfd_put(sock);
 
-	del_timer(&nbd->timeout_timer);
 }
 
 static void nbd_xmit_timeout(unsigned long arg)
-- 
2.8.0.rc3

Re: [PATCH] nbd: Move socket shutdown out of spinlock

[Pranay Srivastava]

Hi Markus,


On Thu, May 12, 2016 at 3:13 PM, Markus Pargmann <mpa@pengutronix.de> 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>
> ---
>  drivers/block/nbd.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 0b892eed06a0..157bf3da876e 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -173,20 +173,22 @@ static void nbd_end_request(struct nbd_device *nbd, struct request *req)
>   */
>  static void sock_shutdown(struct nbd_device *nbd)
>  {
> +       struct socket *sock;
> +
>         spin_lock_irq(&nbd->sock_lock);
> +       sock = nbd->sock;
> +       nbd->sock = NULL;
> +       spin_unlock_irq(&nbd->sock_lock);
>
> -       if (!nbd->sock) {
> -               spin_unlock_irq(&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(nbd->sock, SHUT_RDWR);
> -       sockfd_put(nbd->sock);
> -       nbd->sock = NULL;
> -       spin_unlock_irq(&nbd->sock_lock);
> +       kernel_sock_shutdown(sock, SHUT_RDWR);
> +       sockfd_put(sock);
>
> -       del_timer(&nbd->timeout_timer);
>  }
>
>  static void nbd_xmit_timeout(unsigned long arg)

I was concerned about nbd_xmit_timeout as well. There's also a call to
kernel_sock_shutdown,
while holding the spin_lock in the timeout. The above is ok for
sock_shutdown but some kind of change
is also required in nbd_xmit_timeout as well. My patch addressed both these.

Can you have a look at that again.


> --
> 2.8.0.rc3
>



-- 
        ---P.K.S

Re: [PATCH] nbd: Move socket shutdown out of spinlock

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 2871 bytes --]

Hi,

On Thursday 12 May 2016 16:42:31 Pranay Srivastava wrote:
> Hi Markus,
> 
> 
> On Thu, May 12, 2016 at 3:13 PM, Markus Pargmann <mpa@pengutronix.de> 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>
> > ---
> >  drivers/block/nbd.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > index 0b892eed06a0..157bf3da876e 100644
> > --- a/drivers/block/nbd.c
> > +++ b/drivers/block/nbd.c
> > @@ -173,20 +173,22 @@ static void nbd_end_request(struct nbd_device *nbd, struct request *req)
> >   */
> >  static void sock_shutdown(struct nbd_device *nbd)
> >  {
> > +       struct socket *sock;
> > +
> >         spin_lock_irq(&nbd->sock_lock);
> > +       sock = nbd->sock;
> > +       nbd->sock = NULL;
> > +       spin_unlock_irq(&nbd->sock_lock);
> >
> > -       if (!nbd->sock) {
> > -               spin_unlock_irq(&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(nbd->sock, SHUT_RDWR);
> > -       sockfd_put(nbd->sock);
> > -       nbd->sock = NULL;
> > -       spin_unlock_irq(&nbd->sock_lock);
> > +       kernel_sock_shutdown(sock, SHUT_RDWR);
> > +       sockfd_put(sock);
> >
> > -       del_timer(&nbd->timeout_timer);
> >  }
> >
> >  static void nbd_xmit_timeout(unsigned long arg)
> 
> I was concerned about nbd_xmit_timeout as well. There's also a call to
> kernel_sock_shutdown,
> while holding the spin_lock in the timeout. The above is ok for
> sock_shutdown but some kind of change
> is also required in nbd_xmit_timeout as well. My patch addressed both these.

Oh I see thanks. Seems there is some duplicate code in
nbd_xmit_timeout and sock_shutdown. I think nbd_xmit_timeout could
perhaps be simplified?

	static void nbd_xmit_timeout(unsigned long arg)
	{
		struct nbd_device *nbd = (struct nbd_device *)arg;

		if (list_empty(&nbd->queue_head))
			return;

		nbd->timedout = true;
		dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connection\n");

		sock_shutdown(nbd);
	}

Thanks,

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 |

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] nbd: Move socket shutdown out of spinlock

[Pranay Srivastava]

On Thu, May 12, 2016 at 6:13 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
> Hi,
>
> On Thursday 12 May 2016 16:42:31 Pranay Srivastava wrote:
>> Hi Markus,
>>
>>
>> On Thu, May 12, 2016 at 3:13 PM, Markus Pargmann <mpa@pengutronix.de> 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>
>> > ---
>> >  drivers/block/nbd.c | 18 ++++++++++--------
>> >  1 file changed, 10 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> > index 0b892eed06a0..157bf3da876e 100644
>> > --- a/drivers/block/nbd.c
>> > +++ b/drivers/block/nbd.c
>> > @@ -173,20 +173,22 @@ static void nbd_end_request(struct nbd_device *nbd, struct request *req)
>> >   */
>> >  static void sock_shutdown(struct nbd_device *nbd)
>> >  {
>> > +       struct socket *sock;
>> > +
>> >         spin_lock_irq(&nbd->sock_lock);
>> > +       sock = nbd->sock;
>> > +       nbd->sock = NULL;
>> > +       spin_unlock_irq(&nbd->sock_lock);
>> >
>> > -       if (!nbd->sock) {
>> > -               spin_unlock_irq(&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(nbd->sock, SHUT_RDWR);
>> > -       sockfd_put(nbd->sock);
>> > -       nbd->sock = NULL;
>> > -       spin_unlock_irq(&nbd->sock_lock);
>> > +       kernel_sock_shutdown(sock, SHUT_RDWR);
>> > +       sockfd_put(sock);
>> >
>> > -       del_timer(&nbd->timeout_timer);
>> >  }
>> >
>> >  static void nbd_xmit_timeout(unsigned long arg)
>>
>> I was concerned about nbd_xmit_timeout as well. There's also a call to
>> kernel_sock_shutdown,
>> while holding the spin_lock in the timeout. The above is ok for
>> sock_shutdown but some kind of change
>> is also required in nbd_xmit_timeout as well. My patch addressed both these.
>
> Oh I see thanks. Seems there is some duplicate code in
> nbd_xmit_timeout and sock_shutdown. I think nbd_xmit_timeout could
> perhaps be simplified?
>
>         static void nbd_xmit_timeout(unsigned long arg)
>         {
>                 struct nbd_device *nbd = (struct nbd_device *)arg;
>
>                 if (list_empty(&nbd->queue_head))
>                         return;
>
>                 nbd->timedout = true;
>                 dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connection\n");
>
>                 sock_shutdown(nbd);

Even then, the timeout is non-process context so there will still be a
might_sleep warning.
So sock_shutdown itself can be simplified, as you posted in earlier patch,
but I don't think sock_shutdown should be called from non-process context[?].

>         }
>
> Thanks,
>
> 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 |



-- 
        ---P.K.S

Re: [PATCH v4 01/18] nbd: Fix might_sleep warning on xmit timeout

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 6185 bytes --]

Hi,

On Wed, May 11, 2016 at 11:18:29AM +0300, Pranay Kr. Srivastava wrote:
> This patch fixes the warning generated when a timeout occurs
> on the request and socket is closed from a non-sleep context
> by
> 
> 1. Moving the socket closing on a timeout to nbd_thread_send

What happens if a send blocks?

> 
> 2. Make sock lock to be a mutex instead of a spin lock, since
>    nbd_xmit_timeout doesn't need to hold it anymore.

I can't see why we need a mutex instead of a spinlock?

> 
> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
> ---
>  drivers/block/nbd.c | 65 ++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 39 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 31e73a7..c79bcd7 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -57,12 +57,12 @@ 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;
>  	/* protects initialization and shutdown of the socket */
> -	spinlock_t sock_lock;
> +	struct mutex sock_lock;
>  	struct task_struct *task_recv;
>  	struct task_struct *task_send;
>  
> @@ -172,10 +172,9 @@ 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);
> -
> +	mutex_lock(&nbd->sock_lock);
>  	if (!nbd->sock) {
> -		spin_unlock_irq(&nbd->sock_lock);
> +		mutex_unlock(&nbd->sock_lock);
>  		return;
>  	}
>  
> @@ -183,27 +182,19 @@ static void sock_shutdown(struct nbd_device *nbd)
>  	kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
>  	sockfd_put(nbd->sock);
>  	nbd->sock = NULL;
> -	spin_unlock_irq(&nbd->sock_lock);
> -
> +	mutex_unlock(&nbd->sock_lock);
>  	del_timer(&nbd->timeout_timer);
>  }
>  
>  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);
> +	wake_up(&nbd->waiting_wq);
>  
>  	dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connection\n");
>  }
> @@ -579,7 +570,27 @@ static int nbd_thread_send(void *data)
>  		/* wait for something to do */
>  		wait_event_interruptible(nbd->waiting_wq,
>  					 kthread_should_stop() ||
> -					 !list_empty(&nbd->waiting_queue));
> +					 !list_empty(&nbd->waiting_queue) ||
> +					 atomic_read(&nbd->timedout));
> +
> +		if (atomic_read(&nbd->timedout)) {
> +			mutex_lock(&nbd->sock_lock);
> +			if (nbd->sock) {
> +				struct request sreq;
> +
> +				blk_rq_init(NULL, &sreq);
> +				sreq.cmd_type = REQ_TYPE_DRV_PRIV;
> +				mutex_lock(&nbd->tx_lock);
> +				nbd->disconnect = true;
> +				nbd_send_req(nbd, &sreq);
> +				mutex_unlock(&nbd->tx_lock);
> +				dev_err(disk_to_dev(nbd->disk),
> +					"Device Timeout occured.Shutting down"
> +					" socket.");
> +			}
> +			mutex_unlock(&nbd->sock_lock);
> +			sock_shutdown(nbd);

Why are you trying to send something on a connection that timed out
(nbd_send_req())? And afterwards you execute a socket shutdown so in most
timeout cases this won't reach the server and we risk a blocking send on
a timedout connection.

Regards,

Markus

> +		}
>  
>  		/* extract request */
>  		if (list_empty(&nbd->waiting_queue))
> @@ -592,7 +603,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;
> @@ -647,7 +662,7 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
>  {
>  	int ret = 0;
>  
> -	spin_lock_irq(&nbd->sock_lock);
> +	mutex_lock(&nbd->sock_lock);
>  
>  	if (nbd->sock) {
>  		ret = -EBUSY;
> @@ -657,7 +672,7 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
>  	nbd->sock = sock;
>  
>  out:
> -	spin_unlock_irq(&nbd->sock_lock);
> +	mutex_unlock(&nbd->sock_lock);
>  
>  	return ret;
>  }
> @@ -666,7 +681,7 @@ 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);
> @@ -803,17 +818,15 @@ 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);
> -
> -		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);
> @@ -1075,7 +1088,7 @@ static int __init nbd_init(void)
>  		nbd_dev[i].magic = NBD_MAGIC;
>  		INIT_LIST_HEAD(&nbd_dev[i].waiting_queue);
>  		spin_lock_init(&nbd_dev[i].queue_lock);
> -		spin_lock_init(&nbd_dev[i].sock_lock);
> +		mutex_init(&nbd_dev[i].sock_lock);
>  		INIT_LIST_HEAD(&nbd_dev[i].queue_head);
>  		mutex_init(&nbd_dev[i].tx_lock);
>  		init_timer(&nbd_dev[i].timeout_timer);
> -- 
> 2.6.2
> 
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 01/18] nbd: Fix might_sleep warning on xmit timeout

[Pranay Srivastava]

On Thu, May 19, 2016 at 11:52 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> Hi,
>
> On Wed, May 11, 2016 at 11:18:29AM +0300, Pranay Kr. Srivastava wrote:
>> This patch fixes the warning generated when a timeout occurs
>> on the request and socket is closed from a non-sleep context
>> by
>>
>> 1. Moving the socket closing on a timeout to nbd_thread_send
>
> What happens if a send blocks?

socket closing needs to be moved to a non-atomic context and,
sender thread seemed to be a good place to do this. If you mean
send blocks just before calling sock_shutdown[?] in nbd_thread_send
then yes I think that should be removed. I need to re-check how nbd-server
behaves in that case.

>
>>
>> 2. Make sock lock to be a mutex instead of a spin lock, since
>>    nbd_xmit_timeout doesn't need to hold it anymore.
>
> I can't see why we need a mutex instead of a spinlock?

you are right, with your earlier patch we don't need it to be a mutex.

>
>>
>> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
>> ---
>>  drivers/block/nbd.c | 65 ++++++++++++++++++++++++++++++++---------------------
>>  1 file changed, 39 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> index 31e73a7..c79bcd7 100644
>> --- a/drivers/block/nbd.c
>> +++ b/drivers/block/nbd.c
>> @@ -57,12 +57,12 @@ 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;
>>       /* protects initialization and shutdown of the socket */
>> -     spinlock_t sock_lock;
>> +     struct mutex sock_lock;
>>       struct task_struct *task_recv;
>>       struct task_struct *task_send;
>>
>> @@ -172,10 +172,9 @@ 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);
>> -
>> +     mutex_lock(&nbd->sock_lock);
>>       if (!nbd->sock) {
>> -             spin_unlock_irq(&nbd->sock_lock);
>> +             mutex_unlock(&nbd->sock_lock);
>>               return;
>>       }
>>
>> @@ -183,27 +182,19 @@ static void sock_shutdown(struct nbd_device *nbd)
>>       kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
>>       sockfd_put(nbd->sock);
>>       nbd->sock = NULL;
>> -     spin_unlock_irq(&nbd->sock_lock);
>> -
>> +     mutex_unlock(&nbd->sock_lock);
>>       del_timer(&nbd->timeout_timer);
>>  }
>>
>>  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);
>> +     wake_up(&nbd->waiting_wq);
>>
>>       dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connection\n");
>>  }
>> @@ -579,7 +570,27 @@ static int nbd_thread_send(void *data)
>>               /* wait for something to do */
>>               wait_event_interruptible(nbd->waiting_wq,
>>                                        kthread_should_stop() ||
>> -                                      !list_empty(&nbd->waiting_queue));
>> +                                      !list_empty(&nbd->waiting_queue) ||
>> +                                      atomic_read(&nbd->timedout));
>> +
>> +             if (atomic_read(&nbd->timedout)) {
>> +                     mutex_lock(&nbd->sock_lock);
>> +                     if (nbd->sock) {
>> +                             struct request sreq;
>> +
>> +                             blk_rq_init(NULL, &sreq);
>> +                             sreq.cmd_type = REQ_TYPE_DRV_PRIV;
>> +                             mutex_lock(&nbd->tx_lock);
>> +                             nbd->disconnect = true;
>> +                             nbd_send_req(nbd, &sreq);
>> +                             mutex_unlock(&nbd->tx_lock);
>> +                             dev_err(disk_to_dev(nbd->disk),
>> +                                     "Device Timeout occured.Shutting down"
>> +                                     " socket.");
>> +                     }
>> +                     mutex_unlock(&nbd->sock_lock);
>> +                     sock_shutdown(nbd);
>
> Why are you trying to send something on a connection that timed out
> (nbd_send_req())? And afterwards you execute a socket shutdown so in most
> timeout cases this won't reach the server and we risk a blocking send on
> a timedout connection.

Ok. I get it. But shouldn't the server side also close it's socket as
well? I don't
think the timeout value is propagated to server or like server can
"ping" to check
if client is there right?

I agree on nbd_send_req in timedout, it shouldn't be there, just a
sock_shutdown should
do. Can you confirm if I'm right about nbd-server side as well like it
won't timeout and close
that socket or did I miss any option while starting it?

>
> Regards,
>
> Markus
>
>> +             }
>>
>>               /* extract request */
>>               if (list_empty(&nbd->waiting_queue))
>> @@ -592,7 +603,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;
>> @@ -647,7 +662,7 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
>>  {
>>       int ret = 0;
>>
>> -     spin_lock_irq(&nbd->sock_lock);
>> +     mutex_lock(&nbd->sock_lock);
>>
>>       if (nbd->sock) {
>>               ret = -EBUSY;
>> @@ -657,7 +672,7 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
>>       nbd->sock = sock;
>>
>>  out:
>> -     spin_unlock_irq(&nbd->sock_lock);
>> +     mutex_unlock(&nbd->sock_lock);
>>
>>       return ret;
>>  }
>> @@ -666,7 +681,7 @@ 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);
>> @@ -803,17 +818,15 @@ 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);
>> -
>> -             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);
>> @@ -1075,7 +1088,7 @@ static int __init nbd_init(void)
>>               nbd_dev[i].magic = NBD_MAGIC;
>>               INIT_LIST_HEAD(&nbd_dev[i].waiting_queue);
>>               spin_lock_init(&nbd_dev[i].queue_lock);
>> -             spin_lock_init(&nbd_dev[i].sock_lock);
>> +             mutex_init(&nbd_dev[i].sock_lock);
>>               INIT_LIST_HEAD(&nbd_dev[i].queue_head);
>>               mutex_init(&nbd_dev[i].tx_lock);
>>               init_timer(&nbd_dev[i].timeout_timer);
>> --
>> 2.6.2
>>
>>
>
> --
> 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

Re: [PATCH v4 01/18] nbd: Fix might_sleep warning on xmit timeout

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 10075 bytes --]

On Friday 20 May 2016 02:05:36 Pranay Srivastava wrote:
> On Thu, May 19, 2016 at 11:52 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> > Hi,
> >
> > On Wed, May 11, 2016 at 11:18:29AM +0300, Pranay Kr. Srivastava wrote:
> >> This patch fixes the warning generated when a timeout occurs
> >> on the request and socket is closed from a non-sleep context
> >> by
> >>
> >> 1. Moving the socket closing on a timeout to nbd_thread_send
> >
> > What happens if a send blocks?
> 
> socket closing needs to be moved to a non-atomic context and,
> sender thread seemed to be a good place to do this. If you mean
> send blocks just before calling sock_shutdown[?] in nbd_thread_send
> then yes I think that should be removed. I need to re-check how nbd-server
> behaves in that case.

No that's not what I meant. Your approach uses the sender thread as a
worker to close the socket. You are using waiting_wq to notify the
sender thread. That's fine so far.

But what happens if the sender thread is at this point trying to send on
the socket which blocks? Then the timeout triggers and waiting_wq will
notify the sending thread as soon as it left the sending routine. But it
will not interrupt the thread that is waiting in kernel_sendmsg() and
the sending thread will be stuck much longer than specified in the
timeout. 

> 
> >
> >>
> >> 2. Make sock lock to be a mutex instead of a spin lock, since
> >>    nbd_xmit_timeout doesn't need to hold it anymore.
> >
> > I can't see why we need a mutex instead of a spinlock?
> 
> you are right, with your earlier patch we don't need it to be a mutex.
> 
> >
> >>
> >> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
> >> ---
> >>  drivers/block/nbd.c | 65 ++++++++++++++++++++++++++++++++---------------------
> >>  1 file changed, 39 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> >> index 31e73a7..c79bcd7 100644
> >> --- a/drivers/block/nbd.c
> >> +++ b/drivers/block/nbd.c
> >> @@ -57,12 +57,12 @@ 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;
> >>       /* protects initialization and shutdown of the socket */
> >> -     spinlock_t sock_lock;
> >> +     struct mutex sock_lock;
> >>       struct task_struct *task_recv;
> >>       struct task_struct *task_send;
> >>
> >> @@ -172,10 +172,9 @@ 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);
> >> -
> >> +     mutex_lock(&nbd->sock_lock);
> >>       if (!nbd->sock) {
> >> -             spin_unlock_irq(&nbd->sock_lock);
> >> +             mutex_unlock(&nbd->sock_lock);
> >>               return;
> >>       }
> >>
> >> @@ -183,27 +182,19 @@ static void sock_shutdown(struct nbd_device *nbd)
> >>       kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> >>       sockfd_put(nbd->sock);
> >>       nbd->sock = NULL;
> >> -     spin_unlock_irq(&nbd->sock_lock);
> >> -
> >> +     mutex_unlock(&nbd->sock_lock);
> >>       del_timer(&nbd->timeout_timer);
> >>  }
> >>
> >>  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);
> >> +     wake_up(&nbd->waiting_wq);
> >>
> >>       dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connection\n");
> >>  }
> >> @@ -579,7 +570,27 @@ static int nbd_thread_send(void *data)
> >>               /* wait for something to do */
> >>               wait_event_interruptible(nbd->waiting_wq,
> >>                                        kthread_should_stop() ||
> >> -                                      !list_empty(&nbd->waiting_queue));
> >> +                                      !list_empty(&nbd->waiting_queue) ||
> >> +                                      atomic_read(&nbd->timedout));
> >> +
> >> +             if (atomic_read(&nbd->timedout)) {
> >> +                     mutex_lock(&nbd->sock_lock);
> >> +                     if (nbd->sock) {
> >> +                             struct request sreq;
> >> +
> >> +                             blk_rq_init(NULL, &sreq);
> >> +                             sreq.cmd_type = REQ_TYPE_DRV_PRIV;
> >> +                             mutex_lock(&nbd->tx_lock);
> >> +                             nbd->disconnect = true;
> >> +                             nbd_send_req(nbd, &sreq);
> >> +                             mutex_unlock(&nbd->tx_lock);
> >> +                             dev_err(disk_to_dev(nbd->disk),
> >> +                                     "Device Timeout occured.Shutting down"
> >> +                                     " socket.");
> >> +                     }
> >> +                     mutex_unlock(&nbd->sock_lock);
> >> +                     sock_shutdown(nbd);
> >
> > Why are you trying to send something on a connection that timed out
> > (nbd_send_req())? And afterwards you execute a socket shutdown so in most
> > timeout cases this won't reach the server and we risk a blocking send on
> > a timedout connection.
> 
> Ok. I get it. But shouldn't the server side also close it's socket as
> well? I don't
> think the timeout value is propagated to server or like server can
> "ping" to check
> if client is there right?
> 
> I agree on nbd_send_req in timedout, it shouldn't be there, just a
> sock_shutdown should
> do. Can you confirm if I'm right about nbd-server side as well like it
> won't timeout and close
> that socket or did I miss any option while starting it?

If the socket is closed the server will notice at some point in the
future at least after the TCP timeout. I am not sure how we could notify
the server without running into the next connection issues.

Best Regards,

Markus

> 
> >
> > Regards,
> >
> > Markus
> >
> >> +             }
> >>
> >>               /* extract request */
> >>               if (list_empty(&nbd->waiting_queue))
> >> @@ -592,7 +603,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;
> >> @@ -647,7 +662,7 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
> >>  {
> >>       int ret = 0;
> >>
> >> -     spin_lock_irq(&nbd->sock_lock);
> >> +     mutex_lock(&nbd->sock_lock);
> >>
> >>       if (nbd->sock) {
> >>               ret = -EBUSY;
> >> @@ -657,7 +672,7 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
> >>       nbd->sock = sock;
> >>
> >>  out:
> >> -     spin_unlock_irq(&nbd->sock_lock);
> >> +     mutex_unlock(&nbd->sock_lock);
> >>
> >>       return ret;
> >>  }
> >> @@ -666,7 +681,7 @@ 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);
> >> @@ -803,17 +818,15 @@ 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);
> >> -
> >> -             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);
> >> @@ -1075,7 +1088,7 @@ static int __init nbd_init(void)
> >>               nbd_dev[i].magic = NBD_MAGIC;
> >>               INIT_LIST_HEAD(&nbd_dev[i].waiting_queue);
> >>               spin_lock_init(&nbd_dev[i].queue_lock);
> >> -             spin_lock_init(&nbd_dev[i].sock_lock);
> >> +             mutex_init(&nbd_dev[i].sock_lock);
> >>               INIT_LIST_HEAD(&nbd_dev[i].queue_head);
> >>               mutex_init(&nbd_dev[i].tx_lock);
> >>               init_timer(&nbd_dev[i].timeout_timer);
> >> --
> >> 2.6.2
> >>
> >>
> >
> > --
> > 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 |
> 
> 
> 
> 

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 01/18] nbd: Fix might_sleep warning on xmit timeout

[Pranay Srivastava]

Hi Markus

On Fri, May 20, 2016 at 1:52 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
> On Friday 20 May 2016 02:05:36 Pranay Srivastava wrote:
>> On Thu, May 19, 2016 at 11:52 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
>> > Hi,
>> >
>> > On Wed, May 11, 2016 at 11:18:29AM +0300, Pranay Kr. Srivastava wrote:
>> >> This patch fixes the warning generated when a timeout occurs
>> >> on the request and socket is closed from a non-sleep context
>> >> by
>> >>
>> >> 1. Moving the socket closing on a timeout to nbd_thread_send
>> >
>> > What happens if a send blocks?
>>
>> socket closing needs to be moved to a non-atomic context and,
>> sender thread seemed to be a good place to do this. If you mean
>> send blocks just before calling sock_shutdown[?] in nbd_thread_send
>> then yes I think that should be removed. I need to re-check how nbd-server
>> behaves in that case.
>
> No that's not what I meant. Your approach uses the sender thread as a
> worker to close the socket. You are using waiting_wq to notify the
> sender thread. That's fine so far.
>
> But what happens if the sender thread is at this point trying to send on
> the socket which blocks? Then the timeout triggers and waiting_wq will
> notify the sending thread as soon as it left the sending routine. But it
> will not interrupt the thread that is waiting in kernel_sendmsg() and
> the sending thread will be stuck much longer than specified in the
> timeout.

So socket shutdown must be triggered immediately. I've done a version using
system_wq for this and appears to be good. I'll be sending that soon after doing
cleanup and applying your sock_shutdown patch you sent earlier.

>
>>
>> >
>> >>
>> >> 2. Make sock lock to be a mutex instead of a spin lock, since
>> >>    nbd_xmit_timeout doesn't need to hold it anymore.
>> >
>> > I can't see why we need a mutex instead of a spinlock?
>>
>> you are right, with your earlier patch we don't need it to be a mutex.
>>
>> >
>> >>
>> >> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
>> >> ---
>> >>  drivers/block/nbd.c | 65 ++++++++++++++++++++++++++++++++---------------------
>> >>  1 file changed, 39 insertions(+), 26 deletions(-)
>> >>
>> >> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> >> index 31e73a7..c79bcd7 100644
>> >> --- a/drivers/block/nbd.c
>> >> +++ b/drivers/block/nbd.c
>> >> @@ -57,12 +57,12 @@ 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;
>> >>       /* protects initialization and shutdown of the socket */
>> >> -     spinlock_t sock_lock;
>> >> +     struct mutex sock_lock;
>> >>       struct task_struct *task_recv;
>> >>       struct task_struct *task_send;
>> >>
>> >> @@ -172,10 +172,9 @@ 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);
>> >> -
>> >> +     mutex_lock(&nbd->sock_lock);
>> >>       if (!nbd->sock) {
>> >> -             spin_unlock_irq(&nbd->sock_lock);
>> >> +             mutex_unlock(&nbd->sock_lock);
>> >>               return;
>> >>       }
>> >>
>> >> @@ -183,27 +182,19 @@ static void sock_shutdown(struct nbd_device *nbd)
>> >>       kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
>> >>       sockfd_put(nbd->sock);
>> >>       nbd->sock = NULL;
>> >> -     spin_unlock_irq(&nbd->sock_lock);
>> >> -
>> >> +     mutex_unlock(&nbd->sock_lock);
>> >>       del_timer(&nbd->timeout_timer);
>> >>  }
>> >>
>> >>  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);
>> >> +     wake_up(&nbd->waiting_wq);
>> >>
>> >>       dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connection\n");
>> >>  }
>> >> @@ -579,7 +570,27 @@ static int nbd_thread_send(void *data)
>> >>               /* wait for something to do */
>> >>               wait_event_interruptible(nbd->waiting_wq,
>> >>                                        kthread_should_stop() ||
>> >> -                                      !list_empty(&nbd->waiting_queue));
>> >> +                                      !list_empty(&nbd->waiting_queue) ||
>> >> +                                      atomic_read(&nbd->timedout));
>> >> +
>> >> +             if (atomic_read(&nbd->timedout)) {
>> >> +                     mutex_lock(&nbd->sock_lock);
>> >> +                     if (nbd->sock) {
>> >> +                             struct request sreq;
>> >> +
>> >> +                             blk_rq_init(NULL, &sreq);
>> >> +                             sreq.cmd_type = REQ_TYPE_DRV_PRIV;
>> >> +                             mutex_lock(&nbd->tx_lock);
>> >> +                             nbd->disconnect = true;
>> >> +                             nbd_send_req(nbd, &sreq);
>> >> +                             mutex_unlock(&nbd->tx_lock);
>> >> +                             dev_err(disk_to_dev(nbd->disk),
>> >> +                                     "Device Timeout occured.Shutting down"
>> >> +                                     " socket.");
>> >> +                     }
>> >> +                     mutex_unlock(&nbd->sock_lock);
>> >> +                     sock_shutdown(nbd);
>> >
>> > Why are you trying to send something on a connection that timed out
>> > (nbd_send_req())? And afterwards you execute a socket shutdown so in most
>> > timeout cases this won't reach the server and we risk a blocking send on
>> > a timedout connection.
>>
>> Ok. I get it. But shouldn't the server side also close it's socket as
>> well? I don't
>> think the timeout value is propagated to server or like server can
>> "ping" to check
>> if client is there right?
>>
>> I agree on nbd_send_req in timedout, it shouldn't be there, just a
>> sock_shutdown should
>> do. Can you confirm if I'm right about nbd-server side as well like it
>> won't timeout and close
>> that socket or did I miss any option while starting it?
>
> If the socket is closed the server will notice at some point in the
> future at least after the TCP timeout. I am not sure how we could notify
> the server without running into the next connection issues.
>
> Best Regards,
>
> Markus
>
>>
>> >
>> > Regards,
>> >
>> > Markus
>> >
>> >> +             }
>> >>
>> >>               /* extract request */
>> >>               if (list_empty(&nbd->waiting_queue))
>> >> @@ -592,7 +603,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;
>> >> @@ -647,7 +662,7 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
>> >>  {
>> >>       int ret = 0;
>> >>
>> >> -     spin_lock_irq(&nbd->sock_lock);
>> >> +     mutex_lock(&nbd->sock_lock);
>> >>
>> >>       if (nbd->sock) {
>> >>               ret = -EBUSY;
>> >> @@ -657,7 +672,7 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
>> >>       nbd->sock = sock;
>> >>
>> >>  out:
>> >> -     spin_unlock_irq(&nbd->sock_lock);
>> >> +     mutex_unlock(&nbd->sock_lock);
>> >>
>> >>       return ret;
>> >>  }
>> >> @@ -666,7 +681,7 @@ 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);
>> >> @@ -803,17 +818,15 @@ 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);
>> >> -
>> >> -             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);
>> >> @@ -1075,7 +1088,7 @@ static int __init nbd_init(void)
>> >>               nbd_dev[i].magic = NBD_MAGIC;
>> >>               INIT_LIST_HEAD(&nbd_dev[i].waiting_queue);
>> >>               spin_lock_init(&nbd_dev[i].queue_lock);
>> >> -             spin_lock_init(&nbd_dev[i].sock_lock);
>> >> +             mutex_init(&nbd_dev[i].sock_lock);
>> >>               INIT_LIST_HEAD(&nbd_dev[i].queue_head);
>> >>               mutex_init(&nbd_dev[i].tx_lock);
>> >>               init_timer(&nbd_dev[i].timeout_timer);
>> >> --
>> >> 2.6.2
>> >>
>> >>
>> >
>> > --
>> > 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 |
>>
>>
>>
>>
>
> --
> 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

Re: [PATCH v4 01/18] nbd: Fix might_sleep warning on xmit timeout

[Pranay Srivastava]

On Mon, May 23, 2016 at 4:02 PM, Pranay Srivastava <pranjas@gmail.com> wrote:
> Hi Markus
>
> On Fri, May 20, 2016 at 1:52 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
>> On Friday 20 May 2016 02:05:36 Pranay Srivastava wrote:
>>> On Thu, May 19, 2016 at 11:52 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
>>> > Hi,
>>> >
>>> > On Wed, May 11, 2016 at 11:18:29AM +0300, Pranay Kr. Srivastava wrote:
>>> >> This patch fixes the warning generated when a timeout occurs
>>> >> on the request and socket is closed from a non-sleep context
>>> >> by
>>> >>
>>> >> 1. Moving the socket closing on a timeout to nbd_thread_send
>>> >
>>> > What happens if a send blocks?
>>>
>>> socket closing needs to be moved to a non-atomic context and,
>>> sender thread seemed to be a good place to do this. If you mean
>>> send blocks just before calling sock_shutdown[?] in nbd_thread_send
>>> then yes I think that should be removed. I need to re-check how nbd-server
>>> behaves in that case.
>>
>> No that's not what I meant. Your approach uses the sender thread as a
>> worker to close the socket. You are using waiting_wq to notify the
>> sender thread. That's fine so far.
>>
>> But what happens if the sender thread is at this point trying to send on
>> the socket which blocks? Then the timeout triggers and waiting_wq will
>> notify the sending thread as soon as it left the sending routine. But it
>> will not interrupt the thread that is waiting in kernel_sendmsg() and
>> the sending thread will be stuck much longer than specified in the
>> timeout.
>
> So socket shutdown must be triggered immediately. I've done a version using
> system_wq for this and appears to be good. I'll be sending that soon after doing
> cleanup and applying your sock_shutdown patch you sent earlier.
>
>>
>>>
>>> >
>>> >>
>>> >> 2. Make sock lock to be a mutex instead of a spin lock, since
>>> >>    nbd_xmit_timeout doesn't need to hold it anymore.
>>> >
>>> > I can't see why we need a mutex instead of a spinlock?
>>>
>>> you are right, with your earlier patch we don't need it to be a mutex.
>>>
>>> >
>>> >>
>>> >> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
>>> >> ---
>>> >>  drivers/block/nbd.c | 65 ++++++++++++++++++++++++++++++++---------------------
>>> >>  1 file changed, 39 insertions(+), 26 deletions(-)
>>> >>
>>> >> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>>> >> index 31e73a7..c79bcd7 100644
>>> >> --- a/drivers/block/nbd.c
>>> >> +++ b/drivers/block/nbd.c
>>> >> @@ -57,12 +57,12 @@ 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;
>>> >>       /* protects initialization and shutdown of the socket */
>>> >> -     spinlock_t sock_lock;
>>> >> +     struct mutex sock_lock;
>>> >>       struct task_struct *task_recv;
>>> >>       struct task_struct *task_send;
>>> >>
>>> >> @@ -172,10 +172,9 @@ 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);
>>> >> -
>>> >> +     mutex_lock(&nbd->sock_lock);
>>> >>       if (!nbd->sock) {
>>> >> -             spin_unlock_irq(&nbd->sock_lock);
>>> >> +             mutex_unlock(&nbd->sock_lock);
>>> >>               return;
>>> >>       }
>>> >>
>>> >> @@ -183,27 +182,19 @@ static void sock_shutdown(struct nbd_device *nbd)
>>> >>       kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
>>> >>       sockfd_put(nbd->sock);
>>> >>       nbd->sock = NULL;
>>> >> -     spin_unlock_irq(&nbd->sock_lock);
>>> >> -
>>> >> +     mutex_unlock(&nbd->sock_lock);
>>> >>       del_timer(&nbd->timeout_timer);
>>> >>  }
>>> >>
>>> >>  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);
>>> >> +     wake_up(&nbd->waiting_wq);
>>> >>
>>> >>       dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connection\n");
>>> >>  }
>>> >> @@ -579,7 +570,27 @@ static int nbd_thread_send(void *data)
>>> >>               /* wait for something to do */
>>> >>               wait_event_interruptible(nbd->waiting_wq,
>>> >>                                        kthread_should_stop() ||
>>> >> -                                      !list_empty(&nbd->waiting_queue));
>>> >> +                                      !list_empty(&nbd->waiting_queue) ||
>>> >> +                                      atomic_read(&nbd->timedout));
>>> >> +
>>> >> +             if (atomic_read(&nbd->timedout)) {
>>> >> +                     mutex_lock(&nbd->sock_lock);
>>> >> +                     if (nbd->sock) {
>>> >> +                             struct request sreq;
>>> >> +
>>> >> +                             blk_rq_init(NULL, &sreq);
>>> >> +                             sreq.cmd_type = REQ_TYPE_DRV_PRIV;
>>> >> +                             mutex_lock(&nbd->tx_lock);
>>> >> +                             nbd->disconnect = true;
>>> >> +                             nbd_send_req(nbd, &sreq);
>>> >> +                             mutex_unlock(&nbd->tx_lock);
>>> >> +                             dev_err(disk_to_dev(nbd->disk),
>>> >> +                                     "Device Timeout occured.Shutting down"
>>> >> +                                     " socket.");
>>> >> +                     }
>>> >> +                     mutex_unlock(&nbd->sock_lock);
>>> >> +                     sock_shutdown(nbd);
>>> >
>>> > Why are you trying to send something on a connection that timed out
>>> > (nbd_send_req())? And afterwards you execute a socket shutdown so in most
>>> > timeout cases this won't reach the server and we risk a blocking send on
>>> > a timedout connection.
>>>
>>> Ok. I get it. But shouldn't the server side also close it's socket as
>>> well? I don't
>>> think the timeout value is propagated to server or like server can
>>> "ping" to check
>>> if client is there right?
>>>
>>> I agree on nbd_send_req in timedout, it shouldn't be there, just a
>>> sock_shutdown should
>>> do. Can you confirm if I'm right about nbd-server side as well like it
>>> won't timeout and close
>>> that socket or did I miss any option while starting it?
>>
>> If the socket is closed the server will notice at some point in the
>> future at least after the TCP timeout. I am not sure how we could notify
>> the server without running into the next connection issues.
>>
>> Best Regards,
>>
>> Markus
>>
>>>
>>> >
>>> > Regards,
>>> >
>>> > Markus
>>> >
>>> >> +             }
>>> >>
>>> >>               /* extract request */
>>> >>               if (list_empty(&nbd->waiting_queue))
>>> >> @@ -592,7 +603,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;
>>> >> @@ -647,7 +662,7 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
>>> >>  {
>>> >>       int ret = 0;
>>> >>
>>> >> -     spin_lock_irq(&nbd->sock_lock);
>>> >> +     mutex_lock(&nbd->sock_lock);
>>> >>
>>> >>       if (nbd->sock) {
>>> >>               ret = -EBUSY;
>>> >> @@ -657,7 +672,7 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
>>> >>       nbd->sock = sock;
>>> >>
>>> >>  out:
>>> >> -     spin_unlock_irq(&nbd->sock_lock);
>>> >> +     mutex_unlock(&nbd->sock_lock);
>>> >>
>>> >>       return ret;
>>> >>  }
>>> >> @@ -666,7 +681,7 @@ 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);
>>> >> @@ -803,17 +818,15 @@ 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);
>>> >> -
>>> >> -             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);
>>> >> @@ -1075,7 +1088,7 @@ static int __init nbd_init(void)
>>> >>               nbd_dev[i].magic = NBD_MAGIC;
>>> >>               INIT_LIST_HEAD(&nbd_dev[i].waiting_queue);
>>> >>               spin_lock_init(&nbd_dev[i].queue_lock);
>>> >> -             spin_lock_init(&nbd_dev[i].sock_lock);
>>> >> +             mutex_init(&nbd_dev[i].sock_lock);
>>> >>               INIT_LIST_HEAD(&nbd_dev[i].queue_head);
>>> >>               mutex_init(&nbd_dev[i].tx_lock);
>>> >>               init_timer(&nbd_dev[i].timeout_timer);
>>> >> --
>>> >> 2.6.2
>>> >>
>>> >>
>>> >
>>> > --
>>> > 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 |
>>>
>>>
>>>
>>>
>>
>> --
>> 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

I've sent 4 new patches for your review. It would be great if you can
let me know about them so I
can work on any required changes over weekend.

-- 
        ---P.K.S

[PATCH v4 02/18] nbd: fix checkpatch trailing space warning.

[Pranay Kr. Srivastava]

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index c79bcd7..82aac42 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>
  *
-- 
2.6.2

Re: [PATCH v4 02/18] nbd: fix checkpatch trailing space warning.

[Greg KH]

On Wed, May 11, 2016 at 11:18:30AM +0300, Pranay Kr. Srivastava wrote:
> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
> ---
>  drivers/block/nbd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I'm not the maintainer of these files or driver (which is why I don't
know why you are sending them to me), but I know I do not accept patches
without any changelog text at all in them, as that's just lazy.

But, the nbd.c maintainer might accept them, you might get lucky!

good luck,

greg k-h

Fwd: [PATCH v4 02/18] nbd: fix checkpatch trailing space warning.

[Pranay Srivastava]

Greg,

Resending as I missed the cc list earlier.

On Wed, May 11, 2016 at 2:03 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, May 11, 2016 at 11:18:30AM +0300, Pranay Kr. Srivastava wrote:
>> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
>> ---
>>  drivers/block/nbd.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> I'm not the maintainer of these files or driver (which is why I don't

The series contained some checkpatch changes so I had included you as well.

> know why you are sending them to me), but I know I do not accept patches
> without any changelog text at all in them, as that's just lazy.

That should be per patch or can appear in a cover letter for the patches?

Actually I've made more patches in this series after I had sent the
earlier ones,
but the earlier ones are not changed at all. It's only the addition of
newer patches
to the series. So I'm not sure how exactly would you like these to be.

>
> But, the nbd.c maintainer might accept them, you might get lucky!
>

Thanks again for the reply. Just in case I don't get any reply from
the maintainer,
which I've not as of today, do I consider these patches as dump or
should I resend
these to somebody else. I would really appreciate if you can answer that.

I still have one more patch to send over these which is an enhancement
rather than
a fix but I've not yet got any review of the patches.


> good luck,
>
> greg k-h


--
        ---P.K.S


-- 
        ---P.K.S

Re: [Nbd] Fwd: [PATCH v4 02/18] nbd: fix checkpatch trailing space warning.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1547 bytes --]

On 05/11/2016 03:38 AM, Pranay Srivastava wrote:

> 
> The series contained some checkpatch changes so I had included you as well.
> 
>> know why you are sending them to me), but I know I do not accept patches
>> without any changelog text at all in them, as that's just lazy.
> 
> That should be per patch or can appear in a cover letter for the patches?

Per patch.  However, if it were me, I would not have split into quite so
many patches.  The mantra is one patch per one fix, but I think it is
reasonable to state that "silence all checkpatch warnings" counts as one
fix, rather than 16 separate fixes.  If you DO consolidate the
checkpatch changes into a single patch, then the commit message body
should call out a bulleted list of all the changes you are making, as
well as a justification why it is worth churning the entire file rather
than just making smaller checkpatch fixes in just the areas that your
other patches touch.

> 
> Actually I've made more patches in this series after I had sent the
> earlier ones,
> but the earlier ones are not changed at all. It's only the addition of
> newer patches
> to the series.

The cover letter is a great place to point out how v4 differs from v3,
but also to point out which patches are unchanged from v3, to save
reviewer's time.  So if all you did was add new patches, a cover-letter
mention of which patches remain unchanged might be helpful.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Nbd] Fwd: [PATCH v4 02/18] nbd: fix checkpatch trailing space warning.

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 2132 bytes --]

Hi,

On Wednesday 11 May 2016 07:46:25 Eric Blake wrote:
> On 05/11/2016 03:38 AM, Pranay Srivastava wrote:
> 
> > 
> > The series contained some checkpatch changes so I had included you as well.
> > 
> >> know why you are sending them to me), but I know I do not accept patches
> >> without any changelog text at all in them, as that's just lazy.
> > 
> > That should be per patch or can appear in a cover letter for the patches?
> 
> Per patch.  However, if it were me, I would not have split into quite so
> many patches.  The mantra is one patch per one fix, but I think it is
> reasonable to state that "silence all checkpatch warnings" counts as one
> fix, rather than 16 separate fixes.  If you DO consolidate the
> checkpatch changes into a single patch, then the commit message body
> should call out a bulleted list of all the changes you are making, as
> well as a justification why it is worth churning the entire file rather
> than just making smaller checkpatch fixes in just the areas that your
> other patches touch.

Yes, I think about this similarly. There is simply no benefit in having
the history full of patches that partly even have the exact same
subject. So please squash them into one and describe your changes in the
commit message.

Thanks,

Markus

> 
> > 
> > Actually I've made more patches in this series after I had sent the
> > earlier ones,
> > but the earlier ones are not changed at all. It's only the addition of
> > newer patches
> > to the series.
> 
> The cover letter is a great place to point out how v4 differs from v3,
> but also to point out which patches are unchanged from v3, to save
> reviewer's time.  So if all you did was add new patches, a cover-letter
> mention of which patches remain unchanged might be helpful.
> 
> 

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Nbd] Fwd: [PATCH v4 02/18] nbd: fix checkpatch trailing space warning.

[Pavel Machek]

On Wed 2016-05-11 07:46:25, Eric Blake wrote:
> On 05/11/2016 03:38 AM, Pranay Srivastava wrote:
> 
> > 
> > The series contained some checkpatch changes so I had included you as well.
> > 
> >> know why you are sending them to me), but I know I do not accept patches
> >> without any changelog text at all in them, as that's just lazy.
> > 
> > That should be per patch or can appear in a cover letter for the patches?
> 
> Per patch.  However, if it were me, I would not have split into quite so
> many patches.  The mantra is one patch per one fix, but I think it is
> reasonable to state that "silence all checkpatch warnings" counts as one
> fix, rather than 16 separate fixes.  If you DO consolidate the
> checkpatch changes into a single patch, then the commit message body
> should call out a bulleted list of all the changes you are making, as
> well as a justification why it is worth churning the entire file rather
> than just making smaller checkpatch fixes in just the areas that your
> other patches touch.

Unfortunately, Greg told him to split it like this. I also liked the
single patch more.

I wonder why mpa@pengutronix.de was not cc-ed on this series? Anyway,
the cleanups are so trivial they should just be applied.... No need to
spend huge time arguing.

Best regards,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH v4 03/18] nbd: fix checkpatch warning use linux/uaccess.h

[Pranay Kr. Srivastava]

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 82aac42..c7ccde7 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -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>
-- 
2.6.2

[PATCH v4 04/18] nbd : fix checkpatch pointer declaration warning

[Pranay Kr. Srivastava]

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index c7ccde7..786aaac 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -42,7 +42,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;
-- 
2.6.2

[PATCH v4 05/18] nbd: fix checkpatch warning no newline after decleration.

[Pranay Kr. Srivastava]

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 786aaac..2192c0e 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -257,6 +257,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);
-- 
2.6.2

[PATCH v4 06/18] nbd: fix checkpatch warning no newline after decleration.

[Pranay Kr. Srivastava]

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 2192c0e..6a4dc3a 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -355,6 +355,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);
-- 
2.6.2

[PATCH v4 07/18] nbd: fix checkpatch split string warning.

[Pranay Kr. Srivastava]

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 6a4dc3a..7a5b8ef 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -587,8 +587,7 @@ static int nbd_thread_send(void *data)
 				nbd_send_req(nbd, &sreq);
 				mutex_unlock(&nbd->tx_lock);
 				dev_err(disk_to_dev(nbd->disk),
-					"Device Timeout occured.Shutting down"
-					" socket.");
+				"Device Timeout occured.Shutting down socket.");
 			}
 			mutex_unlock(&nbd->sock_lock);
 			sock_shutdown(nbd);
-- 
2.6.2

Re: [PATCH v4 07/18] nbd: fix checkpatch split string warning.

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 1200 bytes --]

Hi,

On Wednesday 11 May 2016 11:18:35 Pranay Kr. Srivastava wrote:
> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
> ---
>  drivers/block/nbd.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 6a4dc3a..7a5b8ef 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -587,8 +587,7 @@ static int nbd_thread_send(void *data)
>  				nbd_send_req(nbd, &sreq);
>  				mutex_unlock(&nbd->tx_lock);
>  				dev_err(disk_to_dev(nbd->disk),
> -					"Device Timeout occured.Shutting down"
> -					" socket.");
> +				"Device Timeout occured.Shutting down socket.");

Please keep the indentation to the opening bracket here. Shouldn't this
create a checkpatch warning as well?

Regards,

Markus

>  			}
>  			mutex_unlock(&nbd->sock_lock);
>  			sock_shutdown(nbd);
> 

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v4 08/18] nbd : fix checkpatch line over 80 char warning

[Pranay Kr. Srivastava]

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 7a5b8ef..224b44eb 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -617,8 +617,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 ); }
  */
-- 
2.6.2

[PATCH v4 09/18] nbd: fix checkpatch trailing whitespace warning.

[Pranay Kr. Srivastava]

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 224b44eb..2f1e5d0 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -627,7 +627,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;
 
-- 
2.6.2

[PATCH v4 10/18] nbd: fix checkpatch trailing whitespace warning.

[Pranay Kr. Srivastava]

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 2f1e5d0..0bc73dd 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -745,7 +745,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);
-- 
2.6.2

[PATCH v4 11/18] nbd : fix checkpatch structure declaration braces on next line warning.

[Pranay Kr. Srivastava]

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 0bc73dd..a6f11c3 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -870,8 +870,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,
-- 
2.6.2

[PATCH v4 12/18] nbd : fix checkpatch trailing whitespace warning

[Pranay Kr. Srivastava]

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index a6f11c3..4fd3016 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1005,7 +1005,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 :-)
  */
 
-- 
2.6.2

[PATCH v4 13/18] nbd : fix checkpatch printk warning

[Pranay Kr. Srivastava]

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 4fd3016..690e734 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1018,7 +1018,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;
 	}
 
-- 
2.6.2

[PATCH v4 14/18] nbd: fix checkpatch no extra line after decleration warning

[Pranay Kr. Srivastava]

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 690e734..6633ab2 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1049,6 +1049,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;
-- 
2.6.2

[PATCH v4 15/18] nbd: fix checkpatch printk warning to pr_info

[Pranay Kr. Srivastava]

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 6633ab2..9ce350b 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1080,7 +1080,7 @@ 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();
 
-- 
2.6.2

[PATCH v4 16/18] nbd: fix checkpatch no new line after decleration warning

[Pranay Kr. Srivastava]

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 9ce350b..e308f8b 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1086,6 +1086,7 @@ static int __init nbd_init(void)
 
 	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);
-- 
2.6.2

[PATCH v4 17/18] nbd: fix checkpatch printk warning to pr_info

[Pranay Kr. Srivastava]

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index e308f8b..482a3c0 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1134,7 +1134,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);
-- 
2.6.2

[PATCH v4 18/18] make nbd device wait for its users in case of timeout

[Pranay Kr. Srivastava]

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.

The change is described below:
a) Add a users count to nbd_device structure.
b) Add a bit flag to nbd_device structure of unsigned long.

If the current user count is not 1 then make nbd-client wait
for the in_use bit to be cleared.

Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
---
 drivers/block/nbd.c      | 40 ++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/nbd.h |  1 +
 2 files changed, 41 insertions(+)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 482a3c0..9b024d8 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -59,6 +59,7 @@ struct nbd_device {
 	int xmit_timeout;
 	atomic_t timedout;
 	bool disconnect; /* a disconnect has been requested by user */
+	u32 users;
 
 	struct timer_list timeout_timer;
 	/* protects initialization and shutdown of the socket */
@@ -69,6 +70,7 @@ struct nbd_device {
 #if IS_ENABLED(CONFIG_DEBUG_FS)
 	struct dentry *dbg_dir;
 #endif
+	unsigned long bflags;	/* word size bit flags for use. */
 };
 
 #if IS_ENABLED(CONFIG_DEBUG_FS)
@@ -822,6 +824,15 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		sock_shutdown(nbd);
 		mutex_lock(&nbd->tx_lock);
 		nbd_clear_que(nbd);
+		/*
+		 * Wait for any users currently using
+		 * this block device.
+		 */
+		mutex_unlock(&nbd->tx_lock);
+		pr_info("Waiting for users to release device %s ...\n",
+						bdev->bd_disk->disk_name);
+		wait_on_bit(&nbd->bflags, NBD_BFLAG_INUSE_BIT, TASK_INTERRUPTIBLE);
+		mutex_lock(&nbd->tx_lock);
 		kill_bdev(bdev);
 		nbd_bdev_reset(bdev);
 
@@ -870,10 +881,39 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
 	return error;
 }
 
+static int nbd_open(struct block_device *bdev, fmode_t mode)
+{
+	struct nbd_device *nbd_dev = bdev->bd_disk->private_data;
+	nbd_dev->users++;
+	pr_debug("Opening nbd_dev %s. Active users = %u\n",
+			bdev->bd_disk->disk_name, nbd_dev->users);
+	if (nbd_dev->users > 1)
+	{
+		set_bit(NBD_BFLAG_INUSE_BIT, &nbd_dev->bflags);
+	}
+	return 0;
+}
+
+static void nbd_release(struct gendisk *disk, fmode_t mode)
+{
+	struct nbd_device *nbd_dev = disk->private_data;
+	nbd_dev->users--;
+	pr_debug("Closing nbd_dev %s. Active users = %u\n",
+			disk->disk_name, nbd_dev->users);
+	if (nbd_dev->users == 1)
+	{
+		clear_bit(NBD_BFLAG_INUSE_BIT, &nbd_dev->bflags);
+		smp_mb();
+		wake_up_bit(&nbd_dev->bflags, NBD_BFLAG_INUSE_BIT);
+	}
+}
+
 static const struct block_device_operations nbd_fops = {
 	.owner =	THIS_MODULE,
 	.ioctl =	nbd_ioctl,
 	.compat_ioctl =	nbd_ioctl,
+	.open = 	nbd_open,
+	.release = 	nbd_release
 };
 
 #if IS_ENABLED(CONFIG_DEBUG_FS)
diff --git a/include/uapi/linux/nbd.h b/include/uapi/linux/nbd.h
index e08e413..8f3d3f0 100644
--- a/include/uapi/linux/nbd.h
+++ b/include/uapi/linux/nbd.h
@@ -44,6 +44,7 @@ enum {
 /* there is a gap here to match userspace */
 #define NBD_FLAG_SEND_TRIM    (1 << 5) /* send trim/discard */
 
+#define NBD_BFLAG_INUSE_BIT	(1) /* bit number for bflags */
 /* userspace doesn't need the nbd_device structure */
 
 /* These are sent over the network in the request/reply magic fields */
-- 
2.6.2

Re: [PATCH v4 18/18] make nbd device wait for its users in case of timeout

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 4448 bytes --]

Hi,

On Wednesday 11 May 2016 11:18:46 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.
> 
> The change is described below:
> a) Add a users count to nbd_device structure.
> b) Add a bit flag to nbd_device structure of unsigned long.
> 
> If the current user count is not 1 then make nbd-client wait
> for the in_use bit to be cleared.

Thanks, I like this approach much more.

> 
> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
> ---
>  drivers/block/nbd.c      | 40 ++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/nbd.h |  1 +
>  2 files changed, 41 insertions(+)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 482a3c0..9b024d8 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -59,6 +59,7 @@ struct nbd_device {
>  	int xmit_timeout;
>  	atomic_t timedout;
>  	bool disconnect; /* a disconnect has been requested by user */
> +	u32 users;

Perhaps it is better to use kref for this?

>  
>  	struct timer_list timeout_timer;
>  	/* protects initialization and shutdown of the socket */
> @@ -69,6 +70,7 @@ struct nbd_device {
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>  	struct dentry *dbg_dir;
>  #endif
> +	unsigned long bflags;	/* word size bit flags for use. */

Maybe it is better to use a completion instead of a bitfield.

>  };
>  
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -822,6 +824,15 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>  		sock_shutdown(nbd);
>  		mutex_lock(&nbd->tx_lock);
>  		nbd_clear_que(nbd);
> +		/*
> +		 * Wait for any users currently using
> +		 * this block device.
> +		 */
> +		mutex_unlock(&nbd->tx_lock);
> +		pr_info("Waiting for users to release device %s ...\n",
> +						bdev->bd_disk->disk_name);
> +		wait_on_bit(&nbd->bflags, NBD_BFLAG_INUSE_BIT, TASK_INTERRUPTIBLE);
> +		mutex_lock(&nbd->tx_lock);
>  		kill_bdev(bdev);
>  		nbd_bdev_reset(bdev);
>  
> @@ -870,10 +881,39 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
>  	return error;
>  }
>  
> +static int nbd_open(struct block_device *bdev, fmode_t mode)
> +{
> +	struct nbd_device *nbd_dev = bdev->bd_disk->private_data;

Here is a new line missing otherwise checkpatch will probably warn about
this?

Should we check here if we are connected here? And check whether the
connection is about to be closed?

Best Regards,

Markus

> +	nbd_dev->users++;
> +	pr_debug("Opening nbd_dev %s. Active users = %u\n",
> +			bdev->bd_disk->disk_name, nbd_dev->users);
> +	if (nbd_dev->users > 1)
> +	{
> +		set_bit(NBD_BFLAG_INUSE_BIT, &nbd_dev->bflags);
> +	}
> +	return 0;
> +}
> +
> +static void nbd_release(struct gendisk *disk, fmode_t mode)
> +{
> +	struct nbd_device *nbd_dev = disk->private_data;
> +	nbd_dev->users--;
> +	pr_debug("Closing nbd_dev %s. Active users = %u\n",
> +			disk->disk_name, nbd_dev->users);
> +	if (nbd_dev->users == 1)
> +	{
> +		clear_bit(NBD_BFLAG_INUSE_BIT, &nbd_dev->bflags);
> +		smp_mb();
> +		wake_up_bit(&nbd_dev->bflags, NBD_BFLAG_INUSE_BIT);
> +	}
> +}
> +
>  static const struct block_device_operations nbd_fops = {
>  	.owner =	THIS_MODULE,
>  	.ioctl =	nbd_ioctl,
>  	.compat_ioctl =	nbd_ioctl,
> +	.open = 	nbd_open,
> +	.release = 	nbd_release
>  };
>  
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> diff --git a/include/uapi/linux/nbd.h b/include/uapi/linux/nbd.h
> index e08e413..8f3d3f0 100644
> --- a/include/uapi/linux/nbd.h
> +++ b/include/uapi/linux/nbd.h
> @@ -44,6 +44,7 @@ enum {
>  /* there is a gap here to match userspace */
>  #define NBD_FLAG_SEND_TRIM    (1 << 5) /* send trim/discard */
>  
> +#define NBD_BFLAG_INUSE_BIT	(1) /* bit number for bflags */
>  /* userspace doesn't need the nbd_device structure */
>  
>  /* These are sent over the network in the request/reply magic fields */
> 

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 18/18] make nbd device wait for its users in case of timeout

[Pranay Srivastava]

On Thu, May 12, 2016 at 2:49 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
> Hi,
>
> On Wednesday 11 May 2016 11:18:46 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.
>>
>> The change is described below:
>> a) Add a users count to nbd_device structure.
>> b) Add a bit flag to nbd_device structure of unsigned long.
>>
>> If the current user count is not 1 then make nbd-client wait
>> for the in_use bit to be cleared.
>
> Thanks, I like this approach much more.
>
>>
>> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
>> ---
>>  drivers/block/nbd.c      | 40 ++++++++++++++++++++++++++++++++++++++++
>>  include/uapi/linux/nbd.h |  1 +
>>  2 files changed, 41 insertions(+)
>>
>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> index 482a3c0..9b024d8 100644
>> --- a/drivers/block/nbd.c
>> +++ b/drivers/block/nbd.c
>> @@ -59,6 +59,7 @@ struct nbd_device {
>>       int xmit_timeout;
>>       atomic_t timedout;
>>       bool disconnect; /* a disconnect has been requested by user */
>> +     u32 users;
>
> Perhaps it is better to use kref for this?

Yup will do.

>
>>
>>       struct timer_list timeout_timer;
>>       /* protects initialization and shutdown of the socket */
>> @@ -69,6 +70,7 @@ struct nbd_device {
>>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>>       struct dentry *dbg_dir;
>>  #endif
>> +     unsigned long bflags;   /* word size bit flags for use. */
>
> Maybe it is better to use a completion instead of a bitfield.

Ok.

>
>>  };
>>
>>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> @@ -822,6 +824,15 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>>               sock_shutdown(nbd);
>>               mutex_lock(&nbd->tx_lock);
>>               nbd_clear_que(nbd);
>> +             /*
>> +              * Wait for any users currently using
>> +              * this block device.
>> +              */
>> +             mutex_unlock(&nbd->tx_lock);
>> +             pr_info("Waiting for users to release device %s ...\n",
>> +                                             bdev->bd_disk->disk_name);
>> +             wait_on_bit(&nbd->bflags, NBD_BFLAG_INUSE_BIT, TASK_INTERRUPTIBLE);
>> +             mutex_lock(&nbd->tx_lock);
>>               kill_bdev(bdev);
>>               nbd_bdev_reset(bdev);
>>
>> @@ -870,10 +881,39 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
>>       return error;
>>  }
>>
>> +static int nbd_open(struct block_device *bdev, fmode_t mode)
>> +{
>> +     struct nbd_device *nbd_dev = bdev->bd_disk->private_data;
>
> Here is a new line missing otherwise checkpatch will probably warn about
> this?
>
> Should we check here if we are connected here? And check whether the
> connection is about to be closed?

I don't think it should matter if we are connected or not. We already
handle that case
correctly. Requests would fail and those failures will be reported to
userland. The idea
here is not to enforce "harsh measures" on the user on failure but to
report them instead.

>
> Best Regards,
>
> Markus

I'll send in a new patch after making changes as per your review.

Thanks alot!.

>
>> +     nbd_dev->users++;
>> +     pr_debug("Opening nbd_dev %s. Active users = %u\n",
>> +                     bdev->bd_disk->disk_name, nbd_dev->users);
>> +     if (nbd_dev->users > 1)
>> +     {
>> +             set_bit(NBD_BFLAG_INUSE_BIT, &nbd_dev->bflags);
>> +     }
>> +     return 0;
>> +}
>> +
>> +static void nbd_release(struct gendisk *disk, fmode_t mode)
>> +{
>> +     struct nbd_device *nbd_dev = disk->private_data;
>> +     nbd_dev->users--;
>> +     pr_debug("Closing nbd_dev %s. Active users = %u\n",
>> +                     disk->disk_name, nbd_dev->users);
>> +     if (nbd_dev->users == 1)
>> +     {
>> +             clear_bit(NBD_BFLAG_INUSE_BIT, &nbd_dev->bflags);
>> +             smp_mb();
>> +             wake_up_bit(&nbd_dev->bflags, NBD_BFLAG_INUSE_BIT);
>> +     }
>> +}
>> +
>>  static const struct block_device_operations nbd_fops = {
>>       .owner =        THIS_MODULE,
>>       .ioctl =        nbd_ioctl,
>>       .compat_ioctl = nbd_ioctl,
>> +     .open =         nbd_open,
>> +     .release =      nbd_release
>>  };
>>
>>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> diff --git a/include/uapi/linux/nbd.h b/include/uapi/linux/nbd.h
>> index e08e413..8f3d3f0 100644
>> --- a/include/uapi/linux/nbd.h
>> +++ b/include/uapi/linux/nbd.h
>> @@ -44,6 +44,7 @@ enum {
>>  /* there is a gap here to match userspace */
>>  #define NBD_FLAG_SEND_TRIM    (1 << 5) /* send trim/discard */
>>
>> +#define NBD_BFLAG_INUSE_BIT  (1) /* bit number for bflags */
>>  /* userspace doesn't need the nbd_device structure */
>>
>>  /* These are sent over the network in the request/reply magic fields */
>>
>
> --
> 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

Re: [PATCH v4 18/18] make nbd device wait for its users in case of timeout

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 6247 bytes --]

Hi,

On Thu, May 12, 2016 at 08:49:00PM +0530, Pranay Srivastava wrote:
> On Thu, May 12, 2016 at 2:49 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
> > Hi,
> >
> > On Wednesday 11 May 2016 11:18:46 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.
> >>
> >> The change is described below:
> >> a) Add a users count to nbd_device structure.
> >> b) Add a bit flag to nbd_device structure of unsigned long.
> >>
> >> If the current user count is not 1 then make nbd-client wait
> >> for the in_use bit to be cleared.
> >
> > Thanks, I like this approach much more.
> >
> >>
> >> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
> >> ---
> >>  drivers/block/nbd.c      | 40 ++++++++++++++++++++++++++++++++++++++++
> >>  include/uapi/linux/nbd.h |  1 +
> >>  2 files changed, 41 insertions(+)
> >>
> >> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> >> index 482a3c0..9b024d8 100644
> >> --- a/drivers/block/nbd.c
> >> +++ b/drivers/block/nbd.c
> >> @@ -59,6 +59,7 @@ struct nbd_device {
> >>       int xmit_timeout;
> >>       atomic_t timedout;
> >>       bool disconnect; /* a disconnect has been requested by user */
> >> +     u32 users;
> >
> > Perhaps it is better to use kref for this?
> 
> Yup will do.
> 
> >
> >>
> >>       struct timer_list timeout_timer;
> >>       /* protects initialization and shutdown of the socket */
> >> @@ -69,6 +70,7 @@ struct nbd_device {
> >>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> >>       struct dentry *dbg_dir;
> >>  #endif
> >> +     unsigned long bflags;   /* word size bit flags for use. */
> >
> > Maybe it is better to use a completion instead of a bitfield.
> 
> Ok.
> 
> >
> >>  };
> >>
> >>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> >> @@ -822,6 +824,15 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
> >>               sock_shutdown(nbd);
> >>               mutex_lock(&nbd->tx_lock);
> >>               nbd_clear_que(nbd);
> >> +             /*
> >> +              * Wait for any users currently using
> >> +              * this block device.
> >> +              */
> >> +             mutex_unlock(&nbd->tx_lock);
> >> +             pr_info("Waiting for users to release device %s ...\n",
> >> +                                             bdev->bd_disk->disk_name);
> >> +             wait_on_bit(&nbd->bflags, NBD_BFLAG_INUSE_BIT, TASK_INTERRUPTIBLE);
> >> +             mutex_lock(&nbd->tx_lock);
> >>               kill_bdev(bdev);
> >>               nbd_bdev_reset(bdev);
> >>
> >> @@ -870,10 +881,39 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
> >>       return error;
> >>  }
> >>
> >> +static int nbd_open(struct block_device *bdev, fmode_t mode)
> >> +{
> >> +     struct nbd_device *nbd_dev = bdev->bd_disk->private_data;
> >
> > Here is a new line missing otherwise checkpatch will probably warn about
> > this?
> >
> > Should we check here if we are connected here? And check whether the
> > connection is about to be closed?
> 
> I don't think it should matter if we are connected or not. We already
> handle that case
> correctly. Requests would fail and those failures will be reported to
> userland. The idea
> here is not to enforce "harsh measures" on the user on failure but to
> report them instead.

I see. Yes that's fine.

Thanks,

Markus

> 
> >
> > Best Regards,
> >
> > Markus
> 
> I'll send in a new patch after making changes as per your review.
> 
> Thanks alot!.
> 
> >
> >> +     nbd_dev->users++;
> >> +     pr_debug("Opening nbd_dev %s. Active users = %u\n",
> >> +                     bdev->bd_disk->disk_name, nbd_dev->users);
> >> +     if (nbd_dev->users > 1)
> >> +     {
> >> +             set_bit(NBD_BFLAG_INUSE_BIT, &nbd_dev->bflags);
> >> +     }
> >> +     return 0;
> >> +}
> >> +
> >> +static void nbd_release(struct gendisk *disk, fmode_t mode)
> >> +{
> >> +     struct nbd_device *nbd_dev = disk->private_data;
> >> +     nbd_dev->users--;
> >> +     pr_debug("Closing nbd_dev %s. Active users = %u\n",
> >> +                     disk->disk_name, nbd_dev->users);
> >> +     if (nbd_dev->users == 1)
> >> +     {
> >> +             clear_bit(NBD_BFLAG_INUSE_BIT, &nbd_dev->bflags);
> >> +             smp_mb();
> >> +             wake_up_bit(&nbd_dev->bflags, NBD_BFLAG_INUSE_BIT);
> >> +     }
> >> +}
> >> +
> >>  static const struct block_device_operations nbd_fops = {
> >>       .owner =        THIS_MODULE,
> >>       .ioctl =        nbd_ioctl,
> >>       .compat_ioctl = nbd_ioctl,
> >> +     .open =         nbd_open,
> >> +     .release =      nbd_release
> >>  };
> >>
> >>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> >> diff --git a/include/uapi/linux/nbd.h b/include/uapi/linux/nbd.h
> >> index e08e413..8f3d3f0 100644
> >> --- a/include/uapi/linux/nbd.h
> >> +++ b/include/uapi/linux/nbd.h
> >> @@ -44,6 +44,7 @@ enum {
> >>  /* there is a gap here to match userspace */
> >>  #define NBD_FLAG_SEND_TRIM    (1 << 5) /* send trim/discard */
> >>
> >> +#define NBD_BFLAG_INUSE_BIT  (1) /* bit number for bflags */
> >>  /* userspace doesn't need the nbd_device structure */
> >>
> >>  /* These are sent over the network in the request/reply magic fields */
> >>
> >
> > --
> > 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
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]