[{"id":1766934,"web_url":"http://patchwork.ozlabs.org/comment/1766934/","msgid":"<20170912120603.GE17633@redhat.com>","list_archive_url":null,"date":"2017-09-12T12:06:03","subject":"Re: [Qemu-devel] [PATCH v3 5/7] block: convert crypto driver to\n\tbdrv_co_preadv|pwritev","submitter":{"id":2694,"url":"http://patchwork.ozlabs.org/api/people/2694/","name":"Daniel P. Berrange","email":"berrange@redhat.com"},"content":"On Tue, Sep 12, 2017 at 12:28:53PM +0100, Daniel P. Berrange wrote:\n> Make the crypto driver implement the bdrv_co_preadv|pwritev\n> callbacks, and also use bdrv_co_preadv|pwritev for I/O\n> with the protocol driver beneath.\n> \n> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>\n> ---\n>  block/crypto.c | 104 +++++++++++++++++++++++++++++++--------------------------\n>  1 file changed, 56 insertions(+), 48 deletions(-)\n> \n> diff --git a/block/crypto.c b/block/crypto.c\n> index 49d6d4c058..d004e9cef4 100644\n> --- a/block/crypto.c\n> +++ b/block/crypto.c\n> @@ -383,19 +383,23 @@ static void block_crypto_close(BlockDriverState *bs)\n>  #define BLOCK_CRYPTO_MAX_SECTORS 2048\n>  \n>  static coroutine_fn int\n> -block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,\n> -                      int remaining_sectors, QEMUIOVector *qiov)\n> +block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,\n> +                       QEMUIOVector *qiov, int flags)\n>  {\n>      BlockCrypto *crypto = bs->opaque;\n> -    int cur_nr_sectors; /* number of sectors in current iteration */\n> +    uint64_t cur_bytes; /* number of bytes in current iteration */\n>      uint64_t bytes_done = 0;\n>      uint8_t *cipher_data = NULL;\n>      QEMUIOVector hd_qiov;\n>      int ret = 0;\n>      uint64_t sector_size = qcrypto_block_get_sector_size(crypto->block);\n> -    uint64_t payload_offset =\n> -        qcrypto_block_get_payload_offset(crypto->block) / sector_size;\n> -    assert(payload_offset < (INT64_MAX / 512));\n> +    size_t payload_offset = qcrypto_block_get_payload_offset(crypto->block);\n\nOpps, rebase merge error - that should be uint64_t - this is what the\npatchew failure complained about.\n\n> +    uint64_t sector_num = offset / sector_size;\n> +\n> +    assert(!flags);\n> +    assert(payload_offset < INT64_MAX);\n> +    assert(QEMU_IS_ALIGNED(offset, sector_size));\n> +    assert(QEMU_IS_ALIGNED(bytes, sector_size));\n>  \n>      qemu_iovec_init(&hd_qiov, qiov->niov);\n>  \n> @@ -410,37 +414,33 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,\n>          goto cleanup;\n>      }\n>  \n> -    while (remaining_sectors) {\n> -        cur_nr_sectors = remaining_sectors;\n> +    while (bytes) {\n> +        cur_bytes = bytes;\n>  \n> -        if (cur_nr_sectors > BLOCK_CRYPTO_MAX_SECTORS) {\n> -            cur_nr_sectors = BLOCK_CRYPTO_MAX_SECTORS;\n> +        if (cur_bytes > (BLOCK_CRYPTO_MAX_SECTORS * sector_size)) {\n> +            cur_bytes = BLOCK_CRYPTO_MAX_SECTORS * sector_size;\n>          }\n>  \n>          qemu_iovec_reset(&hd_qiov);\n> -        qemu_iovec_add(&hd_qiov, cipher_data, cur_nr_sectors * sector_size);\n> +        qemu_iovec_add(&hd_qiov, cipher_data, cur_bytes);\n>  \n> -        ret = bdrv_co_readv(bs->file,\n> -                            payload_offset + sector_num,\n> -                            cur_nr_sectors, &hd_qiov);\n> +        ret = bdrv_co_preadv(bs->file, payload_offset + offset + bytes_done,\n> +                             cur_bytes, &hd_qiov, 0);\n>          if (ret < 0) {\n>              goto cleanup;\n>          }\n>  \n> -        if (qcrypto_block_decrypt(crypto->block,\n> -                                  sector_num,\n> -                                  cipher_data, cur_nr_sectors * sector_size,\n> -                                  NULL) < 0) {\n> +        if (qcrypto_block_decrypt(crypto->block, sector_num, cipher_data,\n> +                                  cur_bytes, NULL) < 0) {\n>              ret = -EIO;\n>              goto cleanup;\n>          }\n>  \n> -        qemu_iovec_from_buf(qiov, bytes_done,\n> -                            cipher_data, cur_nr_sectors * sector_size);\n> +        qemu_iovec_from_buf(qiov, bytes_done, cipher_data, cur_bytes);\n>  \n> -        remaining_sectors -= cur_nr_sectors;\n> -        sector_num += cur_nr_sectors;\n> -        bytes_done += cur_nr_sectors * sector_size;\n> +        sector_num += cur_bytes / sector_size;\n> +        bytes -= cur_bytes;\n> +        bytes_done += cur_bytes;\n>      }\n>  \n>   cleanup:\n> @@ -452,19 +452,23 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,\n>  \n>  \n>  static coroutine_fn int\n> -block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,\n> -                       int remaining_sectors, QEMUIOVector *qiov)\n> +block_crypto_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,\n> +                        QEMUIOVector *qiov, int flags)\n>  {\n>      BlockCrypto *crypto = bs->opaque;\n> -    int cur_nr_sectors; /* number of sectors in current iteration */\n> +    uint64_t cur_bytes; /* number of bytes in current iteration */\n>      uint64_t bytes_done = 0;\n>      uint8_t *cipher_data = NULL;\n>      QEMUIOVector hd_qiov;\n>      int ret = 0;\n>      uint64_t sector_size = qcrypto_block_get_sector_size(crypto->block);\n> -    uint64_t payload_offset =\n> -        qcrypto_block_get_payload_offset(crypto->block) / sector_size;\n> -    assert(payload_offset < (INT64_MAX / 512));\n> +    uint64_t payload_offset = qcrypto_block_get_payload_offset(crypto->block);\n> +    uint64_t sector_num = offset / sector_size;\n> +\n> +    assert(!flags);\n> +    assert(payload_offset < INT64_MAX);\n> +    assert(QEMU_IS_ALIGNED(offset, sector_size));\n> +    assert(QEMU_IS_ALIGNED(bytes, sector_size));\n>  \n>      qemu_iovec_init(&hd_qiov, qiov->niov);\n>  \n> @@ -479,37 +483,33 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,\n>          goto cleanup;\n>      }\n>  \n> -    while (remaining_sectors) {\n> -        cur_nr_sectors = remaining_sectors;\n> +    while (bytes) {\n> +        cur_bytes = bytes;\n>  \n> -        if (cur_nr_sectors > BLOCK_CRYPTO_MAX_SECTORS) {\n> -            cur_nr_sectors = BLOCK_CRYPTO_MAX_SECTORS;\n> +        if (cur_bytes > (BLOCK_CRYPTO_MAX_SECTORS * sector_size)) {\n> +            cur_bytes = BLOCK_CRYPTO_MAX_SECTORS * sector_size;\n>          }\n>  \n> -        qemu_iovec_to_buf(qiov, bytes_done,\n> -                          cipher_data, cur_nr_sectors * sector_size);\n> +        qemu_iovec_to_buf(qiov, bytes_done, cipher_data, cur_bytes);\n>  \n> -        if (qcrypto_block_encrypt(crypto->block,\n> -                                  sector_num,\n> -                                  cipher_data, cur_nr_sectors * sector_size,\n> -                                  NULL) < 0) {\n> +        if (qcrypto_block_encrypt(crypto->block, sector_num, cipher_data,\n> +                                  cur_bytes, NULL) < 0) {\n>              ret = -EIO;\n>              goto cleanup;\n>          }\n>  \n>          qemu_iovec_reset(&hd_qiov);\n> -        qemu_iovec_add(&hd_qiov, cipher_data, cur_nr_sectors * sector_size);\n> +        qemu_iovec_add(&hd_qiov, cipher_data, cur_bytes);\n>  \n> -        ret = bdrv_co_writev(bs->file,\n> -                             payload_offset + sector_num,\n> -                             cur_nr_sectors, &hd_qiov);\n> +        ret = bdrv_co_pwritev(bs->file, payload_offset + offset + bytes_done,\n> +                              cur_bytes, &hd_qiov, 0);\n>          if (ret < 0) {\n>              goto cleanup;\n>          }\n>  \n> -        remaining_sectors -= cur_nr_sectors;\n> -        sector_num += cur_nr_sectors;\n> -        bytes_done += cur_nr_sectors * sector_size;\n> +        sector_num += cur_bytes / sector_size;\n> +        bytes -= cur_bytes;\n> +        bytes_done += cur_bytes;\n>      }\n>  \n>   cleanup:\n> @@ -519,6 +519,13 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,\n>      return ret;\n>  }\n>  \n> +static void block_crypto_refresh_limits(BlockDriverState *bs, Error **errp)\n> +{\n> +    BlockCrypto *crypto = bs->opaque;\n> +    uint64_t sector_size = qcrypto_block_get_sector_size(crypto->block);\n> +    bs->bl.request_alignment = sector_size; /* No sub-sector I/O */\n> +}\n> +\n>  \n>  static int64_t block_crypto_getlength(BlockDriverState *bs)\n>  {\n> @@ -618,8 +625,9 @@ BlockDriver bdrv_crypto_luks = {\n>      .bdrv_truncate      = block_crypto_truncate,\n>      .create_opts        = &block_crypto_create_opts_luks,\n>  \n> -    .bdrv_co_readv      = block_crypto_co_readv,\n> -    .bdrv_co_writev     = block_crypto_co_writev,\n> +    .bdrv_refresh_limits = block_crypto_refresh_limits,\n> +    .bdrv_co_preadv     = block_crypto_co_preadv,\n> +    .bdrv_co_pwritev    = block_crypto_co_pwritev,\n>      .bdrv_getlength     = block_crypto_getlength,\n>      .bdrv_get_info      = block_crypto_get_info_luks,\n>      .bdrv_get_specific_info = block_crypto_get_specific_info_luks,\n> -- \n> 2.13.5\n> \n\nRegards,\nDaniel","headers":{"Return-Path":"<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=<UNKNOWN>)","ext-mx01.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx01.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=berrange@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xs3T70VCkz9s7M\n\tfor <incoming@patchwork.ozlabs.org>;\n\tTue, 12 Sep 2017 22:06:48 +1000 (AEST)","from localhost ([::1]:35448 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>)\n\tid 1drjxm-0005k6-GH\n\tfor incoming@patchwork.ozlabs.org; Tue, 12 Sep 2017 08:06:46 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:37990)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from <berrange@redhat.com>) id 1drjxQ-0005iU-NY\n\tfor qemu-devel@nongnu.org; Tue, 12 Sep 2017 08:06:26 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from <berrange@redhat.com>) id 1drjxP-0001FH-0D\n\tfor qemu-devel@nongnu.org; Tue, 12 Sep 2017 08:06:24 -0400","from mx1.redhat.com ([209.132.183.28]:34864)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from <berrange@redhat.com>)\n\tid 1drjx9-0000zH-P1; Tue, 12 Sep 2017 08:06:07 -0400","from smtp.corp.redhat.com\n\t(int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id CEC4181DF9;\n\tTue, 12 Sep 2017 12:06:06 +0000 (UTC)","from redhat.com (unknown [10.42.22.189])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 4199F6BF78;\n\tTue, 12 Sep 2017 12:06:05 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com CEC4181DF9","Date":"Tue, 12 Sep 2017 13:06:03 +0100","From":"\"Daniel P. Berrange\" <berrange@redhat.com>","To":"qemu-devel@nongnu.org","Message-ID":"<20170912120603.GE17633@redhat.com>","References":"<20170912112855.24269-1-berrange@redhat.com>\n\t<20170912112855.24269-6-berrange@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20170912112855.24269-6-berrange@redhat.com>","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.13","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.25]);\n\tTue, 12 Sep 2017 12:06:07 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH v3 5/7] block: convert crypto driver to\n\tbdrv_co_preadv|pwritev","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"<qemu-devel.nongnu.org>","List-Unsubscribe":"<https://lists.nongnu.org/mailman/options/qemu-devel>,\n\t<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>","List-Archive":"<http://lists.nongnu.org/archive/html/qemu-devel/>","List-Post":"<mailto:qemu-devel@nongnu.org>","List-Help":"<mailto:qemu-devel-request@nongnu.org?subject=help>","List-Subscribe":"<https://lists.nongnu.org/mailman/listinfo/qemu-devel>,\n\t<mailto:qemu-devel-request@nongnu.org?subject=subscribe>","Reply-To":"\"Daniel P. Berrange\" <berrange@redhat.com>","Cc":"Kevin Wolf <kwolf@redhat.com>, Stefan Hajnoczi <stefanha@gmail.com>,\n\tqemu-block@nongnu.org, Max Reitz <mreitz@redhat.com>","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>"}},{"id":1769624,"web_url":"http://patchwork.ozlabs.org/comment/1769624/","msgid":"<bca76897-ec7d-035e-9473-b304b63f5d7b@redhat.com>","list_archive_url":null,"date":"2017-09-16T16:54:58","subject":"Re: [Qemu-devel] [PATCH v3 5/7] block: convert crypto driver to\n\tbdrv_co_preadv|pwritev","submitter":{"id":36836,"url":"http://patchwork.ozlabs.org/api/people/36836/","name":"Max Reitz","email":"mreitz@redhat.com"},"content":"On 2017-09-12 13:28, Daniel P. Berrange wrote:\n> Make the crypto driver implement the bdrv_co_preadv|pwritev\n> callbacks, and also use bdrv_co_preadv|pwritev for I/O\n> with the protocol driver beneath.\n> \n> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>\n> ---\n>  block/crypto.c | 104 +++++++++++++++++++++++++++++++--------------------------\n>  1 file changed, 56 insertions(+), 48 deletions(-)\n\nReviewed-by: Max Reitz <mreitz@redhat.com>\n\nSome notes below.\n\n> diff --git a/block/crypto.c b/block/crypto.c\n> index 49d6d4c058..d004e9cef4 100644\n> --- a/block/crypto.c\n> +++ b/block/crypto.c\n\n[...]\n\n> @@ -410,37 +414,33 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,\n>          goto cleanup;\n>      }\n>  \n> -    while (remaining_sectors) {\n> -        cur_nr_sectors = remaining_sectors;\n> +    while (bytes) {\n> +        cur_bytes = bytes;\n>  \n> -        if (cur_nr_sectors > BLOCK_CRYPTO_MAX_SECTORS) {\n> -            cur_nr_sectors = BLOCK_CRYPTO_MAX_SECTORS;\n> +        if (cur_bytes > (BLOCK_CRYPTO_MAX_SECTORS * sector_size)) {\n> +            cur_bytes = BLOCK_CRYPTO_MAX_SECTORS * sector_size;\n\nIt's a bit weird that now the bounce buffer's size is now no longer\nfixed at 1 MB but variable depending on the crypto driver's block size.\nIt also doesn't seem too intentional when looking at the first patch's\ncommit message...\n\nIn any case, that would be an issue in the previous patch, though.  In\ngeneral, I'm wondering whether maybe you should squash this patch into\nthe previous one...  Yes, that would make the for a larger patch, but it\nwouldn't leave some not-quite-right state in between where sector_size\nis generally assumed to be equal to BDRV_SECTOR_SIZE -- which it is in\npractice, but not necessarily in theory.\n\n>          }\n\nAlso, just a note: It would be shorter with a MIN(). :-)\n\n(cur_bytes = MIN(bytes, BLOCK_CRYPTO_MAX_SECTORS * sector_size))\n\nMax","headers":{"Return-Path":"<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=<UNKNOWN>)","ext-mx07.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx07.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=mreitz@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xvdgV3CT9z9s78\n\tfor <incoming@patchwork.ozlabs.org>;\n\tSun, 17 Sep 2017 02:55:34 +1000 (AEST)","from localhost ([::1]:58023 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>)\n\tid 1dtGNQ-0006jt-Kq\n\tfor incoming@patchwork.ozlabs.org; Sat, 16 Sep 2017 12:55:32 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:56395)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from <mreitz@redhat.com>) id 1dtGN4-0006fG-DE\n\tfor qemu-devel@nongnu.org; Sat, 16 Sep 2017 12:55:11 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from <mreitz@redhat.com>) id 1dtGN3-0003op-CB\n\tfor qemu-devel@nongnu.org; Sat, 16 Sep 2017 12:55:10 -0400","from mx1.redhat.com ([209.132.183.28]:59034)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from <mreitz@redhat.com>)\n\tid 1dtGMx-0003jw-11; Sat, 16 Sep 2017 12:55:03 -0400","from smtp.corp.redhat.com\n\t(int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id BF871C047B66;\n\tSat, 16 Sep 2017 16:55:01 +0000 (UTC)","from dresden.str.redhat.com (ovpn-204-52.brq.redhat.com\n\t[10.40.204.52])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 3B3CA600C0;\n\tSat, 16 Sep 2017 16:55:00 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com BF871C047B66","To":"\"Daniel P. Berrange\" <berrange@redhat.com>, qemu-devel@nongnu.org","References":"<20170912112855.24269-1-berrange@redhat.com>\n\t<20170912112855.24269-6-berrange@redhat.com>","From":"Max Reitz <mreitz@redhat.com>","Message-ID":"<bca76897-ec7d-035e-9473-b304b63f5d7b@redhat.com>","Date":"Sat, 16 Sep 2017 18:54:58 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170912112855.24269-6-berrange@redhat.com>","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"fOv7uXqRUivpuBJHD4knEv44teVmeNjKt\"","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.11","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.31]);\n\tSat, 16 Sep 2017 16:55:01 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","X-Content-Filtered-By":"Mailman/MimeDel 2.1.21","Subject":"Re: [Qemu-devel] [PATCH v3 5/7] block: convert crypto driver to\n\tbdrv_co_preadv|pwritev","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"<qemu-devel.nongnu.org>","List-Unsubscribe":"<https://lists.nongnu.org/mailman/options/qemu-devel>,\n\t<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>","List-Archive":"<http://lists.nongnu.org/archive/html/qemu-devel/>","List-Post":"<mailto:qemu-devel@nongnu.org>","List-Help":"<mailto:qemu-devel-request@nongnu.org?subject=help>","List-Subscribe":"<https://lists.nongnu.org/mailman/listinfo/qemu-devel>,\n\t<mailto:qemu-devel-request@nongnu.org?subject=subscribe>","Cc":"Kevin Wolf <kwolf@redhat.com>, Stefan Hajnoczi <stefanha@gmail.com>,\n\tqemu-block@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>"}},{"id":1776309,"web_url":"http://patchwork.ozlabs.org/comment/1776309/","msgid":"<20170927123442.GF12223@redhat.com>","list_archive_url":null,"date":"2017-09-27T12:34:42","subject":"Re: [Qemu-devel] [PATCH v3 5/7] block: convert crypto driver to\n\tbdrv_co_preadv|pwritev","submitter":{"id":2694,"url":"http://patchwork.ozlabs.org/api/people/2694/","name":"Daniel P. Berrange","email":"berrange@redhat.com"},"content":"On Sat, Sep 16, 2017 at 06:54:58PM +0200, Max Reitz wrote:\n> On 2017-09-12 13:28, Daniel P. Berrange wrote:\n> > Make the crypto driver implement the bdrv_co_preadv|pwritev\n> > callbacks, and also use bdrv_co_preadv|pwritev for I/O\n> > with the protocol driver beneath.\n> > \n> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>\n> > ---\n> >  block/crypto.c | 104 +++++++++++++++++++++++++++++++--------------------------\n> >  1 file changed, 56 insertions(+), 48 deletions(-)\n> \n> Reviewed-by: Max Reitz <mreitz@redhat.com>\n> \n> Some notes below.\n> \n> > diff --git a/block/crypto.c b/block/crypto.c\n> > index 49d6d4c058..d004e9cef4 100644\n> > --- a/block/crypto.c\n> > +++ b/block/crypto.c\n> \n> [...]\n> \n> > @@ -410,37 +414,33 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,\n> >          goto cleanup;\n> >      }\n> >  \n> > -    while (remaining_sectors) {\n> > -        cur_nr_sectors = remaining_sectors;\n> > +    while (bytes) {\n> > +        cur_bytes = bytes;\n> >  \n> > -        if (cur_nr_sectors > BLOCK_CRYPTO_MAX_SECTORS) {\n> > -            cur_nr_sectors = BLOCK_CRYPTO_MAX_SECTORS;\n> > +        if (cur_bytes > (BLOCK_CRYPTO_MAX_SECTORS * sector_size)) {\n> > +            cur_bytes = BLOCK_CRYPTO_MAX_SECTORS * sector_size;\n> \n> It's a bit weird that now the bounce buffer's size is now no longer\n> fixed at 1 MB but variable depending on the crypto driver's block size.\n> It also doesn't seem too intentional when looking at the first patch's\n> commit message...\n> \n> In any case, that would be an issue in the previous patch, though.  In\n> general, I'm wondering whether maybe you should squash this patch into\n> the previous one...  Yes, that would make the for a larger patch, but it\n> wouldn't leave some not-quite-right state in between where sector_size\n> is generally assumed to be equal to BDRV_SECTOR_SIZE -- which it is in\n> practice, but not necessarily in theory.\n\nIn the end i'm going with this approach - just dropping the previous\npatch entirely, since 99% of what it does is then removed in this\npatch and the next one.\n\n\nRegards,\nDaniel","headers":{"Return-Path":"<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=<UNKNOWN>)","ext-mx01.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx01.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=berrange@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y2HNC1SC5z9tXT\n\tfor <incoming@patchwork.ozlabs.org>;\n\tWed, 27 Sep 2017 22:35:23 +1000 (AEST)","from localhost ([::1]:54642 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>)\n\tid 1dxBYf-0006r1-AQ\n\tfor incoming@patchwork.ozlabs.org; Wed, 27 Sep 2017 08:35:21 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:43919)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from <berrange@redhat.com>) id 1dxBYK-0006qW-5i\n\tfor qemu-devel@nongnu.org; Wed, 27 Sep 2017 08:35:04 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from <berrange@redhat.com>) id 1dxBYE-0000SB-6y\n\tfor qemu-devel@nongnu.org; Wed, 27 Sep 2017 08:35:00 -0400","from mx1.redhat.com ([209.132.183.28]:58628)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from <berrange@redhat.com>)\n\tid 1dxBY8-0000Qw-Pl; Wed, 27 Sep 2017 08:34:48 -0400","from smtp.corp.redhat.com\n\t(int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id A598881E0E;\n\tWed, 27 Sep 2017 12:34:47 +0000 (UTC)","from redhat.com (unknown [10.42.22.189])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id C21247F5C9;\n\tWed, 27 Sep 2017 12:34:45 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com A598881E0E","Date":"Wed, 27 Sep 2017 13:34:42 +0100","From":"\"Daniel P. Berrange\" <berrange@redhat.com>","To":"Max Reitz <mreitz@redhat.com>","Message-ID":"<20170927123442.GF12223@redhat.com>","References":"<20170912112855.24269-1-berrange@redhat.com>\n\t<20170912112855.24269-6-berrange@redhat.com>\n\t<bca76897-ec7d-035e-9473-b304b63f5d7b@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<bca76897-ec7d-035e-9473-b304b63f5d7b@redhat.com>","User-Agent":"Mutt/1.9.0 (2017-09-02)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.12","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.25]);\n\tWed, 27 Sep 2017 12:34:47 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH v3 5/7] block: convert crypto driver to\n\tbdrv_co_preadv|pwritev","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"<qemu-devel.nongnu.org>","List-Unsubscribe":"<https://lists.nongnu.org/mailman/options/qemu-devel>,\n\t<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>","List-Archive":"<http://lists.nongnu.org/archive/html/qemu-devel/>","List-Post":"<mailto:qemu-devel@nongnu.org>","List-Help":"<mailto:qemu-devel-request@nongnu.org?subject=help>","List-Subscribe":"<https://lists.nongnu.org/mailman/listinfo/qemu-devel>,\n\t<mailto:qemu-devel-request@nongnu.org?subject=subscribe>","Reply-To":"\"Daniel P. Berrange\" <berrange@redhat.com>","Cc":"Kevin Wolf <kwolf@redhat.com>, Stefan Hajnoczi <stefanha@gmail.com>,\n\tqemu-devel@nongnu.org, qemu-block@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>"}}]