Re: [Qemu-devel] [PATCH v3 04/11] quorum: Fix close path

[Max Reitz]

On 2015-02-25 at 02:12, Fam Zheng wrote:
> On Tue, 02/24 10:35, Max Reitz wrote:
> > bdrv_unref() can lead to bdrv_close(), which in turn will result in
> > bdrv_drain_all(). This function will later be called blk_drain_all() and
> > iterate only over the BlockBackends for which blk_is_inserted() holds
> > true; therefore, bdrv_is_inserted() and thus quorum_is_inserted() will
> > probably be called.
> >
> > This patch makes quorum_is_inserted() aware of the fact that some
> > children may have been closed already.
> >
> > Signed-off-by: Max Reitz <address@hidden>
> > Reviewed-by: Eric Blake <address@hidden>
> > ---
> >   block/quorum.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/quorum.c b/block/quorum.c
> > index 7a75cea..5ae2398 100644
> > --- a/block/quorum.c
> > +++ b/block/quorum.c
> > @@ -1005,6 +1005,7 @@ static void quorum_close(BlockDriverState *bs)
> >   
> >       for (i = 0; i < s->num_children; i++) {
> >           bdrv_unref(s->bs[i]);
> > +        s->bs[i] = NULL;
> Is quorum_is_inserted called by bdrv_close in this bdrv_unref?

Maybe. In any case, the BDS will still be valid at that point (it's 
called through bdrv_drain_all() which is called from bdrv_close() before 
it did any modifications to the BDS; also note that this 
bdrv_drain_all() is actually supposed to affect the BDS being closed 
since bdrv_make_anon() is called only afterwards).

In effect it probably doesn't really matter whether to do the 
bdrv_unref() before or after s->bs[i] has been reset. If it is done 
before, quorum_is_inserted() will return true for the first BDS being 
closed (and false for all the others) which most probably won't do 
anything; if it is done after, quorum_is_inserted() will return false 
every time.

Max

> If yes, I think
> unsetting s->bs[i] should happen before bdrv_unref(), so quorum_is_inserted
> won't count this one again.
>
> Fam
>
> >       }
> >   
> >       g_free(s->bs);
> > @@ -1070,7 +1071,7 @@ static bool quorum_is_inserted(BlockDriverState *bs)
> >       int i;
> >   
> >       for (i = 0; i < s->num_children; i++) {
> > -        if (!bdrv_is_inserted(s->bs[i])) {
> > +        if (s->bs[i] && !bdrv_is_inserted(s->bs[i])) {
> >               return false;
> >           }
> >       }
> > --
> > 2.1.0