Re: [PATCH v11 04/13] copy-on-read: pass overlay base node name to COR driver

[Vladimir Sementsov-Ogievskiy]

14.10.2020 14:57, Max Reitz wrote:
> On 14.10.20 13:09, Max Reitz wrote:
> > On 12.10.20 19:43, Andrey Shinkevich wrote:
> > > We are going to use the COR-filter for a block-stream job.
> > > To limit COR operations by the base node in the backing chain during
> > > stream job, pass the name of overlay base node to the copy-on-read
> > > driver as base node itself may change due to possible concurrent jobs.
> > > The rest of the functionality will be implemented in the patch that
> > > follows.
> > >
> > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > > ---
> > >   block/copy-on-read.c | 14 ++++++++++++++
> > >   1 file changed, 14 insertions(+)
> >
> > Is there a reason why you didn’t add this option to QAPI (as part of a
> > yet-to-be-created BlockdevOptionsCor)?  Because I’d really like it there.
> >
> > > diff --git a/block/copy-on-read.c b/block/copy-on-read.c
> > > index bcccf0f..c578b1b 100644
> > > --- a/block/copy-on-read.c
> > > +++ b/block/copy-on-read.c
> > > @@ -24,19 +24,24 @@
> > >   #include "block/block_int.h"
> > >   #include "qemu/module.h"
> > >   #include "qapi/error.h"
> > > +#include "qapi/qmp/qerror.h"
> > >   #include "qapi/qmp/qdict.h"
> > >   #include "block/copy-on-read.h"
> > >   
> > >   
> > >   typedef struct BDRVStateCOR {
> > >       bool active;
> > > +    BlockDriverState *base_overlay;
> > >   } BDRVStateCOR;
> > >   
> > >   
> > >   static int cor_open(BlockDriverState *bs, QDict *options, int flags,
> > >                       Error **errp)
> > >   {
> > > +    BlockDriverState *base_overlay = NULL;
> > >       BDRVStateCOR *state = bs->opaque;
> > > +    /* We need the base overlay node rather than the base itself */
> > > +    const char *base_overlay_node = qdict_get_try_str(options, "base");
> >
> > Shouldn’t it be called base-overlay or above-base then?
>
> While reviewing patch 5, I realized this sould indeed be base-overlay
> (i.e. the next allocation-bearing layer above the base, not just a
> filter on top of base), but that should be noted somewhere, of course.
> Best would be the QAPI documentation for this option. O:)

What about naming it just "bottom" or "bottom-node"? If we don't have base, it's strange to have something 
"relative to base". And we can document, that "bottom" must be a non-filter node in a backing chain of 
"top".


--
Best regards,
Vladimir