Re: [PATCH] scsi_lib: increase {host|target|device}_busy count after dispatch cmd
Hi, Bart:
2018-03-02 7:11 GMT+08:00 Bart Van Assche <bart.vanass...@wdc.com>:
> On Mon, 2017-06-05 at 17:37 +0800, Ganesh Mahendran wrote:
>> In android system, when there are lots of threads running. Thread A
>> holding *host_busy* count is easily to be preempted, and if at the
>> same time, thread B set *host_blocked*, then all other threads will
>> be io blocked.
>
> Hello Ganesh,
>
> Have you considered to insert preempt_disable() and preempt_enable() calls
> where necessary to achieve the same effect? I think that would result in a
> much less intrusive patch.
Yes, preempt_disable()preempt_enable will also achieve the same effect.
But I just think preempt_disable()preempt_enable may be a little heavy for
this problem which can be fixed by increaseing {host|target|device}_busy count
after dispatch cmd.
Thanks.
>
> Thanks,
>
> Bart.
>
>
Re: [PATCH] scsi_lib: increase {host|target|device}_busy count after dispatch cmd
Ping~ Willing to hear some feed back :-)
Thanks
2017-06-05 17:37 GMT+08:00 Ganesh Mahendran <opensource.gan...@gmail.com>:
> In android system, when there are lots of threads running. Thread A
> holding *host_busy* count is easily to be preempted, and if at the
> same time, thread B set *host_blocked*, then all other threads will
> be io blocked.
>
> Below the detail:
> 1). Thread A calls scsi_request_fn() and it increases *host_busy*.
> But soon it is preempted.
> 2). Thread B call scsi_request_fn(), and it got failure from
> scsi_dispatch_cmd(). So it set *host_blocked*
> 3). All the io blocked...
> 4). Thread A is scheduled again, and it decreases *host_busy*
> in scsi_device_unbusy()
>
> Afer step 2), all the io will be blocked, since scsi_host_queue_ready()
> will always return 0.
>
> scsi_host_queue_ready
> {
> if (atomic_read(>host_blocked) > 0) {
> if (busy) ==> true after step 2
> goto starved;
> }
>
>
> The system will be unblocked after step 4).
>
> This patch increases {host|target|device}_busy count after dispatch cmd.
>
> Signed-off-by: Ganesh Mahendran <opensource.gan...@gmail.com>
> ---
> drivers/scsi/scsi_lib.c | 66
> -
> 1 file changed, 32 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 884aaa8..9cac272 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -311,6 +311,16 @@ static void scsi_init_cmd_errh(struct scsi_cmnd *cmd)
> cmd->cmd_len = scsi_command_size(cmd->cmnd);
> }
>
> +static void scsi_device_busy(struct scsi_device *sdev)
> +{
> + struct Scsi_Host *shost = sdev->host;
> + struct scsi_target *starget = scsi_target(sdev);
> +
> + atomic_inc(>device_busy);
> + atomic_inc(>host_busy);
> + atomic_inc(>target_busy);
> +}
> +
> void scsi_device_unbusy(struct scsi_device *sdev)
> {
> struct Scsi_Host *shost = sdev->host;
> @@ -1352,12 +1362,13 @@ static void scsi_unprep_fn(struct request_queue *q,
> struct request *req)
> static inline int scsi_dev_queue_ready(struct request_queue *q,
> struct scsi_device *sdev)
> {
> + int ret = 0;
> unsigned int busy;
>
> - busy = atomic_inc_return(>device_busy) - 1;
> + busy = atomic_read(>device_busy);
> if (atomic_read(>device_blocked)) {
> if (busy)
> - goto out_dec;
> + goto out;
>
> /*
> * unblock after device_blocked iterates to zero
> @@ -1368,19 +1379,18 @@ static inline int scsi_dev_queue_ready(struct
> request_queue *q,
> */
> if (!q->mq_ops)
> blk_delay_queue(q, SCSI_QUEUE_DELAY);
> - goto out_dec;
> + goto out;
> }
> SCSI_LOG_MLQUEUE(3, sdev_printk(KERN_INFO, sdev,
>"unblocking device at zero depth\n"));
> }
>
> if (busy >= sdev->queue_depth)
> - goto out_dec;
> + goto out;
>
> - return 1;
> -out_dec:
> - atomic_dec(>device_busy);
> - return 0;
> + ret = 1;
> +out:
> + return ret;
> }
>
> /*
> @@ -1407,7 +1417,7 @@ static inline int scsi_target_queue_ready(struct
> Scsi_Host *shost,
> if (starget->can_queue <= 0)
> return 1;
>
> - busy = atomic_inc_return(>target_busy) - 1;
> + busy = atomic_read(>target_busy);
> if (atomic_read(>target_blocked) > 0) {
> if (busy)
> goto starved;
> @@ -1416,7 +1426,7 @@ static inline int scsi_target_queue_ready(struct
> Scsi_Host *shost,
> * unblock after target_blocked iterates to zero
> */
> if (atomic_dec_return(>target_blocked) > 0)
> - goto out_dec;
> + goto out;
>
> SCSI_LOG_MLQUEUE(3, starget_printk(KERN_INFO, starget,
> "unblocking target at zero depth\n"));
> @@ -1431,9 +1441,7 @@ static inline int scsi_target_queue_ready(struct
> Scsi_Host *shost,
> spin_lock_irq(shost->host_lock);
> list_move_tail(>starved_entry, >starved_list);
> spin_unlock_irq(shost->host_lock);
> -out_de
[PATCH] scsi_lib: increase {host|target|device}_busy count after dispatch cmd
In android system, when there are lots of threads running. Thread A
holding *host_busy* count is easily to be preempted, and if at the
same time, thread B set *host_blocked*, then all other threads will
be io blocked.
Below the detail:
1). Thread A calls scsi_request_fn() and it increases *host_busy*.
But soon it is preempted.
2). Thread B call scsi_request_fn(), and it got failure from
scsi_dispatch_cmd(). So it set *host_blocked*
3). All the io blocked...
4). Thread A is scheduled again, and it decreases *host_busy*
in scsi_device_unbusy()
Afer step 2), all the io will be blocked, since scsi_host_queue_ready()
will always return 0.
scsi_host_queue_ready
{
if (atomic_read(>host_blocked) > 0) {
if (busy) ==> true after step 2
goto starved;
}
The system will be unblocked after step 4).
This patch increases {host|target|device}_busy count after dispatch cmd.
Signed-off-by: Ganesh Mahendran <opensource.gan...@gmail.com>
---
drivers/scsi/scsi_lib.c | 66 -
1 file changed, 32 insertions(+), 34 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 884aaa8..9cac272 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -311,6 +311,16 @@ static void scsi_init_cmd_errh(struct scsi_cmnd *cmd)
cmd->cmd_len = scsi_command_size(cmd->cmnd);
}
+static void scsi_device_busy(struct scsi_device *sdev)
+{
+ struct Scsi_Host *shost = sdev->host;
+ struct scsi_target *starget = scsi_target(sdev);
+
+ atomic_inc(>device_busy);
+ atomic_inc(>host_busy);
+ atomic_inc(>target_busy);
+}
+
void scsi_device_unbusy(struct scsi_device *sdev)
{
struct Scsi_Host *shost = sdev->host;
@@ -1352,12 +1362,13 @@ static void scsi_unprep_fn(struct request_queue *q,
struct request *req)
static inline int scsi_dev_queue_ready(struct request_queue *q,
struct scsi_device *sdev)
{
+ int ret = 0;
unsigned int busy;
- busy = atomic_inc_return(>device_busy) - 1;
+ busy = atomic_read(>device_busy);
if (atomic_read(>device_blocked)) {
if (busy)
- goto out_dec;
+ goto out;
/*
* unblock after device_blocked iterates to zero
@@ -1368,19 +1379,18 @@ static inline int scsi_dev_queue_ready(struct
request_queue *q,
*/
if (!q->mq_ops)
blk_delay_queue(q, SCSI_QUEUE_DELAY);
- goto out_dec;
+ goto out;
}
SCSI_LOG_MLQUEUE(3, sdev_printk(KERN_INFO, sdev,
"unblocking device at zero depth\n"));
}
if (busy >= sdev->queue_depth)
- goto out_dec;
+ goto out;
- return 1;
-out_dec:
- atomic_dec(>device_busy);
- return 0;
+ ret = 1;
+out:
+ return ret;
}
/*
@@ -1407,7 +1417,7 @@ static inline int scsi_target_queue_ready(struct
Scsi_Host *shost,
if (starget->can_queue <= 0)
return 1;
- busy = atomic_inc_return(>target_busy) - 1;
+ busy = atomic_read(>target_busy);
if (atomic_read(>target_blocked) > 0) {
if (busy)
goto starved;
@@ -1416,7 +1426,7 @@ static inline int scsi_target_queue_ready(struct
Scsi_Host *shost,
* unblock after target_blocked iterates to zero
*/
if (atomic_dec_return(>target_blocked) > 0)
- goto out_dec;
+ goto out;
SCSI_LOG_MLQUEUE(3, starget_printk(KERN_INFO, starget,
"unblocking target at zero depth\n"));
@@ -1431,9 +1441,7 @@ static inline int scsi_target_queue_ready(struct
Scsi_Host *shost,
spin_lock_irq(shost->host_lock);
list_move_tail(>starved_entry, >starved_list);
spin_unlock_irq(shost->host_lock);
-out_dec:
- if (starget->can_queue > 0)
- atomic_dec(>target_busy);
+out:
return 0;
}
@@ -1451,7 +1459,7 @@ static inline int scsi_host_queue_ready(struct
request_queue *q,
if (scsi_host_in_recovery(shost))
return 0;
- busy = atomic_inc_return(>host_busy) - 1;
+ busy = atomic_read(>host_busy);
if (atomic_read(>host_blocked) > 0) {
if (busy)
goto starved;
@@ -1460,7 +1468,7 @@ static inline int scsi_host_queue_ready(struct
request_queue *q,
* unblock after host_blocked iterates to zero
*/
if (atomic_dec_return(>host_blocked) > 0)
-
