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

[Kevin Wolf]

Am 05.04.2018 um 13:59 hat Paolo Bonzini geschrieben:
> On 12/03/2018 12:58, Kevin Wolf wrote:
> >> - users of dirty bitmaps would call use perm/shared_perm as in your
> >> message above
> >>
> >> - dirty bitmaps creation calls bdrv_get_cumulative_perm (which should
> >> now become public) and checks that it doesn't have BLK_PERM_BYPASS in
> >> shared_perm
> > 
> > My proposal was really that users of dirty bitmaps don't change
> > anything, but we do everything in the dirty bitmap implementation. Dirty
> > bitmap creation would add a BdrvChild with the above permissions.
> > Deleting a dirty bitmap would remove the BdrvChild again.
> 
> This is also better because it works if somebody requests
> BLK_PERM_BYPASS after dirty bitmap creation.
> 
> However, is there any better way than also creating a dummy BlockDriver
> and BlockDriverState?  I first thought of a root role similar to
> BlockBackend's, but BdrvChildRole doesn't have a way to inject its own
> permissions.  I then tried moving bdrv_child_perm to BdrvChildRole, and
> that almost works except that child_backing has special requirements
> (mostly due to commit_top and mirror_top's special block drivers).

Have a look at block_job_add_bdrv(), which does the same thing to
restrict permissions while a block job is working on the subchain.

Essentially, yes, you'll probably have a new BdrvChildRole (I suppose
with .stay_at_node = true and .get_parent_desc only), but the place
where you specify the permissions is the bdrv_root_attach_child() call.
When you're done, you call bdrv_root_unref_child().

Kevin

> Perhaps child_perm should be in BdrvChildRole and BlockDriverState
> should only have a bdrv_get_backing_perm (called by
> child_backing.child_perm).  This makes sense to me since those
> permissions are specific to the driver, e.g. whether it has metadata at
> all.  But this becomes 2.13 material.
> 
> Do you still object to the two opblocker patches?
> 
> Paolo