[PATCH 0/2] block: loop: fix stacked loop and performance regression

[PATCH 0/2] block: loop: fix stacked loop and performance regression

[Ming Lei]

Hi,

The 1st patch convers to per-device workqueue because loop devices
can be stacked.

The 2nd patch decreases max active works as 16, so that fedora 22's
boot performance regression can be fixed.

 drivers/block/loop.c | 30 ++++++++++++++----------------
 drivers/block/loop.h |  1 +
 2 files changed, 15 insertions(+), 16 deletions(-)


Thanks,
Ming Lei

[PATCH 1/2] block: loop: convert to per-device workqueue

[Ming Lei]

Documentation/workqueue.txt:
	If there is dependency among multiple work items used
	during memory reclaim, they should be queued to separate
	wq each with WQ_MEM_RECLAIM.

Loop devices can be stacked, so we have to convert to per-device
workqueue. One example is Fedora live CD.

Fixes: b5dd2f6047ca108001328aac0e8588edd15f1778
Cc: stable@vger.kernel.org (v4.0)
Cc: Justin M. Forbes <jforbes@fedoraproject.org>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/block/loop.c | 30 ++++++++++++++----------------
 drivers/block/loop.h |  1 +
 2 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index ae3fcb4..3dc1598 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -86,8 +86,6 @@ static DEFINE_MUTEX(loop_index_mutex);
 static int max_part;
 static int part_shift;
 
