Re: [Qemu-devel] [PATCH v15 05/14] block: Add bdrv_set_backing_hd()

[Fam Zheng <famz@redhat.com>]

On Wed, 02/26 17:35, Jeff Cody wrote:
> On Sun, Feb 23, 2014 at 09:54:46AM +0800, Fam Zheng wrote:
> > This is the common but non-trivial steps to assign or change the
> > backing_hd of BDS.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block.c               | 46 ++++++++++++++++++++++++++++++++++++++--------
> >  include/block/block.h |  1 +
> >  2 files changed, 39 insertions(+), 8 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 684b9d6..9caade9 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1041,6 +1041,32 @@ fail:
> >      return ret;
> >  }
> >  
> > +void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
> > +{
> > +    if (backing_hd) {
> > +        /* Grab the reference before unref original backing_hd, so we are safe
> > +         * when rebasing in the backing chain.
> > +         */
> > +        bdrv_ref(backing_hd);
> 
> I think the problem is performing this bdrv_ref() makes the
> assumptions that:
> 
> A) bs->backing_hd is non-NULL, and
> B) backing_hd is currently a backing file, at some level, of
>    bs->backing_chain.
> 
> The above conditions are not always true, which is what led to my
> concerns in my previous email.  I think we could avoid the spurious
> bdrv_ref() if we check for both conditions A and B before calling
> bdrv_ref(backing_hd).
> 
> But I think there could still be a problem...
> 
> > +    }
> > +
> > +    if (bs->backing_hd) {
> > +        bdrv_unref(bs->backing_hd);
> 
> Only if conditions A and B are true would this bdrv_unref()
> potentially lead to a bdrv_unref() being called on backing_hd.
> 
> But what if the refcnt on bs->backing_hd is > 1?  Then even if
> conditions A and B are met, we still won't eventually unref 
> backing_hd, making the bdrv_ref(backing_hd) spurious.
> 
> But as I mentioned before, manually checking refcnt, or making
> assumptions on refcnt, seems very wrong.
> 
> It is almost like what is needed, are some conditional refcnt
> implementations.  Something like:
> 
>    void bdrv_cond_ref(BlockDriverState *bs_cond, BlockDriverState *bs)
> 
> That would increase the refcnt on bs_cond IFF:
> 
> 1) bs is non-NULL
> 2) bs_cond is in the backing chain of bs
> 3) bs is at risk of deletion on the next unref
> 

I see the problem, however these rules (bdrv_cond_ref) still look hard to
infer.

To keep it simple, I prefer to remove bdrv_ref/bdrv_unref in
bdrv_set_backing_hd and leave it to caller, which is the most readable I think.

Fam