* [Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray @ 2016-06-10 21:59 John Snow 2016-06-10 22:42 ` Eric Blake ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: John Snow @ 2016-06-10 21:59 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, qemu-devel, mreitz, John Snow 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 commit: fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf block: Move some bdrv_*_all() functions to BB Signed-off-by: John Snow <jsnow@redhat.com> --- block/block-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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) int blk_flush(BlockBackend *blk) { - if (!blk_is_available(blk)) { + if (!blk_is_inserted(blk)) { return -ENOMEDIUM; } -- 2.4.11 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray 2016-06-10 21:59 [Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray John Snow @ 2016-06-10 22:42 ` Eric Blake 2016-06-13 17:57 ` John Snow 2016-06-12 2:26 ` Fam Zheng 2016-06-14 13:19 ` Max Reitz 2 siblings, 1 reply; 8+ messages in thread From: Eric Blake @ 2016-06-10 22:42 UTC (permalink / raw) To: John Snow, qemu-block; +Cc: kwolf, qemu-devel, mreitz [-- Attachment #1: Type: text/plain, Size: 1381 bytes --] On 06/10/2016 03:59 PM, 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 commit: > fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf > block: Move some bdrv_*_all() functions to BB > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block/block-backend.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Worth testsuite coverage to prevent future regressions? At any rate, Reviewed-by: Eric Blake <eblake@redhat.com> > > 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) > > int blk_flush(BlockBackend *blk) > { > - if (!blk_is_available(blk)) { > + if (!blk_is_inserted(blk)) { > return -ENOMEDIUM; > } > > -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray 2016-06-10 22:42 ` Eric Blake @ 2016-06-13 17:57 ` John Snow 0 siblings, 0 replies; 8+ messages in thread From: John Snow @ 2016-06-13 17:57 UTC (permalink / raw) To: Eric Blake, qemu-block; +Cc: kwolf, qemu-devel, mreitz On 06/10/2016 06:42 PM, Eric Blake wrote: > On 06/10/2016 03:59 PM, 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 commit: >> fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf >> block: Move some bdrv_*_all() functions to BB >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block/block-backend.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > Worth testsuite coverage to prevent future regressions? > I'll look into it. (Ugh, migration tests. wah. etc.) --js > At any rate, > Reviewed-by: Eric Blake <eblake@redhat.com> > >> >> 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) >> >> int blk_flush(BlockBackend *blk) >> { >> - if (!blk_is_available(blk)) { >> + if (!blk_is_inserted(blk)) { >> return -ENOMEDIUM; >> } >> >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray 2016-06-10 21:59 [Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray John Snow 2016-06-10 22:42 ` Eric Blake @ 2016-06-12 2:26 ` Fam Zheng 2016-06-14 13:19 ` Max Reitz 2 siblings, 0 replies; 8+ messages in thread From: Fam Zheng @ 2016-06-12 2:26 UTC (permalink / raw) To: John Snow; +Cc: qemu-block, kwolf, qemu-devel, mreitz On Fri, 06/10 17: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 commit: > fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf > block: Move some bdrv_*_all() functions to BB > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block/block-backend.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > 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) > > int blk_flush(BlockBackend *blk) > { > - if (!blk_is_available(blk)) { > + if (!blk_is_inserted(blk)) { > return -ENOMEDIUM; > } > > -- > 2.4.11 > > Reviewed-by: Fam Zheng <famz@redhat.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray 2016-06-10 21:59 [Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray John Snow 2016-06-10 22:42 ` Eric Blake 2016-06-12 2:26 ` Fam Zheng @ 2016-06-14 13:19 ` Max Reitz 2016-06-14 15:54 ` John Snow 2 siblings, 1 reply; 8+ messages in thread From: Max Reitz @ 2016-06-14 13:19 UTC (permalink / raw) To: John Snow, qemu-block; +Cc: kwolf, qemu-devel [-- Attachment #1: Type: text/plain, Size: 1849 bytes --] 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 commit: > fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf > block: Move some bdrv_*_all() functions to BB > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > 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. 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 the guest can actually flush something on an open tray, I don't think anyone would complain. Max > 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) > > int blk_flush(BlockBackend *blk) > { > - if (!blk_is_available(blk)) { > + if (!blk_is_inserted(blk)) { > return -ENOMEDIUM; > } > > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray 2016-06-14 13:19 ` Max Reitz @ 2016-06-14 15:54 ` John Snow 2016-06-14 16:13 ` Max Reitz 0 siblings, 1 reply; 8+ messages in thread From: John Snow @ 2016-06-14 15:54 UTC (permalink / raw) To: Max Reitz, qemu-block; +Cc: kwolf, qemu-devel 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 commit: >> fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf >> block: Move some bdrv_*_all() functions to BB >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> 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. Yeah, I didn't investigate the full path. Just making the minimal fixes. Is there a concern that this may still leave certain pathways broken when the CDROM tray is open? I don't know of any immediately without digging again. > > 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 the > guest can actually flush something on an open tray, I don't think anyone > would complain. > > Max > 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 to your judgment. >> 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) >> >> int blk_flush(BlockBackend *blk) >> { >> - if (!blk_is_available(blk)) { >> + if (!blk_is_inserted(blk)) { >> return -ENOMEDIUM; >> } >> >> > > Is this a NACK unless I attempt to address the wider design issue? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray 2016-06-14 15:54 ` John Snow @ 2016-06-14 16:13 ` Max Reitz 2016-06-15 7:51 ` Kevin Wolf 0 siblings, 1 reply; 8+ messages in thread From: Max Reitz @ 2016-06-14 16:13 UTC (permalink / raw) To: John Snow, qemu-block; +Cc: kwolf, qemu-devel [-- Attachment #1: Type: text/plain, Size: 2999 bytes --] On 14.06.2016 17:54, John Snow wrote: > > > 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 commit: >>> fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf >>> block: Move some bdrv_*_all() functions to BB >>> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> 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. > > Yeah, I didn't investigate the full path. Just making the minimal fixes. > Is there a concern that this may still leave certain pathways broken > when the CDROM tray is open? > > I don't know of any immediately without digging again. > >> >> 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 the >> guest can actually flush something on an open tray, I don't think anyone >> would complain. >> >> Max >> > > 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 to > your judgment. > >>> 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) >>> >>> int blk_flush(BlockBackend *blk) >>> { >>> - if (!blk_is_available(blk)) { >>> + if (!blk_is_inserted(blk)) { >>> return -ENOMEDIUM; >>> } >>> >>> >> >> > > 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 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray 2016-06-14 16:13 ` Max Reitz @ 2016-06-15 7:51 ` Kevin Wolf 0 siblings, 0 replies; 8+ messages in thread From: Kevin Wolf @ 2016-06-15 7:51 UTC (permalink / raw) To: Max Reitz; +Cc: John Snow, qemu-block, qemu-devel [-- Attachment #1: Type: text/plain, Size: 3239 bytes --] Am 14.06.2016 um 18:13 hat Max Reitz geschrieben: > On 14.06.2016 17:54, John Snow wrote: > > > > > > 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 commit: > >>> fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf > >>> block: Move some bdrv_*_all() functions to BB > >>> > >>> Signed-off-by: John Snow <jsnow@redhat.com> > >>> --- > >>> 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. > > > > Yeah, I didn't investigate the full path. Just making the minimal fixes. > > Is there a concern that this may still leave certain pathways broken > > when the CDROM tray is open? > > > > I don't know of any immediately without digging again. > > > >> > >> 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 the > >> guest can actually flush something on an open tray, I don't think anyone > >> would complain. > >> > >> Max > >> > > > > 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 to > > your judgment. > > > >>> 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) > >>> > >>> int blk_flush(BlockBackend *blk) > >>> { > >>> - if (!blk_is_available(blk)) { > >>> + if (!blk_is_inserted(blk)) { > >>> return -ENOMEDIUM; > >>> } > >>> > >>> > >> > >> > > > > 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. I agree, if we can, we should keep the behaviour consistent between all interfaces types (sync/AIO/coroutine, byte-based/sector-based) for the same operation. Eric also rightfully said that we need a test cases, so a v2 would be good anyway. Kevin [-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-06-15 7:51 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-10 21:59 [Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray John Snow 2016-06-10 22:42 ` Eric Blake 2016-06-13 17:57 ` John Snow 2016-06-12 2:26 ` Fam Zheng 2016-06-14 13:19 ` Max Reitz 2016-06-14 15:54 ` John Snow 2016-06-14 16:13 ` Max Reitz 2016-06-15 7:51 ` Kevin Wolf
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.