From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41878) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFTz0-0000US-KU for qemu-devel@nongnu.org; Tue, 21 Jun 2016 18:17:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bFTyy-0002Gf-GV for qemu-devel@nongnu.org; Tue, 21 Jun 2016 18:17:21 -0400 References: <1465939839-30097-1-git-send-email-eblake@redhat.com> <1465939839-30097-9-git-send-email-eblake@redhat.com> <20160621132719.GF4520@noname.redhat.com> From: Eric Blake Message-ID: <5769BCE7.3030505@redhat.com> Date: Tue, 21 Jun 2016 16:17:11 -0600 MIME-Version: 1.0 In-Reply-To: <20160621132719.GF4520@noname.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="gshtQa6D6Ocrbjbkdbx7HCosX8wtTOew0" Subject: Re: [Qemu-devel] [PATCH v2 08/17] blkdebug: Set request_alignment during .bdrv_refresh_limits() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, Max Reitz This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --gshtQa6D6Ocrbjbkdbx7HCosX8wtTOew0 From: Eric Blake To: Kevin Wolf Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, Max Reitz Message-ID: <5769BCE7.3030505@redhat.com> Subject: Re: [PATCH v2 08/17] blkdebug: Set request_alignment during .bdrv_refresh_limits() References: <1465939839-30097-1-git-send-email-eblake@redhat.com> <1465939839-30097-9-git-send-email-eblake@redhat.com> <20160621132719.GF4520@noname.redhat.com> In-Reply-To: <20160621132719.GF4520@noname.redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 06/21/2016 07:27 AM, Kevin Wolf wrote: > Am 14.06.2016 um 23:30 hat Eric Blake geschrieben: >> We want to eventually stick request_alignment alongside other >> BlockLimits, but first, we must ensure it is populated at the >> same time as all other limits, rather than being a special case >> that is set only when a block is first opened. >> >> qemu-iotests 77 is particularly sensitive to the fact that we >> can specify an artificial alignment override in blkdebug, and >> that override must continue to work even when limits are >> refreshed on an already open device. >> >> A later patch will be altering when bs->request_alignment >> defaults are set, so fall back to sector alignment if we have >> not inherited anything yet. >> >> Signed-off-by: Eric Blake >> /* Set request alignment */ >> - align =3D qemu_opt_get_size(opts, "align", bs->request_alignment)= ; >> - if (align > 0 && align < INT_MAX && !(align & (align - 1))) { >> - bs->request_alignment =3D align; >> + align =3D qemu_opt_get_size(opts, "align", >> + bs->request_alignment ?: BDRV_SECTOR_SI= ZE); >=20 > In the state as of this patch: How would bs->request_alignment ever be > zero? It is always initialised as 512 (because blkdebug doesn't > implement byte-based interfaces). Correct. >=20 > Later in this series, you move the initialisation of the field to > bdrv_refresh_limits(). However, the check still doesn't make sense > because now it's always 0 and you always use the BDRV_SECTOR_SIZE > fallback. >=20 Correct again. > I think you should either just unconditionally use BDRV_SECTOR_SIZE or > even better just store 0 in s->align if the option isn't given. Your > implementation of blkdebug_refresh_limits() already leaves the default > bs->request_alignment alone if s->align =3D=3D 0. I like that idea; I guess that means I instead need to tweak the logic to test if "align" is present in opts (in which case it must be non-zero), or absent (in which case leaving things as 0 is a nicer approach than trying to guess which default to stick things to). >=20 >> + if (align > 0 && align < INT_MAX && is_power_of_2(align)) { And while I'm at it, align > 0 is redundant with is_power_of_2(align); will simplify on v3. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --gshtQa6D6Ocrbjbkdbx7HCosX8wtTOew0 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJXabznAAoJEKeha0olJ0NqBhYIAJo6dZGDuOMqj9Dn9xbJGjD/ 0DuGZHSKTBhozN0T+q9Nac1BMGGDcZNQcP3Dx6W2jsbtLauw+ZPujK148pkOTiSy 2Ag/wR+xw7U0YubeZiGZlo5ZDAc54DGE9PcrNs9/dmwuY+iBw10aHE5MXaConmfo /OiWtfm6CkN3ytdC1lNr6yHVhqBG/ukkRMJbbfRIPiriEtCUjNa+f7Uujlvz14Au w6FxcR+aUkFf3/AaG933xA71ByYIykhDOS+g4GLgh6ET5j1Eax/Lh/khJEl61/W/ tsl+/bKhyme/Sc8Oodmbrv5NXyhjrRqFzOYEa0gn7hXWTYded+61W7kW5vB0nRU= =kzpH -----END PGP SIGNATURE----- --gshtQa6D6Ocrbjbkdbx7HCosX8wtTOew0--