v5: rebase on master, some conflicts resolved due to data-file feature

01: new patch, just move test from cover letter to a file. I really hope that it
    will not hang the whole series, so, if we don't want it as is or with really
    tiny improvements, I'd prefer to skip it and queue 02-10 first.
09: "true" parameter added to moved qcow2_pre_write_overlap_check() call due to
    rebase on master (both before and after patch). Seems OK, so keep Alberto's r-b.

performance:

after 01:
 # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
14.18
 # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
13.77

after 10:
 # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
14.35
 # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
5.62

Vladimir Sementsov-Ogievskiy (10):
  tests/perf: Test qemu-img convert from raw to encrypted qcow2
  qcow2.h: add missing include
  qcow2: add separate file for threaded data processing functions
  qcow2-threads: use thread_pool_submit_co
  qcow2-threads: qcow2_co_do_compress: protect queuing by mutex
  qcow2-threads: split out generic path
  qcow2: qcow2_co_preadv: improve locking
  qcow2: qcow2_co_preadv: skip using hd_qiov when possible
  qcow2: bdrv_co_pwritev: move encryption code out of the lock
  qcow2: do encryption in threads

 block/qcow2.h                               |  20 +-
 block/qcow2-bitmap.c                        |   1 -
 block/qcow2-cache.c                         |   1 -
 block/qcow2-cluster.c                       |   8 +-
 block/qcow2-refcount.c                      |   1 -
 block/qcow2-snapshot.c                      |   1 -
 block/qcow2-threads.c                       | 268 +++++++++++++++++++
 block/qcow2.c                               | 275 ++++----------------
 block/Makefile.objs                         |   2 +-
 tests/perf/block/qcow2/convert-to-encrypted |  48 ++++
 10 files changed, 389 insertions(+), 236 deletions(-)
 create mode 100644 block/qcow2-threads.c
 create mode 100755 tests/perf/block/qcow2/convert-to-encrypted

-- 
2.18.0

ping

02.04.2019 18:37, Vladimir Sementsov-Ogievskiy wrote:
> v5: rebase on master, some conflicts resolved due to data-file feature
> 
> 01: new patch, just move test from cover letter to a file. I really hope that it
>      will not hang the whole series, so, if we don't want it as is or with really
>      tiny improvements, I'd prefer to skip it and queue 02-10 first.
> 09: "true" parameter added to moved qcow2_pre_write_overlap_check() call due to
>      rebase on master (both before and after patch). Seems OK, so keep Alberto's r-b.
> 
> performance:
> 
> after 01:
>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
> 14.18
>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
> 13.77
> 
> after 10:
>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
> 14.35
>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
> 5.62
> 
> Vladimir Sementsov-Ogievskiy (10):
>    tests/perf: Test qemu-img convert from raw to encrypted qcow2
>    qcow2.h: add missing include
>    qcow2: add separate file for threaded data processing functions
>    qcow2-threads: use thread_pool_submit_co
>    qcow2-threads: qcow2_co_do_compress: protect queuing by mutex
>    qcow2-threads: split out generic path
>    qcow2: qcow2_co_preadv: improve locking
>    qcow2: qcow2_co_preadv: skip using hd_qiov when possible
>    qcow2: bdrv_co_pwritev: move encryption code out of the lock
>    qcow2: do encryption in threads
> 
>   block/qcow2.h                               |  20 +-
>   block/qcow2-bitmap.c                        |   1 -
>   block/qcow2-cache.c                         |   1 -
>   block/qcow2-cluster.c                       |   8 +-
>   block/qcow2-refcount.c                      |   1 -
>   block/qcow2-snapshot.c                      |   1 -
>   block/qcow2-threads.c                       | 268 +++++++++++++++++++
>   block/qcow2.c                               | 275 ++++----------------
>   block/Makefile.objs                         |   2 +-
>   tests/perf/block/qcow2/convert-to-encrypted |  48 ++++
>   10 files changed, 389 insertions(+), 236 deletions(-)
>   create mode 100644 block/qcow2-threads.c
>   create mode 100755 tests/perf/block/qcow2/convert-to-encrypted
> 


-- 
Best regards,
Vladimir

On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
> v5: rebase on master, some conflicts resolved due to data-file feature
> 
> 01: new patch, just move test from cover letter to a file. I really hope that it
>     will not hang the whole series, so, if we don't want it as is or with really
>     tiny improvements, I'd prefer to skip it and queue 02-10 first.
> 09: "true" parameter added to moved qcow2_pre_write_overlap_check() call due to
>     rebase on master (both before and after patch). Seems OK, so keep Alberto's r-b.

