Am 12.03.2018 um 12:10 hat Paolo Bonzini geschrieben: > On 12/02/2018 15:50, Paolo Bonzini wrote: > > On 12/02/2018 15:48, Kevin Wolf wrote: > >> Am 12.02.2018 um 15:32 hat Paolo Bonzini geschrieben: > >>> Okay, we are in agreement about this and you expressed very well why I > >>> (at the gut feeling level) didn't like the old op blockers. But you > >>> bypassed the real question, which is: should I send a pull request for > >>> these two patches or not? :) > >> I didn't spell it out that explicitly, but this is essentially a NACK. > >> I'd very much prefer if you could replace it with the proper solution. > >> Of course, we can always make exceptions when there is a good reason, > >> but with 2.12 still two months away, I doubt we have one. > > Ok, I don't mind explicitness. I'll keep these two patches in the queue > > for now. > > It's now one month away. Regarding the solution below: > > > I propose a new BLK_PERM_BYPASS that allows its users to bypass the > > block layer I/O functions. In other words, bdrv_aio_ioctl() would > > require that you got this permission. A dirty bitmap would keep a > > BdrvChild with perm=0, shared=BLK_PERM_ALL & ~BLK_PERM_BYPASS, so you > > can never have a dirty bitmap and a device using ioctls attached to the > > BDS at the same time. > > I suppose it would be like: > > - scsi-block/scsi-generic call blk_set_perm with perm == shared == > BLK_PERM_BYPASS
perm = BLK_PERM_BYPASS is fine, but for shared it seems overly restrictive. I don't think the device minds another user accessing the device. Other block devices do this in blkconf_apply_backend_options(): shared_perm = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED | BLK_PERM_GRAPH_MOD; if (resizable) { shared_perm |= BLK_PERM_RESIZE; } if (conf->share_rw) { shared_perm |= BLK_PERM_WRITE; } I suppose scsi-generic is never resizable, so that part can go away, but we do have a share-rw qdev property that can be used. > - 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. Then you don't need to manually call bdrv_get_cumulative_perm(), because the permission check is included when you attach the BdrvChild. > Anything I'm missing? Ideally, bdrv_co_ioctl() should take a BdrvChild instead of a BDS and assert that the caller correctly requested the permission: assert(child->perm & BLK_PERM_BYPASS); Kevin