[PATCH][next] loop: Fix missing max_active argument in alloc_workqueue call

[PATCH][next] loop: Fix missing max_active argument in alloc_workqueue call

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

The 3rd argument to alloc_workqueue should be the max_active count,
however currently it is the lo->lo_number that is intended for the
loop%d number. Fix this by adding in the missing max_active count.

Addresses-Coverity: ("Missing argument to printf")
Fixes: 08ad7f822739 ("loop: Use worker per cgroup instead of kworker")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/block/loop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index f2f9e4127847..ee2a6c1bc093 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1192,7 +1192,7 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
 	lo->workqueue = alloc_workqueue("loop%d",
 					WQ_UNBOUND | WQ_FREEZABLE |
 					WQ_MEM_RECLAIM,
-					lo->lo_number);
+					1, lo->lo_number);
 	if (!lo->workqueue) {
 		error = -ENOMEM;
 		goto out_unlock;
-- 
2.30.2

Re: [PATCH][next] loop: Fix missing max_active argument in alloc_workqueue call

[Muhammad Usama Anjum]

On Thu, 2021-03-18 at 15:16 +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> The 3rd argument to alloc_workqueue should be the max_active count,
> however currently it is the lo->lo_number that is intended for the
> loop%d number. Fix this by adding in the missing max_active count.
> 
> Addresses-Coverity: ("Missing argument to printf")
> Fixes: 08ad7f822739 ("loop: Use worker per cgroup instead of kworker")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/block/loop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index f2f9e4127847..ee2a6c1bc093 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1192,7 +1192,7 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
>  	lo->workqueue = alloc_workqueue("loop%d",
>  					WQ_UNBOUND | WQ_FREEZABLE |
>  					WQ_MEM_RECLAIM,
> -					lo->lo_number);
> +					1, lo->lo_number);
>  	if (!lo->workqueue) {
>  		error = -ENOMEM;
>  		goto out_unlock;

Nice catch.

Reviewed-by: Muhammad Usama Anjum <musamaanjum@gmail.com>

Re: [PATCH][next] loop: Fix missing max_active argument in alloc_workqueue call

[Jens Axboe]

On 3/18/21 9:16 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> The 3rd argument to alloc_workqueue should be the max_active count,
> however currently it is the lo->lo_number that is intended for the
> loop%d number. Fix this by adding in the missing max_active count.

Dan, please fold this (or something similar) in when you're redoing the
series.

-- 
Jens Axboe

Re: [PATCH][next] loop: Fix missing max_active argument in alloc_workqueue call

[Colin Ian King]

On 18/03/2021 20:12, Jens Axboe wrote:
> On 3/18/21 9:16 AM, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> The 3rd argument to alloc_workqueue should be the max_active count,
>> however currently it is the lo->lo_number that is intended for the
>> loop%d number. Fix this by adding in the missing max_active count.
> 
> Dan, please fold this (or something similar) in when you're redoing the
> series.
> 
Appreciate this fix being picked up. Are we going to lose the SoB?

Colin

Re: [PATCH][next] loop: Fix missing max_active argument in alloc_workqueue call

[Jens Axboe]

On 3/18/21 2:24 PM, Colin Ian King wrote:
> On 18/03/2021 20:12, Jens Axboe wrote:
>> On 3/18/21 9:16 AM, Colin King wrote:
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> The 3rd argument to alloc_workqueue should be the max_active count,
>>> however currently it is the lo->lo_number that is intended for the
>>> loop%d number. Fix this by adding in the missing max_active count.
>>
>> Dan, please fold this (or something similar) in when you're redoing the
>> series.
>>
> Appreciate this fix being picked up. Are we going to lose the SoB?

If it's being redone, would be silly to have that error in there. Do
we have a tag that's appropriate for this? I often wonder when I'm
folding in a fix. Ala Fixes-by: or something like that.

-- 
Jens Axboe

Re: [PATCH][next] loop: Fix missing max_active argument in alloc_workqueue call

[Dan Carpenter]

On Thu, Mar 18, 2021 at 02:42:33PM -0600, Jens Axboe wrote:
> On 3/18/21 2:24 PM, Colin Ian King wrote:
> > On 18/03/2021 20:12, Jens Axboe wrote:
> >> On 3/18/21 9:16 AM, Colin King wrote:
> >>> From: Colin Ian King <colin.king@canonical.com>
> >>>
> >>> The 3rd argument to alloc_workqueue should be the max_active count,
> >>> however currently it is the lo->lo_number that is intended for the
> >>> loop%d number. Fix this by adding in the missing max_active count.
> >>
> >> Dan, please fold this (or something similar) in when you're redoing the
> >> series.
> >>
> > Appreciate this fix being picked up. Are we going to lose the SoB?
> 
> If it's being redone, would be silly to have that error in there. Do
> we have a tag that's appropriate for this? I often wonder when I'm
> folding in a fix. Ala Fixes-by: or something like that.

I've always lobied for a Fixes-from: tag, but the kbuild-bot tells
everyone to add a Reported-by: tag.  But then a lot of people are like
Reported-by doesn't make sense.  And other people are like Reported-by
is fine, what's wrong with it?

regards,
dan carpenter

Re: [PATCH][next] loop: Fix missing max_active argument in alloc_workqueue call

[Krzysztof Kozlowski]

On 19/03/2021 10:47, Dan Carpenter wrote:
> On Thu, Mar 18, 2021 at 02:42:33PM -0600, Jens Axboe wrote:
>> On 3/18/21 2:24 PM, Colin Ian King wrote:
>>> On 18/03/2021 20:12, Jens Axboe wrote:
>>>> On 3/18/21 9:16 AM, Colin King wrote:
>>>>> From: Colin Ian King <colin.king@canonical.com>
>>>>>
>>>>> The 3rd argument to alloc_workqueue should be the max_active count,
>>>>> however currently it is the lo->lo_number that is intended for the
>>>>> loop%d number. Fix this by adding in the missing max_active count.
>>>>
>>>> Dan, please fold this (or something similar) in when you're redoing the
>>>> series.
>>>>
>>> Appreciate this fix being picked up. Are we going to lose the SoB?
>>
>> If it's being redone, would be silly to have that error in there. Do
>> we have a tag that's appropriate for this? I often wonder when I'm
>> folding in a fix. Ala Fixes-by: or something like that.
> 
> I've always lobied for a Fixes-from: tag, but the kbuild-bot tells
> everyone to add a Reported-by: tag.  But then a lot of people are like
> Reported-by doesn't make sense.  And other people are like Reported-by
> is fine, what's wrong with it?

If the original commit is a fix and the fix for it is being squashed,
then Reported-by might mislead.

kbuild-bot tests also patches from list directly, so in such case the
patch can be re-done with a risk of loosing kbuild's credits. But when
the patch is already in the maintainer tree - just create a fixup. You
preserve the development history and the kbuild's credits.


Best regards,
Krzysztof

Re: [PATCH][next] loop: Fix missing max_active argument in alloc_workqueue call

[Jens Axboe]

On 3/19/21 3:47 AM, Dan Carpenter wrote:
> On Thu, Mar 18, 2021 at 02:42:33PM -0600, Jens Axboe wrote:
>> On 3/18/21 2:24 PM, Colin Ian King wrote:
>>> On 18/03/2021 20:12, Jens Axboe wrote:
>>>> On 3/18/21 9:16 AM, Colin King wrote:
>>>>> From: Colin Ian King <colin.king@canonical.com>
>>>>>
>>>>> The 3rd argument to alloc_workqueue should be the max_active count,
>>>>> however currently it is the lo->lo_number that is intended for the
>>>>> loop%d number. Fix this by adding in the missing max_active count.
>>>>
>>>> Dan, please fold this (or something similar) in when you're redoing the
>>>> series.
>>>>
>>> Appreciate this fix being picked up. Are we going to lose the SoB?
>>
>> If it's being redone, would be silly to have that error in there. Do
>> we have a tag that's appropriate for this? I often wonder when I'm
>> folding in a fix. Ala Fixes-by: or something like that.
> 
> I've always lobied for a Fixes-from: tag, but the kbuild-bot tells
> everyone to add a Reported-by: tag.  But then a lot of people are like
> Reported-by doesn't make sense.  And other people are like Reported-by
> is fine, what's wrong with it?

I don't reported-by for this use either, as that is a lot more
appropriate for a single fix that fixes an issue that was reported by
(duh) that specific person.

Fixes-from seems a lot better.

-- 
Jens Axboe

Re: [PATCH][next] loop: Fix missing max_active argument in alloc_workqueue call

[Krzysztof Kozlowski]

On 18/03/2021 21:42, Jens Axboe wrote:
> On 3/18/21 2:24 PM, Colin Ian King wrote:
>> On 18/03/2021 20:12, Jens Axboe wrote:
>>> On 3/18/21 9:16 AM, Colin King wrote:
>>>> From: Colin Ian King <colin.king@canonical.com>
>>>>
>>>> The 3rd argument to alloc_workqueue should be the max_active count,
>>>> however currently it is the lo->lo_number that is intended for the
>>>> loop%d number. Fix this by adding in the missing max_active count.
>>>
>>> Dan, please fold this (or something similar) in when you're redoing the
>>> series.
>>>
>> Appreciate this fix being picked up. Are we going to lose the SoB?
> 
> If it's being redone, would be silly to have that error in there. Do
> we have a tag that's appropriate for this? I often wonder when I'm
> folding in a fix. Ala Fixes-by: or something like that.

Why it is being redone if it was put into next? And even then, several
other maintainers just apply a fix on top (I think Andrew Morton, Greg
KH, Mark Brown) to avoid rebasing, preserve the history and also give
credits to the fixer.

Anyway, if it is going to be squashed at least SoB would be nice (as Dan
will take Colin's code).

Best regards,
Krzysztof

Re: [PATCH][next] loop: Fix missing max_active argument in alloc_workqueue call

[Jens Axboe]

On 3/19/21 3:59 AM, Krzysztof Kozlowski wrote:
> On 18/03/2021 21:42, Jens Axboe wrote:
>> On 3/18/21 2:24 PM, Colin Ian King wrote:
>>> On 18/03/2021 20:12, Jens Axboe wrote:
>>>> On 3/18/21 9:16 AM, Colin King wrote:
>>>>> From: Colin Ian King <colin.king@canonical.com>
>>>>>
>>>>> The 3rd argument to alloc_workqueue should be the max_active count,
>>>>> however currently it is the lo->lo_number that is intended for the
>>>>> loop%d number. Fix this by adding in the missing max_active count.
>>>>
>>>> Dan, please fold this (or something similar) in when you're redoing the
>>>> series.
>>>>
>>> Appreciate this fix being picked up. Are we going to lose the SoB?
>>
>> If it's being redone, would be silly to have that error in there. Do
>> we have a tag that's appropriate for this? I often wonder when I'm
>> folding in a fix. Ala Fixes-by: or something like that.
> 
> Why it is being redone if it was put into next? And even then, several
> other maintainers just apply a fix on top (I think Andrew Morton, Greg
> KH, Mark Brown) to avoid rebasing, preserve the history and also give
> credits to the fixer.

linux-next doesn't have continual history, and I rebase my for-next
all the time. Since the series was going to be re-done and applied to
a different tree even, it would be silly to retain a bug _just_ so that
we can have credits to the fixer separately. For this case, it's
rebased anyway, and there's honestly not any history to preserve here.
The only downside is losing the fixer attribution, which I do agree is
annoying and hence why I asked/lobbied in a fixes-by kind of tag.

-- 
Jens Axboe

Re: [PATCH][next] loop: Fix missing max_active argument in alloc_workqueue call

[Dan Schatzberg]

On Thu, Mar 18, 2021 at 02:12:10PM -0600, Jens Axboe wrote:
> On 3/18/21 9:16 AM, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> > 
> > The 3rd argument to alloc_workqueue should be the max_active count,
> > however currently it is the lo->lo_number that is intended for the
> > loop%d number. Fix this by adding in the missing max_active count.
> 
> Dan, please fold this (or something similar) in when you're redoing the
> series.
> 
> -- 
> Jens Axboe
> 

Will do.

Re: [PATCH][next] loop: Fix missing max_active argument in alloc_workqueue call

[Dan Schatzberg]

On Thu, Mar 18, 2021 at 03:16:26PM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> The 3rd argument to alloc_workqueue should be the max_active count,
> however currently it is the lo->lo_number that is intended for the
> loop%d number. Fix this by adding in the missing max_active count.
> 

Thanks for catching this Colin. I'm fairly new to kernel development.
Is there some tool I could have run locally to catch this?

Re: [PATCH][next] loop: Fix missing max_active argument in alloc_workqueue call

[Colin Ian King]

On 19/03/2021 15:54, Dan Schatzberg wrote:
> On Thu, Mar 18, 2021 at 03:16:26PM +0000, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> The 3rd argument to alloc_workqueue should be the max_active count,
>> however currently it is the lo->lo_number that is intended for the
>> loop%d number. Fix this by adding in the missing max_active count.
>>
> 
> Thanks for catching this Colin. I'm fairly new to kernel development.
> Is there some tool I could have run locally to catch this?
> 
I'm using Coverity to find these issues. There is a free version [1],
but the one I use is licensed and has the scan level turned up quite
high to catch more issues than the free service.

Refs: [1] https://scan.coverity.com/projects/linux-next-weekly-scan

Colin