storvsc uses the spinlock of the hv_device's primary channel to
serialize modifications of stor_chns[] performed by storvsc_do_io()
and storvsc_change_target_cpu(), when it could/should use a (per-)
storvsc_device spinlock: this change untangles the synchronization
mechanism for the (storvsc-specific) stor_chns[] array from the
"generic" VMBus code and data structures, clarifying the scope of
this synchronization mechanism.
Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
---
drivers/scsi/storvsc_drv.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 072ed87286578..8ff21e69a8be8 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -462,6 +462,11 @@ struct storvsc_device {
* Mask of CPUs bound to subchannels.
*/
struct cpumask alloced_cpus;
+ /*
+ * Serializes modifications of stor_chns[] from storvsc_do_io()
+ * and storvsc_change_target_cpu().
+ */
+ spinlock_t lock;
/* Used for vsc/vsp channel reset process */
struct storvsc_cmd_request init_request;
struct storvsc_cmd_request reset_request;
@@ -639,7 +644,7 @@ static void storvsc_change_target_cpu(struct vmbus_channel *channel, u32 old,
return;
/* See storvsc_do_io() -> get_og_chn(). */
- spin_lock_irqsave(&device->channel->lock, flags);
+ spin_lock_irqsave(&stor_device->lock, flags);
/*
* Determines if the storvsc device has other channels assigned to
@@ -676,7 +681,7 @@ static void storvsc_change_target_cpu(struct vmbus_channel *channel, u32 old,
WRITE_ONCE(stor_device->stor_chns[new], channel);
cpumask_set_cpu(new, &stor_device->alloced_cpus);
- spin_unlock_irqrestore(&device->channel->lock, flags);
+ spin_unlock_irqrestore(&stor_device->lock, flags);
}
static void handle_sc_creation(struct vmbus_channel *new_sc)
@@ -1433,14 +1438,14 @@ static int storvsc_do_io(struct hv_device *device,
}
}
} else {
- spin_lock_irqsave(&device->channel->lock, flags);
+ spin_lock_irqsave(&stor_device->lock, flags);
outgoing_channel = stor_device->stor_chns[q_num];
if (outgoing_channel != NULL) {
- spin_unlock_irqrestore(&device->channel->lock, flags);
+ spin_unlock_irqrestore(&stor_device->lock, flags);
goto found_channel;
}
outgoing_channel = get_og_chn(stor_device, q_num);
- spin_unlock_irqrestore(&device->channel->lock, flags);
+ spin_unlock_irqrestore(&stor_device->lock, flags);
}
found_channel:
@@ -1881,6 +1886,7 @@ static int storvsc_probe(struct hv_device *device,
init_waitqueue_head(&stor_device->waiting_to_drain);
stor_device->device = device;
stor_device->host = host;
+ spin_lock_init(&stor_device->lock);
hv_set_drvdata(device, stor_device);
stor_device->port_number = host->host_no;
--
2.25.1
From: Andrea Parri (Microsoft) <[email protected]> Sent: Wednesday, June 17, 2020 9:47 AM
>
> storvsc uses the spinlock of the hv_device's primary channel to
> serialize modifications of stor_chns[] performed by storvsc_do_io()
> and storvsc_change_target_cpu(), when it could/should use a (per-)
> storvsc_device spinlock: this change untangles the synchronization
> mechanism for the (storvsc-specific) stor_chns[] array from the
> "generic" VMBus code and data structures, clarifying the scope of
> this synchronization mechanism.
>
> Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
> ---
> drivers/scsi/storvsc_drv.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
FWIW, this patch will need to get routed to the SCSI maintainers and go through
the SCSI tree.
Reviewed-by: Michael Kelley <[email protected]>
Cc SCSI maintainers
This patch should go via the hyperv tree because a later patch is
dependent on it. It requires and ack from SCSI maintainers though.
Thanks,
Wei.
On Wed, Jun 17, 2020 at 06:46:41PM +0200, Andrea Parri (Microsoft) wrote:
> storvsc uses the spinlock of the hv_device's primary channel to
> serialize modifications of stor_chns[] performed by storvsc_do_io()
> and storvsc_change_target_cpu(), when it could/should use a (per-)
> storvsc_device spinlock: this change untangles the synchronization
> mechanism for the (storvsc-specific) stor_chns[] array from the
> "generic" VMBus code and data structures, clarifying the scope of
> this synchronization mechanism.
>
> Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
> ---
> drivers/scsi/storvsc_drv.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 072ed87286578..8ff21e69a8be8 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -462,6 +462,11 @@ struct storvsc_device {
> * Mask of CPUs bound to subchannels.
> */
> struct cpumask alloced_cpus;
> + /*
> + * Serializes modifications of stor_chns[] from storvsc_do_io()
> + * and storvsc_change_target_cpu().
> + */
> + spinlock_t lock;
> /* Used for vsc/vsp channel reset process */
> struct storvsc_cmd_request init_request;
> struct storvsc_cmd_request reset_request;
> @@ -639,7 +644,7 @@ static void storvsc_change_target_cpu(struct vmbus_channel *channel, u32 old,
> return;
>
> /* See storvsc_do_io() -> get_og_chn(). */
> - spin_lock_irqsave(&device->channel->lock, flags);
> + spin_lock_irqsave(&stor_device->lock, flags);
>
> /*
> * Determines if the storvsc device has other channels assigned to
> @@ -676,7 +681,7 @@ static void storvsc_change_target_cpu(struct vmbus_channel *channel, u32 old,
> WRITE_ONCE(stor_device->stor_chns[new], channel);
> cpumask_set_cpu(new, &stor_device->alloced_cpus);
>
> - spin_unlock_irqrestore(&device->channel->lock, flags);
> + spin_unlock_irqrestore(&stor_device->lock, flags);
> }
>
> static void handle_sc_creation(struct vmbus_channel *new_sc)
> @@ -1433,14 +1438,14 @@ static int storvsc_do_io(struct hv_device *device,
> }
> }
> } else {
> - spin_lock_irqsave(&device->channel->lock, flags);
> + spin_lock_irqsave(&stor_device->lock, flags);
> outgoing_channel = stor_device->stor_chns[q_num];
> if (outgoing_channel != NULL) {
> - spin_unlock_irqrestore(&device->channel->lock, flags);
> + spin_unlock_irqrestore(&stor_device->lock, flags);
> goto found_channel;
> }
> outgoing_channel = get_og_chn(stor_device, q_num);
> - spin_unlock_irqrestore(&device->channel->lock, flags);
> + spin_unlock_irqrestore(&stor_device->lock, flags);
> }
>
> found_channel:
> @@ -1881,6 +1886,7 @@ static int storvsc_probe(struct hv_device *device,
> init_waitqueue_head(&stor_device->waiting_to_drain);
> stor_device->device = device;
> stor_device->host = host;
> + spin_lock_init(&stor_device->lock);
> hv_set_drvdata(device, stor_device);
>
> stor_device->port_number = host->host_no;
> --
> 2.25.1
>
On Fri, Jun 19, 2020 at 04:01:36PM +0000, Wei Liu wrote:
> Cc SCSI maintainers
>
> This patch should go via the hyperv tree because a later patch is
> dependent on it. It requires and ack from SCSI maintainers though.
Right. Sorry for the Cc: omission... ;-/
SCSI maintainers, please let me know if you prefer me to send you a new
series with this patch (7/8) and the later/dependent hyperv patch (8/8).
(1-6/8 of this series are hyperv-specific only and have been applied to
the hyperv tree, so this would only 7-8/8 of this series out.)
Thanks,
Andrea
Andrea,
>> This patch should go via the hyperv tree because a later patch is
>> dependent on it. It requires and ack from SCSI maintainers though.
Looks OK to me.
Acked-by: Martin K. Petersen <[email protected]>
--
Martin K. Petersen Oracle Linux Engineering
On Fri, Jun 19, 2020 at 10:58:40PM -0400, Martin K. Petersen wrote:
>
> Andrea,
>
> >> This patch should go via the hyperv tree because a later patch is
> >> dependent on it. It requires and ack from SCSI maintainers though.
>
> Looks OK to me.
>
> Acked-by: Martin K. Petersen <[email protected]>
Thanks Martin.
>
> --
> Martin K. Petersen Oracle Linux Engineering