Re: [Qemu-block] [PATCH v4 3/5] block/qcow2: refactor qcow2_co_preadv_part
On 13.09.19 13:34, Kevin Wolf wrote: > Am 13.09.2019 um 13:06 hat Max Reitz geschrieben: >> On 13.09.19 12:53, Vladimir Sementsov-Ogievskiy wrote: >>> 13.09.2019 13:01, Kevin Wolf wrote: Am 16.08.2019 um 17:30 hat Vladimir Sementsov-Ogievskiy geschrieben: > Further patch will run partial requests of iterations of > qcow2_co_preadv in parallel for performance reasons. To prepare for > this, separate part which may be parallelized into separate function > (qcow2_co_preadv_task). > > While being here, also separate encrypted clusters reading to own > function, like it is done for compressed reading. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > Reviewed-by: Max Reitz > --- > qapi/block-core.json | 2 +- > block/qcow2.c| 205 +++ > 2 files changed, 111 insertions(+), 96 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 0d43d4f37c..dd80aa11db 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -3266,7 +3266,7 @@ > 'pwritev_rmw_tail', 'pwritev_rmw_after_tail', 'pwritev', > 'pwritev_zero', 'pwritev_done', 'empty_image_prepare', > 'l1_shrink_write_table', 'l1_shrink_free_l2_clusters', > -'cor_write', 'cluster_alloc_space', 'none'] } > +'cor_write', 'cluster_alloc_space', 'none', > 'read_encrypted'] } What's the point of this new blkdebug event? Obviously, read_aio for an encrypted image must mean a read of encrypted data. The same image can never trigger both read_aio and read_encrypted, so why do we need to distinguish them as two different events? >>> >>> Seems I just done it looking at qcow2_co_preadv_compressed.. >>> >>> Anyway, I think you are right, so, I don't mind if Max drops this new event >>> and use read_aio in his branch, or I can resend the series or send a >>> follow-up, >>> whichever you prefer. >> >> Should I squash this in? > > Looks good to me. OK, done. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v4 3/5] block/qcow2: refactor qcow2_co_preadv_part
Am 13.09.2019 um 13:06 hat Max Reitz geschrieben: > On 13.09.19 12:53, Vladimir Sementsov-Ogievskiy wrote: > > 13.09.2019 13:01, Kevin Wolf wrote: > >> Am 16.08.2019 um 17:30 hat Vladimir Sementsov-Ogievskiy geschrieben: > >>> Further patch will run partial requests of iterations of > >>> qcow2_co_preadv in parallel for performance reasons. To prepare for > >>> this, separate part which may be parallelized into separate function > >>> (qcow2_co_preadv_task). > >>> > >>> While being here, also separate encrypted clusters reading to own > >>> function, like it is done for compressed reading. > >>> > >>> Signed-off-by: Vladimir Sementsov-Ogievskiy > >>> Reviewed-by: Max Reitz > >>> --- > >>> qapi/block-core.json | 2 +- > >>> block/qcow2.c| 205 +++ > >>> 2 files changed, 111 insertions(+), 96 deletions(-) > >>> > >>> diff --git a/qapi/block-core.json b/qapi/block-core.json > >>> index 0d43d4f37c..dd80aa11db 100644 > >>> --- a/qapi/block-core.json > >>> +++ b/qapi/block-core.json > >>> @@ -3266,7 +3266,7 @@ > >>> 'pwritev_rmw_tail', 'pwritev_rmw_after_tail', 'pwritev', > >>> 'pwritev_zero', 'pwritev_done', 'empty_image_prepare', > >>> 'l1_shrink_write_table', 'l1_shrink_free_l2_clusters', > >>> -'cor_write', 'cluster_alloc_space', 'none'] } > >>> +'cor_write', 'cluster_alloc_space', 'none', > >>> 'read_encrypted'] } > >> > >> What's the point of this new blkdebug event? > >> > >> Obviously, read_aio for an encrypted image must mean a read of encrypted > >> data. The same image can never trigger both read_aio and > >> read_encrypted, so why do we need to distinguish them as two different > >> events? > >> > > > > Seems I just done it looking at qcow2_co_preadv_compressed.. > > > > Anyway, I think you are right, so, I don't mind if Max drops this new event > > and use read_aio in his branch, or I can resend the series or send a > > follow-up, > > whichever you prefer. > > Should I squash this in? Looks good to me. signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH v4 3/5] block/qcow2: refactor qcow2_co_preadv_part
On 13.09.19 12:53, Vladimir Sementsov-Ogievskiy wrote: > 13.09.2019 13:01, Kevin Wolf wrote: >> Am 16.08.2019 um 17:30 hat Vladimir Sementsov-Ogievskiy geschrieben: >>> Further patch will run partial requests of iterations of >>> qcow2_co_preadv in parallel for performance reasons. To prepare for >>> this, separate part which may be parallelized into separate function >>> (qcow2_co_preadv_task). >>> >>> While being here, also separate encrypted clusters reading to own >>> function, like it is done for compressed reading. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>> Reviewed-by: Max Reitz >>> --- >>> qapi/block-core.json | 2 +- >>> block/qcow2.c| 205 +++ >>> 2 files changed, 111 insertions(+), 96 deletions(-) >>> >>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>> index 0d43d4f37c..dd80aa11db 100644 >>> --- a/qapi/block-core.json >>> +++ b/qapi/block-core.json >>> @@ -3266,7 +3266,7 @@ >>> 'pwritev_rmw_tail', 'pwritev_rmw_after_tail', 'pwritev', >>> 'pwritev_zero', 'pwritev_done', 'empty_image_prepare', >>> 'l1_shrink_write_table', 'l1_shrink_free_l2_clusters', >>> -'cor_write', 'cluster_alloc_space', 'none'] } >>> +'cor_write', 'cluster_alloc_space', 'none', 'read_encrypted'] } >> >> What's the point of this new blkdebug event? >> >> Obviously, read_aio for an encrypted image must mean a read of encrypted >> data. The same image can never trigger both read_aio and >> read_encrypted, so why do we need to distinguish them as two different >> events? >> > > Seems I just done it looking at qcow2_co_preadv_compressed.. > > Anyway, I think you are right, so, I don't mind if Max drops this new event > and use read_aio in his branch, or I can resend the series or send a > follow-up, > whichever you prefer. Should I squash this in? diff --git a/qapi/block-core.json b/qapi/block-core.json index d9ae73a43c..e6edd641f1 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3264,7 +3264,7 @@ 'pwritev_rmw_tail', 'pwritev_rmw_after_tail', 'pwritev', 'pwritev_zero', 'pwritev_done', 'empty_image_prepare', 'l1_shrink_write_table', 'l1_shrink_free_l2_clusters', -'cor_write', 'cluster_alloc_space', 'none', 'read_encrypted'] } +'cor_write', 'cluster_alloc_space', 'none'] } ## # @BlkdebugIOType: diff --git a/block/qcow2.c b/block/qcow2.c index b5fe014b20..c07ce84d54 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2001,7 +2001,7 @@ qcow2_co_preadv_encrypted(BlockDriverState *bs, return -ENOMEM; } -BLKDBG_EVENT(bs->file, BLKDBG_READ_ENCRYPTED); +BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO); ret = bdrv_co_pread(s->data_file, file_cluster_offset + offset_into_cluster(s, offset), bytes, buf, 0); signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v4 3/5] block/qcow2: refactor qcow2_co_preadv_part
13.09.2019 13:01, Kevin Wolf wrote: > Am 16.08.2019 um 17:30 hat Vladimir Sementsov-Ogievskiy geschrieben: >> Further patch will run partial requests of iterations of >> qcow2_co_preadv in parallel for performance reasons. To prepare for >> this, separate part which may be parallelized into separate function >> (qcow2_co_preadv_task). >> >> While being here, also separate encrypted clusters reading to own >> function, like it is done for compressed reading. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy >> Reviewed-by: Max Reitz >> --- >> qapi/block-core.json | 2 +- >> block/qcow2.c| 205 +++ >> 2 files changed, 111 insertions(+), 96 deletions(-) >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index 0d43d4f37c..dd80aa11db 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -3266,7 +3266,7 @@ >> 'pwritev_rmw_tail', 'pwritev_rmw_after_tail', 'pwritev', >> 'pwritev_zero', 'pwritev_done', 'empty_image_prepare', >> 'l1_shrink_write_table', 'l1_shrink_free_l2_clusters', >> -'cor_write', 'cluster_alloc_space', 'none'] } >> +'cor_write', 'cluster_alloc_space', 'none', 'read_encrypted'] } > > What's the point of this new blkdebug event? > > Obviously, read_aio for an encrypted image must mean a read of encrypted > data. The same image can never trigger both read_aio and > read_encrypted, so why do we need to distinguish them as two different > events? > Seems I just done it looking at qcow2_co_preadv_compressed.. Anyway, I think you are right, so, I don't mind if Max drops this new event and use read_aio in his branch, or I can resend the series or send a follow-up, whichever you prefer. -- Best regards, Vladimir
Re: [Qemu-block] [PATCH v4 3/5] block/qcow2: refactor qcow2_co_preadv_part
Am 16.08.2019 um 17:30 hat Vladimir Sementsov-Ogievskiy geschrieben: > Further patch will run partial requests of iterations of > qcow2_co_preadv in parallel for performance reasons. To prepare for > this, separate part which may be parallelized into separate function > (qcow2_co_preadv_task). > > While being here, also separate encrypted clusters reading to own > function, like it is done for compressed reading. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > Reviewed-by: Max Reitz > --- > qapi/block-core.json | 2 +- > block/qcow2.c| 205 +++ > 2 files changed, 111 insertions(+), 96 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 0d43d4f37c..dd80aa11db 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -3266,7 +3266,7 @@ > 'pwritev_rmw_tail', 'pwritev_rmw_after_tail', 'pwritev', > 'pwritev_zero', 'pwritev_done', 'empty_image_prepare', > 'l1_shrink_write_table', 'l1_shrink_free_l2_clusters', > -'cor_write', 'cluster_alloc_space', 'none'] } > +'cor_write', 'cluster_alloc_space', 'none', 'read_encrypted'] } What's the point of this new blkdebug event? Obviously, read_aio for an encrypted image must mean a read of encrypted data. The same image can never trigger both read_aio and read_encrypted, so why do we need to distinguish them as two different events? Kevin
