The "perf_counter_reset" case has already been handled above.
Moreover "ORANGEFS_PARAM_REQUEST_OP_READAHEAD_COUNT_SIZE" is not a really
consistent.
It is likely that this (dead) code is a cut and paste left over.
Signed-off-by: Christophe JAILLET <[email protected]>
---
fs/orangefs/orangefs-sysfs.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/fs/orangefs/orangefs-sysfs.c b/fs/orangefs/orangefs-sysfs.c
index a799546a67f7..084954448f18 100644
--- a/fs/orangefs/orangefs-sysfs.c
+++ b/fs/orangefs/orangefs-sysfs.c
@@ -609,15 +609,6 @@ static ssize_t sysfs_service_op_store(struct kobject *kobj,
new_op->upcall.req.param.u.value32[0] = val1;
new_op->upcall.req.param.u.value32[1] = val2;
goto value_set;
- } else if (!strcmp(attr->attr.name,
- "perf_counter_reset")) {
- if ((val > 0)) {
- new_op->upcall.req.param.op =
- ORANGEFS_PARAM_REQUEST_OP_READAHEAD_COUNT_SIZE;
- } else {
- rc = 0;
- goto out;
- }
}
} else if (!strcmp(kobj->name, ACACHE_KOBJ_ID)) {
--
2.9.3
On Mon, 21 Nov 2016, Christophe JAILLET wrote:
> The "perf_counter_reset" case has already been handled above.
> Moreover "ORANGEFS_PARAM_REQUEST_OP_READAHEAD_COUNT_SIZE" is not a really
> consistent.
> It is likely that this (dead) code is a cut and paste left over.
That's exactly what this is.
Reviewed-by: Martin Brandenburg <[email protected]>
>
> Signed-off-by: Christophe JAILLET <[email protected]>
> ---
> fs/orangefs/orangefs-sysfs.c | 9 ---------
> 1 file changed, 9 deletions(-)
>
> diff --git a/fs/orangefs/orangefs-sysfs.c b/fs/orangefs/orangefs-sysfs.c
> index a799546a67f7..084954448f18 100644
> --- a/fs/orangefs/orangefs-sysfs.c
> +++ b/fs/orangefs/orangefs-sysfs.c
> @@ -609,15 +609,6 @@ static ssize_t sysfs_service_op_store(struct kobject *kobj,
> new_op->upcall.req.param.u.value32[0] = val1;
> new_op->upcall.req.param.u.value32[1] = val2;
> goto value_set;
> - } else if (!strcmp(attr->attr.name,
> - "perf_counter_reset")) {
> - if ((val > 0)) {
> - new_op->upcall.req.param.op =
> - ORANGEFS_PARAM_REQUEST_OP_READAHEAD_COUNT_SIZE;
> - } else {
> - rc = 0;
> - goto out;
> - }
> }
>
> } else if (!strcmp(kobj->name, ACACHE_KOBJ_ID)) {
> --
> 2.9.3
>
>
This seems like a good and proper patch to me, and simple too.
But like all changes, it needs tested. While I was testing it, I
discovered a regression in the associated userspace code. I
"bisected" (we use SVN for the userspace part of Orangefs)
down to the commit that caused the regression, and some
of the userspace folks are going to fix it.
I don't think I should ask Linus to pull this patch until I can
test it. Since we're about to go into rc7, it might not go in
until the next go around...
-Mike
On Tue, Nov 22, 2016 at 10:49 AM, Martin Brandenburg
<[email protected]> wrote:
> On Mon, 21 Nov 2016, Christophe JAILLET wrote:
>
>> The "perf_counter_reset" case has already been handled above.
>> Moreover "ORANGEFS_PARAM_REQUEST_OP_READAHEAD_COUNT_SIZE" is not a really
>> consistent.
>> It is likely that this (dead) code is a cut and paste left over.
>
> That's exactly what this is.
>
> Reviewed-by: Martin Brandenburg <[email protected]>
>
>>
>> Signed-off-by: Christophe JAILLET <[email protected]>
>> ---
>> fs/orangefs/orangefs-sysfs.c | 9 ---------
>> 1 file changed, 9 deletions(-)
>>
>> diff --git a/fs/orangefs/orangefs-sysfs.c b/fs/orangefs/orangefs-sysfs.c
>> index a799546a67f7..084954448f18 100644
>> --- a/fs/orangefs/orangefs-sysfs.c
>> +++ b/fs/orangefs/orangefs-sysfs.c
>> @@ -609,15 +609,6 @@ static ssize_t sysfs_service_op_store(struct kobject *kobj,
>> new_op->upcall.req.param.u.value32[0] = val1;
>> new_op->upcall.req.param.u.value32[1] = val2;
>> goto value_set;
>> - } else if (!strcmp(attr->attr.name,
>> - "perf_counter_reset")) {
>> - if ((val > 0)) {
>> - new_op->upcall.req.param.op =
>> - ORANGEFS_PARAM_REQUEST_OP_READAHEAD_COUNT_SIZE;
>> - } else {
>> - rc = 0;
>> - goto out;
>> - }
>> }
>>
>> } else if (!strcmp(kobj->name, ACACHE_KOBJ_ID)) {
>> --
>> 2.9.3
>>
>>
On Thu, Nov 24, 2016 at 07:31:11AM -0500, Mike Marshall wrote:
> This seems like a good and proper patch to me, and simple too.
> But like all changes, it needs tested. While I was testing it, I
> discovered a regression in the associated userspace code. I
> "bisected" (we use SVN for the userspace part of Orangefs)
> down to the commit that caused the regression, and some
> of the userspace folks are going to fix it.
>
> I don't think I should ask Linus to pull this patch until I can
> test it. Since we're about to go into rc7, it might not go in
> until the next go around...
>
Linus is on 4.9-rc7 so you're saying this would probably go into 4.11?
regards,
dan carpenter
We're on rc7 now. Linus said in LWN that there might be a rc8 this time.
I'll try to get this pulled in 4.9-rc8 I hope, or sometime in 4.10...
it is just a
few lines of code that I don't think can be reached. Sorry for the confusion.
-Mike
On Fri, Nov 25, 2016 at 4:51 PM, Dan Carpenter <[email protected]> wrote:
> On Thu, Nov 24, 2016 at 07:31:11AM -0500, Mike Marshall wrote:
>> This seems like a good and proper patch to me, and simple too.
>> But like all changes, it needs tested. While I was testing it, I
>> discovered a regression in the associated userspace code. I
>> "bisected" (we use SVN for the userspace part of Orangefs)
>> down to the commit that caused the regression, and some
>> of the userspace folks are going to fix it.
>>
>> I don't think I should ask Linus to pull this patch until I can
>> test it. Since we're about to go into rc7, it might not go in
>> until the next go around...
>>
>
> Linus is on 4.9-rc7 so you're saying this would probably go into 4.11?
>
> regards,
> dan carpenter
>
On Fri, Nov 25, 2016 at 06:09:39PM -0500, Mike Marshall wrote:
> We're on rc7 now. Linus said in LWN that there might be a rc8 this time.
> I'll try to get this pulled in 4.9-rc8 I hope, or sometime in 4.10...
> it is just a
> few lines of code that I don't think can be reached. Sorry for the confusion.
It's not -rc material at all. Especially not after -rc2. If you had
said 4.11, I would have been secretly disapointed at your lack of
courage in my heart but it would have been normal and fine. Merging
this in 4.10 is the earliest reasonable.
regards,
dan carpenter
I think I understand what you're saying, except for this part:
> would have been secretly disapointed at your lack of
> courage in my heart but it would have been normal and fine.
I'm pretty sure that Linus won't accept a pull request from me
at the wrong time and that I won't send one at the wrong time
on purpose.
I've been laboring under the belief that the rc period is when
we "push only patches that do not include new functionalities",
and I would have thought that stripping out a few lines of dead
code would be appropriate then.
-Mike
On Sat, Nov 26, 2016 at 5:02 AM, Dan Carpenter <[email protected]> wrote:
> On Fri, Nov 25, 2016 at 06:09:39PM -0500, Mike Marshall wrote:
>> We're on rc7 now. Linus said in LWN that there might be a rc8 this time.
>> I'll try to get this pulled in 4.9-rc8 I hope, or sometime in 4.10...
>> it is just a
>> few lines of code that I don't think can be reached. Sorry for the confusion.
>
> It's not -rc material at all. Especially not after -rc2. If you had
> said 4.11, I would have been secretly disapointed at your lack of
> courage in my heart but it would have been normal and fine. Merging
> this in 4.10 is the earliest reasonable.
>
> regards,
> dan carpenter
>
On Sat, Nov 26, 2016 at 08:51:57AM -0500, Mike Marshall wrote:
> I think I understand what you're saying, except for this part:
>
> > would have been secretly disapointed at your lack of
> > courage in my heart but it would have been normal and fine.
>
What I'm saying is that for some people the cut off for 4.10 happens
the week or two before 4.9 is released. I'm sending bugfixes and they
still push them out to 4.11. It annoys me, secretly.
> I'm pretty sure that Linus won't accept a pull request from me
> at the wrong time and that I won't send one at the wrong time
> on purpose.
Linus pulls lots of things that make him unhappy. If he didn't
compromise he would go mad.
>
> I've been laboring under the belief that the rc period is when
> we "push only patches that do not include new functionalities",
> and I would have thought that stripping out a few lines of dead
> code would be appropriate then.
>
No. -rc is for fixing regressions only. If it's a fix for a bug that
has *always* been there, then think carefully about how important it is
because that's not a regression fix. If it's a bug fix, but it's not a
regression fix and it's not critical then wait. Non-bugfixes should
only go in during the merge window.
regards,
dan carpenter
Perhaps we should modify Greg KH's "be-all, end-all document"
on "HOWTO do Linux kernel development" then... you've
contributed a boatload of work to the kernel since as far
back as 2006, but I'm a newbie who just works in an
isolated subsystem... people like me need a reliable
and authoritative cheat-sheet to go by...
I think you believe I should ask for this to be pulled only
during a merge window. Since this patch doesn't involve new
functionality, or even any functionality, it seems like pull-fodder
anytime after it is vetted, based on Greg's HOWTO...
My original intent on posting to this thread was to let
[email protected] know that I saw and
appreciate his review and the good patch he supplied.
-Mike
On Mon, Nov 28, 2016 at 6:07 AM, Dan Carpenter <[email protected]> wrote:
> On Sat, Nov 26, 2016 at 08:51:57AM -0500, Mike Marshall wrote:
>> I think I understand what you're saying, except for this part:
>>
>> > would have been secretly disapointed at your lack of
>> > courage in my heart but it would have been normal and fine.
>>
> What I'm saying is that for some people the cut off for 4.10 happens
> the week or two before 4.9 is released. I'm sending bugfixes and they
> still push them out to 4.11. It annoys me, secretly.
>
>
>> I'm pretty sure that Linus won't accept a pull request from me
>> at the wrong time and that I won't send one at the wrong time
>> on purpose.
>
> Linus pulls lots of things that make him unhappy. If he didn't
> compromise he would go mad.
>
>>
>> I've been laboring under the belief that the rc period is when
>> we "push only patches that do not include new functionalities",
>> and I would have thought that stripping out a few lines of dead
>> code would be appropriate then.
>>
>
> No. -rc is for fixing regressions only. If it's a fix for a bug that
> has *always* been there, then think carefully about how important it is
> because that's not a regression fix. If it's a bug fix, but it's not a
> regression fix and it's not critical then wait. Non-bugfixes should
> only go in during the merge window.
>
> regards,
> dan carpenter
>
On Mon, Nov 28, 2016 at 09:06:12AM -0500, Mike Marshall wrote:
> Perhaps we should modify Greg KH's "be-all, end-all document"
> on "HOWTO do Linux kernel development" then... you've
> contributed a boatload of work to the kernel since as far
> back as 2006, but I'm a newbie who just works in an
> isolated subsystem... people like me need a reliable
> and authoritative cheat-sheet to go by...
>
> I think you believe I should ask for this to be pulled only
> during a merge window. Since this patch doesn't involve new
> functionality, or even any functionality, it seems like pull-fodder
> anytime after it is vetted, based on Greg's HOWTO...
Hm. Yes. I can see how that document is misleading. I'll send a
patch.
>
> My original intent on posting to this thread was to let
> [email protected] know that I saw and
> appreciate his review and the good patch he supplied.
>
Yes. Of course. But this discussion has been valuable, I hope.
regards,
dan carpenter