Re: [PATCH] nbd: create a recv workqueue per nbd device

[Josef Bacik]

> On Jan 14, 2017, at 4:15 PM, Sagi Grimberg  wrote:
> 
> 
>>> Hey Josef,
>>> 
 Since we are in the memory reclaim path we need our recv work to be on a
 workqueue that has WQ_MEM_RECLAIM set so we can avoid deadlocks.  Also
 set WQ_HIGHPRI since we are in the completion path for IO.
>>> 
>>> Really a workqueue per device?? Did this really give performance
>>> advantage? Can this really scale with number of devices?
>> 
>> I don't see why not, especially since these things run the whole time the 
>> device is active.  I have patches forthcoming to make device creation 
>> dynamic so we don't have a bunch all at once.  That being said I'm not 
>> married to the idea, just seemed like a good idea at the time and not 
>> particularly harmful.  Thanks,
> 
> I just don't see how having a worqueue per device helps anything? There
> are plenty of active workers per workqueue and even if its not enough
> you can specify more with max_active.
> 
> I guess what I'm trying to say is that I don't understand what this is
> solving. The commit message explains why you need WQ_MEM_RECLAIM and why
> you want WQ_HIGHPRI, but does not explain why workqueue per device is
> helping/solving anything.

There's no reason for it, that's just the way I did it. I will test both ways 
on Tuesday and if there's no measurable difference then I'll do a global one.  
Thanks,

Josef--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nbd: create a recv workqueue per nbd device

[Sagi Grimberg]

Hey Josef,


Since we are in the memory reclaim path we need our recv work to be on a
workqueue that has WQ_MEM_RECLAIM set so we can avoid deadlocks.  Also
set WQ_HIGHPRI since we are in the completion path for IO.


Really a workqueue per device?? Did this really give performance
advantage? Can this really scale with number of devices?


I don't see why not, especially since these things run the whole time the 
device is active.  I have patches forthcoming to make device creation dynamic 
so we don't have a bunch all at once.  That being said I'm not married to the 
idea, just seemed like a good idea at the time and not particularly harmful.  
Thanks,


I just don't see how having a worqueue per device helps anything? There
are plenty of active workers per workqueue and even if its not enough
you can specify more with max_active.

I guess what I'm trying to say is that I don't understand what this is
solving. The commit message explains why you need WQ_MEM_RECLAIM and why
you want WQ_HIGHPRI, but does not explain why workqueue per device is
helping/solving anything.
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nbd: create a recv workqueue per nbd device

[Josef Bacik]

> On Jan 13, 2017, at 5:24 PM, Sagi Grimberg  wrote:
> 
> Hey Josef,
> 
>> Since we are in the memory reclaim path we need our recv work to be on a
>> workqueue that has WQ_MEM_RECLAIM set so we can avoid deadlocks.  Also
>> set WQ_HIGHPRI since we are in the completion path for IO.
> 
> Really a workqueue per device?? Did this really give performance
> advantage? Can this really scale with number of devices?

I don't see why not, especially since these things run the whole time the 
device is active.  I have patches forthcoming to make device creation dynamic 
so we don't have a bunch all at once.  That being said I'm not married to the 
idea, just seemed like a good idea at the time and not particularly harmful.  
Thanks,

Josef--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nbd: create a recv workqueue per nbd device

[Sagi Grimberg]

Hey Josef,


Since we are in the memory reclaim path we need our recv work to be on a
workqueue that has WQ_MEM_RECLAIM set so we can avoid deadlocks.  Also
set WQ_HIGHPRI since we are in the completion path for IO.


Really a workqueue per device?? Did this really give performance
advantage? Can this really scale with number of devices?
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] nbd: create a recv workqueue per nbd device

[Josef Bacik]

Since we are in the memory reclaim path we need our recv work to be on a
workqueue that has WQ_MEM_RECLAIM set so we can avoid deadlocks.  Also
set WQ_HIGHPRI since we are in the completion path for IO.

Signed-off-by: Josef Bacik 
---
 drivers/block/nbd.c | 15 ++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 99c8446..e0a8d51 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -70,6 +70,7 @@ struct nbd_device {
struct task_struct *task_recv;
struct task_struct *task_setup;
 
+   struct workqueue_struct *recv_workqueue;
 #if IS_ENABLED(CONFIG_DEBUG_FS)
struct dentry *dbg_dir;
 #endif
@@ -787,7 +788,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct 
nbd_device *nbd,
INIT_WORK([i].work, recv_work);
args[i].nbd = nbd;
args[i].index = i;
-   queue_work(system_long_wq, [i].work);
+   queue_work(nbd->recv_workqueue, [i].work);
}
wait_event_interruptible(nbd->recv_wq,
 atomic_read(>recv_threads) == 0);
@@ -1074,6 +1075,16 @@ static int __init nbd_init(void)
goto out;
}
 
+   nbd_dev[i].recv_workqueue =
+   alloc_workqueue("knbd-recv",
+   WQ_MEM_RECLAIM | WQ_HIGHPRI, 0);
+   if (!nbd_dev[i].recv_workqueue) {
+   blk_mq_free_tag_set(_dev[i].tag_set);
+   blk_cleanup_queue(disk->queue);
+   put_disk(disk);
+   goto out;
+   }
+
/*
 * Tell the block layer that we are not a rotational device
 */
@@ -1115,6 +1126,7 @@ static int __init nbd_init(void)
blk_mq_free_tag_set(_dev[i].tag_set);
blk_cleanup_queue(nbd_dev[i].disk->queue);
put_disk(nbd_dev[i].disk);
+   destroy_workqueue(nbd_dev[i].recv_workqueue);
}
kfree(nbd_dev);
return err;
@@ -1134,6 +1146,7 @@ static void __exit nbd_cleanup(void)
blk_cleanup_queue(disk->queue);
blk_mq_free_tag_set(_dev[i].tag_set);
put_disk(disk);
+   destroy_workqueue(nbd_dev[i].recv_workqueue);
}
}
unregister_blkdev(NBD_MAJOR, "nbd");
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html