Re: [PATCH 41/47] staging/lustre/llite: remove dead code

2014-04-30 Thread Dan Carpenter 
On Tue, Apr 29, 2014 at 11:21:48PM -0400, Oleg Drokin wrote:
> I can rediff this patch with this particular part dropped if there are
> any concerns and it's possible to reintegrate it in a changed form.

No no.  I knew that Greg had already merged this stuff by the time I got
around to reviewing it.  It was just some bonus grumbling after the
fact.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 41/47] staging/lustre/llite: remove dead code

2014-04-30 Thread Dan Carpenter 
On Tue, Apr 29, 2014 at 11:21:48PM -0400, Oleg Drokin wrote:
 I can rediff this patch with this particular part dropped if there are
 any concerns and it's possible to reintegrate it in a changed form.

No no.  I knew that Greg had already merged this stuff by the time I got
around to reviewing it.  It was just some bonus grumbling after the
fact.

regards,
dan carpenter

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 41/47] staging/lustre/llite: remove dead code

2014-04-29 Thread Oleg Drokin 
Hello!

On Apr 29, 2014, at 7:02 AM, Dan Carpenter wrote:

>> diff --git a/drivers/staging/lustre/lustre/llite/statahead.c 
>> b/drivers/staging/lustre/lustre/llite/statahead.c
>> index 51c5327..1b47774 100644
>> --- a/drivers/staging/lustre/lustre/llite/statahead.c
>> +++ b/drivers/staging/lustre/lustre/llite/statahead.c
>> @@ -1230,9 +1230,7 @@ do_it:
>>   */
>>  ll_release_page(page, le32_to_cpu(dp->ldp_flags) &
>>LDF_COLLIDE);
>> -sai->sai_in_readpage = 1;
>>  page = ll_get_dir_page(dir, pos, );
>> -sai->sai_in_readpage = 0;
>>  } else {
>>  LASSERT(le32_to_cpu(dp->ldp_flags) & LDF_COLLIDE);
>>  ll_release_page(page, 1);
>> @@ -1563,12 +1561,6 @@ int do_statahead_enter(struct inode *dir, struct 
>> dentry **dentryp,
>>  return entry ? 1 : -EAGAIN;
>>  }
>> 
>> -/* if statahead is busy in readdir, help it do post-work */
>> -while (!ll_sa_entry_stated(entry) &&
>> -   sai->sai_in_readpage &&
>> -   !sa_received_empty(sai))
>> -ll_post_statahead(sai);
>> -
>>  if (!ll_sa_entry_stated(entry)) {
>>  sai->sai_index_wait = entry->se_index;
>>  lwi = LWI_TIMEOUT_INTR(cfs_time_seconds(30), NULL,
> 
> What is this change about really?  I've already waded through 1271 lines
> of random changes at this point and now I have to figure out what
> ll_post_statahead() does and why we don't need to call it now?
> 
> Anyways, please explain this change.

Ok, so after some digging.
The first hunk of the above was gone in our code with the move of a lot of 
directory
processing down onto mdc layer as part of support for multiple metadata servers 
(a new feature).
Then the second hunk was the cleanup sine nobody was setting the 
sai_in_readpage ever.

sai_in_readpage in itself originally was devised as performance optimization 
for the case where the
statahead thread is waiting for READDIR RPC to return while there are already 
some GETATTR RPCs
that have already returned and are waiting to be interpreted.
This does not affect correctness in any way, though. And removing it brings the 
code closer to
our current tree which is hopefully a good thing.
I can rediff this patch with this particular part dropped if there are any 
concerns and it's possible
to reintegrate it in a changed form.

Bye,
Oleg--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 41/47] staging/lustre/llite: remove dead code

2014-04-29 Thread Dan Carpenter 
On Tue, Apr 29, 2014 at 07:16:54PM +, Hammond, John wrote:
> > -Original Message-
> > From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> > Sent: Tuesday, April 29, 2014 6:03 AM
> > To: Oleg Drokin
> > Cc: Greg Kroah-Hartman; linux-kernel@vger.kernel.org;
> > de...@driverdev.osuosl.org; Drokin, Oleg; Hammond, John
> > Subject: Re: [PATCH 41/47] staging/lustre/llite: remove dead code
> > 
> > On Sun, Apr 27, 2014 at 01:07:05PM -0400, Oleg Drokin wrote:
> > > From: "John L. Hammond" 
> > >
> > > In llite remove unused declarations, parameters, types, and unused,
> > > get-only, or set-only structure members. Add static and const
> > > qualifiers to declarations where possible.
> > >
> ...
> > 
> > This is a random grab bag of changes to lots of files.  One thing per
> > patch, etc, next time.
> 
> OK, granted. But some guidance would be welcome here.

No problem.  This is something a lot of people have questions about.

> For
> clean-up work like this, do you want a patch that const-corrects one
> function, a patch that const corrects all functions in a file, or a
> patch that const corrects all functions in a module?

All the functions in the module is fine.

> Is it OK to do const and static correction in the same change?

That's a borderline case.  My first instinct is to say no.

Are we talking about a single variable and making it const in one
patch and then const static in the next?  That's obviously better done
as one change.  But if you're talking about different variables, then
maybe it's better as two changes.  But then maybe some variables should
be made into "static const" and some should be just "static".

These things depend on how you sell it.

[patch] staging: lustre/llite: tighten up static and const declarations

That would be ok probably.

> Is it OK to do const, static, and dead-code in a single file?

No.

Greg compile tests patches but I normally don't.  When I review +static
patches then I pipe it to a command that strips out all the +static
changes and I verify that nothing else changed.  When I review dead code
patches I scan it for places which add code and look at why it does
that.  Then I quickly rescan to verify that the dead code really is
dead.  Normally I can tell just from looking at the patch because there
is an "#if 0" but if it's something more complicated then hopefully it's
explained in the changelog.  If you mix the two kinds of changes then it
messes up my review process.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 41/47] staging/lustre/llite: remove dead code

2014-04-29 Thread Hammond, John 
> -Original Message-
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Tuesday, April 29, 2014 6:03 AM
> To: Oleg Drokin
> Cc: Greg Kroah-Hartman; linux-kernel@vger.kernel.org;
> de...@driverdev.osuosl.org; Drokin, Oleg; Hammond, John
> Subject: Re: [PATCH 41/47] staging/lustre/llite: remove dead code
> 
> On Sun, Apr 27, 2014 at 01:07:05PM -0400, Oleg Drokin wrote:
> > From: "John L. Hammond" 
> >
> > In llite remove unused declarations, parameters, types, and unused,
> > get-only, or set-only structure members. Add static and const
> > qualifiers to declarations where possible.
> >
...
> 
> This is a random grab bag of changes to lots of files.  One thing per
> patch, etc, next time.

OK, granted. But some guidance would be welcome here. For clean-up work like 
this, do you want a patch that const-corrects one function, a patch that const 
corrects all functions in a file, or a patch that const corrects all functions 
in a module? Is it OK to do const and static correction in the same change? Is 
it OK to do const, static, and dead-code in a single file?

> > diff --git a/drivers/staging/lustre/lustre/llite/dcache.c
> b/drivers/staging/lustre/lustre/llite/dcache.c
> > index 8b55080..7d520d8 100644
> > --- a/drivers/staging/lustre/lustre/llite/dcache.c
> > +++ b/drivers/staging/lustre/lustre/llite/dcache.c
> > @@ -69,8 +69,7 @@ static void ll_release(struct dentry *de)
> > ll_intent_release(lld->lld_it);
> > OBD_FREE(lld->lld_it, sizeof(*lld->lld_it));
> > }
> > -   LASSERT(lld->lld_cwd_count == 0);
> > -   LASSERT(lld->lld_mnt_count == 0);
> > +
> 
> I'm totally in favour of removing LASSERT() calls...  But is this a
> "set only" struct member?  It's totally unclear from the patch
> description.

