Skip to contentSkip to conversation

NBD: allow nbd to be used locally

PATCH In review Paul Clements 3 participants 6 messages Mar 9, 2008 – Mar 24, 2008

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 NBD: allow nbd to be used locally

This patch allows a Network Block Device to be mounted locally (nbd-client to nbd-server over 127.0.0.1).

It creates a kthread to avoid the deadlock described in NBD tools documentation. So, if nbd-client hangs waiting for pages, the kblockd thread can continue its work and free pages.

I have tested the patch to verify that it avoids the hang that occurs when writing to a localhost nbd connection. I have also tested to verify that no performance degradation results from the additional thread and queue.

Patch originally from Laurent Vivier.

Thanks,
Paul

This patch allows Network Block Device to be mounted locally (nbd-client to nbd-server over 127.0.0.1).

It creates a kthread to avoid the deadlock described in NBD tools documentation. So, if nbd-client hangs waiting for pages, the kblockd thread can continue its work and free pages.

I have tested the patch to verify that it avoids the hang that always occurs when writing to a localhost nbd connection. I have also tested to verify that no performance degradation results from the additional thread and queue.

Patch originally from Laurent Vivier.

Thanks,
Paul

drivers/block/nbd.c renamed from drivers/block/nbd.c.deadline +95 -49
@@ -29,6 +29,7 @@
 #include <linux/kernel.h>
 #include <net/sock.h>
 #include <linux/net.h>
+#include <linux/kthread.h>
 
 #include <asm/uaccess.h>
 #include <asm/system.h>
@@ -434,6 +435,85 @@ static void nbd_clear_que(struct nbd_dev
 }
 
 
