nbd: fixes for might_sleep warning, checkpatch warning and device wait.

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 <[email protected]>

---
 drivers/block/nbd.c | 65 ++++++++++++++++++++++++++++++++---------------------
 1 file changed, 39 insertions(+), 26 deletions(-)

drivers/block/nbd.c +39 -26

index 31e73a7..c79bcd7 100644
@@ -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);

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 <[email protected]>

Signed-off-by Markus Pargmann <[email protected]>

---
 drivers/block/nbd.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

drivers/block/nbd.c +8 -4

index 0b892eed06a0..157bf3da876e 100644
@@ -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);

--
2.8.0.rc3

Hi Markus,

Quoted 44 lines · 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 <[email protected]>

Signed-off-by Markus Pargmann <[email protected]>

---
 drivers/block/nbd.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

drivers/block/nbd.c +10 -8

index 0b892eed06a0..157bf3da876e 100644
@@ -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);
 }
 

drivers/block/nbd.c static void nbd_end_request(struct nbd_device *nbd, struct request *req) · line 194

Quoted 1 line

drivers/block/nbd.c +0 -0

 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.

Hi,

Quoted 48 lines · On Thursday 12 May 2016 16:42:31 Pranay Srivastava wrote:

Hi Markus,

Quoted 44 lines · 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 <[email protected]>

Signed-off-by Markus Pargmann <[email protected]>

---
 drivers/block/nbd.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

drivers/block/nbd.c +10 -8

index 0b892eed06a0..157bf3da876e 100644
@@ -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);
 }
 

Quoted 7 lines

drivers/block/nbd.c static void nbd_end_request(struct nbd_device *nbd, struct request *req) · line 194

Quoted 1 line

drivers/block/nbd.c +0 -0

 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 |

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 <[email protected]>

---
 drivers/block/nbd.c | 65 ++++++++++++++++++++++++++++++++---------------------
 1 file changed, 39 insertions(+), 26 deletions(-)

drivers/block/nbd.c +17 -17

index 31e73a7..c79bcd7 100644
@@ -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;

drivers/block/nbd.c +12 -9

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

Signature · 1 line

2.6.2

Hi,

Quoted 5 lines · 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

What happens if a send blocks?

Quoted 4 lines

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?

Quoted 92 lines

Signed-off-by Pranay Kr. Srivastava <[email protected]>

---
 drivers/block/nbd.c | 65 ++++++++++++++++++++++++++++++++---------------------
 1 file changed, 39 insertions(+), 26 deletions(-)

drivers/block/nbd.c +24 -17

index 31e73a7..c79bcd7 100644
@@ -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.");

drivers/block/nbd.c static int nbd_thread_send(void *data) · line 592

Quoted 3 lines

drivers/block/nbd.c +3 -0

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

Quoted 74 lines

drivers/block/nbd.c +12 -10

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

Quoted 3 lines

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 |

Quoted 9 lines · Markus Pargmann

Hi,

Quoted 5 lines · 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

Quoted 1 line

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.

Quoted 7 lines

Quoted 5 lines

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.

Quoted 92 lines

Quoted 92 lines

Signed-off-by Pranay Kr. Srivastava <[email protected]>

---
 drivers/block/nbd.c | 65 ++++++++++++++++++++++++++++++++---------------------
 1 file changed, 39 insertions(+), 26 deletions(-)

drivers/block/nbd.c +24 -17

index 31e73a7..c79bcd7 100644
@@ -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.");

Quoted 8 lines

drivers/block/nbd.c static int nbd_thread_send(void *data) · line 592

Quoted 3 lines

drivers/block/nbd.c +3 -0

+                     }
+                     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?

Quoted 89 lines

Regards,

Markus

Quoted 74 lines

drivers/block/nbd.c +12 -10

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

Quoted 3 lines

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 |

Quoted 10 lines · On Friday 20 May 2016 02:05:36 Pranay Srivastava wrote:

Quoted 9 lines · Markus Pargmann

Hi,

Quoted 5 lines · 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

Quoted 7 lines

Quoted 1 line

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.

Quoted 118 lines

Quoted 8 lines

Quoted 6 lines

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.

Quoted 92 lines

Quoted 92 lines

Signed-off-by Pranay Kr. Srivastava <[email protected]>

---
 drivers/block/nbd.c | 65 ++++++++++++++++++++++++++++++++---------------------
 1 file changed, 39 insertions(+), 26 deletions(-)

drivers/block/nbd.c +24 -17

index 31e73a7..c79bcd7 100644
@@ -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.");

Quoted 8 lines

drivers/block/nbd.c static int nbd_thread_send(void *data) · line 592

Quoted 3 lines

drivers/block/nbd.c +3 -0

+                     }
+                     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?

Quoted 5 lines

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

Quoted 81 lines

Quoted 81 lines

Regards,

Markus

Quoted 75 lines

drivers/block/nbd.c +12 -10

