Re: [PATCH 1/2] scsi: core: cleanup scsi_dev_queue_ready()

[Damien Le Moal]

On 2023/09/22 2:38, Wenchao Hao wrote:
> This is just a cleanup for scsi_dev_queue_ready() to avoid
> redundant goto and if statement, it did not change the origin
> logic.
>
> Signed-off-by: Wenchao Hao <haowe...@huawei.com>
> ---
> drivers/scsi/scsi_lib.c | 35 ++++++++++++++++++-----------------
> 1 file changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index ca5eb058d5c7..f3e388127dbd 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1254,28 +1254,29 @@ static inline int scsi_dev_queue_ready(struct request_queue *q,
> int token;
>
> token = sbitmap_get(&sdev->budget_map);
> - if (atomic_read(&sdev->device_blocked)) {
> - if (token < 0)
> - goto out;
> + if (token < 0)
> + return -1;

This is changing how this function works...

>
> - if (scsi_device_busy(sdev) > 1)
> - goto out_dec;
> + /*
> + * device_blocked is not set at mostly time, so check it first
> + * and return token when it is not set.
> + */
> + if (!atomic_read(&sdev->device_blocked))
> + return token;

...because you reversed the tests order.

>
> - /*
> - * unblock after device_blocked iterates to zero
> - */
> - if (atomic_dec_return(&sdev->device_blocked) > 0)
> - goto out_dec;
> - SCSI_LOG_MLQUEUE(3, sdev_printk(KERN_INFO, sdev,
> - "unblocking device at zero depth\n"));
> + /*
> + * unblock after device_blocked iterates to zero
> + */
> + if (scsi_device_busy(sdev) > 1 ||
> + atomic_dec_return(&sdev->device_blocked) > 0) {

And here too, you are changing how the function works. The atomic_dec may not be
done if the first condition is true.

> + sbitmap_put(&sdev->budget_map, token);
> + return -1;
> }
>
> + SCSI_LOG_MLQUEUE(3, sdev_printk(KERN_INFO, sdev,
> + "unblocking device at zero depth\n"));
> +
> return token;
> -out_dec:
> - if (token >= 0)
> - sbitmap_put(&sdev->budget_map, token);
> -out:
> - return -1;
> }
>
> /*

--
Damien Le Moal
Western Digital Research

[Wenchao Hao]

This is just a cleanup for scsi_dev_queue_ready() to avoid
redundant goto and if statement, it did not change the origin
logic.

Signed-off-by: Wenchao Hao <haowe...@huawei.com>
---
drivers/scsi/scsi_lib.c | 35 ++++++++++++++++++-----------------
1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ca5eb058d5c7..f3e388127dbd 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1254,28 +1254,29 @@ static inline int scsi_dev_queue_ready(struct request_queue *q,
int token;

token = sbitmap_get(&sdev->budget_map);
- if (atomic_read(&sdev->device_blocked)) {
- if (token < 0)
- goto out;
+ if (token < 0)
+ return -1;

- if (scsi_device_busy(sdev) > 1)
- goto out_dec;
+ /*
+ * device_blocked is not set at mostly time, so check it first
+ * and return token when it is not set.
+ */
+ if (!atomic_read(&sdev->device_blocked))
+ return token;

- /*
- * unblock after device_blocked iterates to zero
- */
- if (atomic_dec_return(&sdev->device_blocked) > 0)
- goto out_dec;
- SCSI_LOG_MLQUEUE(3, sdev_printk(KERN_INFO, sdev,
- "unblocking device at zero depth\n"));
+ /*
+ * unblock after device_blocked iterates to zero
+ */
+ if (scsi_device_busy(sdev) > 1 ||
+ atomic_dec_return(&sdev->device_blocked) > 0) {

+ sbitmap_put(&sdev->budget_map, token);
+ return -1;
}

+ SCSI_LOG_MLQUEUE(3, sdev_printk(KERN_INFO, sdev,
+ "unblocking device at zero depth\n"));
+
return token;
-out_dec:
- if (token >= 0)
- sbitmap_put(&sdev->budget_map, token);
-out:
- return -1;
}

/*
--

2.32.0

[Wenchao Hao]

This is a cleanup patchset, no logic changed.

The first patch just cleanup scsi_dev_queue_ready();
The second patch add comment for target_destroy callback of
scsi_host_template to tell it is called in atomic context.

Wenchao Hao (2):
scsi: core: cleanup scsi_dev_queue_ready()
scsi: Add comment of target_destroy in scsi_host_template

drivers/scsi/scsi_lib.c | 35 ++++++++++++++++++-----------------
include/scsi/scsi_host.h | 3 +++
2 files changed, 21 insertions(+), 17 deletions(-)

--
2.32.0

[Wenchao Hao]

Add comment to tell callback function target_destroy of
scsi_host_template is called in atomic context.

Signed-off-by: Wenchao Hao <haowe...@huawei.com>
---

include/scsi/scsi_host.h | 3 +++
1 file changed, 3 insertions(+)

diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 49f768d0ff37..a72248fa5adf 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -245,6 +245,9 @@ struct scsi_host_template {
* midlayer calls this point so that the driver may deallocate
* and terminate any references to the target.
*
+ * Note: this callback in called with spin_lock held, so donot
+ * call functions might cause schedule
+ *
* Status: OPTIONAL
*/
void (* target_destroy)(struct scsi_target *);
--
2.32.0

[Bart Van Assche]

On 9/22/23 02:38, Wenchao Hao wrote:
> Add comment to tell callback function target_destroy of
> scsi_host_template is called in atomic context.
>
> Signed-off-by: Wenchao Hao <haowe...@huawei.com>
> ---
> include/scsi/scsi_host.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 49f768d0ff37..a72248fa5adf 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -245,6 +245,9 @@ struct scsi_host_template {
> * midlayer calls this point so that the driver may deallocate
> * and terminate any references to the target.
> *
> + * Note: this callback in called with spin_lock held, so donot
> + * call functions might cause schedule
> + *

This comment should mention which spinlock is held.

Thanks,

Bart.

[Wenchao Hao]

Would update, thanks for your review suggestion.

> Thanks,
>
> Bart.
>
>