Re: [PATCH 06/16] virtio_blk: drop config_enable
On Sun, 5 Oct 2014 19:07:07 +0300 Michael S. Tsirkin m...@redhat.com wrote: Now that virtio core ensures config changes don't arrive during probing, drop config_enable flag in virtio blk. On removal, flush is now sufficient to guarantee that no change work is queued. This help simplify the driver, and will allow setting DRIVER_OK earlier without losing config change notifications. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/block/virtio_blk.c | 19 ++- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 0a58140..c8cf6a1 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -772,9 +766,7 @@ static void virtblk_remove(struct virtio_device *vdev) int refc; /* Prevent config work handler from accessing the device. */ /* Common code ensures no further work will be queued. */ instead? - mutex_lock(vblk-config_lock); - vblk-config_enable = false; - mutex_unlock(vblk-config_lock); + flush_work(vblk-config_work); del_gendisk(vblk-disk); blk_cleanup_queue(vblk-disk-queue); @@ -806,10 +796,6 @@ static int virtblk_freeze(struct virtio_device *vdev) vdev-config-reset(vdev); /* Prevent config work handler from accessing the device. */ dito on the comment - mutex_lock(vblk-config_lock); - vblk-config_enable = false; - mutex_unlock(vblk-config_lock); - flush_work(vblk-config_work); blk_mq_stop_hw_queues(vblk-disk-queue); ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 06/16] virtio_blk: drop config_enable
On Mon, 6 Oct 2014 15:09:53 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Mon, Oct 06, 2014 at 01:42:29PM +0200, Cornelia Huck wrote: On Sun, 5 Oct 2014 19:07:07 +0300 Michael S. Tsirkin m...@redhat.com wrote: Now that virtio core ensures config changes don't arrive during probing, drop config_enable flag in virtio blk. On removal, flush is now sufficient to guarantee that no change work is queued. This help simplify the driver, and will allow setting DRIVER_OK earlier without losing config change notifications. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/block/virtio_blk.c | 19 ++- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 0a58140..c8cf6a1 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -772,9 +766,7 @@ static void virtblk_remove(struct virtio_device *vdev) int refc; /* Prevent config work handler from accessing the device. */ /* Common code ensures no further work will be queued. */ instead? No, I think you missed the point: this comment now refers to the flush below: flush is required to ensure work handler is not running. Agree? I think we both mean the same thing. Preventing the handler from access sounds to me more like when the handler starts running, it is prevented from accessing the device (like with setting config_enable, as the code did before). What I meant was common code has already ensured that our work-queueing function will not be called, therefore flushing the workqueue is enough. (same for net) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 06/16] virtio_blk: drop config_enable
On Mon, Oct 06, 2014 at 02:20:38PM +0200, Cornelia Huck wrote: On Mon, 6 Oct 2014 15:09:53 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Mon, Oct 06, 2014 at 01:42:29PM +0200, Cornelia Huck wrote: On Sun, 5 Oct 2014 19:07:07 +0300 Michael S. Tsirkin m...@redhat.com wrote: Now that virtio core ensures config changes don't arrive during probing, drop config_enable flag in virtio blk. On removal, flush is now sufficient to guarantee that no change work is queued. This help simplify the driver, and will allow setting DRIVER_OK earlier without losing config change notifications. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/block/virtio_blk.c | 19 ++- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 0a58140..c8cf6a1 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -772,9 +766,7 @@ static void virtblk_remove(struct virtio_device *vdev) int refc; /* Prevent config work handler from accessing the device. */ /* Common code ensures no further work will be queued. */ instead? No, I think you missed the point: this comment now refers to the flush below: flush is required to ensure work handler is not running. Agree? I think we both mean the same thing. Preventing the handler from access sounds to me more like when the handler starts running, it is prevented from accessing the device (like with setting config_enable, as the code did before). What I meant was common code has already ensured that our work-queueing function will not be called, therefore flushing the workqueue is enough. (same for net) OK so I'll rewrite this to /* Make sure no work handler is accessing the device. */ ? I prefer not duplicating core guarantees in all devices. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 06/16] virtio_blk: drop config_enable
On Mon, 6 Oct 2014 15:31:10 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Mon, Oct 06, 2014 at 02:20:38PM +0200, Cornelia Huck wrote: On Mon, 6 Oct 2014 15:09:53 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Mon, Oct 06, 2014 at 01:42:29PM +0200, Cornelia Huck wrote: On Sun, 5 Oct 2014 19:07:07 +0300 Michael S. Tsirkin m...@redhat.com wrote: Now that virtio core ensures config changes don't arrive during probing, drop config_enable flag in virtio blk. On removal, flush is now sufficient to guarantee that no change work is queued. This help simplify the driver, and will allow setting DRIVER_OK earlier without losing config change notifications. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/block/virtio_blk.c | 19 ++- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 0a58140..c8cf6a1 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -772,9 +766,7 @@ static void virtblk_remove(struct virtio_device *vdev) int refc; /* Prevent config work handler from accessing the device. */ /* Common code ensures no further work will be queued. */ instead? No, I think you missed the point: this comment now refers to the flush below: flush is required to ensure work handler is not running. Agree? I think we both mean the same thing. Preventing the handler from access sounds to me more like when the handler starts running, it is prevented from accessing the device (like with setting config_enable, as the code did before). What I meant was common code has already ensured that our work-queueing function will not be called, therefore flushing the workqueue is enough. (same for net) OK so I'll rewrite this to /* Make sure no work handler is accessing the device. */ ? I prefer not duplicating core guarantees in all devices. Sounds good to me. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 06/16] virtio_blk: drop config_enable
Now that virtio core ensures config changes don't arrive during probing, drop config_enable flag in virtio blk. On removal, flush is now sufficient to guarantee that no change work is queued. This help simplify the driver, and will allow setting DRIVER_OK earlier without losing config change notifications. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/block/virtio_blk.c | 19 ++- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 0a58140..c8cf6a1 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -44,9 +44,6 @@ struct virtio_blk /* Lock for config space updates */ struct mutex config_lock; - /* enable config space updates */ - bool config_enable; - /* What host tells us, plus 2 for header tailer. */ unsigned int sg_elems; @@ -348,8 +345,6 @@ static void virtblk_config_changed_work(struct work_struct *work) u64 capacity, size; mutex_lock(vblk-config_lock); - if (!vblk-config_enable) - goto done; /* Host must always specify the capacity. */ virtio_cread(vdev, struct virtio_blk_config, capacity, capacity); @@ -374,7 +369,7 @@ static void virtblk_config_changed_work(struct work_struct *work) set_capacity(vblk-disk, capacity); revalidate_disk(vblk-disk); kobject_uevent_env(disk_to_dev(vblk-disk)-kobj, KOBJ_CHANGE, envp); -done: + mutex_unlock(vblk-config_lock); } @@ -609,7 +604,6 @@ static int virtblk_probe(struct virtio_device *vdev) mutex_init(vblk-config_lock); INIT_WORK(vblk-config_work, virtblk_config_changed_work); - vblk-config_enable = true; err = init_vq(vblk); if (err) @@ -772,9 +766,7 @@ static void virtblk_remove(struct virtio_device *vdev) int refc; /* Prevent config work handler from accessing the device. */ - mutex_lock(vblk-config_lock); - vblk-config_enable = false; - mutex_unlock(vblk-config_lock); + flush_work(vblk-config_work); del_gendisk(vblk-disk); blk_cleanup_queue(vblk-disk-queue); @@ -784,8 +776,6 @@ static void virtblk_remove(struct virtio_device *vdev) /* Stop all the virtqueues. */ vdev-config-reset(vdev); - flush_work(vblk-config_work); - refc = atomic_read(disk_to_dev(vblk-disk)-kobj.kref.refcount); put_disk(vblk-disk); vdev-config-del_vqs(vdev); @@ -806,10 +796,6 @@ static int virtblk_freeze(struct virtio_device *vdev) vdev-config-reset(vdev); /* Prevent config work handler from accessing the device. */ - mutex_lock(vblk-config_lock); - vblk-config_enable = false; - mutex_unlock(vblk-config_lock); - flush_work(vblk-config_work); blk_mq_stop_hw_queues(vblk-disk-queue); @@ -823,7 +809,6 @@ static int virtblk_restore(struct virtio_device *vdev) struct virtio_blk *vblk = vdev-priv; int ret; - vblk-config_enable = true; ret = init_vq(vdev-priv); if (!ret) blk_mq_start_stopped_hw_queues(vblk-disk-queue, true); -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
