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 <f...@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