-static struct workqueue_struct *loop_wq;
-
 static int transfer_xor(struct loop_device *lo, int cmd,
 			struct page *raw_page, unsigned raw_off,
 			struct page *loop_page, unsigned loop_off,
@@ -725,6 +723,12 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	size = get_loop_size(lo, file);
 	if ((loff_t)(sector_t)size != size)
 		goto out_putf;
+	error = -ENOMEM;
+	lo->wq = alloc_workqueue("kloopd%d",
+			WQ_MEM_RECLAIM | WQ_HIGHPRI | WQ_UNBOUND, 0,
+			lo->lo_number);
+	if (!lo->wq)
+		goto out_putf;
 
 	error = 0;
 
@@ -872,6 +876,8 @@ static int loop_clr_fd(struct loop_device *lo)
 	lo->lo_flags = 0;
 	if (!part_shift)
 		lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN;
+	destroy_workqueue(lo->wq);
+	lo->wq = NULL;
 	mutex_unlock(&lo->lo_ctl_mutex);
 	/*
 	 * Need not hold lo_ctl_mutex to fput backing file.
@@ -1425,9 +1431,13 @@ static int loop_queue_rq(struct blk_mq_hw_ctx *hctx,
 		const struct blk_mq_queue_data *bd)
 {
 	struct loop_cmd *cmd = blk_mq_rq_to_pdu(bd->rq);
+	struct loop_device *lo = cmd->rq->q->queuedata;
 
 	blk_mq_start_request(bd->rq);
 
+	if (lo->lo_state != Lo_bound)
+		return -EIO;
+
 	if (cmd->rq->cmd_flags & REQ_WRITE) {
 		struct loop_device *lo = cmd->rq->q->queuedata;
 		bool need_sched = true;
@@ -1441,9 +1451,9 @@ static int loop_queue_rq(struct blk_mq_hw_ctx *hctx,
 		spin_unlock_irq(&lo->lo_lock);
 
 		if (need_sched)
-			queue_work(loop_wq, &lo->write_work);
+			queue_work(lo->wq, &lo->write_work);
 	} else {
-		queue_work(loop_wq, &cmd->read_work);
+		queue_work(lo->wq, &cmd->read_work);
 	}
 
 	return BLK_MQ_RQ_QUEUE_OK;
@@ -1455,9 +1465,6 @@ static void loop_handle_cmd(struct loop_cmd *cmd)
 	struct loop_device *lo = cmd->rq->q->queuedata;
 	int ret = -EIO;
 
-	if (lo->lo_state != Lo_bound)
-		goto failed;
-
 	if (write && (lo->lo_flags & LO_FLAGS_READ_ONLY))
 		goto failed;
 
@@ -1806,13 +1813,6 @@ static int __init loop_init(void)
 		goto misc_out;
 	}
 
-	loop_wq = alloc_workqueue("kloopd",
-			WQ_MEM_RECLAIM | WQ_HIGHPRI | WQ_UNBOUND, 0);
-	if (!loop_wq) {
-		err = -ENOMEM;
-		goto misc_out;
-	}
-
 	blk_register_region(MKDEV(LOOP_MAJOR, 0), range,
 				  THIS_MODULE, loop_probe, NULL, NULL);
 
@@ -1850,8 +1850,6 @@ static void __exit loop_exit(void)
 	blk_unregister_region(MKDEV(LOOP_MAJOR, 0), range);
 	unregister_blkdev(LOOP_MAJOR, "loop");
 
-	destroy_workqueue(loop_wq);
-
 	misc_deregister(&loop_misc);
 }
 
diff --git a/drivers/block/loop.h b/drivers/block/loop.h
index 301c27f..49564ed 100644
--- a/drivers/block/loop.h
+++ b/drivers/block/loop.h
@@ -54,6 +54,7 @@ struct loop_device {
 	gfp_t		old_gfp_mask;
 
 	spinlock_t		lo_lock;
+	struct workqueue_struct *wq;
 	struct list_head	write_cmd_head;
 	struct work_struct	write_work;
 	bool			write_started;
-- 
1.9.1

Re: [PATCH 1/2] block: loop: convert to per-device workqueue

[Tejun Heo]

On Tue, May 05, 2015 at 07:49:54PM +0800, Ming Lei wrote:
> Documentation/workqueue.txt:
> 	If there is dependency among multiple work items used
> 	during memory reclaim, they should be queued to separate
> 	wq each with WQ_MEM_RECLAIM.
> 
> Loop devices can be stacked, so we have to convert to per-device
> workqueue. One example is Fedora live CD.
> 
> Fixes: b5dd2f6047ca108001328aac0e8588edd15f1778
> Cc: stable@vger.kernel.org (v4.0)
> Cc: Justin M. Forbes <jforbes@fedoraproject.org>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH 1/2] block: loop: convert to per-device workqueue

[Dave Kleikamp]

On 05/05/2015 06:49 AM, Ming Lei wrote:
> Documentation/workqueue.txt:
> 	If there is dependency among multiple work items used
> 	during memory reclaim, they should be queued to separate
> 	wq each with WQ_MEM_RECLAIM.
> 
> Loop devices can be stacked, so we have to convert to per-device
> workqueue. One example is Fedora live CD.

I'm just now looking at this, but I found a minor nit. I'm not sure if
it's worth replacing the patch or just fixing it with a trivial cleanup
patch.

> 
> Fixes: b5dd2f6047ca108001328aac0e8588edd15f1778
> Cc: stable@vger.kernel.org (v4.0)
> Cc: Justin M. Forbes <jforbes@fedoraproject.org>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/block/loop.c | 30 ++++++++++++++----------------
>  drivers/block/loop.h |  1 +
>  2 files changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index ae3fcb4..3dc1598 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -86,8 +86,6 @@ static DEFINE_MUTEX(loop_index_mutex);
>  static int max_part;
>  static int part_shift;
>  
> -static struct workqueue_struct *loop_wq;
> -
>  static int transfer_xor(struct loop_device *lo, int cmd,
>  			struct page *raw_page, unsigned raw_off,
>  			struct page *loop_page, unsigned loop_off,
> @@ -725,6 +723,12 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
>  	size = get_loop_size(lo, file);
>  	if ((loff_t)(sector_t)size != size)
>  		goto out_putf;
> +	error = -ENOMEM;
> +	lo->wq = alloc_workqueue("kloopd%d",
> +			WQ_MEM_RECLAIM | WQ_HIGHPRI | WQ_UNBOUND, 0,
> +			lo->lo_number);
> +	if (!lo->wq)
> +		goto out_putf;
>  
>  	error = 0;
>  
> @@ -872,6 +876,8 @@ static int loop_clr_fd(struct loop_device *lo)
>  	lo->lo_flags = 0;
>  	if (!part_shift)
>  		lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN;
> +	destroy_workqueue(lo->wq);
> +	lo->wq = NULL;
>  	mutex_unlock(&lo->lo_ctl_mutex);
>  	/*
>  	 * Need not hold lo_ctl_mutex to fput backing file.
> @@ -1425,9 +1431,13 @@ static int loop_queue_rq(struct blk_mq_hw_ctx *hctx,
>  		const struct blk_mq_queue_data *bd)
>  {
>  	struct loop_cmd *cmd = blk_mq_rq_to_pdu(bd->rq);
> +	struct loop_device *lo = cmd->rq->q->queuedata;
>  
>  	blk_mq_start_request(bd->rq);
>  
> +	if (lo->lo_state != Lo_bound)
> +		return -EIO;
> +
>  	if (cmd->rq->cmd_flags & REQ_WRITE) {
>  		struct loop_device *lo = cmd->rq->q->queuedata;

The above definition of lo becomes redundant now.

>  		bool need_sched = true;
> @@ -1441,9 +1451,9 @@ static int loop_queue_rq(struct blk_mq_hw_ctx *hctx,
>  		spin_unlock_irq(&lo->lo_lock);
>  
>  		if (need_sched)
> -			queue_work(loop_wq, &lo->write_work);
> +			queue_work(lo->wq, &lo->write_work);
>  	} else {
> -		queue_work(loop_wq, &cmd->read_work);
> +		queue_work(lo->wq, &cmd->read_work);
>  	}
>  
>  	return BLK_MQ_RQ_QUEUE_OK;
> @@ -1455,9 +1465,6 @@ static void loop_handle_cmd(struct loop_cmd *cmd)
>  	struct loop_device *lo = cmd->rq->q->queuedata;
>  	int ret = -EIO;
>  
> -	if (lo->lo_state != Lo_bound)
> -		goto failed;
> -
>  	if (write && (lo->lo_flags & LO_FLAGS_READ_ONLY))
>  		goto failed;
>  
> @@ -1806,13 +1813,6 @@ static int __init loop_init(void)
>  		goto misc_out;
>  	}
>  
> -	loop_wq = alloc_workqueue("kloopd",
> -			WQ_MEM_RECLAIM | WQ_HIGHPRI | WQ_UNBOUND, 0);
> -	if (!loop_wq) {
> -		err = -ENOMEM;
> -		goto misc_out;
> -	}
> -
>  	blk_register_region(MKDEV(LOOP_MAJOR, 0), range,
>  				  THIS_MODULE, loop_probe, NULL, NULL);
>  
> @@ -1850,8 +1850,6 @@ static void __exit loop_exit(void)
>  	blk_unregister_region(MKDEV(LOOP_MAJOR, 0), range);
>  	unregister_blkdev(LOOP_MAJOR, "loop");
>  
> -	destroy_workqueue(loop_wq);
> -
>  	misc_deregister(&loop_misc);
>  }
>  
> diff --git a/drivers/block/loop.h b/drivers/block/loop.h
> index 301c27f..49564ed 100644
> --- a/drivers/block/loop.h
> +++ b/drivers/block/loop.h
> @@ -54,6 +54,7 @@ struct loop_device {
>  	gfp_t		old_gfp_mask;
>  
>  	spinlock_t		lo_lock;
> +	struct workqueue_struct *wq;
>  	struct list_head	write_cmd_head;
>  	struct work_struct	write_work;
>  	bool			write_started;
> 

[PATCH 2/2] block: loop: avoiding too many pending per work I/O

[Ming Lei]

If there are too many pending per work I/O, too many
high priority work thread can be generated so that
system performance can be effected.

This patch limits the max_active parameter of workqueue as 16.

This patch fixes Fedora 22 live booting performance
regression when it is booted from squashfs over dm
based on loop, and looks the following reasons are
related with the problem:

- not like other filesyststems(such as ext4), squashfs
is a bit special, and I observed that increasing I/O jobs
to access file in squashfs only improve I/O performance a
little, but it can make big difference for ext4

- nested loop: both squashfs.img and ext3fs.img are mounted
as loop block, and ext3fs.img is inside the squashfs

- during booting, lots of tasks may run concurrently

Fixes: b5dd2f6047ca108001328aac0e8588edd15f1778
Cc: stable@vger.kernel.org (v4.0)
Cc: Justin M. Forbes <jforbes@fedoraproject.org>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/block/loop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 3dc1598..1bee523 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -725,7 +725,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 		goto out_putf;
 	error = -ENOMEM;
 	lo->wq = alloc_workqueue("kloopd%d",
-			WQ_MEM_RECLAIM | WQ_HIGHPRI | WQ_UNBOUND, 0,
+			WQ_MEM_RECLAIM | WQ_HIGHPRI | WQ_UNBOUND, 16,
 			lo->lo_number);
 	if (!lo->wq)
 		goto out_putf;
-- 
1.9.1

Re: [PATCH 2/2] block: loop: avoiding too many pending per work I/O

[Tejun Heo]

On Tue, May 05, 2015 at 07:49:55PM +0800, Ming Lei wrote:
...
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 3dc1598..1bee523 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -725,7 +725,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
>  		goto out_putf;
>  	error = -ENOMEM;
>  	lo->wq = alloc_workqueue("kloopd%d",
> -			WQ_MEM_RECLAIM | WQ_HIGHPRI | WQ_UNBOUND, 0,
> +			WQ_MEM_RECLAIM | WQ_HIGHPRI | WQ_UNBOUND, 16,

It's a bit weird to hard code this to 16 as this effectively becomes a
hidden bottleneck for concurrency.  For cases where 16 isn't a good
value, hunting down what's going on can be painful as it's not visible
anywhere.  I still think the right knob to control concurrency is
nr_requests for the loop device.  You said that for linear IOs, it's
better to have higher nr_requests than concurrency but can you
elaborate why?

Thanks.

-- 
tejun

Re: [PATCH 2/2] block: loop: avoiding too many pending per work I/O

[Ming Lei]

On Tue, May 5, 2015 at 9:59 PM, Tejun Heo <tj@kernel.org> wrote:
> On Tue, May 05, 2015 at 07:49:55PM +0800, Ming Lei wrote:
> ...
>> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
>> index 3dc1598..1bee523 100644
>> --- a/drivers/block/loop.c
>> +++ b/drivers/block/loop.c
>> @@ -725,7 +725,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
>>               goto out_putf;
>>       error = -ENOMEM;
>>       lo->wq = alloc_workqueue("kloopd%d",
>> -                     WQ_MEM_RECLAIM | WQ_HIGHPRI | WQ_UNBOUND, 0,
>> +                     WQ_MEM_RECLAIM | WQ_HIGHPRI | WQ_UNBOUND, 16,
>
> It's a bit weird to hard code this to 16 as this effectively becomes a
> hidden bottleneck for concurrency.  For cases where 16 isn't a good
> value, hunting down what's going on can be painful as it's not visible
> anywhere.  I still think the right knob to control concurrency is
> nr_requests for the loop device.  You said that for linear IOs, it's
> better to have higher nr_requests than concurrency but can you
> elaborate why?

I mean, in case of sequential IO, the IO may hit page cache a bit easier,
so handling the IO may be quite quick, then it is often more efficient to
handle them in one same context(such as, handle one by one from IO
queue) than from different contexts(scheduled from different worker
threads). And that can be made by setting a bigger nr_requests(queue_depth).

Thanks,
Ming Lei

Re: [PATCH 2/2] block: loop: avoiding too many pending per work I/O

[Tejun Heo]

Hello, Ming.

On Tue, May 05, 2015 at 10:46:10PM +0800, Ming Lei wrote:
> On Tue, May 5, 2015 at 9:59 PM, Tejun Heo <tj@kernel.org> wrote:
> > It's a bit weird to hard code this to 16 as this effectively becomes a
> > hidden bottleneck for concurrency.  For cases where 16 isn't a good
> > value, hunting down what's going on can be painful as it's not visible
> > anywhere.  I still think the right knob to control concurrency is
> > nr_requests for the loop device.  You said that for linear IOs, it's
> > better to have higher nr_requests than concurrency but can you
> > elaborate why?
> 
> I mean, in case of sequential IO, the IO may hit page cache a bit easier,
> so handling the IO may be quite quick, then it is often more efficient to
> handle them in one same context(such as, handle one by one from IO
> queue) than from different contexts(scheduled from different worker
> threads). And that can be made by setting a bigger nr_requests(queue_depth).

Ah, so, it's about the queueing latency.  Blocking the issuer from
get_request side for the same level of concurrency would incur a lot
longer latency before the next IO can be dispatched.  The arbitrary 16
is still bothering but for now it's fine I guess, but we need to
revisit the whole thing including WQ_HIGHPRI thing.  Maybe it made
sense when we had only one thread servicing all IOs but w/ high
concurrency I don't think it's a good idea.

Please feel free to add

 Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH 2/2] block: loop: avoiding too many pending per work I/O

[Ming Lei]

On Wed, May 6, 2015 at 12:55 AM, Tejun Heo <tj@kernel.org> wrote:
> Hello, Ming.
>
> On Tue, May 05, 2015 at 10:46:10PM +0800, Ming Lei wrote:
>> On Tue, May 5, 2015 at 9:59 PM, Tejun Heo <tj@kernel.org> wrote:
>> > It's a bit weird to hard code this to 16 as this effectively becomes a
>> > hidden bottleneck for concurrency.  For cases where 16 isn't a good
>> > value, hunting down what's going on can be painful as it's not visible
>> > anywhere.  I still think the right knob to control concurrency is
>> > nr_requests for the loop device.  You said that for linear IOs, it's
>> > better to have higher nr_requests than concurrency but can you
>> > elaborate why?
>>
>> I mean, in case of sequential IO, the IO may hit page cache a bit easier,
>> so handling the IO may be quite quick, then it is often more efficient to
>> handle them in one same context(such as, handle one by one from IO
>> queue) than from different contexts(scheduled from different worker
>> threads). And that can be made by setting a bigger nr_requests(queue_depth).
>
> Ah, so, it's about the queueing latency.  Blocking the issuer from
> get_request side for the same level of concurrency would incur a lot
> longer latency before the next IO can be dispatched.  The arbitrary 16
> is still bothering but for now it's fine I guess, but we need to
> revisit the whole thing including WQ_HIGHPRI thing.  Maybe it made
> sense when we had only one thread servicing all IOs but w/ high
> concurrency I don't think it's a good idea.

Yes, I was thinking about it too, but concurrency can improve
random I/O throughput a lot in my tests.

Also I have patches to use aio/dio for loop, then one thread is enough,
and both double cache and high context switch can be avoided.

I will post them later for review.

>
> Please feel free to add
>
>  Acked-by: Tejun Heo <tj@kernel.org>

Thanks for your review and ack!

thanks,
Ming Lei

Re: [PATCH 2/2] block: loop: avoiding too many pending per work I/O

[Ming Lei]

On Wed, May 6, 2015 at 11:14 AM, Ming Lei <ming.lei@canonical.com> wrote:
> On Wed, May 6, 2015 at 12:55 AM, Tejun Heo <tj@kernel.org> wrote:
>> Hello, Ming.
>>
>> On Tue, May 05, 2015 at 10:46:10PM +0800, Ming Lei wrote:
>>> On Tue, May 5, 2015 at 9:59 PM, Tejun Heo <tj@kernel.org> wrote:
>>> > It's a bit weird to hard code this to 16 as this effectively becomes a
>>> > hidden bottleneck for concurrency.  For cases where 16 isn't a good
>>> > value, hunting down what's going on can be painful as it's not visible
>>> > anywhere.  I still think the right knob to control concurrency is
>>> > nr_requests for the loop device.  You said that for linear IOs, it's
>>> > better to have higher nr_requests than concurrency but can you
>>> > elaborate why?
>>>
>>> I mean, in case of sequential IO, the IO may hit page cache a bit easier,
>>> so handling the IO may be quite quick, then it is often more efficient to
>>> handle them in one same context(such as, handle one by one from IO
>>> queue) than from different contexts(scheduled from different worker
>>> threads). And that can be made by setting a bigger nr_requests(queue_depth).
>>
>> Ah, so, it's about the queueing latency.  Blocking the issuer from
>> get_request side for the same level of concurrency would incur a lot
>> longer latency before the next IO can be dispatched.  The arbitrary 16
>> is still bothering but for now it's fine I guess, but we need to
>> revisit the whole thing including WQ_HIGHPRI thing.  Maybe it made
>> sense when we had only one thread servicing all IOs but w/ high
>> concurrency I don't think it's a good idea.
>
> Yes, I was thinking about it too, but concurrency can improve
> random I/O throughput a lot in my tests.

Thinking of it further, the problem becomes very similar with
'Non-blocking buffered file read operations'[1] which was discussed
last year.  If the read IO can be predicted as buffered I/O, we
handle it in single thread, otherwise handle it concurrently,
and this approach should be more efficient if possible, I think.

But I still prefer to dio/aio approach because double cache can
be avoided, which is a big win in my previous tests.

[1], https://lwn.net/Articles/612483/

Thanks,
Ming Lei

Re: [PATCH 2/2] block: loop: avoiding too many pending per work I/O

[Christoph Hellwig]

On Wed, May 06, 2015 at 01:17:33PM +0800, Ming Lei wrote:
> But I still prefer to dio/aio approach because double cache can
> be avoided, which is a big win in my previous tests.

Can yo respin and resend the patches?  With Al's iov_iter work and my
kiocb changes it should be fairly easy and non-intrusive to do.

Re: [PATCH 2/2] block: loop: avoiding too many pending per work I/O

[Ming Lei]

On Wed, May 6, 2015 at 3:36 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Wed, May 06, 2015 at 01:17:33PM +0800, Ming Lei wrote:
>> But I still prefer to dio/aio approach because double cache can
>> be avoided, which is a big win in my previous tests.
>
> Can yo respin and resend the patches?  With Al's iov_iter work and my
> kiocb changes it should be fairly easy and non-intrusive to do.

I will post them later.

thanks,

Re: [PATCH 2/2] block: loop: avoiding too many pending per work I/O

[Josh Boyer]

On Tue, May 5, 2015 at 7:49 AM, Ming Lei <ming.lei@canonical.com> wrote:
> If there are too many pending per work I/O, too many
> high priority work thread can be generated so that
> system performance can be effected.
>
> This patch limits the max_active parameter of workqueue as 16.
>
> This patch fixes Fedora 22 live booting performance
> regression when it is booted from squashfs over dm
> based on loop, and looks the following reasons are
> related with the problem:
>
> - not like other filesyststems(such as ext4), squashfs
> is a bit special, and I observed that increasing I/O jobs
> to access file in squashfs only improve I/O performance a
> little, but it can make big difference for ext4
>
> - nested loop: both squashfs.img and ext3fs.img are mounted
> as loop block, and ext3fs.img is inside the squashfs
>
> - during booting, lots of tasks may run concurrently
>
> Fixes: b5dd2f6047ca108001328aac0e8588edd15f1778
> Cc: stable@vger.kernel.org (v4.0)
> Cc: Justin M. Forbes <jforbes@fedoraproject.org>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>

Did we ever come to conclusion on this and patch 1/2 in the series?
Fedora has them applied to it's 4.0.y based kernels to fix the
performance regression we saw, and we're carrying them in rawhide as
well.  I'm curious if these will go into 4.1 or if they're queued at
all for 4.2?

josh

> ---
>  drivers/block/loop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 3dc1598..1bee523 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -725,7 +725,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
>                 goto out_putf;
>         error = -ENOMEM;
>         lo->wq = alloc_workqueue("kloopd%d",
> -                       WQ_MEM_RECLAIM | WQ_HIGHPRI | WQ_UNBOUND, 0,
> +                       WQ_MEM_RECLAIM | WQ_HIGHPRI | WQ_UNBOUND, 16,
>                         lo->lo_number);
>         if (!lo->wq)
>                 goto out_putf;
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe stable" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] block: loop: avoiding too many pending per work I/O

[Ming Lei]

On Fri, May 22, 2015 at 8:36 PM, Josh Boyer <jwboyer@fedoraproject.org> wrote:
> On Tue, May 5, 2015 at 7:49 AM, Ming Lei <ming.lei@canonical.com> wrote:
>> If there are too many pending per work I/O, too many
>> high priority work thread can be generated so that
>> system performance can be effected.
>>
>> This patch limits the max_active parameter of workqueue as 16.
>>
>> This patch fixes Fedora 22 live booting performance
>> regression when it is booted from squashfs over dm
>> based on loop, and looks the following reasons are
>> related with the problem:
>>
>> - not like other filesyststems(such as ext4), squashfs
>> is a bit special, and I observed that increasing I/O jobs
>> to access file in squashfs only improve I/O performance a
>> little, but it can make big difference for ext4
>>
>> - nested loop: both squashfs.img and ext3fs.img are mounted
>> as loop block, and ext3fs.img is inside the squashfs
>>
>> - during booting, lots of tasks may run concurrently
>>
>> Fixes: b5dd2f6047ca108001328aac0e8588edd15f1778
>> Cc: stable@vger.kernel.org (v4.0)
>> Cc: Justin M. Forbes <jforbes@fedoraproject.org>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>
> Did we ever come to conclusion on this and patch 1/2 in the series?
> Fedora has them applied to it's 4.0.y based kernels to fix the
> performance regression we saw, and we're carrying them in rawhide as
> well.  I'm curious if these will go into 4.1 or if they're queued at
> all for 4.2?

I saw it queued in for-next branch of block tree, so it should be merged
to 4.2.

>
> josh
>
>> ---
>>  drivers/block/loop.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
>> index 3dc1598..1bee523 100644
>> --- a/drivers/block/loop.c
>> +++ b/drivers/block/loop.c
>> @@ -725,7 +725,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
>>                 goto out_putf;
>>         error = -ENOMEM;
>>         lo->wq = alloc_workqueue("kloopd%d",
>> -                       WQ_MEM_RECLAIM | WQ_HIGHPRI | WQ_UNBOUND, 0,
>> +                       WQ_MEM_RECLAIM | WQ_HIGHPRI | WQ_UNBOUND, 16,
>>                         lo->lo_number);
>>         if (!lo->wq)
>>                 goto out_putf;
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe stable" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] block: loop: fix stacked loop and performance regression

[Justin M. Forbes]

On Tue, 2015-05-05 at 19:49 +0800, Ming Lei wrote:
> Hi,
> 
> The 1st patch convers to per-device workqueue because loop devices
> can be stacked.
> 
> The 2nd patch decreases max active works as 16, so that fedora 22's
> boot performance regression can be fixed.
> 
>  drivers/block/loop.c | 30 ++++++++++++++----------------
>  drivers/block/loop.h |  1 +
>  2 files changed, 15 insertions(+), 16 deletions(-)
> 
> 
> Thanks,
> Ming Lei
> 

Tested with Fedora 22 ISOs, these still solve the problem for us.

Tested-by: Justin M. Forbes <jforbes@fedoraproject.org>