From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54094) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCpdI-0002ws-GW for qemu-devel@nongnu.org; Tue, 14 Jun 2016 10:48:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bCpdG-0000Ln-6s for qemu-devel@nongnu.org; Tue, 14 Jun 2016 10:47:59 -0400 References: <1464973388-15821-1-git-send-email-eblake@redhat.com> <1464973388-15821-6-git-send-email-eblake@redhat.com> <5751C32F.3040806@redhat.com> <5751F9E8.6090900@redhat.com> <20160607100810.GB4684@noname.str.redhat.com> <575F8A87.6000406@redhat.com> <20160614080546.GA4916@noname.str.redhat.com> From: Eric Blake Message-ID: <57601916.4010408@redhat.com> Date: Tue, 14 Jun 2016 08:47:50 -0600 MIME-Version: 1.0 In-Reply-To: <20160614080546.GA4916@noname.str.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="h31KgajnhWmRluUoe1snaFmb0cL9RW065" Subject: Re: [Qemu-devel] [PATCH 5/5] block: Move request_alignment into BlockLimit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, Fam Zheng , Ronnie Sahlberg , qemu-block@nongnu.org, Peter Lieven , mreitz@redhat.com, Stefan Hajnoczi , Paolo Bonzini This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --h31KgajnhWmRluUoe1snaFmb0cL9RW065 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 06/14/2016 02:05 AM, Kevin Wolf wrote: >>>> static void raw_refresh_limits(BlockDriverState *bs, Error **errp) >>>> { >>>> + /* Inherit all limits except for request_alignment */ >>>> + int request_alignment =3D bs->bl.request_alignment; >>>> + >>>> bs->bl =3D bs->file->bs->bl; >>>> + bs->bl.request_alignment =3D request_alignment; >> >> Any ideas on how to fix the test, then? Have two blkdebug devices >> nested atop one another, since those are the devices where we can >> explicitly override alignment? >=20 > Interesting idea. Maybe that's a good option if it works. >=20 >> (normally, you'd _expect_ the chain to >> inherit the worst-case alignment of all BDS in the chain, so blkdebug = is >> the way around it). >=20 > Actually, I think there are two cases. >=20 > The first one is that you get a request and want to know what to do wit= h > it. Here you don't want to inherit the worst-case alignment. You'd > rather want to enforce alignment only when it is actually needed. For > example, if you have a cache=3Dnone backing file with 4k alignment, but= a > cache=3Dwriteback overlay, and you don't actually access the backing fi= le > with this request, it would be wasteful to align the request. You also > don't really know that a driver will issue a misaligned request (or any= > request at all) to the lower layer just because it got called with one.= >=20 > The second case is when you want to issue a request. For example, let's= > take the qcow2 cache here, which has 64k cached and needs to update a > few bytes. Currently it always writes the full 64k (and with 1 MB > clusters a full megabyte), but what it really should do is consider the= > worst-case alignment. >=20 > This is comparable to the difference between bdrv_opt_mem_align(), whic= h > is used in qemu_blockalign() where we want to create a buffer that work= s > even in the worst case, and bdrv_min_mem_align(), which is used in > bdrv_qiov_is_aligned() in order to determine whether we must align an > existing request. >=20 >> That's the only thing left before I repost the series, so I may just >> post the last patch as RFC and play with it a bit more... >=20 > And in the light of the above, maybe the solution is as easy as changin= g > raw_refresh_limits() to set bs->bl.request_alignment =3D 1. Or maybe split the main bdrv_refresh_limits() into two pieces: one that merges limits from one BDS into another (the limits that are worth merging, and in the correct direction: opt merges to MAX, max merges to MIN_NON_ZERO, request_alignment is not merged), the other that calls merge(bs, bs->file->bs); then have raw_refresh_limits() also use the common merge functionality rather than straight copy. Testing that approach now... --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --h31KgajnhWmRluUoe1snaFmb0cL9RW065 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/ iQEcBAEBCAAGBQJXYBkXAAoJEKeha0olJ0NqtSYH/1M5V5UQfEzZv9B1IwD0o+um cUgNAMbxPFa6Ndi0sX3HSZN1W3xVRqzBPDstHMdJYRWDyGncCSlvaZm/30KGoXB5 KTdJO4QqdleMX8di+G0ivN7qj1PiJiSd25gzYfcr6+N2yqzv6vizotMgB293WMup UIprZwpq5wNl0bS2QdYOqqLmdnObCp/8x+M2ESZqh2KisFRBHi/tZvh8+aLyY936 FUoeBIZLNcoWKXLc7VPhj0C+GSt3Yzvi2eGRTuvwlQh+qWlKyVw4gslUYMOOSoIb XpXmsVSYAiqYP+d02SBVcY3IgV3UHWKVPNdOrsEK40ySMSb+hmke+2Wn23gpmoo= =0K20 -----END PGP SIGNATURE----- --h31KgajnhWmRluUoe1snaFmb0cL9RW065--