From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54083) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCqxk-0002Zw-KI for qemu-devel@nongnu.org; Tue, 14 Jun 2016 12:13:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bCqxi-0006RU-EI for qemu-devel@nongnu.org; Tue, 14 Jun 2016 12:13:11 -0400 References: <1465595961-4762-1-git-send-email-jsnow@redhat.com> <905ed66e-9f55-b27f-6a98-1c2a845f3759@redhat.com> From: Max Reitz Message-ID: <9058ee75-0e95-a35f-4ab1-ca801baafa96@redhat.com> Date: Tue, 14 Jun 2016 18:13:00 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="6JG67u2OEgcCL98KSoFh9SnQJgUC1IHlI" Subject: Re: [Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-block@nongnu.org Cc: kwolf@redhat.com, qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --6JG67u2OEgcCL98KSoFh9SnQJgUC1IHlI From: Max Reitz To: John Snow , qemu-block@nongnu.org Cc: kwolf@redhat.com, qemu-devel@nongnu.org Message-ID: <9058ee75-0e95-a35f-4ab1-ca801baafa96@redhat.com> Subject: Re: [PATCH] block-backend: allow flush on devices with open tray References: <1465595961-4762-1-git-send-email-jsnow@redhat.com> <905ed66e-9f55-b27f-6a98-1c2a845f3759@redhat.com> In-Reply-To: Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 14.06.2016 17:54, John Snow wrote: >=20 >=20 > On 06/14/2016 09:19 AM, Max Reitz wrote: >> On 10.06.2016 23:59, John Snow wrote: >>> If a device still has an attached BDS because the medium has not yet >>> been removed, we will be unable to migrate to a new host because >>> blk_flush will return an error for that backend. >>> >>> Replace the call to blk_is_available to blk_is_inserted to weaken >>> the check and allow flushes from the backend to work, while still >>> disallowing flushes from the frontend/device model to work. >>> >>> This fixes a regression present in 2.6.0 caused by the following comm= it: >>> fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf >>> block: Move some bdrv_*_all() functions to BB >>> >>> Signed-off-by: John Snow >>> --- >>> block/block-backend.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> I'm still not sure we shouldn't do the same for blk_{co,aio}_flush(). = I >> guess you exclude them here because you specifically want to fix the >> issue mentioned in the commit message, but then we could just make >> blk_flush_all() ignore an -ENOMEDIUM. >=20 > Yeah, I didn't investigate the full path. Just making the minimal fixes= =2E > Is there a concern that this may still leave certain pathways broken > when the CDROM tray is open? >=20 > I don't know of any immediately without digging again. >=20 >> >> I personally think we should make all blk_*flush() functions use >> blk_is_inserted() instead of blk_is_available(). As we have discussed = on >> IRC, there are probably not that many cases a guest can flush a medium= >> in an open tray anyway (because the main use case are read-only >> CD-ROMs), and even if so, that wouldn't change any data, so even if th= e >> guest can actually flush something on an open tray, I don't think anyo= ne >> would complain. >> >> Max >> >=20 > I have difficulty making pragmatic arguments when purity is at stake, > but I've already wandered outside of my device model, so I will defer t= o > your judgment. >=20 >>> diff --git a/block/block-backend.c b/block/block-backend.c >>> index 34500e6..d1e875e 100644 >>> --- a/block/block-backend.c >>> +++ b/block/block-backend.c >>> @@ -1122,7 +1122,7 @@ int blk_co_flush(BlockBackend *blk) >>> =20 >>> int blk_flush(BlockBackend *blk) >>> { >>> - if (!blk_is_available(blk)) { >>> + if (!blk_is_inserted(blk)) { >>> return -ENOMEDIUM; >>> } >>> =20 >>> >> >> >=20 > Is this a NACK unless I attempt to address the wider design issue? I just don't see a point in using blk_is_inserted() here but blk_is_available() in the other blk_*flush() functions. If blk_is_inserted() is correct for blk_flush(), it should be correct for blk_co_flush() and blk_aio_flush(), too. Maybe I should emphasize that I decided between is_available() and is_inserted() basically on what felt right to me. There's not really that much research behind it, so changing it is completely fine. Max --6JG67u2OEgcCL98KSoFh9SnQJgUC1IHlI 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 iQEcBAEBCAAGBQJXYC0MAAoJEDuxQgLoOKytqwsH/2iXOcyfDlKocvPnkeUdMhUo YaY1buJ0lB/LpbWvXDd7BbzLUZsz70+IncNJqoagU8MEAHxoIGOTtgKo28W3Hr8Y ZBvUHmqFpXjbe7oisXZh/R7g6daLFcdNQONtB4+fkGkthhPtD4pY+d3RbI+anqLR pzjjNyG9iGTCf0gSs+MpFADyO1Pmm/NbfP+9hEjMXn5INL5CScqAtgUm23a24qAO Z9HB8cbaUGcaGOec1FjWuJEFzHlX1T6jPDOivTvFFTrxiTpjM3DMavtAJ3G/kJY/ p0L7yjQ+TQ0hPQuKXcJ2cpR5Ql7RpVjl1q9bh3CHGnsBZV5/Mr6cJ9lNdfHcaaI= =KTGn -----END PGP SIGNATURE----- --6JG67u2OEgcCL98KSoFh9SnQJgUC1IHlI--