From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42598) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCqI8-0003Tm-IH for qemu-devel@nongnu.org; Tue, 14 Jun 2016 11:30:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bCqI6-0004TM-Fj for qemu-devel@nongnu.org; Tue, 14 Jun 2016 11:30:11 -0400 Date: Tue, 14 Jun 2016 17:30:00 +0200 From: Kevin Wolf Message-ID: <20160614153000.GB14803@noname.str.redhat.com> 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> <57601916.4010408@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="3uo+9/B/ebqu+fSQ" Content-Disposition: inline In-Reply-To: <57601916.4010408@redhat.com> 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: Eric Blake Cc: qemu-devel@nongnu.org, Fam Zheng , Ronnie Sahlberg , qemu-block@nongnu.org, Peter Lieven , mreitz@redhat.com, Stefan Hajnoczi , Paolo Bonzini --3uo+9/B/ebqu+fSQ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Am 14.06.2016 um 16:47 hat Eric Blake geschrieben: > On 06/14/2016 02:05 AM, Kevin Wolf wrote: >=20 > >>>> 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 with > > 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(), which > > is used in qemu_blockalign() where we want to create a buffer that works > > 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 changing > > raw_refresh_limits() to set bs->bl.request_alignment =3D 1. >=20 > 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... So you don't agree with what I said above? I think that block drivers should only set limits that they require for themselves. The block layer bdrv_refresh_limits() function can then merge things where needed; drivers shouldn't be involved there. Kevin --3uo+9/B/ebqu+fSQ Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJXYCL4AAoJEH8JsnLIjy/WgpAP/AzSQY1oJwIxfVXu5YNhSeZH LSH3KwYcSGBQVUz4bUCl1hfqMPam/RHTAC3jS9OEbWT7/I1k/6hsYEoS23P0045/ fDtnWKMgtMkq2/agBVYjy5xwpD44mRnYnwBbnINEb3fdi3RsfL1TGuKw6VnByJk9 e8Vdg2WpoUba9qTNzPuuZZ2sS7HT8SvHJv5leigIGpZkcLOGg+mHiE6Ngr7SM/F2 g1A4tKRVN7wAkdV+Vc+bLSJwsiXs5i/vj9ROn5XwswblFW6++bRpBiUGppmZD/0d LjQ0NwMRcFpTY/ONJQYcZRe/Bv3H8b7tF8TT7sXO2aLMVDJ3ODdq8wQs30kRQTAv rrUtv5L5agt0k+XYzwuAmpG0lDnebOudwDDgnLEduZA/BPCz18V97Jr3aY8d5ULT CWMHHcGE0lYUQhzHBfH1mWILFH9/qdORZW7i89QM8Z0SLQq5yOMc6SjQNTsdEZHc Henc0N6ZsIxNoQnfH1MGjl4U/d0Tu4DXnZeB0O9EwPKLuSaMz5Wi3mSpgAOpNw4p rJLse5QhecAxRetNZ9R9HwQkLgHoUQ+k4wwEb0u9r6ZT4RLdgj5ix7wGqPDMtlR8 DE8u2te/GPPiz522N1u6KWPI0pNXO6x55DcAik5kPGnAo+G2bDTW/V950pKlaN3T rEZoS0mvca5WItuFsFPL =lkut -----END PGP SIGNATURE----- --3uo+9/B/ebqu+fSQ--