block: improve luks driver perf & switch to byte APIs
Subsystems (2)
v2 cover block: improve luks driver perf & switch to byte APIs
This series includes a previously posted patch that improves performance of the luks crypto driver:
https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg00781.html
And then adds three patches that switch over to use byte based APIs for I/O, rather than the legacy sector based APIs.
Daniel P. Berrange (4):
block: use 1 MB bounce buffers for crypto instead of 16KB
block: use BDRV_SECTOR_SIZE in crypto driver
block: convert crypto driver to bdrv_co_preadv|pwritev
block: convert qcrypto_block_encrypt|decrypt to take bytes offset
block/crypto.c | 119 +++++++++++++++++++++++++------------------------ block/qcow.c | 7 ++- block/qcow2-cluster.c | 8 ++-- block/qcow2.c | 4 +- crypto/block-luks.c | 12 +++-- crypto/block-qcow.c | 12 +++-- crypto/block.c | 14 +++--- crypto/blockpriv.h | 4 +- include/crypto/block.h | 14 +++--- 9 files changed, 104 insertions(+), 90 deletions(-)
Signature · 1 line
2.13.5
PATCH v2 1/4 block: use 1 MB bounce buffers for crypto instead of 16KB
Using 16KB bounce buffers creates a significant performance penalty for I/O to encrypted volumes on storage which high I/O latency (rotating rust & network drives), because it triggers lots of fairly small I/O operations.
On tests with rotating rust, and cache=none|directsync, write speed increased from 2MiB/s to 32MiB/s, on a par with that achieved by the in-kernel luks driver. With other cache modes the in-kernel driver is still notably faster because it is able to report completion of the I/O request before any encryption is done, while the in-QEMU driver must encrypt the data before completion.
--- block/crypto.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
block/crypto.c +5 -7
@@ -379,7 +379,7 @@ static void block_crypto_close(BlockDriverState *bs) } -#define BLOCK_CRYPTO_MAX_SECTORS 32 +#define BLOCK_CRYPTO_MAX_SECTORS 2048 static coroutine_fn int block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
@@ -396,9 +396,8 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,qemu_iovec_init (&hd_qiov, qiov->niov); -/* Bounce buffer so we have a linear mem region for -* entire sector. XXX optimize so we avoid bounce -* buffer in case that qiov->niov == 1 +/* Bounce buffer because we don't wish to expose cipher text +* in qiov which points to guest memory. */ cipher_data = qemu_try_blockalign(bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * 512,
@@ -464,9 +463,8 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,qemu_iovec_init (&hd_qiov, qiov->niov); -/* Bounce buffer so we have a linear mem region for -* entire sector. XXX optimize so we avoid bounce -* buffer in case that qiov->niov == 1 +/* Bounce buffer because we're not permitted to touch +* contents of qiov - it points to guest memory. */ cipher_data = qemu_try_blockalign(bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * 512,
Signature · 1 line
2.13.5
PATCH v2 2/4 block: use BDRV_SECTOR_SIZE in crypto driver
In reply to Daniel P. Berrange--- block/crypto.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)
block/crypto.c +16 -16
@@ -392,16 +392,16 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,QEMUIOVector hd_qiov;int ret =0 ;size_t payload_offset = -qcrypto_block_get_payload_offset (crypto->block) /512 ; +qcrypto_block_get_payload_offset (crypto->block) /BDRV_SECTOR_SIZE ;qemu_iovec_init (&hd_qiov, qiov->niov);/* Bounce buffer because we don't wish to expose cipher text * in qiov which points to guest memory. */ - cipher_data = -qemu_try_blockalign (bs->file->bs,MIN (BLOCK_CRYPTO_MAX_SECTORS *512 , - qiov->size)); + cipher_data =qemu_try_blockalign ( + bs->file->bs,MIN (BLOCK_CRYPTO_MAX_SECTORS *BDRV_SECTOR_SIZE , + qiov->size));if (cipher_data ==NULL ) { ret = -ENOMEM ; goto cleanup;
@@ -415,7 +415,7 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num, }qemu_iovec_reset (&hd_qiov); -qemu_iovec_add (&hd_qiov, cipher_data, cur_nr_sectors *512 ); +qemu_iovec_add (&hd_qiov, cipher_data, cur_nr_sectors *BDRV_SECTOR_SIZE ); ret = bdrv_co_readv(bs->file, payload_offset + sector_num,
@@ -426,18 +426,18 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,if (qcrypto_block_decrypt (crypto->block, sector_num, - cipher_data, cur_nr_sectors *512 , + cipher_data, cur_nr_sectors *BDRV_SECTOR_SIZE ,NULL ) <0 ) { ret = -EIO ; goto cleanup; }qemu_iovec_from_buf (qiov, bytes_done, - cipher_data, cur_nr_sectors *512 ); + cipher_data, cur_nr_sectors *BDRV_SECTOR_SIZE ); remaining_sectors -= cur_nr_sectors; sector_num += cur_nr_sectors; - bytes_done += cur_nr_sectors *512 ; + bytes_done += cur_nr_sectors *BDRV_SECTOR_SIZE ; } cleanup:
@@ -459,16 +459,16 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,QEMUIOVector hd_qiov;int ret =0 ;size_t payload_offset = -qcrypto_block_get_payload_offset (crypto->block) /512 ; +qcrypto_block_get_payload_offset (crypto->block) /BDRV_SECTOR_SIZE ;qemu_iovec_init (&hd_qiov, qiov->niov);/* Bounce buffer because we're not permitted to touch * contents of qiov - it points to guest memory. */ - cipher_data = -qemu_try_blockalign (bs->file->bs,MIN (BLOCK_CRYPTO_MAX_SECTORS *512 , - qiov->size)); + cipher_data =qemu_try_blockalign ( + bs->file->bs,MIN (BLOCK_CRYPTO_MAX_SECTORS *BDRV_SECTOR_SIZE , + qiov->size));if (cipher_data ==NULL ) { ret = -ENOMEM ; goto cleanup;
@@ -482,18 +482,18 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num, }qemu_iovec_to_buf (qiov, bytes_done, - cipher_data, cur_nr_sectors *512 ); + cipher_data, cur_nr_sectors *BDRV_SECTOR_SIZE );if (qcrypto_block_encrypt (crypto->block, sector_num, - cipher_data, cur_nr_sectors *512 , + cipher_data, cur_nr_sectors *BDRV_SECTOR_SIZE ,NULL ) <0 ) { ret = -EIO ; goto cleanup; }qemu_iovec_reset (&hd_qiov); -qemu_iovec_add (&hd_qiov, cipher_data, cur_nr_sectors *512 ); +qemu_iovec_add (&hd_qiov, cipher_data, cur_nr_sectors *BDRV_SECTOR_SIZE ); ret = bdrv_co_writev(bs->file, payload_offset + sector_num,
@@ -504,7 +504,7 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num, remaining_sectors -= cur_nr_sectors; sector_num += cur_nr_sectors; - bytes_done += cur_nr_sectors *512 ; + bytes_done += cur_nr_sectors *BDRV_SECTOR_SIZE ; } cleanup:
Signature · 1 line
2.13.5
Re: [Qemu-devel] [PATCH v2 2/4] block: use BDRV_SECTOR_SIZE in crypto driver
Quoted 5 lines · Daniel P. Berrange
Signed-off-by Daniel P. Berrange <[email protected]>--- block/crypto.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2 2/4] block: use BDRV_SECTOR_SIZE in crypto driver
In reply to Daniel P. BerrangeAm 31.08.2017 um 13:05 hat Daniel P. Berrange geschrieben:
Quoted 4 lines
Signed-off-by Daniel P. Berrange <[email protected]>--- block/crypto.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)
I'm actually not sure about this one. Anything that is left after patch 3 is probably not the arbitrary unit that qemu uses internally for some interfaces, but the unit in which data is encrypted.
Basically, if just for fun we ever changed the unit of things like bdrv_write() from 512 to 4096, then everything that needs to or at least can be changed to use 4096 is BDRV_SECTOR_SIZE. But everything that needs to stay on 512 (like I suspect most of the occurrences in the crypto driver) is a different constant really (QCRYPTO_SECTOR_SIZE?).
Kevin
Re: [Qemu-devel] [PATCH v2 2/4] block: use BDRV_SECTOR_SIZE in crypto driver
Quoted 10 lines · Kevin Wolf
Am 31.08.2017 um 13:05 hat Daniel P. Berrange geschrieben:
Quoted 4 lines
Signed-off-by Daniel P. Berrange <[email protected]>--- block/crypto.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)I'm actually not sure about this one. Anything that is left after patch 3 is probably not the arbitrary unit that qemu uses internally for some interfaces, but the unit in which data is encrypted.
Quoted 5 lines
Basically, if just for fun we ever changed the unit of things like
bdrv_write()from 512 to 4096, then everything that needs to or at least can be changed to use 4096 isBDRV_SECTOR_SIZE. But everything that needs to stay on 512 (like I suspect most of the occurrences in the crypto driver) is a different constant really (QCRYPTO_SECTOR_SIZE?).
Yeah for sure LUKSv1 & legacy qcow2 encryption need to stay 512 forever, so if BDRV_SECTOR_SIZE were to change that would be a problem.
I wrote this on the assumption that we would never change BDRV_SECTOR_SIZE though. If we did need different sector sizes in the block layer I figured it would surely end up being a dynamic property per disk, rather than just changing the compile time constant. So from that POV I thought it ok to use BDRV_SECTOR_SIZE.
Perhaps I should instead add a qcrypto_block_get_sector_size() API though and use that, so we can fetch the sector size per encryption scheme in case we ever get a scheme using a non-512 sector size for encryption.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH v2 2/4] block: use BDRV_SECTOR_SIZE in crypto driver
Am 05.09.2017 um 12:05 hat Daniel P. Berrange geschrieben:
Quoted 21 lines · Kevin Wolf
Quoted 10 lines
Am 31.08.2017 um 13:05 hat Daniel P. Berrange geschrieben:
Quoted 4 lines
Signed-off-by Daniel P. Berrange <[email protected]>--- block/crypto.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)I'm actually not sure about this one. Anything that is left after patch 3 is probably not the arbitrary unit that qemu uses internally for some interfaces, but the unit in which data is encrypted.
Quoted 5 lines
Basically, if just for fun we ever changed the unit of things like
bdrv_write()from 512 to 4096, then everything that needs to or at least can be changed to use 4096 isBDRV_SECTOR_SIZE. But everything that needs to stay on 512 (like I suspect most of the occurrences in the crypto driver) is a different constant really (QCRYPTO_SECTOR_SIZE?).Yeah for sure LUKSv1 & legacy qcow2 encryption need to stay 512 forever, so if
BDRV_SECTOR_SIZEwere to change that would be a problem.
Quoted 5 lines
I wrote this on the assumption that we would never change
BDRV_SECTOR_SIZEthough. If we did need different sector sizes in the block layer I figured it would surely end up being a dynamic property per disk, rather than just changing the compile time constant. So from that POV I thought it ok to useBDRV_SECTOR_SIZE.
Yes, I agree that we'll never BDRV_SECTOR_SIZE in practice. I just use this as a way to check whether this is really BDRV_SECTOR_SIZE, or whether we have two different quantities that just happen to be both 512.
For example, QDICT_BUCKET_MAX is also 512, but that doesn't make it right to use here, even though it works and is unlikely to change.
What we may actually want to do at some point (won't be in the near future, though) is removing BDRV_SECTOR_SIZE because it refers to an arbitrary unit in APIs and we're in the process of making everything byte-based instead. This is another way to check whether some value isBDRV_SECTOR_SIZE: If it will remain even after removing APIs with sector-based parameters, it's probably something different.
Quoted 3 lines
Perhaps I should instead add a
qcrypto_block_get_sector_size()API though and use that, so we can fetch the sector size per encryption scheme in case we ever get a scheme using a non-512 sector size for encryption.
If you have a use for it, sure. Otherwise, I'd just suggest introducing another constant for now.
Kevin
PATCH v2 3/4 block: convert crypto driver to bdrv_co_preadv|pwritev
In reply to Daniel P. BerrangeMake the crypto driver implement the bdrv_co_preadv|pwritev callbacks, and also use bdrv_co_preadv|pwritev for I/O with the protocol driver beneath.
--- block/crypto.c | 103 +++++++++++++++++++++++++++++---------------------------- 1 file changed, 53 insertions(+), 50 deletions(-)
block/crypto.c +53 -50
@@ -380,19 +380,23 @@ static void block_crypto_close(BlockDriverState *bs)#define BLOCK_CRYPTO_MAX_SECTORS 2048 +#define BLOCK_CRYPTO_MAX_LENGTH (BLOCK_CRYPTO_MAX_SECTORS * BDRV_SECTOR_SIZE) static coroutine_fn int -block_crypto_co_readv (BlockDriverState *bs,int64_t sector_num, -int remaining_sectors,QEMUIOVector *qiov) +block_crypto_co_preadv (BlockDriverState *bs,uint64_t offset,uint64_t bytes, +QEMUIOVector *qiov,int flags) {BlockCrypto *crypto = bs->opaque; -int cur_nr_sectors;/* number of sectors in current iteration */ +uint64_t cur_bytes;/* number of bytes in current iteration */ uint64_t bytes_done =0 ;uint8_t *cipher_data =NULL ;QEMUIOVector hd_qiov;int ret =0 ; -size_t payload_offset = -qcrypto_block_get_payload_offset (crypto->block) /BDRV_SECTOR_SIZE ; +size_t payload_offset =qcrypto_block_get_payload_offset (crypto->block); +uint64_t sector_num = offset /BDRV_SECTOR_SIZE ; + +assert ((offset %BDRV_SECTOR_SIZE ) ==0 ); +assert ((bytes %BDRV_SECTOR_SIZE ) ==0 );qemu_iovec_init (&hd_qiov, qiov->niov);
@@ -400,44 +404,39 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,* in qiov which points to guest memory. */ cipher_data =qemu_try_blockalign ( - bs->file->bs,MIN (BLOCK_CRYPTO_MAX_SECTORS *BDRV_SECTOR_SIZE , - qiov->size)); + bs->file->bs,MIN (BLOCK_CRYPTO_MAX_LENGTH , qiov->size));if (cipher_data ==NULL ) { ret = -ENOMEM ; goto cleanup; } -while (remaining_sectors) { - cur_nr_sectors = remaining_sectors; +while (bytes) { + cur_bytes = bytes; -if (cur_nr_sectors >BLOCK_CRYPTO_MAX_SECTORS ) { - cur_nr_sectors =BLOCK_CRYPTO_MAX_SECTORS ; +if (cur_bytes >BLOCK_CRYPTO_MAX_LENGTH ) { + cur_bytes =BLOCK_CRYPTO_MAX_LENGTH ; }qemu_iovec_reset (&hd_qiov); -qemu_iovec_add (&hd_qiov, cipher_data, cur_nr_sectors *BDRV_SECTOR_SIZE ); +qemu_iovec_add (&hd_qiov, cipher_data, cur_bytes); - ret =bdrv_co_readv (bs->file, - payload_offset + sector_num, - cur_nr_sectors, &hd_qiov); + ret =bdrv_co_preadv (bs->file, payload_offset + offset + bytes_done, + cur_bytes, &hd_qiov,0 );if (ret <0 ) { goto cleanup; } -if (qcrypto_block_decrypt (crypto->block, - sector_num, - cipher_data, cur_nr_sectors *BDRV_SECTOR_SIZE , -NULL ) <0 ) { +if (qcrypto_block_decrypt (crypto->block, sector_num, cipher_data, + cur_bytes,NULL ) <0 ) { ret = -EIO ; goto cleanup; } -qemu_iovec_from_buf (qiov, bytes_done, - cipher_data, cur_nr_sectors *BDRV_SECTOR_SIZE ); +qemu_iovec_from_buf (qiov, bytes_done, cipher_data, cur_bytes); - remaining_sectors -= cur_nr_sectors; - sector_num += cur_nr_sectors; - bytes_done += cur_nr_sectors *BDRV_SECTOR_SIZE ; + sector_num += cur_bytes /BDRV_SECTOR_SIZE ; + bytes -= cur_bytes; + bytes_done += cur_bytes; } cleanup:
@@ -449,17 +448,20 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num, static coroutine_fn int -block_crypto_co_writev (BlockDriverState *bs,int64_t sector_num, -int remaining_sectors,QEMUIOVector *qiov) +block_crypto_co_pwritev (BlockDriverState *bs,uint64_t offset,uint64_t bytes, +QEMUIOVector *qiov,int flags) {BlockCrypto *crypto = bs->opaque; -int cur_nr_sectors;/* number of sectors in current iteration */ +uint64_t cur_bytes;/* number of bytes in current iteration */ uint64_t bytes_done =0 ;uint8_t *cipher_data =NULL ;QEMUIOVector hd_qiov;int ret =0 ; -size_t payload_offset = -qcrypto_block_get_payload_offset (crypto->block) /BDRV_SECTOR_SIZE ; +size_t payload_offset =qcrypto_block_get_payload_offset (crypto->block); +uint64_t sector_num = offset /BDRV_SECTOR_SIZE ; + +assert ((offset %BDRV_SECTOR_SIZE ) ==0 ); +assert ((bytes %BDRV_SECTOR_SIZE ) ==0 );qemu_iovec_init (&hd_qiov, qiov->niov);
@@ -467,44 +469,39 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,* contents of qiov - it points to guest memory. */ cipher_data =qemu_try_blockalign ( - bs->file->bs,MIN (BLOCK_CRYPTO_MAX_SECTORS *BDRV_SECTOR_SIZE , - qiov->size)); + bs->file->bs,MIN (BLOCK_CRYPTO_MAX_LENGTH , qiov->size));if (cipher_data ==NULL ) { ret = -ENOMEM ; goto cleanup; } -while (remaining_sectors) { - cur_nr_sectors = remaining_sectors; +while (bytes) { + cur_bytes = bytes; -if (cur_nr_sectors >BLOCK_CRYPTO_MAX_SECTORS ) { - cur_nr_sectors =BLOCK_CRYPTO_MAX_SECTORS ; +if (cur_bytes >BLOCK_CRYPTO_MAX_LENGTH ) { + cur_bytes =BLOCK_CRYPTO_MAX_LENGTH ; } -qemu_iovec_to_buf (qiov, bytes_done, - cipher_data, cur_nr_sectors *BDRV_SECTOR_SIZE ); +qemu_iovec_to_buf (qiov, bytes_done, cipher_data, cur_bytes); -if (qcrypto_block_encrypt (crypto->block, - sector_num, - cipher_data, cur_nr_sectors *BDRV_SECTOR_SIZE , -NULL ) <0 ) { +if (qcrypto_block_encrypt (crypto->block, sector_num, cipher_data, + cur_bytes,NULL ) <0 ) { ret = -EIO ; goto cleanup; }qemu_iovec_reset (&hd_qiov); -qemu_iovec_add (&hd_qiov, cipher_data, cur_nr_sectors *BDRV_SECTOR_SIZE ); +qemu_iovec_add (&hd_qiov, cipher_data, cur_bytes); - ret =bdrv_co_writev (bs->file, - payload_offset + sector_num, - cur_nr_sectors, &hd_qiov); + ret =bdrv_co_pwritev (bs->file, payload_offset + offset + bytes_done, + cur_bytes, &hd_qiov,0 );if (ret <0 ) { goto cleanup; } - remaining_sectors -= cur_nr_sectors; - sector_num += cur_nr_sectors; - bytes_done += cur_nr_sectors *BDRV_SECTOR_SIZE ; + sector_num += cur_bytes /BDRV_SECTOR_SIZE ; + bytes -= cur_bytes; + bytes_done += cur_bytes; } cleanup:
@@ -514,6 +511,11 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,return ret; } +static void block_crypto_refresh_limits (BlockDriverState *bs,Error **errp) +{ + bs->bl.request_alignment =BDRV_SECTOR_SIZE ;/* No sub-sector I/O */ +} +static int64_t block_crypto_getlength (BlockDriverState *bs) {
@@ -611,8 +613,9 @@ BlockDriver bdrv_crypto_luks = { .bdrv_truncate = block_crypto_truncate, .create_opts = &block_crypto_create_opts_luks, - .bdrv_co_readv = block_crypto_co_readv, - .bdrv_co_writev = block_crypto_co_writev, + .bdrv_refresh_limits = block_crypto_refresh_limits, + .bdrv_co_preadv = block_crypto_co_preadv, + .bdrv_co_pwritev = block_crypto_co_pwritev, .bdrv_getlength = block_crypto_getlength, .bdrv_get_info = block_crypto_get_info_luks, .bdrv_get_specific_info = block_crypto_get_specific_info_luks,
Signature · 1 line
2.13.5
Re: [Qemu-devel] [PATCH v2 3/4] block: convert crypto driver to bdrv_co_preadv|pwritev
Quoted 17 lines · Daniel P. Berrange
Make the crypto driver implement the bdrv_co_preadv|pwritev callbacks, and also use bdrv_co_preadv|pwritev for I/O with the protocol driver beneath.
Signed-off-by Daniel P. Berrange <[email protected]>--- block/crypto.c | 103 +++++++++++++++++++++++++++++---------------------------- 1 file changed, 53 insertions(+), 50 deletions(-)static
coroutine_fnint(quoted diff) +2
-2-block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num, - int remaining_sectors, QEMUIOVector *qiov) +block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes, + QEMUIOVector *qiov, int flags) {
Review
Quoted 10 lines
(quoted diff) +2
-3BlockCrypto *crypto = bs->opaque; - int cur_nr_sectors; /* number of sectors in current iteration */ + uint64_t cur_bytes; /* number of bytes in current iteration */ uint64_t bytes_done = 0; uint8_t *cipher_data = NULL; QEMUIOVector hd_qiov; int ret = 0; - size_t payload_offset = - qcrypto_block_get_payload_offset(crypto->block) / BDRV_SECTOR_SIZE; + size_t payload_offset = qcrypto_block_get_payload_offset(crypto->block);
Pre-existing: is size_t the right type, or can we overflow a 64-bit offset on a 32-bit host?
block/crypto.c static void block_crypto_close(BlockDriverState *bs) · line 399
Quoted 4 lines
block/crypto.c +4
-0+uint64_t sector_num = offset /BDRV_SECTOR_SIZE ; + +assert ((offset %BDRV_SECTOR_SIZE ) ==0 ); +assert ((bytes %BDRV_SECTOR_SIZE ) ==0 );
The osdep.h macros might be nicer than open-coding; furthermore, if desired, you could shorten to:
assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
block/crypto.c block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num, · line 453
Quoted 7 lines
static
coroutine_fnintblock/crypto.c +2
-2-block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num, - int remaining_sectors, QEMUIOVector *qiov) +block_crypto_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes, + QEMUIOVector *qiov, int flags) {
Hmm - you don't set supported_write_flags. But presumably, if the underlying BDS supports BDRV_REQUEST_FUA, then crypto can likewise support that flag by passing it through to the underlying device after encryption.
Quoted 4 lines
BlockCrypto *crypto = bs->opaque;block/crypto.c +1
-1-int cur_nr_sectors;/* number of sectors in current iteration */ +uint64_t cur_bytes;/* number of bytes in current iteration */ uint64_t bytes_done =0 ;
block/crypto.c static void block_crypto_close(BlockDriverState *bs) · line 399
Quoted 10 lines
block/crypto.c +5
-2uint8_t *cipher_data =NULL ;QEMUIOVector hd_qiov;int ret =0 ; -size_t payload_offset = -qcrypto_block_get_payload_offset (crypto->block) /BDRV_SECTOR_SIZE ; +size_t payload_offset =qcrypto_block_get_payload_offset (crypto->block); +uint64_t sector_num = offset /BDRV_SECTOR_SIZE ; + +assert ((offset %BDRV_SECTOR_SIZE ) ==0 ); +assert ((bytes %BDRV_SECTOR_SIZE ) ==0 );
Same comment as for read.
block/crypto.c BlockDriver bdrv_crypto_luks = { · line 621
Quoted 12 lines
block/crypto.c +3
-2@@ -611,8 +613,9 @@ BlockDriver bdrv_crypto_luks = { .bdrv_truncate = block_crypto_truncate, .create_opts = &block_crypto_create_opts_luks, - .bdrv_co_readv = block_crypto_co_readv, - .bdrv_co_writev = block_crypto_co_writev, + .bdrv_refresh_limits = block_crypto_refresh_limits, + .bdrv_co_preadv = block_crypto_co_preadv, + .bdrv_co_pwritev = block_crypto_co_pwritev, .bdrv_getlength = block_crypto_getlength, .bdrv_get_info = block_crypto_get_info_luks, .bdrv_get_specific_info = block_crypto_get_specific_info_luks,
Looks weird when = isn't consistently aligned, but we use more than one space. My preference is to just use one space everywhere, as adding a longer name to the list doesn't require a mass re-format of all other entries; but I'm not opposed when people like the aligned = for legibility. So up to you if you do anything in response to my nit.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2 3/4] block: convert crypto driver to bdrv_co_preadv|pwritev
Quoted 29 lines · Eric Blake
Quoted 17 lines · Daniel P. Berrange
Make the crypto driver implement the bdrv_co_preadv|pwritev callbacks, and also use bdrv_co_preadv|pwritev for I/O with the protocol driver beneath.
Signed-off-by Daniel P. Berrange <[email protected]>--- block/crypto.c | 103 +++++++++++++++++++++++++++++---------------------------- 1 file changed, 53 insertions(+), 50 deletions(-)static
coroutine_fnint(quoted diff) +2
-2-block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num, - int remaining_sectors, QEMUIOVector *qiov) +block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes, + QEMUIOVector *qiov, int flags) {Review
Quoted 10 lines
(quoted diff) +2
-3BlockCrypto *crypto = bs->opaque; - int cur_nr_sectors; /* number of sectors in current iteration */ + uint64_t cur_bytes; /* number of bytes in current iteration */ uint64_t bytes_done = 0; uint8_t *cipher_data = NULL; QEMUIOVector hd_qiov; int ret = 0; - size_t payload_offset = - qcrypto_block_get_payload_offset(crypto->block) / BDRV_SECTOR_SIZE; + size_t payload_offset = qcrypto_block_get_payload_offset(crypto->block);
Quoted 2 lines
Pre-existing: is
size_tthe right type, or can we overflow a 64-bit offset on a 32-bit host?
No, it is bad. I'm fixing that as a separate patch, since it is a good to cleanup.
Quoted 10 lines
block/crypto.c static void block_crypto_close(BlockDriverState *bs) · line 399
Quoted 5 lines
block/crypto.c +4
-0+uint64_t sector_num = offset /BDRV_SECTOR_SIZE ; + +assert ((offset %BDRV_SECTOR_SIZE ) ==0 ); +assert ((bytes %BDRV_SECTOR_SIZE ) ==0 );The
osdep.hmacros might be nicer than open-coding; furthermore, if desired, you could shorten to:
assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
Yep, makes sense.
Quoted 11 lines
block/crypto.c block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num, · line 453
Quoted 6 lines
static
coroutine_fnintblock/crypto.c +2
-2-block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num, - int remaining_sectors, QEMUIOVector *qiov) +block_crypto_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes, + QEMUIOVector *qiov, int flags) {Hmm - you don't set
supported_write_flags. But presumably, if the underlying BDS supportsBDRV_REQUEST_FUA, then crypto can likewise support that flag by passing it through to the underlying device after encryption.
Something to be added as a separate patch
Quoted 4 lines
Quoted 4 lines
block/crypto.c +0
-0@@ -611,8 +613,9 @@ BlockDriver bdrv_crypto_luks = { .bdrv_truncate = block_crypto_truncate, .create_opts = &block_crypto_create_opts_luks,
Quoted 14 lines
block/crypto.c BlockDriver bdrv_crypto_luks = { · line 621
Quoted 8 lines
block/crypto.c +3
-2- .bdrv_co_readv = block_crypto_co_readv, - .bdrv_co_writev = block_crypto_co_writev, + .bdrv_refresh_limits = block_crypto_refresh_limits, + .bdrv_co_preadv = block_crypto_co_preadv, + .bdrv_co_pwritev = block_crypto_co_pwritev, .bdrv_getlength = block_crypto_getlength, .bdrv_get_info = block_crypto_get_info_luks, .bdrv_get_specific_info = block_crypto_get_specific_info_luks,Looks weird when = isn't consistently aligned, but we use more than one space. My preference is to just use one space everywhere, as adding a longer name to the list doesn't require a mass re-format of all other entries; but I'm not opposed when people like the aligned = for legibility. So up to you if you do anything in response to my nit.
Normally I stick with one space, so don't know what I was thinking when i wrote this.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
PATCH v2 4/4 block: convert qcrypto_block_encrypt|decrypt to take bytes offset
In reply to Daniel P. BerrangeInstead of sector offset, take the bytes offset when encrypting or decrypting data.
--- block/crypto.c | 8 ++++---- block/qcow.c | 7 +++++-- block/qcow2-cluster.c | 8 +++----- block/qcow2.c | 4 ++-- crypto/block-luks.c | 12 ++++++++---- crypto/block-qcow.c | 12 ++++++++---- crypto/block.c | 14 ++++++++------ crypto/blockpriv.h | 4 ++-- include/crypto/block.h | 14 ++++++++------ 9 files changed, 48 insertions(+), 35 deletions(-)
block/crypto.c +4 -4
@@ -426,8 +426,8 @@ block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes, goto cleanup; } -if (qcrypto_block_decrypt (crypto->block, sector_num, cipher_data, - cur_bytes,NULL ) <0 ) { +if (qcrypto_block_decrypt (crypto->block, offset + bytes_done, + cipher_data, cur_bytes,NULL ) <0 ) { ret = -EIO ; goto cleanup; }
@@ -484,8 +484,8 @@ block_crypto_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,qemu_iovec_to_buf (qiov, bytes_done, cipher_data, cur_bytes); -if (qcrypto_block_encrypt (crypto->block, sector_num, cipher_data, - cur_bytes,NULL ) <0 ) { +if (qcrypto_block_encrypt (crypto->block, offset + bytes_done, + cipher_data, cur_bytes,NULL ) <0 ) { ret = -EIO ; goto cleanup; }
block/qcow.c +5 -2
@@ -456,7 +456,9 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,if (i < n_start || i >= n_end) {Error *err =NULL ;memset (s->cluster_data,0x00 ,512 ); -if (qcrypto_block_encrypt (s->crypto, start_sect + i, +if (qcrypto_block_encrypt (s->crypto, + start_sect + i * +BDRV_SECTOR_SIZE , s->cluster_data,BDRV_SECTOR_SIZE , &err) <0 ) {
@@ -636,7 +638,8 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num, }if (bs->encrypted) {assert (s->crypto); -if (qcrypto_block_decrypt (s->crypto, sector_num, buf, +if (qcrypto_block_decrypt (s->crypto, + sector_num *BDRV_SECTOR_SIZE , buf, n *BDRV_SECTOR_SIZE , &err) <0 ) { goto fail; }
block/qcow2-cluster.c +3 -5
@@ -396,15 +396,13 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs, {if (bytes && bs->encrypted) {BDRVQcow2State *s = bs->opaque; -int64_t sector = (s->crypt_physical_offset ? +int64_t offset = (s->crypt_physical_offset ? (cluster_offset + offset_in_cluster) : - (src_cluster_offset + offset_in_cluster)) - >>BDRV_SECTOR_BITS ; + (src_cluster_offset + offset_in_cluster));assert ((offset_in_cluster & ~BDRV_SECTOR_MASK ) ==0 );assert ((bytes & ~BDRV_SECTOR_MASK ) ==0 );assert (s->crypto); -if (qcrypto_block_encrypt (s->crypto, sector, buffer, - bytes,NULL ) <0 ) { +if (qcrypto_block_encrypt (s->crypto, offset, buffer, bytes,NULL ) <0 ) {return false; } }
block/qcow2.c +2 -2
@@ -1824,7 +1824,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,if (qcrypto_block_decrypt (s->crypto, (s->crypt_physical_offset ? cluster_offset + offset_in_cluster : - offset) >>BDRV_SECTOR_BITS , + offset), cluster_data, cur_bytes, &err) <0 ) {
@@ -1961,7 +1961,7 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,if (qcrypto_block_encrypt (s->crypto, (s->crypt_physical_offset ? cluster_offset + offset_in_cluster : - offset) >>BDRV_SECTOR_BITS , + offset), cluster_data, cur_bytes, &err) <0 ) {error_free (err);
crypto/block-luks.c +8 -4
@@ -1403,29 +1403,33 @@ static void qcrypto_block_luks_cleanup(QCryptoBlock *block)static int qcrypto_block_luks_decrypt (QCryptoBlock *block, -uint64_t startsector, +uint64_t offset,uint8_t *buf,size_t len,Error **errp) { +assert (!(offset %QCRYPTO_BLOCK_LUKS_SECTOR_SIZE )); +assert (!(len %QCRYPTO_BLOCK_LUKS_SECTOR_SIZE ));return qcrypto_block_decrypt_helper (block->cipher, block->niv, block->ivgen,QCRYPTO_BLOCK_LUKS_SECTOR_SIZE , - startsector, buf, len, errp); + offset, buf, len, errp); }static int qcrypto_block_luks_encrypt (QCryptoBlock *block, -uint64_t startsector, +uint64_t offset,uint8_t *buf,size_t len,Error **errp) { +assert (!(offset %QCRYPTO_BLOCK_LUKS_SECTOR_SIZE )); +assert (!(len %QCRYPTO_BLOCK_LUKS_SECTOR_SIZE ));return qcrypto_block_encrypt_helper (block->cipher, block->niv, block->ivgen,QCRYPTO_BLOCK_LUKS_SECTOR_SIZE , - startsector, buf, len, errp); + offset, buf, len, errp); }
crypto/block-qcow.c +8 -4
@@ -142,29 +142,33 @@ qcrypto_block_qcow_cleanup(QCryptoBlock *block)static int qcrypto_block_qcow_decrypt (QCryptoBlock *block, -uint64_t startsector, +uint64_t offset,uint8_t *buf,size_t len,Error **errp) { +assert (!(offset %QCRYPTO_BLOCK_QCOW_SECTOR_SIZE )); +assert (!(len %QCRYPTO_BLOCK_QCOW_SECTOR_SIZE ));return qcrypto_block_decrypt_helper (block->cipher, block->niv, block->ivgen,QCRYPTO_BLOCK_QCOW_SECTOR_SIZE , - startsector, buf, len, errp); + offset, buf, len, errp); }static int qcrypto_block_qcow_encrypt (QCryptoBlock *block, -uint64_t startsector, +uint64_t offset,uint8_t *buf,size_t len,Error **errp) { +assert (!(offset %QCRYPTO_BLOCK_QCOW_SECTOR_SIZE )); +assert (!(len %QCRYPTO_BLOCK_QCOW_SECTOR_SIZE ));return qcrypto_block_encrypt_helper (block->cipher, block->niv, block->ivgen,QCRYPTO_BLOCK_QCOW_SECTOR_SIZE , - startsector, buf, len, errp); + offset, buf, len, errp); }
crypto/block.c +8 -6
@@ -127,22 +127,22 @@ QCryptoBlockInfo *qcrypto_block_get_info(QCryptoBlock *block,int qcrypto_block_decrypt (QCryptoBlock *block, -uint64_t startsector, +uint64_t offset,uint8_t *buf,size_t len,Error **errp) { -return block->driver->decrypt (block, startsector, buf, len, errp); +return block->driver->decrypt (block, offset, buf, len, errp); }int qcrypto_block_encrypt (QCryptoBlock *block, -uint64_t startsector, +uint64_t offset,uint8_t *buf,size_t len,Error **errp) { -return block->driver->encrypt (block, startsector, buf, len, errp); +return block->driver->encrypt (block, offset, buf, len, errp); }
@@ -188,13 +188,14 @@ int qcrypto_block_decrypt_helper(QCryptoCipher *cipher,size_t niv, QCryptoIVGen *ivgen, int sectorsize, - uint64_t startsector, + uint64_t offset, uint8_t *buf, size_t len, Error **errp) { uint8_t *iv;int ret =-1 ; +uint64_t startsector = offset / sectorsize; iv = niv ?g_new0 (uint8_t, niv) :NULL ;
@@ -237,13 +238,14 @@ int qcrypto_block_encrypt_helper(QCryptoCipher *cipher,size_t niv, QCryptoIVGen *ivgen, int sectorsize, - uint64_t startsector, + uint64_t offset, uint8_t *buf, size_t len, Error **errp) { uint8_t *iv;int ret =-1 ; +uint64_t startsector = offset / sectorsize; iv = niv ?g_new0 (uint8_t, niv) :NULL ;
crypto/blockpriv.h +2 -2
@@ -81,7 +81,7 @@ int qcrypto_block_decrypt_helper(QCryptoCipher *cipher,size_t niv, QCryptoIVGen *ivgen, int sectorsize, - uint64_t startsector, + uint64_t offset, uint8_t *buf, size_t len, Error **errp);
@@ -90,7 +90,7 @@ int qcrypto_block_encrypt_helper(QCryptoCipher *cipher,size_t niv, QCryptoIVGen *ivgen, int sectorsize, - uint64_t startsector, + uint64_t offset, uint8_t *buf, size_t len, Error **errp);
include/crypto/block.h +8 -6
@@ -161,18 +161,19 @@ QCryptoBlockInfo *qcrypto_block_get_info(QCryptoBlock *block,/** * @qcrypto_block_decrypt: * @block: the block encryption object -* @startsector: the sector from which @buf was read +* @offset: the position at which @iov was read * @buf: the buffer to decrypt * @len: the length of @buf in bytes * @errp: pointer to a NULL-initialized error object * * Decrypt @len bytes of cipher text in @buf, writing -* plain text back into @buf +* plain text back into @buf. @len and @offset must be +* a multiple of the encryption format sector size. * * Returns 0 on success, -1 on failure */ int qcrypto_block_decrypt (QCryptoBlock *block, -uint64_t startsector, +uint64_t offset,uint8_t *buf,size_t len,Error **errp);
@@ -180,18 +181,19 @@ int qcrypto_block_decrypt(QCryptoBlock *block,/** * @qcrypto_block_encrypt: * @block: the block encryption object -* @startsector: the sector to which @buf will be written +* @offset: the position at which @iov will be written * @buf: the buffer to decrypt * @len: the length of @buf in bytes * @errp: pointer to a NULL-initialized error object * * Encrypt @len bytes of plain text in @buf, writing -* cipher text back into @buf +* cipher text back into @buf. @len and @offset must be +* a multiple of the encryption format sector size. * * Returns 0 on success, -1 on failure */ int qcrypto_block_encrypt (QCryptoBlock *block, -uint64_t startsector, +uint64_t offset,uint8_t *buf,size_t len,Error **errp);
Signature · 1 line
2.13.5
Re: [Qemu-devel] [PATCH v2 4/4] block: convert qcrypto_block_encrypt|decrypt to take bytes offset
Quoted 21 lines · Daniel P. Berrange
Instead of sector offset, take the bytes offset when encrypting or decrypting data.
Signed-off-by Daniel P. Berrange <[email protected]>--- block/crypto.c | 8 ++++---- block/qcow.c | 7 +++++-- block/qcow2-cluster.c | 8 +++----- block/qcow2.c | 4 ++-- crypto/block-luks.c | 12 ++++++++---- crypto/block-qcow.c | 12 ++++++++---- crypto/block.c | 14 ++++++++------ crypto/blockpriv.h | 4 ++-- include/crypto/block.h | 14 ++++++++------ 9 files changed, 48 insertions(+), 35 deletions(-)block/crypto.c +0
-0
block/crypto.c block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes, · line 430
Quoted 7 lines
block/crypto.c +2
-2@@ -426,8 +426,8 @@ block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes, goto cleanup; } -if (qcrypto_block_decrypt (crypto->block, sector_num, cipher_data, - cur_bytes,NULL ) <0 ) { +if (qcrypto_block_decrypt (crypto->block, offset + bytes_done, + cipher_data, cur_bytes,NULL ) <0 ) {
How close are we to getting rid of even needing 'sector_num' as a variable?
block/crypto.c static uint64_t get_cluster_offset(BlockDriverState *bs, · line 461
Quoted 9 lines
block/crypto.c +4
-1+++ b/block/qcow.c@@ -456,7 +456,9 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,if (i < n_start || i >= n_end) {Error *err =NULL ;memset (s->cluster_data,0x00 ,512 ); - if (qcrypto_block_encrypt(s->crypto, start_sect + i, + if (qcrypto_block_encrypt(s->crypto, + start_sect + i * + BDRV_SECTOR_SIZE,
Umm, not the same. You want (start_sect + i) * BDRV_SECTOR_SIZE.
block/crypto.c static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs, · line 403
Quoted 13 lines
block/crypto.c +3
-3+++ b/block/qcow2-cluster.c@@ -396,15 +396,13 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs, {if (bytes && bs->encrypted) {BDRVQcow2State *s = bs->opaque; -int64_t sector = (s->crypt_physical_offset ? +int64_t offset = (s->crypt_physical_offset ? (cluster_offset + offset_in_cluster) : - (src_cluster_offset + offset_in_cluster)) - >>BDRV_SECTOR_BITS ; + (src_cluster_offset + offset_in_cluster));assert ((offset_in_cluster & ~BDRV_SECTOR_MASK ) ==0 );assert ((bytes & ~BDRV_SECTOR_MASK ) ==0 );
Pre-existing, but we could use osdep.h macros here, as in QEMU_IS_ALIGNED().
block/crypto.c static void qcrypto_block_luks_cleanup(QCryptoBlock *block) · line 1412
Quoted 13 lines
block/crypto.c +4
-1+++ b/crypto/block-luks.c@@ -1403,29 +1403,33 @@ static void qcrypto_block_luks_cleanup(QCryptoBlock *block)static int qcrypto_block_luks_decrypt (QCryptoBlock *block, -uint64_t startsector, +uint64_t offset,uint8_t *buf,size_t len,Error **errp) { +assert (!(offset %QCRYPTO_BLOCK_LUKS_SECTOR_SIZE )); +assert (!(len %QCRYPTO_BLOCK_LUKS_SECTOR_SIZE ));
Again, QEMU_IS_ALIGNED() might be easier to read - but this time, it's in code you're adding.
Looking forward to v3.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2 4/4] block: convert qcrypto_block_encrypt|decrypt to take bytes offset
Quoted 25 lines · Eric Blake
Quoted 24 lines · Daniel P. Berrange
Instead of sector offset, take the bytes offset when encrypting or decrypting data.
Signed-off-by Daniel P. Berrange <[email protected]>--- block/crypto.c | 8 ++++---- block/qcow.c | 7 +++++-- block/qcow2-cluster.c | 8 +++----- block/qcow2.c | 4 ++-- crypto/block-luks.c | 12 ++++++++---- crypto/block-qcow.c | 12 ++++++++---- crypto/block.c | 14 ++++++++------ crypto/blockpriv.h | 4 ++-- include/crypto/block.h | 14 ++++++++------ 9 files changed, 48 insertions(+), 35 deletions(-)block/crypto.c +0
-0@@ -426,8 +426,8 @@ block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes, goto cleanup; }
Quoted 6 lines
block/crypto.c block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes, · line 430
Quoted 4 lines
block/crypto.c +2
-2-if (qcrypto_block_decrypt (crypto->block, sector_num, cipher_data, - cur_bytes,NULL ) <0 ) { +if (qcrypto_block_decrypt (crypto->block, offset + bytes_done, + cipher_data, cur_bytes,NULL ) <0 ) {How close are we to getting rid of even needing '
sector_num' as a variable?
I thought there was more usage, but I just realized we can in fact remove it in this patch.
Quoted 12 lines
block/crypto.c static uint64_t get_cluster_offset(BlockDriverState *bs, · line 461
Quoted 10 lines
block/crypto.c +4
-1+++ b/block/qcow.c@@ -456,7 +456,9 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,if (i < n_start || i >= n_end) {Error *err =NULL ;memset (s->cluster_data,0x00 ,512 ); - if (qcrypto_block_encrypt(s->crypto, start_sect + i, + if (qcrypto_block_encrypt(s->crypto, + start_sect + i * + BDRV_SECTOR_SIZE,Umm, not the same. You want (
start_sect+ i) *BDRV_SECTOR_SIZE.
Heh, oppps.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|