From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44100) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFf2X-0000CG-OY for qemu-devel@nongnu.org; Wed, 22 Jun 2016 06:05:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bFf2V-0001NA-6j for qemu-devel@nongnu.org; Wed, 22 Jun 2016 06:05:44 -0400 Date: Wed, 22 Jun 2016 12:05:32 +0200 From: Kevin Wolf Message-ID: <20160622100532.GC6134@noname.redhat.com> References: <1465939839-30097-1-git-send-email-eblake@redhat.com> <1465939839-30097-9-git-send-email-eblake@redhat.com> <20160621132719.GF4520@noname.redhat.com> <5769BCE7.3030505@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="XsQoSWH+UP9D9v3l" Content-Disposition: inline In-Reply-To: <5769BCE7.3030505@redhat.com> 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: Eric Blake Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, Max Reitz --XsQoSWH+UP9D9v3l Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Am 22.06.2016 um 00:17 hat Eric Blake geschrieben: > 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 >=20 > >> /* 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). >=20 > Correct. >=20 > >=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 >=20 > Correct again. >=20 > > 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. >=20 > 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). Except that you don't have to check all of that explicitly, the default value for absent options is what the third parameter is for: align =3D qemu_opt_get_size(opts, "align", 0); Kevin --XsQoSWH+UP9D9v3l Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJXamLsAAoJEH8JsnLIjy/W0FcP+gOKwCSoTIQD3h6TTYnd8yYJ kvienWhEcTrPwYa+2plCMX+WvqGezXeAP39BO7q0Jt8QvlT+2jFXkaegXVDop1+h DnTF6OwaKxWmLTgxJ7/A8qR5OTh4B1mgreJm3u9ifaucCNTpxP56bH6l22oRV2A/ IZwrbmmRJoQUXJ7xn/KrFSQEfr9iGhq72qE59WJbYCIptJyKixzoxA4bmb/da3FW OdMxj0o3DmH1soVJ+E45cWPdFAAp15bH1/9bsnRSvxm6LJ5CjWZjSapZ0jTKzMOy 0zz+iIwL01CtzKA5B20hjwbGIaJqXJtHjVNow4Jd3lbERTykprvvFsEpgh0HXC/N hwWiCvRguj5xhzQmlyw8SNAzaVmOIzOz2q4jL90APvUwE03a5ufW+MyRvM+9Jwoq OCG1xjdi4jIYp54L2rHgWCDng2Hba0Q9zFbmcjC5wbZ8tbB34bhsJbzv2b0+2Cqh aWIDNJW0L+swKr8LCpzacEtXtLGDPPpQB5iHzKw7WarphHyHTvWsE0gg2dxOHBav eWtubx6io3hWMX9hEmu+ubclO2s1JBdSB6Sg3WGYgu+rhfAycHf5GAcrIJ7minsb ilGaPUpzcuXAwtTmLoMbOjYoS40SFx4mwDKgiF/oM8oLquTIk1F77VWzr0mUko0z HoeNqFvZdmQ1fVgSmZMf =aQHA -----END PGP SIGNATURE----- --XsQoSWH+UP9D9v3l--