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