Patches 2 – 6, 8 – 10:

Reviewed-by: Max Reitz <mreitz@redhat.com>

For 7 I wonder whether the locking can be even tighter.

Max

On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
> v5: rebase on master, some conflicts resolved due to data-file feature
> 
> 01: new patch, just move test from cover letter to a file. I really hope that it
>     will not hang the whole series, so, if we don't want it as is or with really
>     tiny improvements, I'd prefer to skip it and queue 02-10 first.

Yup, noted.

> 09: "true" parameter added to moved qcow2_pre_write_overlap_check() call due to
>     rebase on master (both before and after patch). Seems OK, so keep Alberto's r-b.
> 
> performance:
> 
> after 01:
>  # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
> 14.18
>  # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
> 13.77
> 
> after 10:
>  # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
> 14.35
>  # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
> 5.62

Hm, I see those results as well:

Before:
w/o -W: 7.15
w/  -W: 6.77

After:
w/o -W: 7.19
w/  -W: 3.65


But with -t none, this is what I get:

Before:
w/o -W: 15.98
w/  -W: 10.91

After:
w/o -W: 19.95
w/  -W: 11.77


For good measure, on tmpfs:

Before:
w/o -W: 6.98
w/  -W: 6.75

After:
w/o -W: 7.04
w/  -W: 3.63


So it looks like the results with cache enabled are really only in the
cache.  When it goes down to disk, this series seems to decrease
performance.

