Re: [Qemu-devel] [PATCH 18/22] vmdk: implement .bdrv_detach/attach_aio_context()

[Stefan Hajnoczi]

On Sun, May 04, 2014 at 06:17:45PM +0800, Fam Zheng wrote:
> On Thu, 05/01 16:54, Stefan Hajnoczi wrote:
> > @@ -2118,6 +2139,8 @@ static BlockDriver bdrv_vmdk = {
> >      .bdrv_has_zero_init           = vmdk_has_zero_init,
> >      .bdrv_get_specific_info       = vmdk_get_specific_info,
> >      .bdrv_refresh_limits          = vmdk_refresh_limits,
> > +    .bdrv_detach_aio_context      = vmdk_detach_aio_context,
> > +    .bdrv_attach_aio_context      = vmdk_attach_aio_context,
> >  
> >      .create_options               = vmdk_create_options,
> >  };
> > -- 
> 
> I'm wondering why we need to separate detach and attach as two functions, and
> also add bdrv_set_aio_context in block.c, instead of a single
> .bdrv_set_aio_context member which is called in bdrv_set_aio_context()? The
> latter seems less code.

I can see it working either way, but here is why I chose to keep them
separate:

The detach/attach happens in two phases:

1. Parents are detached before child nodes - just in case the parent
   still needs the child in order to detach.

2. The new AioContext is acquired and then children are attached before
   their parent nodes - that way the parent knows it can already use its
   children during attach.

Acquiring the new AioContext for the minimum amount of time (attach
only) seems like a good idea.  Remember the AioContext may be
responsible for other I/O devices too so we should minimize the scope of
acquire/release.

Doing it all in a single .bdrv_set_aio_context() forces detach to happen
while the new AioContext is held.

Another reason why separate detach/attach is nice is that it allows
block drivers to avoid code duplication.  .bdrv_open() calls attach()
and .bdrv_close() calls detach().  A single .bdrv_set_aio_context()
function would need extra code to deal with the open (currently not
attached) and close (don't attach to a new context) scenarios.

Stefan