| Message ID | 20180207163622.29935-3-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: add block job opblockers for scsi-block | expand |
On Wed, 02/07 17:36, Paolo Bonzini wrote: > scsi-block bypasses the dirty bitmaps and pre-write notifiers, so it > cannot be the source of a block job. The gist of the fix is to add > op-blockers to the BlockBackend, and remove them at "unrealize" time, > but things are a little more complex because quit closes the BlockBackend > without going through unrealize. > > So use Notifiers: the remove_bs notifier is called by bdrv_close_all, and > the insert_bs notifier might not be really necessary but make things a > little more symmetric. > > Suggested-by: Karen Noel <knoel@redhat.com> :) > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Fam Zheng <famz@redhat.com> Though I have one comment below. > --- > block/block-backend.c | 9 ++++++ > hw/scsi/scsi-disk.c | 62 ++++++++++++++++++++++++++++++++++++++++++ > include/sysemu/block-backend.h | 1 + > 3 files changed, 72 insertions(+) > > diff --git a/block/block-backend.c b/block/block-backend.c > index baef8e7abc..1759639a4a 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -1747,6 +1747,15 @@ bool blk_op_is_blocked(BlockBackend *blk, BlockOpType op, Error **errp) > return bdrv_op_is_blocked(bs, op, errp); > } > > +void blk_op_block(BlockBackend *blk, BlockOpType op, Error *reason) > +{ > + BlockDriverState *bs = blk_bs(blk); > + > + if (bs) { > + bdrv_op_block(bs, op, reason); > + } > +} > + > void blk_op_unblock(BlockBackend *blk, BlockOpType op, Error *reason) > { > BlockDriverState *bs = blk_bs(blk); > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c > index 49d2559d93..023673cb04 100644 > --- a/hw/scsi/scsi-disk.c > +++ b/hw/scsi/scsi-disk.c > @@ -2578,9 +2578,39 @@ static int get_device_type(SCSIDiskState *s) > return 0; > } > > +typedef struct SCSIBlockState { > + SCSIDiskState sd; > + Error *mirror_source; > + Error *backup_source; > + Error *commit_source; > + Notifier insert_bs; > + Notifier remove_bs; > +} SCSIBlockState; > + > +static void scsi_block_insert_bs(Notifier *n, void *opaque) > +{ > + SCSIBlockState *sb = container_of(n, SCSIBlockState, insert_bs); > + SCSIDiskState *s = &sb->sd; > + > + blk_op_block(s->qdev.conf.blk, BLOCK_OP_TYPE_MIRROR_SOURCE, sb->mirror_source); > + blk_op_block(s->qdev.conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, sb->commit_source); > + blk_op_block(s->qdev.conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, sb->backup_source); > +} > + > +static void scsi_block_remove_bs(Notifier *n, void *opaque) > +{ > + SCSIBlockState *sb = container_of(n, SCSIBlockState, remove_bs); > + SCSIDiskState *s = &sb->sd; > + > + blk_op_unblock(s->qdev.conf.blk, BLOCK_OP_TYPE_MIRROR_SOURCE, sb->mirror_source); > + blk_op_unblock(s->qdev.conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, sb->commit_source); > + blk_op_unblock(s->qdev.conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, sb->backup_source); > +} > + > static void scsi_block_realize(SCSIDevice *dev, Error **errp) > { > SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev); > + SCSIBlockState *sb = DO_UPCAST(SCSIBlockState, sd, s); > int sg_version; > int rc; > > @@ -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. > + > + /* For op blockers, due to lack of support for write notifiers. */ > + error_setg(&sb->backup_source, > + "scsi-block does not support acting as a backup source"); > + > + sb->insert_bs.notify = scsi_block_insert_bs; > + blk_add_insert_bs_notifier(s->qdev.conf.blk, &sb->insert_bs); > + sb->remove_bs.notify = scsi_block_remove_bs; > + blk_add_remove_bs_notifier(s->qdev.conf.blk, &sb->remove_bs); > + > + scsi_block_insert_bs(&sb->insert_bs, s->qdev.conf.blk); > +} > + > +static void scsi_block_unrealize(SCSIDevice *dev, Error **errp) > +{ > + SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev); > + SCSIBlockState *sb = DO_UPCAST(SCSIBlockState, sd, s); > + > + notifier_remove(&sb->insert_bs); > + notifier_remove(&sb->remove_bs); > + scsi_block_remove_bs(&sb->insert_bs, s->qdev.conf.blk); > + error_free(sb->mirror_source); > + error_free(sb->commit_source); > + error_free(sb->backup_source); > } > > typedef struct SCSIBlockReq { > @@ -3017,6 +3077,7 @@ static void scsi_block_class_initfn(ObjectClass *klass, void *data) > SCSIDiskClass *sdc = SCSI_DISK_BASE_CLASS(klass); > > sc->realize = scsi_block_realize; > + sc->unrealize = scsi_block_unrealize; > sc->alloc_req = scsi_block_new_request; > sc->parse_cdb = scsi_block_parse_cdb; > sdc->dma_readv = scsi_block_dma_readv; > @@ -3031,6 +3092,7 @@ static const TypeInfo scsi_block_info = { > .name = "scsi-block", > .parent = TYPE_SCSI_DISK_BASE, > .class_init = scsi_block_class_initfn, > + .instance_size = sizeof(SCSIBlockState), > }; > #endif > > diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h > index c4e52a5fa3..a48a49ca79 100644 > --- a/include/sysemu/block-backend.h > +++ b/include/sysemu/block-backend.h > @@ -182,6 +182,7 @@ void blk_set_guest_block_size(BlockBackend *blk, int align); > void *blk_try_blockalign(BlockBackend *blk, size_t size); > void *blk_blockalign(BlockBackend *blk, size_t size); > bool blk_op_is_blocked(BlockBackend *blk, BlockOpType op, Error **errp); > +void blk_op_block(BlockBackend *blk, BlockOpType op, Error *reason); > void blk_op_unblock(BlockBackend *blk, BlockOpType op, Error *reason); > void blk_op_block_all(BlockBackend *blk, Error *reason); > void blk_op_unblock_all(BlockBackend *blk, Error *reason); > -- > 2.14.3 > > Fam
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. Paolo >> + >> + /* For op blockers, due to lack of support for write notifiers. */ >> + error_setg(&sb->backup_source, >> + "scsi-block does not support acting as a backup source"); >> + >> + sb->insert_bs.notify = scsi_block_insert_bs; >> + blk_add_insert_bs_notifier(s->qdev.conf.blk, &sb->insert_bs); >> + sb->remove_bs.notify = scsi_block_remove_bs; >> + blk_add_remove_bs_notifier(s->qdev.conf.blk, &sb->remove_bs); >> + >> + scsi_block_insert_bs(&sb->insert_bs, s->qdev.conf.blk); >> +} >> + >> +static void scsi_block_unrealize(SCSIDevice *dev, Error **errp) >> +{ >> + SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev); >> + SCSIBlockState *sb = DO_UPCAST(SCSIBlockState, sd, s); >> + >> + notifier_remove(&sb->insert_bs); >> + notifier_remove(&sb->remove_bs); >> + scsi_block_remove_bs(&sb->insert_bs, s->qdev.conf.blk); >> + error_free(sb->mirror_source); >> + error_free(sb->commit_source); >> + error_free(sb->backup_source); >> } >> >> typedef struct SCSIBlockReq { >> @@ -3017,6 +3077,7 @@ static void scsi_block_class_initfn(ObjectClass *klass, void *data) >> SCSIDiskClass *sdc = SCSI_DISK_BASE_CLASS(klass); >> >> sc->realize = scsi_block_realize; >> + sc->unrealize = scsi_block_unrealize; >> sc->alloc_req = scsi_block_new_request; >> sc->parse_cdb = scsi_block_parse_cdb; >> sdc->dma_readv = scsi_block_dma_readv; >> @@ -3031,6 +3092,7 @@ static const TypeInfo scsi_block_info = { >> .name = "scsi-block", >> .parent = TYPE_SCSI_DISK_BASE, >> .class_init = scsi_block_class_initfn, >> + .instance_size = sizeof(SCSIBlockState), >> }; >> #endif >> >> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h >> index c4e52a5fa3..a48a49ca79 100644 >> --- a/include/sysemu/block-backend.h >> +++ b/include/sysemu/block-backend.h >> @@ -182,6 +182,7 @@ void blk_set_guest_block_size(BlockBackend *blk, int align); >> void *blk_try_blockalign(BlockBackend *blk, size_t size); >> void *blk_blockalign(BlockBackend *blk, size_t size); >> bool blk_op_is_blocked(BlockBackend *blk, BlockOpType op, Error **errp); >> +void blk_op_block(BlockBackend *blk, BlockOpType op, Error *reason); >> void blk_op_unblock(BlockBackend *blk, BlockOpType op, Error *reason); >> void blk_op_block_all(BlockBackend *blk, Error *reason); >> void blk_op_unblock_all(BlockBackend *blk, Error *reason); >> -- >> 2.14.3 >> >> > > Fam >
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 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. Kevin
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? Paolo > 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.
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
On 12/02/2018 15:30, Kevin Wolf wrote: >>> 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. 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? :) Paolo
Am 12.02.2018 um 15:32 hat Paolo Bonzini geschrieben: > On 12/02/2018 15:30, Kevin Wolf wrote: > >>> 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. > > 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. Kevin
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. Paolo
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 - 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 Anything I'm missing? Paolo
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
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). 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
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
diff --git a/block/block-backend.c b/block/block-backend.c index baef8e7abc..1759639a4a 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -1747,6 +1747,15 @@ bool blk_op_is_blocked(BlockBackend *blk, BlockOpType op, Error **errp) return bdrv_op_is_blocked(bs, op, errp); } +void blk_op_block(BlockBackend *blk, BlockOpType op, Error *reason) +{ + BlockDriverState *bs = blk_bs(blk); + + if (bs) { + bdrv_op_block(bs, op, reason); + } +} + void blk_op_unblock(BlockBackend *blk, BlockOpType op, Error *reason) { BlockDriverState *bs = blk_bs(blk); diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index 49d2559d93..023673cb04 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -2578,9 +2578,39 @@ static int get_device_type(SCSIDiskState *s) return 0; } +typedef struct SCSIBlockState { + SCSIDiskState sd; + Error *mirror_source; + Error *backup_source; + Error *commit_source; + Notifier insert_bs; + Notifier remove_bs; +} SCSIBlockState; + +static void scsi_block_insert_bs(Notifier *n, void *opaque) +{ + SCSIBlockState *sb = container_of(n, SCSIBlockState, insert_bs); + SCSIDiskState *s = &sb->sd; + + blk_op_block(s->qdev.conf.blk, BLOCK_OP_TYPE_MIRROR_SOURCE, sb->mirror_source); + blk_op_block(s->qdev.conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, sb->commit_source); + blk_op_block(s->qdev.conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, sb->backup_source); +} + +static void scsi_block_remove_bs(Notifier *n, void *opaque) +{ + SCSIBlockState *sb = container_of(n, SCSIBlockState, remove_bs); + SCSIDiskState *s = &sb->sd; + + blk_op_unblock(s->qdev.conf.blk, BLOCK_OP_TYPE_MIRROR_SOURCE, sb->mirror_source); + blk_op_unblock(s->qdev.conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, sb->commit_source); + blk_op_unblock(s->qdev.conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, sb->backup_source); +} + static void scsi_block_realize(SCSIDevice *dev, Error **errp) { SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev); + SCSIBlockState *sb = DO_UPCAST(SCSIBlockState, sd, s); int sg_version; int rc; @@ -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"); + + /* For op blockers, due to lack of support for write notifiers. */ + error_setg(&sb->backup_source, + "scsi-block does not support acting as a backup source"); + + sb->insert_bs.notify = scsi_block_insert_bs; + blk_add_insert_bs_notifier(s->qdev.conf.blk, &sb->insert_bs); + sb->remove_bs.notify = scsi_block_remove_bs; + blk_add_remove_bs_notifier(s->qdev.conf.blk, &sb->remove_bs); + + scsi_block_insert_bs(&sb->insert_bs, s->qdev.conf.blk); +} + +static void scsi_block_unrealize(SCSIDevice *dev, Error **errp) +{ + SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev); + SCSIBlockState *sb = DO_UPCAST(SCSIBlockState, sd, s); + + notifier_remove(&sb->insert_bs); + notifier_remove(&sb->remove_bs); + scsi_block_remove_bs(&sb->insert_bs, s->qdev.conf.blk); + error_free(sb->mirror_source); + error_free(sb->commit_source); + error_free(sb->backup_source); } typedef struct SCSIBlockReq { @@ -3017,6 +3077,7 @@ static void scsi_block_class_initfn(ObjectClass *klass, void *data) SCSIDiskClass *sdc = SCSI_DISK_BASE_CLASS(klass); sc->realize = scsi_block_realize; + sc->unrealize = scsi_block_unrealize; sc->alloc_req = scsi_block_new_request; sc->parse_cdb = scsi_block_parse_cdb; sdc->dma_readv = scsi_block_dma_readv; @@ -3031,6 +3092,7 @@ static const TypeInfo scsi_block_info = { .name = "scsi-block", .parent = TYPE_SCSI_DISK_BASE, .class_init = scsi_block_class_initfn, + .instance_size = sizeof(SCSIBlockState), }; #endif diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index c4e52a5fa3..a48a49ca79 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -182,6 +182,7 @@ void blk_set_guest_block_size(BlockBackend *blk, int align); void *blk_try_blockalign(BlockBackend *blk, size_t size); void *blk_blockalign(BlockBackend *blk, size_t size); bool blk_op_is_blocked(BlockBackend *blk, BlockOpType op, Error **errp); +void blk_op_block(BlockBackend *blk, BlockOpType op, Error *reason); void blk_op_unblock(BlockBackend *blk, BlockOpType op, Error *reason); void blk_op_block_all(BlockBackend *blk, Error *reason); void blk_op_unblock_all(BlockBackend *blk, Error *reason);
scsi-block bypasses the dirty bitmaps and pre-write notifiers, so it cannot be the source of a block job. The gist of the fix is to add op-blockers to the BlockBackend, and remove them at "unrealize" time, but things are a little more complex because quit closes the BlockBackend without going through unrealize. So use Notifiers: the remove_bs notifier is called by bdrv_close_all, and the insert_bs notifier might not be really necessary but make things a little more symmetric. Suggested-by: Karen Noel <knoel@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- block/block-backend.c | 9 ++++++ hw/scsi/scsi-disk.c | 62 ++++++++++++++++++++++++++++++++++++++++++ include/sysemu/block-backend.h | 1 + 3 files changed, 72 insertions(+)