Re: [PATCH RFC v2 1/5] block: add bitmap-populate job

[Kevin Wolf]

Am 16.06.2020 um 22:02 hat Eric Blake geschrieben:
> On 6/16/20 2:46 PM, Eric Blake wrote:
> 
> > > > +BlockJob *bitpop_job_create(
> > 
> > > > +Â Â Â  if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
> > > > +Â Â Â Â Â Â Â  return NULL;
> > > > +Â Â Â  }
> > > 
> > > What does this protect? And why does BACKUP_SOURCE describe acccurately
> > > what this job does?
> > 
> > I'm less certain what the BLOCK_OP_TYPE_* constants are supposed to
> > block, or if this is just copy/paste from backup.c. Does BlockOpType in
> > block.h need a new entry?
> 
> As it is, our code base has slowly moved away from op_blockers.

Yes, this is true. We're now trying to express conflicts with the
permission system instead.

> We no longer have any explicit bdrv_op_block() except when blocking
> everything, then immediately followed up with unblocking a mere subset
> of all of the defined op types:

I believe we never had any other pattern because nobody ever could be
bothered to think about specific conflicts. If we had one example of
conflicting uses, we just blocked everything and only unblocked cases
when we needed them (usually not really knowing whether they were really
always safe). So op blocker are almost always overblocking, but
sometimes too permissive, too.

This is why I was asking what we actually protect against here.

> 
> block.c:    bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_COMMIT_TARGET,
> block.c:    bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_STREAM,
> block.c:    bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_BACKUP_SOURCE,
> block.c:    bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_BACKUP_TARGET,
> block/replication.c:        bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE,
> s->blocker);
> blockjob.c:    bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker);
> 
> Are we at the point where we can ditch op_blockers altogether in favor of
> the block permissions system?

I actually started some patches to remove op blockers the other day. The
tricky part is proving that each type is really unnecessary now. I only
did this for RESIZE and DATAPLANE, which seemed relatively easy to
prove, though I guess I could send at least these patches.

Kevin