[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 5/7] block: convert crypto driver to bdrv_co_
From: |
Daniel P. Berrange |
Subject: |
Re: [Qemu-devel] [PATCH v3 5/7] block: convert crypto driver to bdrv_co_preadv|pwritev |
Date: |
Wed, 27 Sep 2017 13:34:42 +0100 |
User-agent: |
Mutt/1.9.0 (2017-09-02) |
On Sat, Sep 16, 2017 at 06:54:58PM +0200, Max Reitz wrote:
> On 2017-09-12 13:28, Daniel P. Berrange wrote:
> > 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 <address@hidden>
> > ---
> > block/crypto.c | 104
> > +++++++++++++++++++++++++++++++--------------------------
> > 1 file changed, 56 insertions(+), 48 deletions(-)
>
> Reviewed-by: Max Reitz <address@hidden>
>
> Some notes below.
>
> > diff --git a/block/crypto.c b/block/crypto.c
> > index 49d6d4c058..d004e9cef4 100644
> > --- a/block/crypto.c
> > +++ b/block/crypto.c
>
> [...]
>
> > @@ -410,37 +414,33 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t
> > sector_num,
> > 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_SECTORS * sector_size)) {
> > + cur_bytes = BLOCK_CRYPTO_MAX_SECTORS * sector_size;
>
> It's a bit weird that now the bounce buffer's size is now no longer
> fixed at 1 MB but variable depending on the crypto driver's block size.
> It also doesn't seem too intentional when looking at the first patch's
> commit message...
>
> In any case, that would be an issue in the previous patch, though. In
> general, I'm wondering whether maybe you should squash this patch into
> the previous one... Yes, that would make the for a larger patch, but it
> wouldn't leave some not-quite-right state in between where sector_size
> is generally assumed to be equal to BDRV_SECTOR_SIZE -- which it is in
> practice, but not necessarily in theory.
In the end i'm going with this approach - just dropping the previous
patch entirely, since 99% of what it does is then removed in this
patch and the next one.
Regards,
Daniel
--
|: 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 v3 3/7] block: fix data type casting for crypto payload offset, (continued)
- [Qemu-devel] [PATCH v3 1/7] block: use 1 MB bounce buffers for crypto instead of 16KB, Daniel P. Berrange, 2017/09/12
- [Qemu-devel] [PATCH v3 4/7] block: don't use constant 512 as sector size in crypto driver, Daniel P. Berrange, 2017/09/12
- [Qemu-devel] [PATCH v3 5/7] block: convert crypto driver to bdrv_co_preadv|pwritev, Daniel P. Berrange, 2017/09/12
- [Qemu-devel] [PATCH v3 7/7] block: support passthrough of BDRV_REQ_FUA in crypto driver, Daniel P. Berrange, 2017/09/12
- [Qemu-devel] [PATCH v3 6/7] block: convert qcrypto_block_encrypt|decrypt to take bytes offset, Daniel P. Berrange, 2017/09/12
- Re: [Qemu-devel] [PATCH v3 0/7] Misc improvements to crypto block driver, no-reply, 2017/09/12