From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49091) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c7VhU-0000qQ-Bq for qemu-devel@nongnu.org; Thu, 17 Nov 2016 18:02:37 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c7VhT-0005bl-1K for qemu-devel@nongnu.org; Thu, 17 Nov 2016 18:02:36 -0500 References: <1479413642-22463-1-git-send-email-eblake@redhat.com> <1479413642-22463-9-git-send-email-eblake@redhat.com> From: Max Reitz Message-ID: <3ae2a9aa-df53-fab2-aa98-a87787d5853e@redhat.com> Date: Fri, 18 Nov 2016 00:02:26 +0100 MIME-Version: 1.0 In-Reply-To: <1479413642-22463-9-git-send-email-eblake@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="53a7svosuG6anwgrj94oCLB2wtXmjMx7p" Subject: Re: [Qemu-devel] [PATCH v2 8/9] blkdebug: Add ability to override unmap geometries List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: qemu-block@nongnu.org, kwolf@redhat.com, pbonzini@redhat.com, Markus Armbruster This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --53a7svosuG6anwgrj94oCLB2wtXmjMx7p From: Max Reitz To: Eric Blake , qemu-devel@nongnu.org Cc: qemu-block@nongnu.org, kwolf@redhat.com, pbonzini@redhat.com, Markus Armbruster Message-ID: <3ae2a9aa-df53-fab2-aa98-a87787d5853e@redhat.com> Subject: Re: [PATCH v2 8/9] blkdebug: Add ability to override unmap geometries References: <1479413642-22463-1-git-send-email-eblake@redhat.com> <1479413642-22463-9-git-send-email-eblake@redhat.com> In-Reply-To: <1479413642-22463-9-git-send-email-eblake@redhat.com> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 17.11.2016 21:14, Eric Blake wrote: > Make it easier to simulate the Dell Equallogic iSCSI with its Somehow I feel bad putting company and product names into commit messages= =2E.. > unusual 15M preferred and maximum unmap and write zero sizing, > or to simulate Linux loopback block devices enforcing a small > max_transfer of 64k, by allowing blkdebug to wrap any other > device with further restrictions on various alignments. >=20 > Signed-off-by: Eric Blake > --- > qapi/block-core.json | 27 ++++++++++++++- > block/blkdebug.c | 97 ++++++++++++++++++++++++++++++++++++++++++++= ++++++-- > 2 files changed, 121 insertions(+), 3 deletions(-) >=20 > diff --git a/qapi/block-core.json b/qapi/block-core.json > index c29bef7..26f3e9f 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -2068,6 +2068,29 @@ > # @align: #optional required alignment for requests in bytes= , > # must be power of 2, or 0 for default > # > +# @max-transfer: #optional maximum size for I/O transfers in bytes,= > +# must be multiple of the larger of @align and and 5= 12 > +# (but need not be a power of 2), or 0 for default > +# (since 2.9) > +# > +# @opt-write-zero: #optional preferred alignment for write zero reque= sts > +# in bytes, must be multiple of the larger of @align= > +# and 512 (but need not be a power of 2), or 0 for > +# default (since 2.9) > +# > +# @max-write-zero: #optional maximum size for write zero requests in = bytes, > +# must be multiple of the larger of @align and 512 (= but > +# need not be a power of 2), or 0 for default (since= 2.9) > +# > +# @opt-discard: #optional preferred alignment for discard requests= > +# in bytes, must be multiple of the larger of @align= > +# and 512 (but need not be a power of 2), or 0 for > +# default (since 2.9) > +# > +# @max-discard: #optional maximum size for discard requests in byt= es, > +# must be multiple of the larger of @align and 512 (= but > +# need not be a power of 2), or 0 for default (since= 2.9) > +# > # @inject-error: #optional array of error injection descriptions > # > # @set-state: #optional array of state-change descriptions > @@ -2077,7 +2100,9 @@ > { 'struct': 'BlockdevOptionsBlkdebug', > 'data': { 'image': 'BlockdevRef', > '*config': 'str', > - '*align': 'int', > + '*align': 'int', '*max-transfer': 'int32', > + '*opt-write-zero': 'int32', '*max-write-zero': 'int32', > + '*opt-discard': 'int32', '*max-discard': 'int32', > '*inject-error': ['BlkdebugInjectErrorOptions'], > '*set-state': ['BlkdebugSetStateOptions'] } } >=20 > diff --git a/block/blkdebug.c b/block/blkdebug.c > index d45826d..3ba7a96 100644 > --- a/block/blkdebug.c > +++ b/block/blkdebug.c > @@ -39,6 +39,11 @@ typedef struct BDRVBlkdebugState { > int state; > int new_state; > int align; > + int max_transfer; > + int opt_write_zero; > + int max_write_zero; > + int opt_discard; > + int max_discard; >=20 > /* For blkdebug_refresh_filename() */ > char *config_file; > @@ -344,6 +349,31 @@ static QemuOptsList runtime_opts =3D { > .type =3D QEMU_OPT_SIZE, > .help =3D "Required alignment in bytes", > }, > + { > + .name =3D "max-transfer", > + .type =3D QEMU_OPT_SIZE, > + .help =3D "Maximum transfer size in bytes", > + }, > + { > + .name =3D "opt-write-zero", > + .type =3D QEMU_OPT_SIZE, > + .help =3D "Optimum write zero size in bytes", s/size/alignment/? > + }, > + { > + .name =3D "max-write-zero", > + .type =3D QEMU_OPT_SIZE, > + .help =3D "Maximum write zero size in bytes", > + }, > + { > + .name =3D "opt-discard", > + .type =3D QEMU_OPT_SIZE, > + .help =3D "Optimum discard size in bytes", s/size/alignment/? > + }, > + { > + .name =3D "max-discard", > + .type =3D QEMU_OPT_SIZE, > + .help =3D "Maximum discard size in bytes", > + }, > { /* end of list */ } > }, > }; > @@ -354,7 +384,8 @@ static int blkdebug_open(BlockDriverState *bs, QDic= t *options, int flags, > BDRVBlkdebugState *s =3D bs->opaque; > QemuOpts *opts; > Error *local_err =3D NULL; > - uint64_t align; > + uint64_t align, max_transfer; > + uint64_t opt_write_zero, max_write_zero, opt_discard, max_discard;= > int ret; >=20 > opts =3D qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); > @@ -389,7 +420,7 @@ static int blkdebug_open(BlockDriverState *bs, QDic= t *options, int flags, > bs->supported_zero_flags =3D (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) &= > bs->file->bs->supported_zero_flags; >=20 > - /* Set request alignment */ > + /* Set alignment overrides */ > align =3D qemu_opt_get_size(opts, "align", 0); > if (align < INT_MAX && is_power_of_2(align)) { > s->align =3D align; > @@ -398,6 +429,53 @@ static int blkdebug_open(BlockDriverState *bs, QDi= ct *options, int flags, > ret =3D -EINVAL; > goto fail_unref; > } > + max_transfer =3D qemu_opt_get_size(opts, "max-transfer", 0); > + if (max_transfer < INT_MAX && > + QEMU_IS_ALIGNED(max_transfer, MAX(align, BDRV_SECTOR_SIZE))) {= > + s->max_transfer =3D max_transfer; > + } else if (max_transfer) { > + error_setg(errp, "Invalid argument"); Could you be more specific? Same in all of the error_setg() calls below. > + ret =3D -EINVAL; > + goto fail_unref; > + } Also, the way this is formatted seems not intuitive to me. I know it's the same as it's been done for "align", but normally I'd use the followin= g: s->value =3D qemu_opt_get_size(...); if (s->value is set and invalid) { /* error out */ } Instead of: value =3D qemu_opt_get_size(...); if (value is valid) { s->value =3D value; } else if (value is set) { /* error out */ } Same for all blocks below. Finally, my eyes would be really grateful for some empty lines here and there, at least one between the handling of different options. Max > + opt_write_zero =3D qemu_opt_get_size(opts, "opt-write-zero", 0); > + if (opt_write_zero < INT_MAX && > + QEMU_IS_ALIGNED(opt_write_zero, MAX(align, BDRV_SECTOR_SIZE)))= { > + s->opt_write_zero =3D opt_write_zero; > + } else if (opt_write_zero) { > + error_setg(errp, "Invalid argument"); > + ret =3D -EINVAL; > + goto fail_unref; > + } > + max_write_zero =3D qemu_opt_get_size(opts, "max-write-zero", 0); > + if (max_write_zero < INT_MAX && > + QEMU_IS_ALIGNED(max_write_zero, MAX(opt_write_zero, > + MAX(align, BDRV_SECTOR_SIZE))= )) { > + s->max_write_zero =3D max_write_zero; > + } else if (max_write_zero) { > + error_setg(errp, "Invalid argument"); > + ret =3D -EINVAL; > + goto fail_unref; > + } > + opt_discard =3D qemu_opt_get_size(opts, "opt-discard", 0); > + if (opt_discard < INT_MAX && > + QEMU_IS_ALIGNED(opt_discard, MAX(align, BDRV_SECTOR_SIZE))) { > + s->opt_discard =3D opt_discard; > + } else if (opt_discard) { > + error_setg(errp, "Invalid argument"); > + ret =3D -EINVAL; > + goto fail_unref; > + } > + max_discard =3D qemu_opt_get_size(opts, "max-discard", 0); > + if (max_discard < INT_MAX && > + QEMU_IS_ALIGNED(max_discard, MAX(opt_discard, > + MAX(align, BDRV_SECTOR_SIZE))= )) { > + s->max_discard =3D max_discard; > + } else if (max_discard) { > + error_setg(errp, "Invalid argument"); > + ret =3D -EINVAL; > + goto fail_unref; > + } >=20 > ret =3D 0; > goto out; > @@ -807,6 +885,21 @@ static void blkdebug_refresh_limits(BlockDriverSta= te *bs, Error **errp) > if (s->align) { > bs->bl.request_alignment =3D s->align; > } > + if (s->max_transfer) { > + bs->bl.max_transfer =3D s->max_transfer; > + } > + if (s->opt_write_zero) { > + bs->bl.pwrite_zeroes_alignment =3D s->opt_write_zero; > + } > + if (s->max_write_zero) { > + bs->bl.max_pwrite_zeroes =3D s->max_write_zero; > + } > + if (s->opt_discard) { > + bs->bl.pdiscard_alignment =3D s->opt_discard; > + } > + if (s->max_discard) { > + bs->bl.max_pdiscard =3D s->max_discard; > + } > } >=20 > static int blkdebug_reopen_prepare(BDRVReopenState *reopen_state, >=20 --53a7svosuG6anwgrj94oCLB2wtXmjMx7p Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEvBAEBCAAZBQJYLjcCEhxtcmVpdHpAcmVkaGF0LmNvbQAKCRD0B9sAYdXPQKoc CACdQaQXS4a1BPEEwQhOo+HYxs9CkX3L0BVghdXs3DcTgUanK6oqglioZHsyPsKE MVvaDbjJ4X4YLsonqyH2gO4WnNEEu3lnF4r7qeT0GjrWUC13iHhMZkG/3F3Y+q1K bCDvmGrp2jMFk5pt6jgnCqB4gzvffatgy4EdaL4rF/allqEBwCId3Hih3Gpvvftx YvBtn/jnb5eb9X6OB41GeOY5xhy2DOrLtzP7+3avM2VZItPLiAANFKAhHrkb4Bou lguqgME7pRvRLZZyPzUHcMupGLfSYEAclUisueuTS/xtiXF0xACxbDPJfLORuxZE hhHEK9JEIMWUDYjTqTzG6Bq9 =7szn -----END PGP SIGNATURE----- --53a7svosuG6anwgrj94oCLB2wtXmjMx7p--