Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1659564 > unrolled thread

[PATCH] Fix loop device flush before configure

Back to article view | Back to linux.kernel

Contents

  [PATCH] Fix loop device flush before configure James Wang <jnwang@suse.com> - 2017-06-07 11:10 +0200
    Re: [PATCH] Fix loop device flush before configure Ming Lei <ming.lei@redhat.com> - 2017-06-07 11:30 +0200
      答复： Re: [PATCH] Fix loop device flush before configure "Jin Nan Wang" <jnwang@suse.com> - 2017-06-07 12:00 +0200
    Re: [PATCH] Fix loop device flush before configure Christoph Hellwig <hch@infradead.org> - 2017-06-07 15:20 +0200
      Re: [PATCH] Fix loop device flush before configure James Wang <jnwang@suse.com> - 2017-06-08 02:50 +0200
        Re: [PATCH] Fix loop device flush before configure Christoph Hellwig <hch@infradead.org> - 2017-06-08 09:00 +0200
          Re: [PATCH] Fix loop device flush before configure James Wang <jnwang@suse.com> - 2017-06-08 09:10 +0200

#1659564 — [PATCH] Fix loop device flush before configure

This condition check was exist at before commit b5dd2f6047ca ("block: loop:
improve performance via blk-mq") When add MQ support to loop device, it be
removed because the member of '->lo_thread' be removed. And then upstream
add '->worker_task', I think they forget add it to here.

When I install SLES-12 product is base on 4.4 kernel, I found installer will
hang +60 second at scan disks. and I found LVM tools would take this action.
finally I found this problem is more obvious on AMD platform. This problem
will impact all scenarios that scan loop devcies.

When the loop device didn't configure backing file or Request Queue, we
shouldn't to cost a lot of time to flush it.

Testing steps are following:
modprobe loop max_loop=64
dd if=/dev/zero of=disk bs=512 count=200K
for((i=0;i<4;i++))do losetup -f disk; done
mkfs.ext4 -F /dev/loop0
for((i=0;i<4;i++))do mkdir t$i; mount /dev/loop$i t$i;done
for f in `ls /dev/loop[0-9]*|sort`; do \
	echo $f; dd if=$f of=/dev/null  bs=512 count=1; \
	done

Testing data is following:
/dev/loop0	<rpm-4.4.68-2> <+patched>
		8.1217e-05 	8.3842e-05
/dev/loop1
		6.1114e-05 	0.000147979
/dev/loop10
		0.414701 	0.000116564
/dev/loop11
		0.7474 		6.7942e-05
/dev/loop12
		0.747986 	8.9082e-05
/dev/loop13
		0.746532 	7.4799e-05
/dev/loop14
		0.480041 	9.3926e-05
/dev/loop15
		1.26453 	7.2522e-05

From /dev/loop10 start, loop isn't mounted. but it take more time than
mounted devices. And The data differ by several orders of magnitude.

Reviewed-by: Hannes Reinecke <hare@suse.com>

Signed-off-by: James Wang <jnwang@suse.com>
---
 drivers/block/loop.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 48f6fa6f810e..c1807e91db08 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -625,6 +625,9 @@ static int loop_switch(struct loop_device *lo, struct file *file)
  */
 static int loop_flush(struct loop_device *lo)
 {
+	/* loop not yet configured, no running thread, nothing to flush */
+	if (!lo->worker_task)
+		return 0;
 	return loop_switch(lo, NULL);
 }
 
-- 
2.12.3

[toc] | [next] | [standalone]

#1659584

On Wed, Jun 07, 2017 at 05:01:04PM +0800, James Wang wrote:
> This condition check was exist at before commit b5dd2f6047ca ("block: loop:
> improve performance via blk-mq") When add MQ support to loop device, it be
> removed because the member of '->lo_thread' be removed. And then upstream
> add '->worker_task', I think they forget add it to here.
> 
> When I install SLES-12 product is base on 4.4 kernel, I found installer will
> hang +60 second at scan disks. and I found LVM tools would take this action.
> finally I found this problem is more obvious on AMD platform. This problem
> will impact all scenarios that scan loop devcies.
> 
> When the loop device didn't configure backing file or Request Queue, we
> shouldn't to cost a lot of time to flush it.
> 
> Testing steps are following:
> modprobe loop max_loop=64
> dd if=/dev/zero of=disk bs=512 count=200K
> for((i=0;i<4;i++))do losetup -f disk; done
> mkfs.ext4 -F /dev/loop0
> for((i=0;i<4;i++))do mkdir t$i; mount /dev/loop$i t$i;done
> for f in `ls /dev/loop[0-9]*|sort`; do \
> 	echo $f; dd if=$f of=/dev/null  bs=512 count=1; \
> 	done
> 
> Testing data is following:
> /dev/loop0	<rpm-4.4.68-2> <+patched>
> 		8.1217e-05 	8.3842e-05
> /dev/loop1
> 		6.1114e-05 	0.000147979
> /dev/loop10
> 		0.414701 	0.000116564
> /dev/loop11
> 		0.7474 		6.7942e-05
> /dev/loop12
> 		0.747986 	8.9082e-05
> /dev/loop13
> 		0.746532 	7.4799e-05
> /dev/loop14
> 		0.480041 	9.3926e-05
> /dev/loop15
> 		1.26453 	7.2522e-05
> 
> From /dev/loop10 start, loop isn't mounted. but it take more time than
> mounted devices. And The data differ by several orders of magnitude.
> 
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> 
> Signed-off-by: James Wang <jnwang@suse.com>
> ---
>  drivers/block/loop.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 48f6fa6f810e..c1807e91db08 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -625,6 +625,9 @@ static int loop_switch(struct loop_device *lo, struct file *file)
>   */
>  static int loop_flush(struct loop_device *lo)
>  {
> +	/* loop not yet configured, no running thread, nothing to flush */
> +	if (!lo->worker_task)
> +		return 0;
>  	return loop_switch(lo, NULL);
>  }

Good catch!

But looks better to add check like the following

	if (lo->lo_state != Lo_bound)
		return 0;

because we don't clear lo->worker_task in loop_unprepare_queue()
and it is more readable to check on lo->lo_state.


thanks,
Ming

[toc] | [prev] | [next] | [standalone]

#1659634 — 答复： Re: [PATCH] Fix loop device flush before configure

>>> Ming Lei <ming.lei@redhat.com> 2017-6-7 下午 17:26 >>>
On Wed, Jun 07, 2017 at 05:01:04PM +0800, James Wang wrote:
> This condition check was exist at before commit b5dd2f6047ca ("block: loop:
> improve performance via blk-mq") When add MQ support to loop device, it be
> removed because the member of '->lo_thread' be removed. And then upstream
> add '->worker_task', I think they forget add it to here.
> 
> When I install SLES-12 product is base on 4.4 kernel, I found installer will
> hang +60 second at scan disks. and I found LVM tools would take this action.
> finally I found this problem is more obvious on AMD platform. This problem
> will impact all scenarios that scan loop devcies.
> 
> When the loop device didn't configure backing file or Request Queue, we
> shouldn't to cost a lot of time to flush it.
> 
> Testing steps are following:
> modprobe loop max_loop=64
> dd if=/dev/zero of=disk bs=512 count=200K
> for((i=0;i<4;i++))do losetup -f disk; done
> mkfs.ext4 -F /dev/loop0
> for((i=0;i<4;i++))do mkdir t$i; mount /dev/loop$i t$i;done
> for f in `ls /dev/loop[0-9]*|sort`; do \
> 	echo $f; dd if=$f of=/dev/null  bs=512 count=1; \
> 	done
> 
> Testing data is following:
> /dev/loop0	<rpm-4.4.68-2> <+patched>
> 		8.1217e-05 	8.3842e-05
> /dev/loop1
> 		6.1114e-05 	0.000147979
> /dev/loop10
> 		0.414701 	0.000116564
> /dev/loop11
> 		0.7474 		6.7942e-05
> /dev/loop12
> 		0.747986 	8.9082e-05
> /dev/loop13
> 		0.746532 	7.4799e-05
> /dev/loop14
> 		0.480041 	9.3926e-05
> /dev/loop15
> 		1.26453 	7.2522e-05
> 
> From /dev/loop10 start, loop isn't mounted. but it take more time than
> mounted devices. And The data differ by several orders of magnitude.
> 
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> 
> Signed-off-by: James Wang <jnwang@suse.com>
> ---
>  drivers/block/loop.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 48f6fa6f810e..c1807e91db08 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -625,6 +625,9 @@ static int loop_switch(struct loop_device *lo, struct file *file)
>   */
>  static int loop_flush(struct loop_device *lo)
>  {
> +	/* loop not yet configured, no running thread, nothing to flush */
> +	if (!lo->worker_task)
> +		return 0;
>  	return loop_switch(lo, NULL);
>  }

Good catch!

But looks better to add check like the following

	if (lo->lo_state != Lo_bound)
		return 0;

because we don't clear lo->worker_task in loop_unprepare_queue()
and it is more readable to check on lo->lo_state.

why not?

thanks,
Ming

[toc] | [prev] | [next] | [standalone]

#1659784

> Testing steps are following:
> modprobe loop max_loop=64
> dd if=/dev/zero of=disk bs=512 count=200K
> for((i=0;i<4;i++))do losetup -f disk; done
> mkfs.ext4 -F /dev/loop0
> for((i=0;i<4;i++))do mkdir t$i; mount /dev/loop$i t$i;done
> for f in `ls /dev/loop[0-9]*|sort`; do \
> 	echo $f; dd if=$f of=/dev/null  bs=512 count=1; \
> 	done

Can you write this up for blktests, please?

[toc] | [prev] | [next] | [standalone]

#1660634

Ok I got it blktests is a suite. I'd like to contribute something. If you need, we adapt you,;-)!
But I have to learn some how to do that, need time.

James
 
From subway.

在 2017年6月7日，21:17，Christoph Hellwig <hch@infradead.org> 写道：

>> Testing steps are following:
>> modprobe loop max_loop=64
>> dd if=/dev/zero of=disk bs=512 count=200K
>> for((i=0;i<4;i++))do losetup -f disk; done
>> mkfs.ext4 -F /dev/loop0
>> for((i=0;i<4;i++))do mkdir t$i; mount /dev/loop$i t$i;done
>> for f in `ls /dev/loop[0-9]*|sort`; do \
>>    echo $f; dd if=$f of=/dev/null  bs=512 count=1; \
>>    done
> 
> Can you write this up for blktests, please?
> 

[toc] | [prev] | [next] | [standalone]

#1660800

On Thu, Jun 08, 2017 at 08:45:31AM +0800, James Wang wrote:
> Ok I got it blktests is a suite. I'd like to contribute something. If you need, we adapt you,;-)!
> But I have to learn some how to do that, need time.

I haven't added test myself to blktests yet either, so I'd have to
learn it as well.  Omar can probably help you though.

[toc] | [prev] | [next] | [standalone]

#1660806

On 06/08/2017 02:56 PM, Christoph Hellwig wrote:
> On Thu, Jun 08, 2017 at 08:45:31AM +0800, James Wang wrote:
>> Ok I got it blktests is a suite. I'd like to contribute something. If you need, we adapt you,;-)!
>> But I have to learn some how to do that, need time.
> I haven't added test myself to blktests yet either, so I'd have to
> learn it as well.  Omar can probably help you though.
>
>
Ah, I have fork this project  in Github.  and write 1 script in 'loop' 
group.

Debuging................


James

-- 
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web