LinuxLists.cc - [PATCH 1/2] scsi: core: cleanup scsi_dev_queue

2023-09-22 09:50:22

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

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 <[email protected]>
---
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

2023-09-22 13:07:51

by Damien Le Moal

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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

2023-09-24 06:29:38

by Wenchao Hao

[permalink] [raw]

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

On 2023/9/22 20:50, Damien Le Moal wrote:
> 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 <[email protected]>
>> ---
>> 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...
>

I don't think so...
The origin function flow:

static inline int scsi_dev_queue_ready(struct request_queue *q,
struct scsi_device *sdev)
{
...
token = sbitmap_get(&sdev->budget_map);
if (atomic_read(&sdev->device_blocked)) {
if (token < 0)
goto out;
}
return token;
out:
return -1
}

If the token is less than 0, it would always return -1. So we can found
it's not necessary to check token after atomic_read().

>>
>> - 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.

As explained in comment, the device_blocked is not set at mostly time,
so when it's not set, just return the 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) {
>
> And here too, you are changing how the function works. The atomic_dec may not be
> done if the first condition is true.
>

the origin flow would not call atomic_dec_return() too when condition
"scsi_device_busy(sdev) > 1" is true like following:

static inline int scsi_dev_queue_ready(struct request_queue *q,
struct scsi_device *sdev)
{
...
if (atomic_read(&sdev->device_blocked)) {
if (scsi_device_busy(sdev) > 1)
goto out_dec;

/*
* 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"));
}
return token;
out_dec:
if (token >= 0)
sbitmap_put(&sdev->budget_map, token);
out:
return -1;
}

Here is the function before and after my change:

The old function:
static inline int scsi_dev_queue_ready(struct request_queue *q,
struct scsi_device *sdev)
{
int token;

token = sbitmap_get(&sdev->budget_map);
if (token < 0)
return -1;

/*
* 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 (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;
}

Function after my change:

static inline int scsi_dev_queue_ready(struct request_queue *q,
struct scsi_device *sdev)
{
int token;

token = sbitmap_get(&sdev->budget_map);
if (token < 0)
return -1;

/*
* 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 (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;
}

I think it looks more clear and readable than before.

>> + 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;
>> }
>>
>> /*
>

2023-09-24 07:50:03

by Wenchao Hao

[permalink] [raw]

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

On 2023/9/24 14:27, Wenchao Hao wrote:
> On 2023/9/22 20:50, Damien Le Moal wrote:
>> 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 <[email protected]>
>>> ---
>>> 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...
>>
>
> I don't think so...
> The origin function flow:
>
> static inline int scsi_dev_queue_ready(struct request_queue *q,
>                   struct scsi_device *sdev)
> {
>     ...
>     token = sbitmap_get(&sdev->budget_map);
>     if (atomic_read(&sdev->device_blocked)) {
>         if (token < 0)
>             goto out;
>     }
>     return token;
> out:
>     return -1
> }
>
> If the token is less than 0, it would always return -1. So we can found
> it's not necessary to check token after atomic_read().
>
>>> -        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.
>
> As explained in comment, the device_blocked is not set at mostly time,
> so when it's not set, just return the 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) {
>>
>> And here too, you are changing how the function works. The atomic_dec may not be
>> done if the first condition is true.
>>
>
> the origin flow would not call atomic_dec_return() too when condition
> "scsi_device_busy(sdev) > 1" is true like following:
>
> static inline int scsi_dev_queue_ready(struct request_queue *q,
>                   struct scsi_device *sdev)
> {
>     ...
>     if (atomic_read(&sdev->device_blocked)) {
>         if (scsi_device_busy(sdev) > 1)
>             goto out_dec;
>
>         /*
>          * 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"));
>     }
>     return token;
> out_dec:
>     if (token >= 0)
>         sbitmap_put(&sdev->budget_map, token);
> out:
>     return -1;
> }
>
> Here is the function before and after my change:
>
> The old function:
> static inline int scsi_dev_queue_ready(struct request_queue *q,
>                   struct scsi_device *sdev)
> {
>     int token;
>
>     token = sbitmap_get(&sdev->budget_map);
>     if (token < 0)
>         return -1;
>
>     /*
>      * 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 (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;
> }
>

Sorry, I paste wrong for old function, the old function is:

static inline int scsi_dev_queue_ready(struct request_queue *q,
struct scsi_device *sdev)
{
int token;

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

if (scsi_device_busy(sdev) > 1)
goto out_dec;

/*
* 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"));
}

return token;
out_dec:
if (token >= 0)
sbitmap_put(&sdev->budget_map, token);
out:
return -1;
}

2023-10-11 12:38:05

by Wenchao Hao

[permalink] [raw]