To confirm whether that’s actually the case, I took another machine with
an SSD where I have more empty space and increased the size to 8 GB (not
the $size, because qemu-io doesn't like that, but well, yeah), and then
ran it again without cache:

Before:
w/o -W: ~50 - ~60 (varies...)
w/  -W: ~50 - ~70

After:
w/o -W: ~60
w/  -W: ~55 - ~85


So it does seem slower, although the values vary so much that it’s
difficult to tell.

Hmm...  And on that machine, there is no difference between before and
after when using -t none.  So I suppose it also depends on the device?



OK, I tried using qemu-img bench.  After patching it to accept --object,
these are the results I got with -t none -w on a preallocated (full) 8
GB image:

Before:
HDD: 17.7 s, 17.8 s, 18.0 s
SSD 1: 12.9 s, 15.8 s, 15.1 s
SSD 2: 1.8 s, 1.7 s, 1.7 s

After:
HDD: 16.1 s, 15.8 s, 15.8 s
SSD 1: 16.3 s, 18.0 s, 17.9 s
SSD 2: 2.0 s, 1.5 s, 1.5 s

Result #1: My SSD 1 is trash.

Result #2: I need more requests for SSD 2 to get a useful results.
Let's try again with 2^20:
Before: 23.8, 23.5, 23.3
After:  21.0, 20.6, 20.5

OK, and I can clearly see that this series increases the CPU usage
(which is good).


So...  Hm.  I suppose I conclude that this series decreases performance
on trashy SSDs?  But it gets better on my HDD and on my good SSD, so...
 Good thing I tested it, or something.

The only really interesting thing that came out of this is that I didn't
see an improvement with qemu-img convert (only on tmpfs), but that I do
see it with qemu-img bench.  So I'm wondering why you aren't using
qemu-img bench in the test in your first patch...?

Max

> Vladimir Sementsov-Ogievskiy (10):
>   tests/perf: Test qemu-img convert from raw to encrypted qcow2
>   qcow2.h: add missing include
>   qcow2: add separate file for threaded data processing functions
>   qcow2-threads: use thread_pool_submit_co
>   qcow2-threads: qcow2_co_do_compress: protect queuing by mutex
>   qcow2-threads: split out generic path
>   qcow2: qcow2_co_preadv: improve locking
>   qcow2: qcow2_co_preadv: skip using hd_qiov when possible
>   qcow2: bdrv_co_pwritev: move encryption code out of the lock
>   qcow2: do encryption in threads
> 
>  block/qcow2.h                               |  20 +-
>  block/qcow2-bitmap.c                        |   1 -
>  block/qcow2-cache.c                         |   1 -
>  block/qcow2-cluster.c                       |   8 +-
>  block/qcow2-refcount.c                      |   1 -
>  block/qcow2-snapshot.c                      |   1 -
>  block/qcow2-threads.c                       | 268 +++++++++++++++++++
>  block/qcow2.c                               | 275 ++++----------------
>  block/Makefile.objs                         |   2 +-
>  tests/perf/block/qcow2/convert-to-encrypted |  48 ++++
>  10 files changed, 389 insertions(+), 236 deletions(-)
>  create mode 100644 block/qcow2-threads.c
>  create mode 100755 tests/perf/block/qcow2/convert-to-encrypted
> 

29.04.2019 3:37, Max Reitz wrote:
> On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
>> v5: rebase on master, some conflicts resolved due to data-file feature
>>
>> 01: new patch, just move test from cover letter to a file. I really hope that it
>>      will not hang the whole series, so, if we don't want it as is or with really
>>      tiny improvements, I'd prefer to skip it and queue 02-10 first.
> 
> Yup, noted.
> 
>> 09: "true" parameter added to moved qcow2_pre_write_overlap_check() call due to
>>      rebase on master (both before and after patch). Seems OK, so keep Alberto's r-b.
>>
>> performance:
>>
>> after 01:
>>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
>> 14.18
>>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
>> 13.77
>>
>> after 10:
>>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
>> 14.35
>>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
>> 5.62
> 
> Hm, I see those results as well:
> 
> Before:
> w/o -W: 7.15
> w/  -W: 6.77
> 
> After:
> w/o -W: 7.19
> w/  -W: 3.65
> 
> 
> But with -t none, this is what I get:
> 
> Before:
> w/o -W: 15.98
> w/  -W: 10.91
> 
> After:
> w/o -W: 19.95
> w/  -W: 11.77
> 
> 
> For good measure, on tmpfs:
> 
> Before:
> w/o -W: 6.98
> w/  -W: 6.75
> 
> After:
> w/o -W: 7.04
> w/  -W: 3.63
> 
> 
> So it looks like the results with cache enabled are really only in the
> cache.  When it goes down to disk, this series seems to decrease
> performance.
> 
> To confirm whether that’s actually the case, I took another machine with
> an SSD where I have more empty space and increased the size to 8 GB (not
> the $size, because qemu-io doesn't like that, but well, yeah), and then
> ran it again without cache:
> 
> Before:
> w/o -W: ~50 - ~60 (varies...)
> w/  -W: ~50 - ~70
> 
> After:
> w/o -W: ~60
> w/  -W: ~55 - ~85
> 
> 
> So it does seem slower, although the values vary so much that it’s
> difficult to tell.
> 
> Hmm...  And on that machine, there is no difference between before and
> after when using -t none.  So I suppose it also depends on the device?
> 
> 
> 
> OK, I tried using qemu-img bench.  After patching it to accept --object,
> these are the results I got with -t none -w on a preallocated (full) 8
> GB image:
> 
> Before:
> HDD: 17.7 s, 17.8 s, 18.0 s
> SSD 1: 12.9 s, 15.8 s, 15.1 s
> SSD 2: 1.8 s, 1.7 s, 1.7 s
> 
> After:
> HDD: 16.1 s, 15.8 s, 15.8 s
> SSD 1: 16.3 s, 18.0 s, 17.9 s
> SSD 2: 2.0 s, 1.5 s, 1.5 s
> 
> Result #1: My SSD 1 is trash.
> 
> Result #2: I need more requests for SSD 2 to get a useful results.
> Let's try again with 2^20:
> Before: 23.8, 23.5, 23.3
> After:  21.0, 20.6, 20.5
> 
> OK, and I can clearly see that this series increases the CPU usage
> (which is good).
> 
> 
> So...  Hm.  I suppose I conclude that this series decreases performance
> on trashy SSDs?  But it gets better on my HDD and on my good SSD, so...
>   Good thing I tested it, or something.

Interesting, thanks for testing! No idea about degradation on bad SSD..
You can try to check threads overhead by set QCOW2_MAX_THREADS to 1..


> 
> The only really interesting thing that came out of this is that I didn't
> see an improvement with qemu-img convert (only on tmpfs), but that I do
> see it with qemu-img bench.  So I'm wondering why you aren't using
> qemu-img bench in the test in your first patch...?
> 

the idea was that performance tests should do one run, and there should be
common script to run test several times and calculate overage.


-- 
Best regards,
Vladimir

On 29.04.19 02:37, Max Reitz wrote:
> On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
>> v5: rebase on master, some conflicts resolved due to data-file feature

(Forgot to note that I'll look at the more interesting patches in this
series later today.  I just got so hung up on the benchmark...)