Actually it's get only. I can label what's what in the commit messages.

> > diff --git a/drivers/staging/lustre/lustre/llite/statahead.c
> b/drivers/staging/lustre/lustre/llite/statahead.c
> > index 51c5327..1b47774 100644
> > --- a/drivers/staging/lustre/lustre/llite/statahead.c
> > +++ b/drivers/staging/lustre/lustre/llite/statahead.c
> > @@ -1230,9 +1230,7 @@ do_it:
> >  */
> > ll_release_page(page, le32_to_cpu(dp->ldp_flags) &
> >   LDF_COLLIDE);
> > -   sai->sai_in_readpage = 1;
> > page = ll_get_dir_page(dir, pos, );
> > -   sai->sai_in_readpage = 0;
> > } else {
> > LASSERT(le32_to_cpu(dp->ldp_flags) & LDF_COLLIDE);
> > ll_release_page(page, 1);
> > @@ -1563,12 +1561,6 @@ int do_statahead_enter(struct inode *dir, struct
> dentry **dentryp,
> > return entry ? 1 : -EAGAIN;
> > }
> >
> > -   /* if statahead is busy in readdir, help it do post-work */
> > -   while (!ll_sa_entry_stated(entry) &&
> > -  sai->sai_in_readpage &&
> > -  !sa_received_empty(sai))
> > -   ll_post_statahead(sai);
> > -
> > if (!ll_sa_entry_stated(entry)) {
> > sai->sai_index_wait = entry->se_index;
> > lwi = LWI_TIMEOUT_INTR(cfs_time_seconds(30), NULL,
> 
> What is this change about really?  I've already waded through 1271 lines
> of random changes at this point and now I have to figure out what
> ll_post_statahead() does and why we don't need to call it now?
> 
> Anyways, please explain this change.

It looks like this change got squished together with something else when it was 
pushed to staging. I've asked to Oleg check. In the original change, 
sai_in_readpage was never set. Hence it was easy to see that the if statement 
was dead code. Sorry for the confusion.

Best,

John

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 41/47] staging/lustre/llite: remove dead code

2014-04-29 Thread Dan Carpenter 
On Sun, Apr 27, 2014 at 01:07:05PM -0400, Oleg Drokin wrote:
> From: "John L. Hammond" 
> 
> In llite remove unused declarations, parameters, types, and unused,
> get-only, or set-only structure members. Add static and const
> qualifiers to declarations where possible.
> 
> Signed-off-by: John L. Hammond 
> Reviewed-on: http://review.whamcloud.com/9767
> Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-2675
> Reviewed-by: Lai Siyao 
> Reviewed-by: Jinshan Xiong 
> Signed-off-by: Oleg Drokin 

This is a random grab bag of changes to lots of files.  One thing per
patch, etc, next time.

> diff --git a/drivers/staging/lustre/lustre/llite/dcache.c 
> b/drivers/staging/lustre/lustre/llite/dcache.c
> index 8b55080..7d520d8 100644
> --- a/drivers/staging/lustre/lustre/llite/dcache.c
> +++ b/drivers/staging/lustre/lustre/llite/dcache.c
> @@ -69,8 +69,7 @@ static void ll_release(struct dentry *de)
>   ll_intent_release(lld->lld_it);
>   OBD_FREE(lld->lld_it, sizeof(*lld->lld_it));
>   }
> - LASSERT(lld->lld_cwd_count == 0);
> - LASSERT(lld->lld_mnt_count == 0);
> +

I'm totally in favour of removing LASSERT() calls...  But is this a
"set only" struct member?  It's totally unclear from the patch
description.

> diff --git a/drivers/staging/lustre/lustre/llite/statahead.c 
> b/drivers/staging/lustre/lustre/llite/statahead.c
> index 51c5327..1b47774 100644
> --- a/drivers/staging/lustre/lustre/llite/statahead.c
> +++ b/drivers/staging/lustre/lustre/llite/statahead.c
> @@ -1230,9 +1230,7 @@ do_it:
>*/
>   ll_release_page(page, le32_to_cpu(dp->ldp_flags) &
> LDF_COLLIDE);
> - sai->sai_in_readpage = 1;
>   page = ll_get_dir_page(dir, pos, );
> - sai->sai_in_readpage = 0;
>   } else {
>   LASSERT(le32_to_cpu(dp->ldp_flags) & LDF_COLLIDE);
>   ll_release_page(page, 1);
> @@ -1563,12 +1561,6 @@ int do_statahead_enter(struct inode *dir, struct 
> dentry **dentryp,
>   return entry ? 1 : -EAGAIN;
>   }
>  
> - /* if statahead is busy in readdir, help it do post-work */
> - while (!ll_sa_entry_stated(entry) &&
> -sai->sai_in_readpage &&
> -!sa_received_empty(sai))
> - ll_post_statahead(sai);
> -
>   if (!ll_sa_entry_stated(entry)) {
>   sai->sai_index_wait = entry->se_index;
>   lwi = LWI_TIMEOUT_INTR(cfs_time_seconds(30), NULL,

What is this change about really?  I've already waded through 1271 lines
of random changes at this point and now I have to figure out what
ll_post_statahead() does and why we don't need to call it now?

Anyways, please explain this change.

Btw, it's very easy to break up a patch into separate patches.  Use
git citool.  Highlight the lines of code which make things static.
Right click and select "Add highlighted lines to commit."  This patch
has three Reviewed-by tags so you guys need to be pushing back on this
kind of stuff, it's obviously outside of the "one thing per patch" rule.

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 41/47] staging/lustre/llite: remove dead code

2014-04-29 Thread Dan Carpenter 
On Sun, Apr 27, 2014 at 01:07:05PM -0400, Oleg Drokin wrote:
 From: John L. Hammond john.hamm...@intel.com
 
 In llite remove unused declarations, parameters, types, and unused,
 get-only, or set-only structure members. Add static and const
 qualifiers to declarations where possible.
 
 Signed-off-by: John L. Hammond john.hamm...@intel.com
 Reviewed-on: http://review.whamcloud.com/9767
 Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-2675
 Reviewed-by: Lai Siyao lai.si...@intel.com
 Reviewed-by: Jinshan Xiong jinshan.xi...@intel.com
 Signed-off-by: Oleg Drokin oleg.dro...@intel.com

This is a random grab bag of changes to lots of files.  One thing per
patch, etc, next time.

 diff --git a/drivers/staging/lustre/lustre/llite/dcache.c 
 b/drivers/staging/lustre/lustre/llite/dcache.c
 index 8b55080..7d520d8 100644
 --- a/drivers/staging/lustre/lustre/llite/dcache.c
 +++ b/drivers/staging/lustre/lustre/llite/dcache.c
 @@ -69,8 +69,7 @@ static void ll_release(struct dentry *de)
   ll_intent_release(lld-lld_it);
   OBD_FREE(lld-lld_it, sizeof(*lld-lld_it));
   }
 - LASSERT(lld-lld_cwd_count == 0);
 - LASSERT(lld-lld_mnt_count == 0);
 +

I'm totally in favour of removing LASSERT() calls...  But is this a
set only struct member?  It's totally unclear from the patch
description.

 diff --git a/drivers/staging/lustre/lustre/llite/statahead.c 
 b/drivers/staging/lustre/lustre/llite/statahead.c
 index 51c5327..1b47774 100644
 --- a/drivers/staging/lustre/lustre/llite/statahead.c
 +++ b/drivers/staging/lustre/lustre/llite/statahead.c
 @@ -1230,9 +1230,7 @@ do_it:
*/
   ll_release_page(page, le32_to_cpu(dp-ldp_flags) 
 LDF_COLLIDE);
 - sai-sai_in_readpage = 1;
   page = ll_get_dir_page(dir, pos, chain);
 - sai-sai_in_readpage = 0;
   } else {
   LASSERT(le32_to_cpu(dp-ldp_flags)  LDF_COLLIDE);
   ll_release_page(page, 1);
 @@ -1563,12 +1561,6 @@ int do_statahead_enter(struct inode *dir, struct 
 dentry **dentryp,
   return entry ? 1 : -EAGAIN;
   }
  
 - /* if statahead is busy in readdir, help it do post-work */
 - while (!ll_sa_entry_stated(entry) 
 -sai-sai_in_readpage 
 -!sa_received_empty(sai))
 - ll_post_statahead(sai);
 -
   if (!ll_sa_entry_stated(entry)) {
   sai-sai_index_wait = entry-se_index;
   lwi = LWI_TIMEOUT_INTR(cfs_time_seconds(30), NULL,

What is this change about really?  I've already waded through 1271 lines
of random changes at this point and now I have to figure out what
ll_post_statahead() does and why we don't need to call it now?

Anyways, please explain this change.

Btw, it's very easy to break up a patch into separate patches.  Use
git citool.  Highlight the lines of code which make things static.
Right click and select Add highlighted lines to commit.  This patch
has three Reviewed-by tags so you guys need to be pushing back on this
kind of stuff, it's obviously outside of the one thing per patch rule.

regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 41/47] staging/lustre/llite: remove dead code

2014-04-29 Thread Hammond, John 
 -Original Message-
 From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
 Sent: Tuesday, April 29, 2014 6:03 AM
 To: Oleg Drokin
 Cc: Greg Kroah-Hartman; linux-kernel@vger.kernel.org;
 de...@driverdev.osuosl.org; Drokin, Oleg; Hammond, John
 Subject: Re: [PATCH 41/47] staging/lustre/llite: remove dead code
 
 On Sun, Apr 27, 2014 at 01:07:05PM -0400, Oleg Drokin wrote:
  From: John L. Hammond john.hamm...@intel.com
 
  In llite remove unused declarations, parameters, types, and unused,
  get-only, or set-only structure members. Add static and const
  qualifiers to declarations where possible.
 
...
 
 This is a random grab bag of changes to lots of files.  One thing per
 patch, etc, next time.

OK, granted. But some guidance would be welcome here. For clean-up work like 
this, do you want a patch that const-corrects one function, a patch that const 
corrects all functions in a file, or a patch that const corrects all functions 
in a module? Is it OK to do const and static correction in the same change? Is 
it OK to do const, static, and dead-code in a single file?

  diff --git a/drivers/staging/lustre/lustre/llite/dcache.c
 b/drivers/staging/lustre/lustre/llite/dcache.c
  index 8b55080..7d520d8 100644
  --- a/drivers/staging/lustre/lustre/llite/dcache.c
  +++ b/drivers/staging/lustre/lustre/llite/dcache.c
  @@ -69,8 +69,7 @@ static void ll_release(struct dentry *de)
  ll_intent_release(lld-lld_it);
  OBD_FREE(lld-lld_it, sizeof(*lld-lld_it));
  }
  -   LASSERT(lld-lld_cwd_count == 0);
  -   LASSERT(lld-lld_mnt_count == 0);
  +
 
 I'm totally in favour of removing LASSERT() calls...  But is this a
 set only struct member?  It's totally unclear from the patch
 description.

Actually it's get only. I can label what's what in the commit messages.

  diff --git a/drivers/staging/lustre/lustre/llite/statahead.c
 b/drivers/staging/lustre/lustre/llite/statahead.c
  index 51c5327..1b47774 100644
  --- a/drivers/staging/lustre/lustre/llite/statahead.c
  +++ b/drivers/staging/lustre/lustre/llite/statahead.c
  @@ -1230,9 +1230,7 @@ do_it:
   */
  ll_release_page(page, le32_to_cpu(dp-ldp_flags) 
LDF_COLLIDE);
  -   sai-sai_in_readpage = 1;
  page = ll_get_dir_page(dir, pos, chain);
  -   sai-sai_in_readpage = 0;
  } else {
  LASSERT(le32_to_cpu(dp-ldp_flags)  LDF_COLLIDE);
  ll_release_page(page, 1);
  @@ -1563,12 +1561,6 @@ int do_statahead_enter(struct inode *dir, struct
 dentry **dentryp,
  return entry ? 1 : -EAGAIN;
  }
 
  -   /* if statahead is busy in readdir, help it do post-work */
  -   while (!ll_sa_entry_stated(entry) 
  -  sai-sai_in_readpage 
  -  !sa_received_empty(sai))
  -   ll_post_statahead(sai);
  -
  if (!ll_sa_entry_stated(entry)) {
  sai-sai_index_wait = entry-se_index;
  lwi = LWI_TIMEOUT_INTR(cfs_time_seconds(30), NULL,
 
 What is this change about really?  I've already waded through 1271 lines
 of random changes at this point and now I have to figure out what
 ll_post_statahead() does and why we don't need to call it now?
 
 Anyways, please explain this change.

It looks like this change got squished together with something else when it was 
pushed to staging. I've asked to Oleg check. In the original change, 
sai_in_readpage was never set. Hence it was easy to see that the if statement 
was dead code. Sorry for the confusion.

Best,

John

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 41/47] staging/lustre/llite: remove dead code

2014-04-29 Thread Dan Carpenter 
On Tue, Apr 29, 2014 at 07:16:54PM +, Hammond, John wrote:
  -Original Message-
  From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
  Sent: Tuesday, April 29, 2014 6:03 AM
  To: Oleg Drokin
  Cc: Greg Kroah-Hartman; linux-kernel@vger.kernel.org;
  de...@driverdev.osuosl.org; Drokin, Oleg; Hammond, John
  Subject: Re: [PATCH 41/47] staging/lustre/llite: remove dead code
  
  On Sun, Apr 27, 2014 at 01:07:05PM -0400, Oleg Drokin wrote:
   From: John L. Hammond john.hamm...@intel.com
  
   In llite remove unused declarations, parameters, types, and unused,
   get-only, or set-only structure members. Add static and const
   qualifiers to declarations where possible.
  
 ...
  
  This is a random grab bag of changes to lots of files.  One thing per
  patch, etc, next time.
 
 OK, granted. But some guidance would be welcome here.

No problem.  This is something a lot of people have questions about.

 For
 clean-up work like this, do you want a patch that const-corrects one
 function, a patch that const corrects all functions in a file, or a
 patch that const corrects all functions in a module?

All the functions in the module is fine.

 Is it OK to do const and static correction in the same change?

That's a borderline case.  My first instinct is to say no.

Are we talking about a single variable and making it const in one
patch and then const static in the next?  That's obviously better done
as one change.  But if you're talking about different variables, then
maybe it's better as two changes.  But then maybe some variables should
be made into static const and some should be just static.

These things depend on how you sell it.

[patch] staging: lustre/llite: tighten up static and const declarations

That would be ok probably.

 Is it OK to do const, static, and dead-code in a single file?

No.

Greg compile tests patches but I normally don't.  When I review +static
patches then I pipe it to a command that strips out all the +static
changes and I verify that nothing else changed.  When I review dead code
patches I scan it for places which add code and look at why it does
that.  Then I quickly rescan to verify that the dead code really is
dead.  Normally I can tell just from looking at the patch because there
is an #if 0 but if it's something more complicated then hopefully it's
explained in the changelog.  If you mix the two kinds of changes then it
messes up my review process.

regards,
dan carpenter

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 41/47] staging/lustre/llite: remove dead code

2014-04-29 Thread Oleg Drokin 
Hello!

On Apr 29, 2014, at 7:02 AM, Dan Carpenter wrote:

 diff --git a/drivers/staging/lustre/lustre/llite/statahead.c 
 b/drivers/staging/lustre/lustre/llite/statahead.c
 index 51c5327..1b47774 100644
 --- a/drivers/staging/lustre/lustre/llite/statahead.c
 +++ b/drivers/staging/lustre/lustre/llite/statahead.c
 @@ -1230,9 +1230,7 @@ do_it:
   */
  ll_release_page(page, le32_to_cpu(dp-ldp_flags) 
LDF_COLLIDE);
 -sai-sai_in_readpage = 1;
  page = ll_get_dir_page(dir, pos, chain);
 -sai-sai_in_readpage = 0;
  } else {
  LASSERT(le32_to_cpu(dp-ldp_flags)  LDF_COLLIDE);
  ll_release_page(page, 1);
 @@ -1563,12 +1561,6 @@ int do_statahead_enter(struct inode *dir, struct 
 dentry **dentryp,
  return entry ? 1 : -EAGAIN;
  }
 
 -/* if statahead is busy in readdir, help it do post-work */
 -while (!ll_sa_entry_stated(entry) 
 -   sai-sai_in_readpage 
 -   !sa_received_empty(sai))
 -ll_post_statahead(sai);
 -
  if (!ll_sa_entry_stated(entry)) {
  sai-sai_index_wait = entry-se_index;
  lwi = LWI_TIMEOUT_INTR(cfs_time_seconds(30), NULL,
 
 What is this change about really?  I've already waded through 1271 lines
 of random changes at this point and now I have to figure out what
 ll_post_statahead() does and why we don't need to call it now?
 
 Anyways, please explain this change.

Ok, so after some digging.
The first hunk of the above was gone in our code with the move of a lot of 
directory
processing down onto mdc layer as part of support for multiple metadata servers 
(a new feature).
Then the second hunk was the cleanup sine nobody was setting the 
sai_in_readpage ever.

sai_in_readpage in itself originally was devised as performance optimization 
for the case where the
statahead thread is waiting for READDIR RPC to return while there are already 
some GETATTR RPCs
that have already returned and are waiting to be interpreted.
This does not affect correctness in any way, though. And removing it brings the 
code closer to
our current tree which is hopefully a good thing.
I can rediff this patch with this particular part dropped if there are any 
concerns and it's possible
to reintegrate it in a changed form.

Bye,
Oleg--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 41/47] staging/lustre/llite: remove dead code

2014-04-27 Thread Oleg Drokin 
From: "John L. Hammond" 

In llite remove unused declarations, parameters, types, and unused,
get-only, or set-only structure members. Add static and const
qualifiers to declarations where possible.

Signed-off-by: John L. Hammond 
Reviewed-on: http://review.whamcloud.com/9767
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-2675
Reviewed-by: Lai Siyao 
Reviewed-by: Jinshan Xiong 
Signed-off-by: Oleg Drokin 
---
 drivers/staging/lustre/lustre/lclient/lcommon_cl.c |   4 +-
 drivers/staging/lustre/lustre/llite/dcache.c   |  26 ++--
 drivers/staging/lustre/lustre/llite/dir.c  |   8 +-
 drivers/staging/lustre/lustre/llite/file.c |  89 ++---
 drivers/staging/lustre/lustre/llite/llite_capa.c   |   4 +-
 .../staging/lustre/lustre/llite/llite_internal.h   | 138 -
 drivers/staging/lustre/lustre/llite/llite_lib.c|  64 +-
 drivers/staging/lustre/lustre/llite/llite_mmap.c   |  23 +---
 drivers/staging/lustre/lustre/llite/lloop.c|   5 -
 drivers/staging/lustre/lustre/llite/lproc_llite.c  |   4 +-
 drivers/staging/lustre/lustre/llite/namei.c|  40 ++
 drivers/staging/lustre/lustre/llite/remote_perm.c  |   2 +-
 drivers/staging/lustre/lustre/llite/rw.c   |   7 --
 drivers/staging/lustre/lustre/llite/rw26.c |  15 +--
 drivers/staging/lustre/lustre/llite/statahead.c|   8 --
 drivers/staging/lustre/lustre/llite/super25.c  |   7 +-
 drivers/staging/lustre/lustre/llite/vvp_dev.c  |   6 +-
 drivers/staging/lustre/lustre/llite/vvp_internal.h |   2 +-
 drivers/staging/lustre/lustre/llite/vvp_io.c   |   2 +-
 drivers/staging/lustre/lustre/llite/vvp_object.c   |   4 +-
 drivers/staging/lustre/lustre/llite/xattr_cache.c  |   2 +-
 21 files changed, 125 insertions(+), 335 deletions(-)

diff --git a/drivers/staging/lustre/lustre/lclient/lcommon_cl.c 
b/drivers/staging/lustre/lustre/lclient/lcommon_cl.c
index 12812fc..dc24cfa 100644
--- a/drivers/staging/lustre/lustre/lclient/lcommon_cl.c
+++ b/drivers/staging/lustre/lustre/lclient/lcommon_cl.c
@@ -63,7 +63,7 @@
 
 #include "../llite/llite_internal.h"
 
-const struct cl_req_operations ccc_req_ops;
+static const struct cl_req_operations ccc_req_ops;
 
 /*
  * ccc_ prefix stands for "Common Client Code".
@@ -962,7 +962,7 @@ void ccc_req_attr_set(const struct lu_env *env,
   JOBSTATS_JOBID_SIZE);
 }
 
-const struct cl_req_operations ccc_req_ops = {
+static const struct cl_req_operations ccc_req_ops = {
.cro_attr_set   = ccc_req_attr_set,
.cro_completion = ccc_req_completion
 };
diff --git a/drivers/staging/lustre/lustre/llite/dcache.c 
b/drivers/staging/lustre/lustre/llite/dcache.c
index 8b55080..7d520d8 100644
--- a/drivers/staging/lustre/lustre/llite/dcache.c
+++ b/drivers/staging/lustre/lustre/llite/dcache.c
@@ -69,8 +69,7 @@ static void ll_release(struct dentry *de)
ll_intent_release(lld->lld_it);
OBD_FREE(lld->lld_it, sizeof(*lld->lld_it));
}
-   LASSERT(lld->lld_cwd_count == 0);
-   LASSERT(lld->lld_mnt_count == 0);
+
de->d_fsdata = NULL;
call_rcu(>lld_rcu_head, free_dentry_data);
 }
@@ -82,8 +81,9 @@ static void ll_release(struct dentry *de)
  * an AST before calling d_revalidate_it().  The dentry still exists (marked
  * INVALID) so d_lookup() matches it, but we have no lock on it (so
  * lock_match() fails) and we spin around real_lookup(). */
-int ll_dcompare(const struct dentry *parent, const struct dentry *dentry,
-   unsigned int len, const char *str, const struct qstr *name)
+static int ll_dcompare(const struct dentry *parent, const struct dentry 
*dentry,
+  unsigned int len, const char *str,
+  const struct qstr *name)
 {
if (len != name->len)
return 1;
@@ -238,7 +238,8 @@ void ll_intent_release(struct lookup_intent *it)
ll_intent_drop_lock(it);
/* We are still holding extra reference on a request, need to free it */
if (it_disposition(it, DISP_ENQ_OPEN_REF))
-ptlrpc_req_finished(it->d.lustre.it_data); /* ll_file_open */
+   ptlrpc_req_finished(it->d.lustre.it_data); /* ll_file_open */
+
if (it_disposition(it, DISP_ENQ_CREATE_REF)) /* create rec */
ptlrpc_req_finished(it->d.lustre.it_data);
 
@@ -316,15 +317,6 @@ void ll_lookup_finish_locks(struct lookup_intent *it, 
struct dentry *dentry)
}
 }
 
-void ll_frob_intent(struct lookup_intent **itp, struct lookup_intent *deft)
-{
-   struct lookup_intent *it = *itp;
-
-   if (!it || it->it_op == IT_GETXATTR)
-   it = *itp = deft;
-
-}
-
 static int ll_revalidate_dentry(struct dentry *dentry,
unsigned int lookup_flags)
 {
@@ -356,7 +348,7 @@ static int ll_revalidate_dentry(struct dentry *dentry,
 /*
  * Always trust cached dentries. Update statahead window if necessary.
  */
-int ll_revalidate_nd(struct dentry

[PATCH 41/47] staging/lustre/llite: remove dead code

2014-04-27 Thread Oleg Drokin 
From: John L. Hammond john.hamm...@intel.com

In llite remove unused declarations, parameters, types, and unused,
get-only, or set-only structure members. Add static and const
qualifiers to declarations where possible.

Signed-off-by: John L. Hammond john.hamm...@intel.com
Reviewed-on: http://review.whamcloud.com/9767
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-2675
Reviewed-by: Lai Siyao lai.si...@intel.com
Reviewed-by: Jinshan Xiong jinshan.xi...@intel.com
Signed-off-by: Oleg Drokin oleg.dro...@intel.com
---
 drivers/staging/lustre/lustre/lclient/lcommon_cl.c |   4 +-
 drivers/staging/lustre/lustre/llite/dcache.c   |  26 ++--
 drivers/staging/lustre/lustre/llite/dir.c  |   8 +-
 drivers/staging/lustre/lustre/llite/file.c |  89 ++---
 drivers/staging/lustre/lustre/llite/llite_capa.c   |   4 +-
 .../staging/lustre/lustre/llite/llite_internal.h   | 138 -
 drivers/staging/lustre/lustre/llite/llite_lib.c|  64 +-
 drivers/staging/lustre/lustre/llite/llite_mmap.c   |  23 +---
 drivers/staging/lustre/lustre/llite/lloop.c|   5 -
 drivers/staging/lustre/lustre/llite/lproc_llite.c  |   4 +-
 drivers/staging/lustre/lustre/llite/namei.c|  40 ++
 drivers/staging/lustre/lustre/llite/remote_perm.c  |   2 +-
 drivers/staging/lustre/lustre/llite/rw.c   |   7 --
 drivers/staging/lustre/lustre/llite/rw26.c |  15 +--
 drivers/staging/lustre/lustre/llite/statahead.c|   8 --
 drivers/staging/lustre/lustre/llite/super25.c  |   7 +-
 drivers/staging/lustre/lustre/llite/vvp_dev.c  |   6 +-
 drivers/staging/lustre/lustre/llite/vvp_internal.h |   2 +-
 drivers/staging/lustre/lustre/llite/vvp_io.c   |   2 +-
 drivers/staging/lustre/lustre/llite/vvp_object.c   |   4 +-
 drivers/staging/lustre/lustre/llite/xattr_cache.c  |   2 +-
 21 files changed, 125 insertions(+), 335 deletions(-)

diff --git a/drivers/staging/lustre/lustre/lclient/lcommon_cl.c 
b/drivers/staging/lustre/lustre/lclient/lcommon_cl.c
index 12812fc..dc24cfa 100644
--- a/drivers/staging/lustre/lustre/lclient/lcommon_cl.c
+++ b/drivers/staging/lustre/lustre/lclient/lcommon_cl.c
@@ -63,7 +63,7 @@
 
 #include ../llite/llite_internal.h
 
-const struct cl_req_operations ccc_req_ops;
+static const struct cl_req_operations ccc_req_ops;
 
 /*
  * ccc_ prefix stands for Common Client Code.
@@ -962,7 +962,7 @@ void ccc_req_attr_set(const struct lu_env *env,
   JOBSTATS_JOBID_SIZE);
 }
 
-const struct cl_req_operations ccc_req_ops = {
+static const struct cl_req_operations ccc_req_ops = {
.cro_attr_set   = ccc_req_attr_set,
.cro_completion = ccc_req_completion
 };
diff --git a/drivers/staging/lustre/lustre/llite/dcache.c 
b/drivers/staging/lustre/lustre/llite/dcache.c
index 8b55080..7d520d8 100644
--- a/drivers/staging/lustre/lustre/llite/dcache.c
+++ b/drivers/staging/lustre/lustre/llite/dcache.c
@@ -69,8 +69,7 @@ static void ll_release(struct dentry *de)
ll_intent_release(lld-lld_it);
OBD_FREE(lld-lld_it, sizeof(*lld-lld_it));
}
-   LASSERT(lld-lld_cwd_count == 0);
-   LASSERT(lld-lld_mnt_count == 0);
+
de-d_fsdata = NULL;
call_rcu(lld-lld_rcu_head, free_dentry_data);
 }
@@ -82,8 +81,9 @@ static void ll_release(struct dentry *de)
  * an AST before calling d_revalidate_it().  The dentry still exists (marked
  * INVALID) so d_lookup() matches it, but we have no lock on it (so
  * lock_match() fails) and we spin around real_lookup(). */
-int ll_dcompare(const struct dentry *parent, const struct dentry *dentry,
-   unsigned int len, const char *str, const struct qstr *name)
+static int ll_dcompare(const struct dentry *parent, const struct dentry 
*dentry,
+  unsigned int len, const char *str,
+  const struct qstr *name)
 {
if (len != name-len)
return 1;
@@ -238,7 +238,8 @@ void ll_intent_release(struct lookup_intent *it)
ll_intent_drop_lock(it);
/* We are still holding extra reference on a request, need to free it */
if (it_disposition(it, DISP_ENQ_OPEN_REF))
-ptlrpc_req_finished(it-d.lustre.it_data); /* ll_file_open */
+   ptlrpc_req_finished(it-d.lustre.it_data); /* ll_file_open */
+
if (it_disposition(it, DISP_ENQ_CREATE_REF)) /* create rec */
ptlrpc_req_finished(it-d.lustre.it_data);
 
@@ -316,15 +317,6 @@ void ll_lookup_finish_locks(struct lookup_intent *it, 
struct dentry *dentry)
}
 }
 
-void ll_frob_intent(struct lookup_intent **itp, struct lookup_intent *deft)
-{
-   struct lookup_intent *it = *itp;
-
-   if (!it || it-it_op == IT_GETXATTR)
-   it = *itp = deft;
-
-}
-
 static int ll_revalidate_dentry(struct dentry *dentry,
unsigned int lookup_flags)
 {
@@ -356,7 +348,7 @@ static int ll_revalidate_dentry(struct dentry *dentry,
 /*
  * Always trust cached