| Message ID | 1447108773-6836-13-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 09.11.2015 um 23:39 hat Max Reitz geschrieben: > Put the code for setting up and removing op blockers into an own > function, respectively. Then, we can invoke those functions whenever a > BDS is removed from an virtio-blk BB or inserted into it. > > Signed-off-by: Max Reitz <mreitz@redhat.com> Do you know of a case where this is observable? I don't think you can change the medium of a virtio-blk device, and blk_set_bs() isn't converted to a wrapper around blk_remove/insert_bs() yet. If this patch is necessary to fix something observable, the latter is probably a bug. > diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c > index c42ddeb..4c95d5b 100644 > --- a/hw/block/dataplane/virtio-blk.c > +++ b/hw/block/dataplane/virtio-blk.c > @@ -39,6 +39,8 @@ struct VirtIOBlockDataPlane { > EventNotifier *guest_notifier; /* irq */ > QEMUBH *bh; /* bh for guest notification */ > > + Notifier insert_notifier, remove_notifier; > + > /* Note that these EventNotifiers are assigned by value. This is > * fine as long as you do not call event_notifier_cleanup on them > * (because you don't own the file descriptor or handle; you just > @@ -137,6 +139,54 @@ static void handle_notify(EventNotifier *e) > blk_io_unplug(s->conf->conf.blk); > } > > +static void data_plane_set_up_op_blockers(VirtIOBlockDataPlane *s) > +{ > + assert(!s->blocker); > + error_setg(&s->blocker, "block device is in use by data plane"); > + blk_op_block_all(s->conf->conf.blk, s->blocker); > + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_RESIZE, s->blocker); > + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_DRIVE_DEL, s->blocker); > + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, s->blocker); > + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_CHANGE, s->blocker); > + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, s->blocker); > + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_TARGET, s->blocker); > + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EJECT, s->blocker); > + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, > + s->blocker); > + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, > + s->blocker); > + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE, > + s->blocker); > + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_MIRROR, s->blocker); > + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_STREAM, s->blocker); > + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_REPLACE, s->blocker); > +} This makes me wonder: What do we even block here any more? If I didn't miss anything, it's only BLOCK_OP_TYPE_BACKUP_TARGET, and I'm not sure why this needs to be blocked, or if we simply forgot to enable it. Kevin
On 25.11.2015 16:57, Kevin Wolf wrote: > Am 09.11.2015 um 23:39 hat Max Reitz geschrieben: >> Put the code for setting up and removing op blockers into an own >> function, respectively. Then, we can invoke those functions whenever a >> BDS is removed from an virtio-blk BB or inserted into it. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> > > Do you know of a case where this is observable? Actually, no. > I don't think you can > change the medium of a virtio-blk device, and blk_set_bs() isn't > converted to a wrapper around blk_remove/insert_bs() yet. If this patch > is necessary to fix something observable, the latter is probably a bug. So I guess that implies "Otherwise, this patch should be dropped"? >> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c >> index c42ddeb..4c95d5b 100644 >> --- a/hw/block/dataplane/virtio-blk.c >> +++ b/hw/block/dataplane/virtio-blk.c >> @@ -39,6 +39,8 @@ struct VirtIOBlockDataPlane { >> EventNotifier *guest_notifier; /* irq */ >> QEMUBH *bh; /* bh for guest notification */ >> >> + Notifier insert_notifier, remove_notifier; >> + >> /* Note that these EventNotifiers are assigned by value. This is >> * fine as long as you do not call event_notifier_cleanup on them >> * (because you don't own the file descriptor or handle; you just >> @@ -137,6 +139,54 @@ static void handle_notify(EventNotifier *e) >> blk_io_unplug(s->conf->conf.blk); >> } >> >> +static void data_plane_set_up_op_blockers(VirtIOBlockDataPlane *s) >> +{ >> + assert(!s->blocker); >> + error_setg(&s->blocker, "block device is in use by data plane"); >> + blk_op_block_all(s->conf->conf.blk, s->blocker); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_RESIZE, s->blocker); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_DRIVE_DEL, s->blocker); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, s->blocker); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_CHANGE, s->blocker); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, s->blocker); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_TARGET, s->blocker); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EJECT, s->blocker); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, >> + s->blocker); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, >> + s->blocker); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE, >> + s->blocker); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_MIRROR, s->blocker); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_STREAM, s->blocker); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_REPLACE, s->blocker); >> +} > > This makes me wonder: What do we even block here any more? If I didn't > miss anything, it's only BLOCK_OP_TYPE_BACKUP_TARGET, and I'm not sure > why this needs to be blocked, or if we simply forgot to enable it. Well, even though in practice this wall of code doesn't make much sense, of course it will be safe for potential additions of new op blockers. And of course we actually don't want these blockers at all anymore... Max
Am 25.11.2015 um 17:03 hat Max Reitz geschrieben: > On 25.11.2015 16:57, Kevin Wolf wrote: > > Am 09.11.2015 um 23:39 hat Max Reitz geschrieben: > >> Put the code for setting up and removing op blockers into an own > >> function, respectively. Then, we can invoke those functions whenever a > >> BDS is removed from an virtio-blk BB or inserted into it. > >> > >> Signed-off-by: Max Reitz <mreitz@redhat.com> > > > > Do you know of a case where this is observable? > > Actually, no. > > > I don't think you can > > change the medium of a virtio-blk device, and blk_set_bs() isn't > > converted to a wrapper around blk_remove/insert_bs() yet. If this patch > > is necessary to fix something observable, the latter is probably a bug. > > So I guess that implies "Otherwise, this patch should be dropped"? I'm not sure. I guess you had a reason to include these patches other than putting the notifiers to use? With blk_set_bs() changed, I think it would have an effect: The op blockers would move from the old BDS to the new top-level one. This sounds desirable to me. > >> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c > >> index c42ddeb..4c95d5b 100644 > >> --- a/hw/block/dataplane/virtio-blk.c > >> +++ b/hw/block/dataplane/virtio-blk.c > >> @@ -39,6 +39,8 @@ struct VirtIOBlockDataPlane { > >> EventNotifier *guest_notifier; /* irq */ > >> QEMUBH *bh; /* bh for guest notification */ > >> > >> + Notifier insert_notifier, remove_notifier; > >> + > >> /* Note that these EventNotifiers are assigned by value. This is > >> * fine as long as you do not call event_notifier_cleanup on them > >> * (because you don't own the file descriptor or handle; you just > >> @@ -137,6 +139,54 @@ static void handle_notify(EventNotifier *e) > >> blk_io_unplug(s->conf->conf.blk); > >> } > >> > >> +static void data_plane_set_up_op_blockers(VirtIOBlockDataPlane *s) > >> +{ > >> + assert(!s->blocker); > >> + error_setg(&s->blocker, "block device is in use by data plane"); > >> + blk_op_block_all(s->conf->conf.blk, s->blocker); > >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_RESIZE, s->blocker); > >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_DRIVE_DEL, s->blocker); > >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, s->blocker); > >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_CHANGE, s->blocker); > >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, s->blocker); > >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_TARGET, s->blocker); > >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EJECT, s->blocker); > >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, > >> + s->blocker); > >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, > >> + s->blocker); > >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE, > >> + s->blocker); > >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_MIRROR, s->blocker); > >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_STREAM, s->blocker); > >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_REPLACE, s->blocker); > >> +} > > > > This makes me wonder: What do we even block here any more? If I didn't > > miss anything, it's only BLOCK_OP_TYPE_BACKUP_TARGET, and I'm not sure > > why this needs to be blocked, or if we simply forgot to enable it. > > Well, even though in practice this wall of code doesn't make much sense, > of course it will be safe for potential additions of new op blockers. > > And of course we actually don't want these blockers at all anymore... Yes, but dataplane shouldn't really be special enough any more that we want to disable features for it initially. By now it sounds more like an easy way to forget unblocking a new feature even though it would work. So perhaps we should really just remove the blockers from dataplane. Then we don't have to answer the question above... Kevin
On 25.11.2015 17:18, Kevin Wolf wrote: > Am 25.11.2015 um 17:03 hat Max Reitz geschrieben: >> On 25.11.2015 16:57, Kevin Wolf wrote: >>> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben: >>>> Put the code for setting up and removing op blockers into an own >>>> function, respectively. Then, we can invoke those functions whenever a >>>> BDS is removed from an virtio-blk BB or inserted into it. >>>> >>>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>> >>> Do you know of a case where this is observable? >> >> Actually, no. >> >>> I don't think you can >>> change the medium of a virtio-blk device, and blk_set_bs() isn't >>> converted to a wrapper around blk_remove/insert_bs() yet. If this patch >>> is necessary to fix something observable, the latter is probably a bug. >> >> So I guess that implies "Otherwise, this patch should be dropped"? > > I'm not sure. I guess you had a reason to include these patches other > than putting the notifiers to use? I'm not sure, it has been so long. :-) I can very well imagine having included it because a similar change is necessary to virtio-scsi, so I included it just because I was already doing work for virtio. Maybe I imagined that virtio-blk may reasonably offer tray devices in the future, but I just now read you saying somewhere else: > Please write your code so that it makes sense today. So that doesn't hold up. :-) Maybe I just saw it unblocking the EJECT op blocker, and so I thought there might be a reason for that. > With blk_set_bs() changed, I think it would have an effect: The op > blockers would move from the old BDS to the new top-level one. This > sounds desirable to me. Hm, thanks for saving me. Yes, that seems useful indeed. [...] >>> This makes me wonder: What do we even block here any more? If I didn't >>> miss anything, it's only BLOCK_OP_TYPE_BACKUP_TARGET, and I'm not sure >>> why this needs to be blocked, or if we simply forgot to enable it. >> >> Well, even though in practice this wall of code doesn't make much sense, >> of course it will be safe for potential additions of new op blockers. >> >> And of course we actually don't want these blockers at all anymore... > > Yes, but dataplane shouldn't really be special enough any more that we > want to disable features for it initially. By now it sounds more like an > easy way to forget unblocking a new feature even though it would work. > > So perhaps we should really just remove the blockers from dataplane. > Then we don't have to answer the question above... Well, maybe. I guess this is up to Stefan. Max
On Wed, Nov 25, 2015 at 05:26:02PM +0100, Max Reitz wrote: > On 25.11.2015 17:18, Kevin Wolf wrote: > > Am 25.11.2015 um 17:03 hat Max Reitz geschrieben: > >> On 25.11.2015 16:57, Kevin Wolf wrote: > >>> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben: > >>> This makes me wonder: What do we even block here any more? If I didn't > >>> miss anything, it's only BLOCK_OP_TYPE_BACKUP_TARGET, and I'm not sure > >>> why this needs to be blocked, or if we simply forgot to enable it. > >> > >> Well, even though in practice this wall of code doesn't make much sense, > >> of course it will be safe for potential additions of new op blockers. > >> > >> And of course we actually don't want these blockers at all anymore... > > > > Yes, but dataplane shouldn't really be special enough any more that we > > want to disable features for it initially. By now it sounds more like an > > easy way to forget unblocking a new feature even though it would work. > > > > So perhaps we should really just remove the blockers from dataplane. > > Then we don't have to answer the question above... > > Well, maybe. I guess this is up to Stefan. At this point blockdev.c and block jobs acquire/release AioContext, hence all these op blockers are being unblocked. I think we can switch from whitelisting (unblocking) nearly everything to blacklisting (blocking) only things that aren't supported yet. Stefan
Am 26.11.2015 um 08:48 hat Stefan Hajnoczi geschrieben: > On Wed, Nov 25, 2015 at 05:26:02PM +0100, Max Reitz wrote: > > On 25.11.2015 17:18, Kevin Wolf wrote: > > > Am 25.11.2015 um 17:03 hat Max Reitz geschrieben: > > >> On 25.11.2015 16:57, Kevin Wolf wrote: > > >>> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben: > > >>> This makes me wonder: What do we even block here any more? If I didn't > > >>> miss anything, it's only BLOCK_OP_TYPE_BACKUP_TARGET, and I'm not sure > > >>> why this needs to be blocked, or if we simply forgot to enable it. > > >> > > >> Well, even though in practice this wall of code doesn't make much sense, > > >> of course it will be safe for potential additions of new op blockers. > > >> > > >> And of course we actually don't want these blockers at all anymore... > > > > > > Yes, but dataplane shouldn't really be special enough any more that we > > > want to disable features for it initially. By now it sounds more like an > > > easy way to forget unblocking a new feature even though it would work. > > > > > > So perhaps we should really just remove the blockers from dataplane. > > > Then we don't have to answer the question above... > > > > Well, maybe. I guess this is up to Stefan. > > At this point blockdev.c and block jobs acquire/release AioContext, > hence all these op blockers are being unblocked. I think we can switch > from whitelisting (unblocking) nearly everything to blacklisting > (blocking) only things that aren't supported yet. As I said, there is only one operation that isn't whitelisted today, BLOCK_OP_TYPE_BACKUP_TARGET, and I would be surprised if it weren't just a bug that it's not whitelisted yet. Kevin
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c index c42ddeb..4c95d5b 100644 --- a/hw/block/dataplane/virtio-blk.c +++ b/hw/block/dataplane/virtio-blk.c @@ -39,6 +39,8 @@ struct VirtIOBlockDataPlane { EventNotifier *guest_notifier; /* irq */ QEMUBH *bh; /* bh for guest notification */ + Notifier insert_notifier, remove_notifier; + /* Note that these EventNotifiers are assigned by value. This is * fine as long as you do not call event_notifier_cleanup on them * (because you don't own the file descriptor or handle; you just @@ -137,6 +139,54 @@ static void handle_notify(EventNotifier *e) blk_io_unplug(s->conf->conf.blk); } +static void data_plane_set_up_op_blockers(VirtIOBlockDataPlane *s) +{ + assert(!s->blocker); + error_setg(&s->blocker, "block device is in use by data plane"); + blk_op_block_all(s->conf->conf.blk, s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_RESIZE, s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_DRIVE_DEL, s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_CHANGE, s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_TARGET, s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EJECT, s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, + s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, + s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE, + s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_MIRROR, s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_STREAM, s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_REPLACE, s->blocker); +} + +static void data_plane_remove_op_blockers(VirtIOBlockDataPlane *s) +{ + if (s->blocker) { + blk_op_unblock_all(s->conf->conf.blk, s->blocker); + error_free(s->blocker); + s->blocker = NULL; + } +} + +static void data_plane_blk_insert_notifier(Notifier *n, void *data) +{ + VirtIOBlockDataPlane *s = container_of(n, VirtIOBlockDataPlane, + insert_notifier); + assert(s->conf->conf.blk == data); + data_plane_set_up_op_blockers(s); +} + +static void data_plane_blk_remove_notifier(Notifier *n, void *data) +{ + VirtIOBlockDataPlane *s = container_of(n, VirtIOBlockDataPlane, + remove_notifier); + assert(s->conf->conf.blk == data); + data_plane_remove_op_blockers(s); +} + /* Context: QEMU global mutex held */ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf, VirtIOBlockDataPlane **dataplane, @@ -193,22 +243,12 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf, s->ctx = iothread_get_aio_context(s->iothread); s->bh = aio_bh_new(s->ctx, notify_guest_bh, s); - error_setg(&s->blocker, "block device is in use by data plane"); - blk_op_block_all(conf->conf.blk, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_RESIZE, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_DRIVE_DEL, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_CHANGE, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_COMMIT_TARGET, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_EJECT, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE, - s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_MIRROR, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_STREAM, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_REPLACE, s->blocker); + s->insert_notifier.notify = data_plane_blk_insert_notifier; + s->remove_notifier.notify = data_plane_blk_remove_notifier; + blk_add_insert_bs_notifier(conf->conf.blk, &s->insert_notifier); + blk_add_remove_bs_notifier(conf->conf.blk, &s->remove_notifier); + + data_plane_set_up_op_blockers(s); *dataplane = s; } @@ -221,8 +261,9 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s) } virtio_blk_data_plane_stop(s); - blk_op_unblock_all(s->conf->conf.blk, s->blocker); - error_free(s->blocker); + data_plane_remove_op_blockers(s); + notifier_remove(&s->insert_notifier); + notifier_remove(&s->remove_notifier); qemu_bh_delete(s->bh); object_unref(OBJECT(s->iothread)); g_free(s);
Put the code for setting up and removing op blockers into an own function, respectively. Then, we can invoke those functions whenever a BDS is removed from an virtio-blk BB or inserted into it. Signed-off-by: Max Reitz <mreitz@redhat.com> --- hw/block/dataplane/virtio-blk.c | 77 +++++++++++++++++++++++++++++++---------- 1 file changed, 59 insertions(+), 18 deletions(-)