Am 14.10.2011 14:42, schrieb Paolo Bonzini: > On 10/14/2011 01:54 PM, Kevin Wolf wrote: >> Am 14.10.2011 13:30, schrieb Paolo Bonzini: >>> On 10/14/2011 01:08 PM, Kevin Wolf wrote: >>>> Am 14.10.2011 10:41, schrieb Paolo Bonzini: >>>>> Add coroutine support for flush and apply the same emulation that >>>>> we already do for read/write. bdrv_aio_flush is simplified to always >>>>> go through a coroutine. >>>>> >>>>> Signed-off-by: Paolo Bonzini<pbonz...@redhat.com> >>>> >>>> To make the implementation more consistent with read/write operations, >>>> wouldn't it make sense to provide a bdrv_co_flush() globally instead of >>>> using the synchronous version as the preferred public interface? >>> >> What I was thinking of looks a bit different: >> >> bdrv_flush >> -> create coroutine or just fast-path to bdrv_flush_co_entry >> -> bdrv_flush_co_entry >> -> bdrv_co_flush >> >> and >> >> bdrv_co_flush >> -> driver >> >> And the reason for this is that bdrv_co_flush would be a function that >> does only little more than passing the function to the driver (just like >> most bdrv_* functions do), with no emulation going on at all. > > It would still host the checks on BDRV_O_NO_FLUSH and bs->drv->*_flush. > It would be the same as bdrv_flush_co_entry is now, minus the > marshalling in/out of the RwCo.
Right. By the way, I like how you handle all three backends in the same function. I think this is a lot more readable than the solution used by read/write (changing the function pointers on driver registration). >> Instead of taking a void* and working on a RwCo structure that is really >> meant for emulation, bdrv_co_flush would take a BlockDriverState and >> improve readability this way. > > I see. Yeah, that's doable, but I'd still need two coroutines (one for > bdrv_flush, one for bdrv_aio_flush) and the patch would be bigger overall... You already have two of them (bdrv_co_flush for AIO and bdrv_flush_co_entry for synchronous), so I don't think it makes a difference there. >> The more complicated and ugly code would be left separated and only used >> for emulation. I think that would make it easier to understand the >> common path without being distracted by emulation code. > > ... and on the other hand the length of the call chain would increse. > It easily gets confusing, it already is for me in the read/write case. Well, depends on what you're looking at. The call chain length would increase for AIO and synchronous bdrv_flush, but it would become shorter for bdrv_co_flush. If we want to declare coroutines as the preferred interface, I think such a change makes sense. > Would bdrv_co_flush be static or not? If not, you also get an > additional entry point of dubious additional value, i.e. more complexity. I think I would make it public. The one that has to go eventually is the synchronous bdrv_flush(). Which is another reason why I wouldn't design everything around it. >> Actually, I'm not so sure about qemu-img. I think we have thought of >> scenarios where converting it to a coroutine based version with a main >> loop would be helpful (can't remember the details, though). > > qemu-img convert might benefit from multiple in-flight requests if on of > the endpoints is remote or perhaps even sparse, I guess. Quite possible, yes. Kevin