+static void nbd_handle_req(struct nbd_device *lo, struct request *req)
+{
+	if (!blk_fs_request(req))
+		goto error_out;
+
+	nbd_cmd(req) = NBD_CMD_READ;
+	if (rq_data_dir(req) == WRITE) {
+		nbd_cmd(req) = NBD_CMD_WRITE;
+		if (lo->flags & NBD_READ_ONLY) {
+			printk(KERN_ERR "%s: Write on read-only\n",
+					lo->disk->disk_name);
+			goto error_out;
+		}
+	}
+
+	req->errors = 0;
+
+	mutex_lock(&lo->tx_lock);
+	if (unlikely(!lo->sock)) {
+		mutex_unlock(&lo->tx_lock);
+		printk(KERN_ERR "%s: Attempted send on closed socket\n",
+		       lo->disk->disk_name);
+		req->errors++;
+		nbd_end_request(req);
+		return;
+	}
+
+	lo->active_req = req;
+
+	if (nbd_send_req(lo, req) != 0) {
+		printk(KERN_ERR "%s: Request send failed\n",
+				lo->disk->disk_name);
+		req->errors++;
+		nbd_end_request(req);
+	} else {
+		spin_lock(&lo->queue_lock);
+		list_add(&req->queuelist, &lo->queue_head);
+		spin_unlock(&lo->queue_lock);
+	}
+
+	lo->active_req = NULL;
+	mutex_unlock(&lo->tx_lock);
+	wake_up_all(&lo->active_wq);
+
+	return;
+
+error_out:
+	req->errors++;
+	nbd_end_request(req);
+}
+
+static int nbd_thread(void *data)
+{
+	struct nbd_device *lo = data;
+	struct request *req;
+
+	set_user_nice(current, -20);
+	while (!kthread_should_stop() || !list_empty(&lo->waiting_queue)) {
+		/* wait for something to do */
+		wait_event_interruptible(lo->waiting_wq,
+					 kthread_should_stop() ||
+					 !list_empty(&lo->waiting_queue));
+
+		/* extract request */
+		if (list_empty(&lo->waiting_queue))
+			continue;
+
+		spin_lock_irq(&lo->queue_lock);
+		req = list_entry(lo->waiting_queue.next, struct request,
+				 queuelist);
+		list_del_init(&req->queuelist);
+		spin_unlock_irq(&lo->queue_lock);
+
+		/* handle request */
+		nbd_handle_req(lo, req);
+	}
+	return 0;
+}
+
 /*
  * We always wait for result of write, for now. It would be nice to make it optional
  * in future
@@ -449,65 +529,23 @@ static void do_nbd_request(struct reques
 		struct nbd_device *lo;
 
 		blkdev_dequeue_request(req);
+
+		spin_unlock_irq(q->queue_lock);
+
 		dprintk(DBG_BLKDEV, "%s: request %p: dequeued (flags=%x)\n",
 				req->rq_disk->disk_name, req, req->cmd_type);
 
-		if (!blk_fs_request(req))
-			goto error_out;
-
 		lo = req->rq_disk->private_data;
 
 		BUG_ON(lo->magic != LO_MAGIC);
 
-		nbd_cmd(req) = NBD_CMD_READ;
-		if (rq_data_dir(req) == WRITE) {
-			nbd_cmd(req) = NBD_CMD_WRITE;
-			if (lo->flags & NBD_READ_ONLY) {
-				printk(KERN_ERR "%s: Write on read-only\n",
-						lo->disk->disk_name);
-				goto error_out;
-			}
-		}
-
-		req->errors = 0;
-		spin_unlock_irq(q->queue_lock);
-
-		mutex_lock(&lo->tx_lock);
-		if (unlikely(!lo->sock)) {
-			mutex_unlock(&lo->tx_lock);
-			printk(KERN_ERR "%s: Attempted send on closed socket\n",
-			       lo->disk->disk_name);
-			req->errors++;
-			nbd_end_request(req);
-			spin_lock_irq(q->queue_lock);
-			continue;
-		}
+		spin_lock_irq(&lo->queue_lock);
+		list_add_tail(&req->queuelist, &lo->waiting_queue);
+		spin_unlock_irq(&lo->queue_lock);
 
-		lo->active_req = req;
-
-		if (nbd_send_req(lo, req) != 0) {
-			printk(KERN_ERR "%s: Request send failed\n",
-					lo->disk->disk_name);
-			req->errors++;
-			nbd_end_request(req);
-		} else {
-			spin_lock(&lo->queue_lock);
-			list_add(&req->queuelist, &lo->queue_head);
-			spin_unlock(&lo->queue_lock);
-		}
-
-		lo->active_req = NULL;
-		mutex_unlock(&lo->tx_lock);
-		wake_up_all(&lo->active_wq);
+		wake_up(&lo->waiting_wq);
 
 		spin_lock_irq(q->queue_lock);
-		continue;
-
-error_out:
-		req->errors++;
-		spin_unlock(q->queue_lock);
-		nbd_end_request(req);
-		spin_lock(q->queue_lock);
 	}
 }
 
@@ -517,6 +555,7 @@ static int nbd_ioctl(struct inode *inode
 	struct nbd_device *lo = inode->i_bdev->bd_disk->private_data;
 	int error;
 	struct request sreq ;
+	struct task_struct *thread;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
@@ -599,7 +638,12 @@ static int nbd_ioctl(struct inode *inode
 	case NBD_DO_IT:
 		if (!lo->file)
 			return -EINVAL;
+		thread = kthread_create(nbd_thread, lo, lo->disk->disk_name);
+		if (IS_ERR(thread))
+			return PTR_ERR(thread);
+		wake_up_process(thread);
 		error = nbd_do_it(lo);
+		kthread_stop(thread);
 		if (error)
 			return error;
 		sock_shutdown(lo, 1);
@@ -688,10 +732,12 @@ static int __init nbd_init(void)
 		nbd_dev[i].file = NULL;
 		nbd_dev[i].magic = LO_MAGIC;
 		nbd_dev[i].flags = 0;
+		INIT_LIST_HEAD(&nbd_dev[i].waiting_queue);
 		spin_lock_init(&nbd_dev[i].queue_lock);
 		INIT_LIST_HEAD(&nbd_dev[i].queue_head);
 		mutex_init(&nbd_dev[i].tx_lock);
 		init_waitqueue_head(&nbd_dev[i].active_wq);
+		init_waitqueue_head(&nbd_dev[i].waiting_wq);
 		nbd_dev[i].blksize = 1024;
 		nbd_dev[i].bytesize = 0;
 		disk->major = NBD_MAJOR;
include/linux/nbd.h renamed from include/linux/nbd.h.max_nbd_killed +3 -1
@@ -56,9 +56,11 @@ struct nbd_device {
 	int magic;
 
 	spinlock_t queue_lock;
-	struct list_head queue_head;/* Requests are added here...	*/
+	struct list_head queue_head;	/* Requests waiting result */
 	struct request *active_req;
 	wait_queue_head_t active_wq;
+	struct list_head waiting_queue;	/* Requests to be sent */
+	wait_queue_head_t waiting_wq;
 
 	struct mutex tx_lock;
 	struct gendisk *disk;
#1 lore raw

Hi!

Quoted 6 lines

This patch allows a Network Block Device to be mounted locally (nbd-client to nbd-server over 127.0.0.1).

