| Message ID | 20210318151626.17442-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [next] loop: Fix missing max_active argument in alloc_workqueue call | expand |
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>
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.
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
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.
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
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
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
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.
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.
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.
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?
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
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;