Re: [Qemu-devel] [PATCH 2/2] scsi: add block job opblockers for scsi-block

[Kevin Wolf]

Am 12.02.2018 um 15:00 hat Paolo Bonzini geschrieben:
> On 12/02/2018 14:52, Kevin Wolf wrote:
> > Am 08.02.2018 um 11:42 hat Paolo Bonzini geschrieben:
> >> On 08/02/2018 02:35, Fam Zheng wrote:
> >>> On Wed, 02/07 17:36, Paolo Bonzini wrote:
> >>>> @@ -2626,6 +2656,36 @@ static void scsi_block_realize(SCSIDevice *dev, 
> >>>> Error **errp)
> >>>>  
> >>>>      scsi_realize(&s->qdev, errp);
> >>>>      scsi_generic_read_device_identification(&s->qdev);
> >>>> +
> >>>> +    /* For op blockers, due to lack of support for dirty bitmaps.  */
> >>>> +    error_setg(&sb->mirror_source,
> >>>> +               "scsi-block does not support acting as a mirroring 
> >>>> source");
> >>>> +    error_setg(&sb->commit_source,
> >>>> +               "scsi-block does not support acting as an active commit 
> >>>> source");
> >>>
> >>> An alternative way would be adding BLOCK_OP_TYPE_DIRTY_BITMAP. The error 
> >>> message
> >>> will not be as nice but it can be useful for another (blockjob) operation 
> >>> that
> >>> requires dirty bitmap support, or another device that doesn't support 
> >>> dirty
> >>> bitmaps. Though there isn't one for now.
> >>
> >> Yeah, I thought about it.  Another possibility is make BLOCK_OP_TYPE_* a
> >> bitmask.  Then you can easily add a single Error * for multiple
> >> blockers, and BLOCK_OP_TYPE_DIRTY_BITMAP can be defined as
> >> BLOCK_OP_TYPE_MIRROR_SOURCE|BLOCK_OP_TYPE_COMMIT_SOURCE; likewise for
> >> notifiers below.
> > 
> > We shouldn't be adding new instances of BLOCK_OP_* at all. I couldn't
> > find the time yet to remove the existing ones, but any new protections
> > should be using the permission system.
> 
> I agree.  But does this include not fixing bugs wherever clients are
> using the old op blockers?

I'm not saying that we shouldn't fix the bug, just that we should fix it
properly with the best infrastructure we have.

The old op blockers are "fixing" the problem at the symptom level, and
you have to check for each high-level operation if it does something
problematic internally. You have to repeat this analysis every time you
add a new operation or modifiy an existing one (which noone ever does).
The risk that this breaks sooner or later is pretty high.

The new permission system, on the other hand, directly addresses the
root cause, and any new feature that uses dirty bitmaps will then
automatically get the protection, too.

So in fact, I would say that the bug isn't really fixed (but at best
papered over) until we add a proper fix on the permission level.

Kevin