From: Colin Ian King <[email protected]>
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 <[email protected]>
---
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
On Thu, 2021-03-18 at 15:16 +0000, Colin King wrote:
> From: Colin Ian King <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
On 3/18/21 9:16 AM, Colin King wrote:
> From: Colin Ian King <[email protected]>
>
> 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
On 18/03/2021 20:12, Jens Axboe wrote:
> On 3/18/21 9:16 AM, Colin King wrote:
>> From: Colin Ian King <[email protected]>
>>
>> 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
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 <[email protected]>
>>>
>>> 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
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 <[email protected]>
> >>>
> >>> 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
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 <[email protected]>
>>>>
>>>> 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
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 <[email protected]>
>>>>>
>>>>> 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
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 <[email protected]>
>>>>>
>>>>> 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
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 <[email protected]>
>>>>>
>>>>> 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
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 <[email protected]>
> >
> > 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.
On Thu, Mar 18, 2021 at 03:16:26PM +0000, Colin King wrote:
> From: Colin Ian King <[email protected]>
>
> 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?
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 <[email protected]>
>>
>> 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