[PATCH] Break up the single NBD lock into one per NBD device

[PATCH] Break up the single NBD lock into one per NBD device

[H.K. Jerry Chu]

From: Jerry Chu <hkchu@google.com>

This patch breaks up the single NBD lock into one per
disk. The single NBD lock has become a serious performance
bottleneck when multiple NBD disks are being used.

The original comment on why a single lock may be ok no
longer holds for today's much faster NICs.

Signed-off-by: H.K. Jerry Chu <hkchu@google.com>
---
 drivers/block/nbd.c |   22 +++++++++-------------
 1 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index f533f33..355e15c 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -58,20 +58,9 @@ static unsigned int debugflags;
 
 static unsigned int nbds_max = 16;
 static struct nbd_device *nbd_dev;
+static spinlock_t *nbd_locks;
 static int max_part;
 
-/*
- * Use just one lock (or at most 1 per NIC). Two arguments for this:
- * 1. Each NIC is essentially a synchronization point for all servers
- *    accessed through that NIC so there's no need to have more locks
- *    than NICs anyway.
- * 2. More locks lead to more "Dirty cache line bouncing" which will slow
- *    down each lock to the point where they're actually slower than just
- *    a single lock.
- * Thanks go to Jens Axboe and Al Viro for their LKML emails explaining this!
- */
-static DEFINE_SPINLOCK(nbd_lock);
-
 #ifndef NDEBUG
 static const char *ioctl_cmd_to_ascii(int cmd)
 {
@@ -753,6 +742,12 @@ static int __init nbd_init(void)
 	if (!nbd_dev)
 		return -ENOMEM;
 
+	nbd_locks = kcalloc(nbds_max, sizeof(*nbd_locks), GFP_KERNEL);
+	if (!nbd_locks) {
+		kfree(nbd_dev);
+		return -ENOMEM;
+	}
+
 	part_shift = 0;
 	if (max_part > 0) {
 		part_shift = fls(max_part);
@@ -784,7 +779,7 @@ static int __init nbd_init(void)
 		 * every gendisk to have its very own request_queue struct.
 		 * These structs are big so we dynamically allocate them.
 		 */
-		disk->queue = blk_init_queue(do_nbd_request, &nbd_lock);
+		disk->queue = blk_init_queue(do_nbd_request, &nbd_locks[i]);
 		if (!disk->queue) {
 			put_disk(disk);
 			goto out;
@@ -832,6 +827,7 @@ out:
 		put_disk(nbd_dev[i].disk);
 	}
 	kfree(nbd_dev);
+	kfree(nbd_locks);
 	return err;
 }
 
-- 
1.7.3.1

Re: [PATCH] Break up the single NBD lock into one per NBD device

[David Miller]

From: "H.K. Jerry Chu" <hkchu@google.com>
Date: Mon, 26 Sep 2011 16:34:12 -0700

> From: Jerry Chu <hkchu@google.com>
> 
> This patch breaks up the single NBD lock into one per
> disk. The single NBD lock has become a serious performance
> bottleneck when multiple NBD disks are being used.
> 
> The original comment on why a single lock may be ok no
> longer holds for today's much faster NICs.
> 
> Signed-off-by: H.K. Jerry Chu <hkchu@google.com>

Acked-by: David S. Miller <davem@davemloft.net>

Even though this is a "networking" block device, I think this change
should go through the Jens Axboe's block layer tree.

Thanks.

Re: [PATCH] Break up the single NBD lock into one per NBD device

[Eric Dumazet]

Le lundi 26 septembre 2011 à 16:34 -0700, H.K. Jerry Chu a écrit :
> From: Jerry Chu <hkchu@google.com>
> 
> This patch breaks up the single NBD lock into one per
> disk. The single NBD lock has become a serious performance
> bottleneck when multiple NBD disks are being used.
> 
> The original comment on why a single lock may be ok no
> longer holds for today's much faster NICs.
> 
> Signed-off-by: H.K. Jerry Chu <hkchu@google.com>
> ---
>  drivers/block/nbd.c |   22 +++++++++-------------
>  1 files changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index f533f33..355e15c 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -58,20 +58,9 @@ static unsigned int debugflags;
>  
>  static unsigned int nbds_max = 16;
>  static struct nbd_device *nbd_dev;
> +static spinlock_t *nbd_locks;

static spinlock_t *nbd_locks __read_mostly;

>  static int max_part;
>  
> -/*
> - * Use just one lock (or at most 1 per NIC). Two arguments for this:
> - * 1. Each NIC is essentially a synchronization point for all servers
> - *    accessed through that NIC so there's no need to have more locks
> - *    than NICs anyway.
> - * 2. More locks lead to more "Dirty cache line bouncing" which will slow
> - *    down each lock to the point where they're actually slower than just
> - *    a single lock.
> - * Thanks go to Jens Axboe and Al Viro for their LKML emails explaining this!
> - */
> -static DEFINE_SPINLOCK(nbd_lock);
> -
>  #ifndef NDEBUG
>  static const char *ioctl_cmd_to_ascii(int cmd)
>  {
> @@ -753,6 +742,12 @@ static int __init nbd_init(void)
>  	if (!nbd_dev)
>  		return -ENOMEM;
>  
> +	nbd_locks = kcalloc(nbds_max, sizeof(*nbd_locks), GFP_KERNEL);
> +	if (!nbd_locks) {
> +		kfree(nbd_dev);
> +		return -ENOMEM;
> +	}
> +

	Please add loop to init spinlocks to help LOCKDEP...

	for (i = 0; i < nbds_max; i++)
		spin_lock_init(&nbd_locks[i]);

>  	part_shift = 0;
>  	if (max_part > 0) {
>  		part_shift = fls(max_part);
> @@ -784,7 +779,7 @@ static int __init nbd_init(void)
>  		 * every gendisk to have its very own request_queue struct.
>  		 * These structs are big so we dynamically allocate them.
>  		 */
> -		disk->queue = blk_init_queue(do_nbd_request, &nbd_lock);
> +		disk->queue = blk_init_queue(do_nbd_request, &nbd_locks[i]);
>  		if (!disk->queue) {
>  			put_disk(disk);
>  			goto out;
> @@ -832,6 +827,7 @@ out:
>  		put_disk(nbd_dev[i].disk);
>  	}
>  	kfree(nbd_dev);
> +	kfree(nbd_locks);
>  	return err;
>  }
>  

Re: [PATCH] Break up the single NBD lock into one per NBD device

[Nicholas Thomas]

Hi,

I was wondering if there was any chance of resurrecting this patch: 

http://comments.gmane.org/gmane.linux.network/207233

My use case is a Linux kernel connecting to up to 6,144 NBD devices (in
practice, probably < 1,000 almost all the time) simultaneously - our
current deployment has a userspace implementation of NBD, and I came
across this patch while investigating potential scalability issues.

I've not performed any benchmarks yet, but I assume the issues Jerry Chu
mentions would affect my case too, and the asked-for changes to get it
accepted seemed minimal.

Regards,
-- 
Nick Thomas
Bytemark Computing

Re: [PATCH] Break up the single NBD lock into one per NBD device

[Jerry Chu]

The change was straightforward but according to David Miller it should go to
"Jens Axboe's block layer tree". You are welcome to make and submit the change
there. (I did not follow nor do I know where that tree is.)

Jerry

On Mon, Feb 4, 2013 at 11:55 PM, Nicholas Thomas <nick@bytemark.co.uk> wrote:
> Hi,
>
> I was wondering if there was any chance of resurrecting this patch:
>
> http://comments.gmane.org/gmane.linux.network/207233
>
> My use case is a Linux kernel connecting to up to 6,144 NBD devices (in
> practice, probably < 1,000 almost all the time) simultaneously - our
> current deployment has a userspace implementation of NBD, and I came
> across this patch while investigating potential scalability issues.
>
> I've not performed any benchmarks yet, but I assume the issues Jerry Chu
> mentions would affect my case too, and the asked-for changes to get it
> accepted seemed minimal.
>
> Regards,
> --
> Nick Thomas
> Bytemark Computing
>
>
>
>
>