It creates a kthread to avoid the deadlock described in NBD tools documentation. So, if nbd-client hangs waiting for pages, the kblockd thread can continue its work and free pages.

What happens if your new kthread blocks on memory allocation?

									Pavel-- 

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

#2 lore raw
Quoted 5 lines · Pavel Machek
Quoted 3 lines

This patch allows a Network Block Device to be mounted locally (nbd-client to nbd-server over 127.0.0.1).

It creates a kthread to avoid the deadlock described in NBD tools documentation. So, if nbd-client hangs waiting for pages, the kblockd thread can continue its work and free pages.

What happens if your new kthread blocks on memory allocation?

Well, we expect that. The reason for the new thread is so that it hangs, rather than kblockd hanging (which on a UP system brings all I/O to a halt). As long as kblockd can continue making progress, we eventually free up memory and then NBD can finish its requests, too.

--
Paul

#3 lore raw
Quoted 9 lines · Paul Clements
Quoted 7 lines · Pavel Machek
Quoted 6 lines

This patch allows a Network Block Device to be mounted locally (nbd-client to nbd-server over 127.0.0.1).

It creates a kthread to avoid the deadlock described in NBD tools documentation. So, if nbd-client hangs waiting for pages, the kblockd thread can continue its work and free pages.

Quoted 6 lines
Quoted 1 line

What happens if your new kthread blocks on memory allocation?

Well, we expect that. The reason for the new thread is so that it hangs, rather than kblockd hanging (which on a UP system brings all I/O to a halt). As long as kblockd can continue making progress, we eventually free up memory and then NBD can finish its requests, too.

...unless all the memory is in dirty buffers for nbd, and nbd server is swapped out or something?
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#4 lore raw
Quoted 12 lines · Pavel Machek
Quoted 11 lines · Paul Clements
Quoted 8 lines · Pavel Machek
Quoted 6 lines

This patch allows a Network Block Device to be mounted locally (nbd-client to nbd-server over 127.0.0.1).

It creates a kthread to avoid the deadlock described in NBD tools documentation. So, if nbd-client hangs waiting for pages, the kblockd thread can continue its work and free pages.

What happens if your new kthread blocks on memory allocation?

Quoted 7 lines
Quoted 4 lines

Well, we expect that. The reason for the new thread is so that it hangs, rather than kblockd hanging (which on a UP system brings all I/O to a halt). As long as kblockd can continue making progress, we eventually free up memory and then NBD can finish its requests, too.

....unless all the memory is in dirty buffers for nbd, and nbd server is swapped out or something?

Note that I'm not a kernel hacker, so might be terribly mistaken here...

but I feel I should point out that this patch solves the issue that no two block devices can flush their dirty buffers at the same time. Without this patch, you can't write to a _filesystem_ on an NBD device if that's connected to a server on the localhost. You are correct that this does not solve the deadlock in swapping to NBD devices, but that's not the only existing deadlock issue in NBD to localhost...

Signature · 2 lines
<Lo-lan-do> Home is where you have to wash the dishes.
  -- #debian-devel, Freenode, 2004-09-22
#5 lore raw

Hi!

Quoted 17 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 6 lines

This patch allows a Network Block Device to be mounted locally (nbd-client to nbd-server over 127.0.0.1).

It creates a kthread to avoid the deadlock described in NBD tools documentation. So, if nbd-client hangs waiting for pages, the kblockd thread can continue its work and free pages.

What happens if your new kthread blocks on memory allocation?

Quoted 7 lines
Quoted 4 lines

Well, we expect that. The reason for the new thread is so that it hangs, rather than kblockd hanging (which on a UP system brings all I/O to a halt). As long as kblockd can continue making progress, we eventually free up memory and then NBD can finish its requests, too.

....unless all the memory is in dirty buffers for nbd, and nbd server is swapped out or something?

Quoted 8 lines

Note that I'm not a kernel hacker, so might be terribly mistaken here...

but I feel I should point out that this patch solves the issue that no two block devices can flush their dirty buffers at the same time. Without this patch, you can't write to a _filesystem_ on an NBD device if that's connected to a server on the localhost. You are correct that this does not solve the deadlock in swapping to NBD devices, but that's not the only existing deadlock issue in NBD to localhost...

Same issue with swapping probably exists with dirty block writeout... swapoff -a, and filesystem becomes very similar to swap.

Pavel

Signature · 3 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
pomozte zachranit klanovicky les:  http://www.ujezdskystrom.info/