From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35396) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCjMC-0000W7-O3 for qemu-devel@nongnu.org; Tue, 14 Jun 2016 04:05:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bCjMB-00078j-GZ for qemu-devel@nongnu.org; Tue, 14 Jun 2016 04:05:56 -0400 Date: Tue, 14 Jun 2016 10:05:46 +0200 From: Kevin Wolf Message-ID: <20160614080546.GA4916@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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="UugvWAfsgieZRqgk" Content-Disposition: inline In-Reply-To: <575F8A87.6000406@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 --UugvWAfsgieZRqgk Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Am 14.06.2016 um 06:39 hat Eric Blake geschrieben: > On 06/07/2016 04:08 AM, Kevin Wolf wrote: >=20 > >> Found it; squash this in (or use it as an argument why we don't want > >> request_alignment in bs->bl after all): > >=20 > > This hunk doesn't make sense to me. For the correctness of the code it > > shouldn't make a difference whether the alignment happens before passing > > the request to file/raw-posix or already in the raw format layer. > >=20 > > The cause for the hang you're seeing is probably that the request is > > already aligned before the blkdebug layer and therefore the blkdebug > > events aren't generated any more. That's a problem with the test (I'm > > considering the blkdebug events part of the test infrastructure), > > however, and not with the code. > >=20 >=20 > Yes, it's definitely a hang caused by the test expecting an unalignment > event, but the inheritance chain now causes things to be aligned to > begin with and nothing unaligned happens after all. >=20 > > Kevin > >=20 > >> diff --git i/block/raw_bsd.c w/block/raw_bsd.c > >> index b1d5237..c3c2246 100644 > >> --- i/block/raw_bsd.c > >> +++ w/block/raw_bsd.c > >> @@ -152,7 +152,11 @@ static int raw_get_info(BlockDriverState *bs, > >> BlockDriverInfo *bdi) > >> > >> 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; >=20 > 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? Interesting idea. Maybe that's a good option if it works. > (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). Actually, I think there are two cases. 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 file 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. 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. 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. > 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... 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. Kevin --UugvWAfsgieZRqgk Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJXX7raAAoJEH8JsnLIjy/WLz8QALNlYP7MjaBYF3SZL3sLHhK8 cnSLyF2+dm6UjPCwh8u59lcdCSAVBcung/iBF24GXQLpJTgg6Dc7qHgTuMQ3HLp5 L9upqFuejIZ2gtzcanSIxXUB2watHmFwT0n5OJGFOt/oJ+/wnZGrllnztX5YQgdy EVNmGvkH5wtWNZwSxyru1EAecX090X7TE4VRsN3OXJR9bw9gNWOrIYdyoj3X4eJv gOynMlZFYGv4Ipj9+kV22VEVGX2waFrAeOkdKwApa6q3YFgei8LJQty/NjAuG6V6 iROgySMUubXAL6t9cENO6qkGWCDmJ+13/dsaXQKbQQHI4VmWkFulOzMbVBoa1rWI TcW2w1Ya3jGvOLx0uhVsyKjFlbwhhKpWVlA92PZn8jkTX52J8MtK+E4WTlt05yZa f1JGCdee9WO86FVEL83iSEMrqypmAG8HVl/o/kEh6Aaaf/OWIuzOzaml1y4xMofP stTQzRQvovdUVQpQNJ14CNGW+AZYQSAHxC0uYojytzVpNBpdawZcH39Wso/Z6KG8 tLNodgoqJppbPeHFqwEN3euFMKmTx1ZjS7NlawHY/YiOYIfa41s16V4exS9iHWOp hoNFQoWkhHqT6Vru3LaY2GOYUq4dSQ/KsUURzJwtNYtabE87WIsjH8TsYDKKlZTP PGV5vkJZB8pVkoy9qYdh =sN/8 -----END PGP SIGNATURE----- --UugvWAfsgieZRqgk--