nbd: Fix might sleep warning.

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.

3. Move sock_shutdown outside the tx_lock in NBD_DO_IT.

---
 drivers/block/nbd.c | 85 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 50 insertions(+), 35 deletions(-)

drivers/block/nbd.c +50 -36

index 31e73a7..a52cc16 100644
@@ -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 <[email protected]>
  * Parts copyright 2001 Steven Whitehouse <[email protected]>
  *

@@ -35,14 +35,14 @@
 #include <linux/types.h>
 #include <linux/debugfs.h>
 
-#include <asm/uaccess.h>
+#include <linux/uaccess.h>
 #include <asm/types.h>
 
 #include <linux/nbd.h>
 
 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;

@@ -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");
 }

@@ -266,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);

@@ -278,6 +270,7 @@ static int nbd_send_req(struct nbd_device *nbd, struct request *req)
        int result, flags;
        struct nbd_request request;
        unsigned long size = blk_rq_bytes(req);
+
        u32 type;
 
        if (req->cmd_type == REQ_TYPE_DRV_PRIV)

@@ -363,6 +356,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);

@@ -579,7 +573,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 +606,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;

@@ -601,8 +619,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 ); }
  */

@@ -611,7 +629,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;
 

@@ -647,7 +665,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 +675,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 +684,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);

@@ -729,7 +747,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);

@@ -803,17 +821,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);

@@ -856,8 +872,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,

@@ -1075,7 +1090,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

Hi,

Can the following patch be reviewed? I'm working on some more changes on top of this change,
so it'll be really helpful if someone can review this patch and let me know of shortcomings/issues
with this.

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

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

3. Move sock_shutdown outside the tx_lock in NBD_DO_IT.

Quoted 4 lines

---
 drivers/block/nbd.c | 85 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 50 insertions(+), 35 deletions(-)

drivers/block/nbd.c line 6

Quoted 10 lines

drivers/block/nbd.c +1 -1

index 31e73a7..a52cc16 100644
@@ -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.
- *
+ *

drivers/block/nbd.c line 38

Quoted 9 lines

drivers/block/nbd.c +1 -1

  * Copyright 1997-2000, 2008 Pavel Machek <[email protected]>
  * Parts copyright 2001 Steven Whitehouse <[email protected]>
  *

@@ -35,14 +35,14 @@
 #include <linux/types.h>
 #include <linux/debugfs.h>
 
-#include <asm/uaccess.h>
+#include <linux/uaccess.h>

drivers/block/nbd.c line 45

Quoted 8 lines

drivers/block/nbd.c +1 -1

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

Quoted 9 lines · Pranay Srivastava

Hi,

Can the following patch be reviewed? I'm working on some more changes on top of this change,
so it'll be really helpful if someone can review this patch and let me know of shortcomings/issues
with this.

Quoted 12 lines

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

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

3. Move sock_shutdown outside the tx_lock in NBD_DO_IT.

Why are you doing three things in one patch?

Quoted 4 lines

Quoted 4 lines

---
 drivers/block/nbd.c | 85 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 50 insertions(+), 35 deletions(-)

Quoted 10 lines

drivers/block/nbd.c line 6

Quoted 10 lines

drivers/block/nbd.c +1 -1

index 31e73a7..a52cc16 100644
@@ -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.
- *
+ *

What did you change here?

Quoted 9 lines

drivers/block/nbd.c line 38

Quoted 9 lines

drivers/block/nbd.c +1 -1

  * Copyright 1997-2000, 2008 Pavel Machek <[email protected]>
  * Parts copyright 2001 Steven Whitehouse <[email protected]>
  *

@@ -35,14 +35,14 @@
 #include <linux/types.h>
 #include <linux/debugfs.h>
 
-#include <asm/uaccess.h>
+#include <linux/uaccess.h>

Why change this?

Quoted 4 lines

Quoted 4 lines

drivers/block/nbd.c +0 -0

 #include <asm/types.h>
 
 #include <linux/nbd.h>
 

Quoted 4 lines

drivers/block/nbd.c line 45

Quoted 4 lines

drivers/block/nbd.c +1 -1

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

You are mixing "code cleanup" changes with "change the logic" changes, please break this up into a series of patches, doing the code cleanup _last_ as you want to fix bugs first.

thanks,

greg k-h