Re: [RFC PATCH 13/22] block/export: Move refcount from NBDExport to BlockExport

From: Kevin Wolf <kwolf@redhat.com>
To: Eric Blake <eblake@redhat.com>
Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com
Subject: Re: [RFC PATCH 13/22] block/export: Move refcount from NBDExport to BlockExport
Date: Thu, 20 Aug 2020 16:15:14 +0200	[thread overview]
Message-ID: <20200820141514.GF99531@linux.fritz.box> (raw)
In-Reply-To: <0ff82c83-16eb-c544-553a-9a58c9453133@redhat.com>

Am 19.08.2020 um 22:58 hat Eric Blake geschrieben:
> On 8/13/20 11:29 AM, Kevin Wolf wrote:
> > Having a refcount makes sense for all types of block exports. It is also
> > a prerequisite for keeping a list of all exports at the BlockExport
> > level.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> 
> > +++ b/include/block/export.h
> > @@ -21,14 +21,24 @@ typedef struct BlockExport BlockExport;
> >   typedef struct BlockExportDriver {
> >       BlockExportType type;
> >       BlockExport *(*create)(BlockExportOptions *, Error **);
> > +    void (*delete)(BlockExport *);
> >   } BlockExportDriver;
> >   struct BlockExport {
> >       const BlockExportDriver *drv;
> > +
> > +    /*
> > +     * Reference count for this block export. This includes strong references
> > +     * both from the owner (qemu-nbd or the monitor) and clients connected to
> > +     * the export.
> 
> I guess 'the monitor' includes qemu-storage-daemon.

Yes, qemu-storage-daemon has a QMP monitor, so I would count it there.
Even the --export command line option only calls the QMP command
internally.

> > +     */
> > +    int refcount;
> >   };
> >   extern const BlockExportDriver blk_exp_nbd;
> >   BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp);
> > +void blk_exp_ref(BlockExport *exp);
> > +void blk_exp_unref(BlockExport *exp);
> 
> Yay, I think this naming is more consistent with the rest of qemu...
> 
> >   #endif
> > diff --git a/include/block/nbd.h b/include/block/nbd.h
> > index 23030db3f1..af8509ab70 100644
> > --- a/include/block/nbd.h
> > +++ b/include/block/nbd.h
> > @@ -336,8 +336,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
> >   void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk);
> >   void nbd_export_close(NBDExport *exp);
> >   void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
> > -void nbd_export_get(NBDExport *exp);
> > -void nbd_export_put(NBDExport *exp);
> 
> ...as opposed to this which is common in kernel but less so in this project.
> No hard feelings from me :)
> 
> > +++ b/blockdev-nbd.c
> > @@ -232,7 +232,7 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
> >       /* The list of named exports has a strong reference to this export now and
> >        * our only way of accessing it is through nbd_export_find(), so we can drop
> >        * the strong reference that is @exp. */
> > -    nbd_export_put(exp);
> > +    blk_exp_unref((BlockExport*) exp);
> 
> Even a helper function that converts NBDBlockExport* to BlockExport* rather
> than a cast might be nicer, but then again, I see from Max's review that
> this may be a temporary state of things.
> (The QAPI contains such type-safe container casts, such as
> qapi_DriveBackup_base(), if that helps...)

Yes, this goes away before the end of the series.

> > @@ -1537,7 +1536,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
> >       exp = g_new0(NBDExport, 1);
> >       exp->common = (BlockExport) {
> > -        .drv = &blk_exp_nbd,
> > +        .drv        = &blk_exp_nbd,
> > +        .refcount   = 1,
> 
> I'm not sure whether trying to align the '=' is good, because the moment you
> add a longer field name, every other line has to be touched.  I'm fine with
> just one space on both side of =, even if it is more ragged to read.  But
> you're the author, so you get to pick.

I generally prefer aligned '=' because the code is read much more often
than it is written or modified, so being friendly for readers is
important.

> > @@ -1626,8 +1625,9 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
> >       exp->ctx = ctx;
> >       blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
> > +    blk_exp_ref(&exp->common);
> >       QTAILQ_INSERT_TAIL(&exports, exp, next);
> > -    nbd_export_get(exp);
> > +
> 
> Is there any consequence to this changed ordering in grabbing the reference
> vs. updating the list?

No intended consequences, but if Max is right that the code (before and
after this series) lacks some locking, it might make a theoretical
difference. If it does, the new code is safer than the old one. If it
doesn't, it's just more consistent with the order I'm used to see in
other places: First take the reference, then use it.

Kevin