Units can be passed to lprocfs_write_frac_u64_helper() via a suffix
(e.g., "...K", "...M", etc.) tacked onto the value. A comment states
that "specified units override the multiplier," though the multiplier is
overridden regardless. Update the conditional logic so that it only
applies when units are specified.
Signed-off-by: Chris Rorvick <[email protected]>
---
drivers/staging/lustre/lustre/obdclass/lprocfs_status.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
index 61e04af..92ed0a0 100644
--- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
+++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
@@ -1910,7 +1910,7 @@ int lprocfs_write_frac_u64_helper(const char *buffer, unsigned long count,
units <<= 10;
}
/* Specified units override the multiplier */
- if (units)
+ if (units > 1)
mult = mult < 0 ? -units : units;
frac *= mult;
--
2.1.0
On Mon, Dec 15, 2014 at 11:41:49PM -0600, Chris Rorvick wrote:
> Units can be passed to lprocfs_write_frac_u64_helper() via a suffix
> (e.g., "...K", "...M", etc.) tacked onto the value. A comment states
> that "specified units override the multiplier," though the multiplier is
> overridden regardless. Update the conditional logic so that it only
> applies when units are specified.
>
That introduces a bug. We need to take the initial '-' into
consideration. Just remove the condition. Also remove the "mult"
parameter since that is always 1.
bool negative = false;
...
if (*pbuf == '-') {
negative = true;
pbuf++;
}
...
mult = negative ? -units : units;
regards,
dan carpenter
On 2014/12/16, 2:41 AM, "Dan Carpenter" <[email protected]> wrote:
>On Mon, Dec 15, 2014 at 11:41:49PM -0600, Chris Rorvick wrote:
>> Units can be passed to lprocfs_write_frac_u64_helper() via a suffix
>> (e.g., "...K", "...M", etc.) tacked onto the value. A comment states
>> that "specified units override the multiplier," though the multiplier is
>> overridden regardless. Update the conditional logic so that it only
>> applies when units are specified.
>>
>
>That introduces a bug. We need to take the initial '-' into
>consideration. Just remove the condition. Also remove the "mult"
>parameter since that is always 1.
>
> bool negative = false;
>
> ...
>
> if (*pbuf == '-') {
> negative = true;
> pbuf++;
> }
>
> ...
>
> mult = negative ? -units : units;
Sorry, that isn't right. Chris' patch is actually doing the right thing
to check for units > 1. The proposed change above discards "mult"
entirely, which breaks the users of this function that are not in this
file (e.g. osc_cached_mb_seq_write() or ll_max_cached_mb_seq_write())
that have tunables in units of MB by default, but can also use parameters
with units like "4.5G" for convenience.
Cheers, Andreas
--
Andreas Dilger
Lustre Software Architect
Intel High Performance Data Division
On Tue, Dec 16, 2014 at 11:14:35AM +0000, Dilger, Andreas wrote:
>
> Sorry, that isn't right. Chris' patch is actually doing the right thing
> to check for units > 1.
It's not right because it discards the negative.
> The proposed change above discards "mult"
> entirely, which breaks the users of this function that are not in this
> file (e.g. osc_cached_mb_seq_write() or ll_max_cached_mb_seq_write())
> that have tunables in units of MB by default, but can also use parameters
> with units like "4.5G" for convenience.
I think you are confusing lprocfs_write_frac_helper() and
lprocfs_write_frac_u64_helper(). There is only one caller for this
function.
! grep lprocfs_write_frac_u64_helper drivers/staging/lustre/ -R | grep -v smatch | grep -v '\.i:'
drivers/staging/lustre/lustre/obdclass/lprocfs_status.c: return lprocfs_write_frac_u64_helper(buffer, count, val, 1);
drivers/staging/lustre/lustre/obdclass/lprocfs_status.c:int lprocfs_write_frac_u64_helper(const char *buffer, unsigned long count,
drivers/staging/lustre/lustre/obdclass/lprocfs_status.c:EXPORT_SYMBOL(lprocfs_write_frac_u64_helper);
drivers/staging/lustre/lustre/include/lprocfs_status.h:extern int lprocfs_write_frac_u64_helper(const char *buffer,
regards,
dan carpenter
On Tue, Dec 16, 2014 at 5:35 AM, Dan Carpenter <[email protected]> wrote:
> On Tue, Dec 16, 2014 at 11:14:35AM +0000, Dilger, Andreas wrote:
> >
> > Sorry, that isn't right. Chris' patch is actually doing the right thing
> > to check for units > 1.
>
> It's not right because it discards the negative.
I don't think this patch introduces a bug. If anything, it was already
there. It looked to me like the value passed in to `mult' was assumed
to be positive and was simply being used as a flag to indicate whether
`buffer' started with a '-' when units were passed.
For example, say the value passed in is "-2K" and the `mult' is 1. The
check for '-' will negate `mult' making it -1. Then the units
conditional will override mult with `-units' (i.e., -1024.)
Now say we pass "-2" with `mult' equal to 1024. The result is same, but
the path is a bit different. `mult' will again be negated due to
`buffer' beginning with '-', but then it will be left alone at the units
check.
In both of the above cases the negative sign is properly accounted for.
> > The proposed change above discards "mult"
> > entirely, which breaks the users of this function that are not in this
> > file (e.g. osc_cached_mb_seq_write() or ll_max_cached_mb_seq_write())
> > that have tunables in units of MB by default, but can also use parameters
> > with units like "4.5G" for convenience.
>
> I think you are confusing lprocfs_write_frac_helper() and
> lprocfs_write_frac_u64_helper(). There is only one caller for this
> function.
By this logic, lprocfs_write_frac_u64_helper() should just be removed
and it's code should be folded into lprocfs_write_u64_helper(), no?
Regards,
Chris
On Tue, Dec 16, 2014 at 06:53:19AM -0600, Chris Rorvick wrote:
> On Tue, Dec 16, 2014 at 5:35 AM, Dan Carpenter <[email protected]> wrote:
> > On Tue, Dec 16, 2014 at 11:14:35AM +0000, Dilger, Andreas wrote:
> > >
> > > Sorry, that isn't right. Chris' patch is actually doing the right thing
> > > to check for units > 1.
> >
> > It's not right because it discards the negative.
>
> I don't think this patch introduces a bug. If anything, it was already
> there.
The original code may be totally buggy. Who knows? Why are we passing
negative numbers here anyway instead of just returning -EINVAL? But the
new code is also buggy and not consistent with itself.
In the original code if the user data is "-1k" or "-1024" that was
treated the same. In the new code, "-1k" means negative 1024 because
the user supplies units but "-1024" means positive 1024 because there
are no units given.
> > > The proposed change above discards "mult"
> > > entirely, which breaks the users of this function that are not in this
> > > file (e.g. osc_cached_mb_seq_write() or ll_max_cached_mb_seq_write())
> > > that have tunables in units of MB by default, but can also use parameters
> > > with units like "4.5G" for convenience.
> >
> > I think you are confusing lprocfs_write_frac_helper() and
> > lprocfs_write_frac_u64_helper(). There is only one caller for this
> > function.
>
> By this logic, lprocfs_write_frac_u64_helper() should just be removed
> and it's code should be folded into lprocfs_write_u64_helper(), no?
>
There are vast swathes of lustre code which need to be deleted but I
haven't looked at this one. Probably.
regards,
dan carpenter
Oh wait... You're right. This doesn't change the code how the code
works. My bad.
Still, it's better to just remove the condition instead of making the
condition even more complicated and confusing.
regards,
dan carpenter