qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Fam Zheng
Subject: Re: [Qemu-devel] [PATCH v15 05/14] block: Add bdrv_set_backing_hd()
Date: Fri, 7 Mar 2014 15:53:37 +0800
User-agent: Mutt/1.5.22 (2013-10-16)

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 <address@hidden>
> > ---
> >  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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]