+             }
 
              /* 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

Quoted 12 lines

Quoted 8 lines

Quoted 2 lines

--

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 |

Hi Markus

Quoted 23 lines · Markus Pargmann

Quoted 10 lines · On Friday 20 May 2016 02:05:36 Pranay Srivastava wrote:

Quoted 9 lines · Markus Pargmann

Hi,

Quoted 5 lines · 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

Quoted 7 lines

Quoted 1 line

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.

Quoted 6 lines

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.

Quoted 226 lines

Quoted 118 lines

Quoted 8 lines

Quoted 6 lines

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.

Quoted 92 lines

Quoted 92 lines

Signed-off-by Pranay Kr. Srivastava <[email protected]>

---
 drivers/block/nbd.c | 65 ++++++++++++++++++++++++++++++++---------------------
 1 file changed, 39 insertions(+), 26 deletions(-)

drivers/block/nbd.c +24 -17

index 31e73a7..c79bcd7 100644
@@ -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.");

Quoted 8 lines

drivers/block/nbd.c static int nbd_thread_send(void *data) · line 592

Quoted 3 lines

drivers/block/nbd.c +3 -0

+                     }
+                     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?

Quoted 5 lines

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

Quoted 81 lines

Quoted 81 lines

Regards,

Markus

Quoted 75 lines

drivers/block/nbd.c +12 -10

+             }
 
              /* 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

Quoted 12 lines

Quoted 8 lines

Quoted 2 lines

--

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 |

Quoted 5 lines

--

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

Signed-off-by Pranay Kr. Srivastava <[email protected]>

---
 drivers/block/nbd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 4 lines · Pranay Kr. Srivastava

Signed-off-by Pranay Kr. Srivastava <[email protected]>

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

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

Quoted 2 lines

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.

Quoted 7 lines · Pranay Srivastava

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

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 7 lines · Pranay Srivastava

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

Quoted 2 lines

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.

Signed-off-by Pranay Kr. Srivastava <[email protected]>

---
 drivers/block/nbd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/block/nbd.c +0 -0

index 6a4dc3a..7a5b8ef 100644

drivers/block/nbd.c +0 -0

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

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 <[email protected]>

---
 drivers/block/nbd.c      | 40 ++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/nbd.h |  1 +
 2 files changed, 41 insertions(+)

drivers/block/nbd.c +0 -0

index 482a3c0..9b024d8 100644

drivers/block/nbd.c +9 -0

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

drivers/block/nbd.c +26 -0

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

include/uapi/linux/nbd.h +1 -0

index e08e413..8f3d3f0 100644
@@ -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 */

Hi,

Quoted 8 lines · 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.

Quoted 6 lines

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.

Quoted 7 lines

Signed-off-by Pranay Kr. Srivastava <[email protected]>

---
 drivers/block/nbd.c      | 40 ++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/nbd.h |  1 +
 2 files changed, 41 insertions(+)

drivers/block/nbd.c struct nbd_device { · line 62

Quoted 9 lines

drivers/block/nbd.c +1 -0

index 482a3c0..9b024d8 100644
@@ -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?

drivers/block/nbd.c struct nbd_device { · line 73

Quoted 9 lines

drivers/block/nbd.c +1 -0

      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.

Quoted 21 lines

drivers/block/nbd.c +9 -0

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

drivers/block/nbd.c static int nbd_ioctl(struct block_device *bdev, fmode_t mode, · line 886

Quoted 6 lines

drivers/block/nbd.c +3 -0

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

Quoted 40 lines

drivers/block/nbd.c +26 -0

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

include/uapi/linux/nbd.h +0 -0

index e08e413..8f3d3f0 100644

include/uapi/linux/nbd.h enum { · line 50

Quoted 7 lines

include/uapi/linux/nbd.h +1 -0

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

Quoted 27 lines · Markus Pargmann

Hi,

Quoted 8 lines · 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.

Quoted 6 lines

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.

Quoted 7 lines

Signed-off-by Pranay Kr. Srivastava <[email protected]>

---
 drivers/block/nbd.c      | 40 ++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/nbd.h |  1 +
 2 files changed, 41 insertions(+)

Quoted 11 lines

drivers/block/nbd.c struct nbd_device { · line 62

Quoted 9 lines

drivers/block/nbd.c +1 -0

index 482a3c0..9b024d8 100644
@@ -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.

Quoted 11 lines

drivers/block/nbd.c struct nbd_device { · line 73

Quoted 9 lines

drivers/block/nbd.c +1 -0

      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.

Quoted 31 lines

Quoted 21 lines

drivers/block/nbd.c +9 -0

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

drivers/block/nbd.c static int nbd_ioctl(struct block_device *bdev, fmode_t mode, · line 886

Quoted 6 lines

drivers/block/nbd.c +3 -0

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

Quoted 2 lines

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.

Quoted 5 lines

Best Regards,

Markus

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

Thanks alot!.

Quoted 53 lines

Quoted 40 lines

drivers/block/nbd.c +26 -0

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

include/uapi/linux/nbd.h +0 -0

index e08e413..8f3d3f0 100644

include/uapi/linux/nbd.h enum { · line 50

Quoted 7 lines

include/uapi/linux/nbd.h +1 -0

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

Signature · 1 line

        ---P.K.S