Re: [PATCH 4/4] staging: lustre: obdclass: change object lookup to no wait mode

[James Simmons]

> > > Anyway, I understand that Intel has been ignoring kernel.org instead of
> > > sending forwarding their patches properly so you're doing a difficult
> > > and thankless job...  Thanks for that.  I'm sure it's frustrating to
> > > look at these patches for you as well.
> > 
> > Thank you for the complement. Also thank you for taking time to review
> > these patches. Your feedback is most welcomed and benefitical to the
> > health of the lustre client.
> > 
> > Sadly its not just Intel but other vendors that don't directly contribute
> > to the linux lustre client. I have spoke to the vendors about contributing 
> > and they all say the same thing. No working with drivers in the staging 
> > tree. Sadly all the parties involved are very interested in the success 
> > of the lustre client. No one has ever told me directly why they don't get
> > involved but I suspect it has to deal with 2 reasons. One is that staging
> > drivers are not normally enabled by distributions so their clients 
> > normally will never deal with the staging lustre client. Secondly vendors
> > just lack the man power to contribute in a meanful way.
> 
> If staging is hurting you, why is it in staging at all?  Why not just
> drop it, go off and spend a few months to clean up all the issues in
> your own tree (with none of those pesky requirements of easy-to-review
> patches) and then submit a "clean" filesystem for inclusion in the
> "real" part of the kernel tree?
> 
> It doesn't sound like anyone is actually using this code in the tree
> as-is, so why even keep it here?

I never said being in staging is hurting the progression of Lustre. In 
fact it is the exact opposite otherwise I wouldn't be active in this work.
What I was pointing out to Dan was that many vendors are reluctant to 
partcipate in broader open source development of this type.

The whole point of this is to evolve Lustre into a proper open source 
project not dependent on vendors for survival. Several years ago Lustre 
changed hands several times and the HPC community was worried about its
survival. Various institutions band togther to raise the resources to 
keep it alive. Over time Lustre has been migrating to a more open source 
community effort. An awesome example is the work the University of Indiana 
did for the sptlrpc layer. Now we see efforts expanding into the realm of 
the linux lustre client. Actually HPC sites that are community members are 
testing and running the linux client. In spite of the lack of vendor 
involvement the linux lustre client is making excellent progress. How 
often do you see style patches anymore? The headers are properly split
between userspace UAPI headers and kernel space. One of the major barriers
to leave staging was the the lack of a strong presence to continue moving
the lustre client forward. That is no longer the case. The finish line is
in view.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] staging: lustre: obdclass: change object lookup to no wait mode

[Joe Perches]

On Wed, 2018-05-16 at 09:12 +, Dilger, Andreas wrote:
> On May 16, 2018, at 02:00, Dan Carpenter  wrote:
> > 
> > On Tue, May 15, 2018 at 04:02:55PM +0100, James Simmons wrote:
> > > 
> > > > >   /*
> > > > >* Allocate new object. This may result in rather complicated
> > > > >* operations, including fld queries, inode loading, etc.
> > > > >*/
> > > > >   o = lu_object_alloc(env, dev, f, conf);
> > > > > - if (IS_ERR(o))
> > > > > + if (unlikely(IS_ERR(o)))
> > > > >   return o;
> > > > > 
> > > > 
> > > > This is an unrelated and totally pointless.  likely/unlikely annotations
> > > > hurt readability, and they should only be added if it's something which
> > > > is going to show up in benchmarking.  lu_object_alloc() is already too
> > > > slow for the unlikely() to make a difference and anyway IS_ERR() has an
> > > > unlikely built in so it's duplicative...
> > > 
> > > Sounds like a good checkpatch case to test for :-)
> > 
> > The likely/unlikely annotations have their place in fast paths so a
> > checkpatch warning would get annoying...
> 
> I think James was suggesting a check for unlikely(IS_ERR()),

Probably so.

$ git grep -P 'likely\s*\(\s*\!?\s*IS_ERR' | wc -l
42

Are there other known likely/unlikely duplications?

>  or possibly
> a check for unlikely() on something that is already unlikely() after CPP
> expansion.

checkpatch isn't the tool for that type of test
as it is a collection of trivial regex tests and
it is not a c90 preprocessor.

Anyway, here's a possible checkpatch patch.
---
 scripts/checkpatch.pl | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index baddac9379f0..20c0973f1c39 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6299,6 +6299,12 @@ sub process {
  "#define of '$1' is wrong - use Kconfig variables 
or standard guards instead\n" . $herecurr);
}
 
+# likely/unlikely tests with IS_ERR (already unlikely)"
+   if ($line =~ 
/\b((?:un)?likely)\s*\(\s*\!?\s*(IS_ERR[A-Z_]*)\s*\(/) {
+   WARN("DUPLICATE_LIKELY",
+"Unnecessary use of $1 with $2 as it already has 
an unlikely\n" . $herecurr);
+   }
+
 # likely/unlikely comparisons similar to "(likely(foo) > 0)"
if ($^V && $^V ge 5.10.0 &&
$line =~ 
/\b((?:un)?likely)\s*\(\s*$FuncArg\s*\)\s*$Compare/) {


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] staging: lustre: obdclass: change object lookup to no wait mode

[Greg Kroah-Hartman]

On Tue, May 15, 2018 at 04:02:55PM +0100, James Simmons wrote:
> > >   /*
> > >* Allocate new object. This may result in rather complicated
> > >* operations, including fld queries, inode loading, etc.
> > >*/
> > >   o = lu_object_alloc(env, dev, f, conf);
> > > - if (IS_ERR(o))
> > > + if (unlikely(IS_ERR(o)))
> > >   return o;
> > >  
> > 
> > This is an unrelated and totally pointless.  likely/unlikely annotations
> > hurt readability, and they should only be added if it's something which
> > is going to show up in benchmarking.  lu_object_alloc() is already too
> > slow for the unlikely() to make a difference and anyway IS_ERR() has an
> > unlikely built in so it's duplicative...
> 
> Sounds like a good checkpatch case to test for :-) Some people like to try
> and milk ever cycle they can. Personally for me I never use those 
> annotations. With modern processors I'm skeptical if their benefits.
> I do cleanup up the patches to some extent to make it compliant with 
> kernel standards but leave the core code in place for people to comment 
> on.
> 
> > Anyway, I understand that Intel has been ignoring kernel.org instead of
> > sending forwarding their patches properly so you're doing a difficult
> > and thankless job...  Thanks for that.  I'm sure it's frustrating to
> > look at these patches for you as well.
> 
> Thank you for the complement. Also thank you for taking time to review
> these patches. Your feedback is most welcomed and benefitical to the
> health of the lustre client.
> 
> Sadly its not just Intel but other vendors that don't directly contribute
> to the linux lustre client. I have spoke to the vendors about contributing 
> and they all say the same thing. No working with drivers in the staging 
> tree. Sadly all the parties involved are very interested in the success 
> of the lustre client. No one has ever told me directly why they don't get
> involved but I suspect it has to deal with 2 reasons. One is that staging
> drivers are not normally enabled by distributions so their clients 
> normally will never deal with the staging lustre client. Secondly vendors
> just lack the man power to contribute in a meanful way.

If staging is hurting you, why is it in staging at all?  Why not just
drop it, go off and spend a few months to clean up all the issues in
your own tree (with none of those pesky requirements of easy-to-review
patches) and then submit a "clean" filesystem for inclusion in the
"real" part of the kernel tree?

It doesn't sound like anyone is actually using this code in the tree
as-is, so why even keep it here?

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] staging: lustre: obdclass: change object lookup to no wait mode

[Dilger, Andreas]

On May 16, 2018, at 02:00, Dan Carpenter  wrote:
> 
> On Tue, May 15, 2018 at 04:02:55PM +0100, James Simmons wrote:
>> 
/*
 * Allocate new object. This may result in rather complicated
 * operations, including fld queries, inode loading, etc.
 */
o = lu_object_alloc(env, dev, f, conf);
 -  if (IS_ERR(o))
 +  if (unlikely(IS_ERR(o)))
return o;
 
>>> 
>>> This is an unrelated and totally pointless.  likely/unlikely annotations
>>> hurt readability, and they should only be added if it's something which
>>> is going to show up in benchmarking.  lu_object_alloc() is already too
>>> slow for the unlikely() to make a difference and anyway IS_ERR() has an
>>> unlikely built in so it's duplicative...
>> 
>> Sounds like a good checkpatch case to test for :-)
> 
> The likely/unlikely annotations have their place in fast paths so a
> checkpatch warning would get annoying...

I think James was suggesting a check for unlikely(IS_ERR()), or possibly
a check for unlikely() on something that is already unlikely() after CPP
expansion.

Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation







___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] staging: lustre: obdclass: change object lookup to no wait mode

[Dan Carpenter]

On Tue, May 15, 2018 at 04:02:55PM +0100, James Simmons wrote:
> 
> > >   /*
> > >* Allocate new object. This may result in rather complicated
> > >* operations, including fld queries, inode loading, etc.
> > >*/
> > >   o = lu_object_alloc(env, dev, f, conf);
> > > - if (IS_ERR(o))
> > > + if (unlikely(IS_ERR(o)))
> > >   return o;
> > >  
> > 
> > This is an unrelated and totally pointless.  likely/unlikely annotations
> > hurt readability, and they should only be added if it's something which
> > is going to show up in benchmarking.  lu_object_alloc() is already too
> > slow for the unlikely() to make a difference and anyway IS_ERR() has an
> > unlikely built in so it's duplicative...
> 
> Sounds like a good checkpatch case to test for :-)

The likely/unlikely annotations have their place in fast paths so a
checkpatch warning would get annoying...

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] staging: lustre: obdclass: change object lookup to no wait mode

[James Simmons]

> > /*
> >  * Allocate new object. This may result in rather complicated
> >  * operations, including fld queries, inode loading, etc.
> >  */
> > o = lu_object_alloc(env, dev, f, conf);
> > -   if (IS_ERR(o))
> > +   if (unlikely(IS_ERR(o)))
> > return o;
> >  
> 
> This is an unrelated and totally pointless.  likely/unlikely annotations
> hurt readability, and they should only be added if it's something which
> is going to show up in benchmarking.  lu_object_alloc() is already too
> slow for the unlikely() to make a difference and anyway IS_ERR() has an
> unlikely built in so it's duplicative...

Sounds like a good checkpatch case to test for :-) Some people like to try
and milk ever cycle they can. Personally for me I never use those 
annotations. With modern processors I'm skeptical if their benefits.
I do cleanup up the patches to some extent to make it compliant with 
kernel standards but leave the core code in place for people to comment 
on.

> Anyway, I understand that Intel has been ignoring kernel.org instead of
> sending forwarding their patches properly so you're doing a difficult
> and thankless job...  Thanks for that.  I'm sure it's frustrating to
> look at these patches for you as well.

Thank you for the complement. Also thank you for taking time to review
these patches. Your feedback is most welcomed and benefitical to the
health of the lustre client.

Sadly its not just Intel but other vendors that don't directly contribute
to the linux lustre client. I have spoke to the vendors about contributing 
and they all say the same thing. No working with drivers in the staging 
tree. Sadly all the parties involved are very interested in the success 
of the lustre client. No one has ever told me directly why they don't get
involved but I suspect it has to deal with 2 reasons. One is that staging
drivers are not normally enabled by distributions so their clients 
normally will never deal with the staging lustre client. Secondly vendors
just lack the man power to contribute in a meanful way.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] staging: lustre: obdclass: change object lookup to no wait mode

[James Simmons]

> >> On Wed, May 02 2018, James Simmons wrote:
> >> 
> >> > From: Lai Siyao 
> >> >
> >> > Currently we set LU_OBJECT_HEARD_BANSHEE on object when we want
> >> > to remove object from cache, but this may lead to deadlock, because
> >> > when other process lookup such object, it needs to wait for this
> >> > object until release (done at last refcount put), while that process
> >> > maybe already hold an LDLM lock.
> >> >
> >> > Now that current code can handle dying object correctly, we can just
> >> > return such object in lookup, thus the above deadlock can be avoided.
> >> 
> >> I think one of the reasons that I didn't apply this to mainline myself
> >> is that "Now that" comment.  When is the "now" that it is referring to?
> >> Are were sure that all code in mainline "can handle dying objects
> >> correctly"??
> >
> > So I talked to Lai and he posted the LU-9049 ticket what patches need to
> > land before this one. Only one patch is of concern and its for LU-9203
> > which doesn't apply to the staging tree since we don't have the LNet SMP
> > updates in our tree. I saved notes about making sure LU-9203 lands 
> > together with the future LNet SMP changes. As it stands it is safe to
> > land to staging.
> 
> Thanks a lot for looking into this.  Nice to have the safety of this
> change confirmed.
> 
> What do you think of:
> 
> >> > @@ -713,36 +691,46 @@ struct lu_object *lu_object_find_at(const struct 
> >> > lu_env *env,
> >> >   * It is unnecessary to perform lookup-alloc-lookup-insert, 
> >> > instead,
> >> >   * just alloc and insert directly.
> >> >   *
> >> > + * If dying object is found during index search, add @waiter to 
> >> > the
> >> > + * site wait-queue and return ERR_PTR(-EAGAIN).
> >> 
> >> It seems odd to add this comment here, when it seems to describe code
> >> that is being removed.
> >> I can see that this comment is added by the upstream patch
> >> Commit: fa14bdf6b648 ("LU-9049 obdclass: change object lookup to no wait 
> >> mode")
> >> but I cannot see what it refers to.
> >> 
> 
> ??
> 
> Am I misunderstanding something, or is that comment wrong?

I think the comment is wrong. That comment was in the other tree before 
the patch was landed. It got included with this push due to me diffing the
tree by accident. I will remove it with the next push.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] staging: lustre: obdclass: change object lookup to no wait mode

[NeilBrown]

On Tue, May 15 2018, James Simmons wrote:

>> On Wed, May 02 2018, James Simmons wrote:
>> 
>> > From: Lai Siyao 
>> >
>> > Currently we set LU_OBJECT_HEARD_BANSHEE on object when we want
>> > to remove object from cache, but this may lead to deadlock, because
>> > when other process lookup such object, it needs to wait for this
>> > object until release (done at last refcount put), while that process
>> > maybe already hold an LDLM lock.
>> >
>> > Now that current code can handle dying object correctly, we can just
>> > return such object in lookup, thus the above deadlock can be avoided.
>> 
>> I think one of the reasons that I didn't apply this to mainline myself
>> is that "Now that" comment.  When is the "now" that it is referring to?
>> Are were sure that all code in mainline "can handle dying objects
>> correctly"??
>
> So I talked to Lai and he posted the LU-9049 ticket what patches need to
> land before this one. Only one patch is of concern and its for LU-9203
> which doesn't apply to the staging tree since we don't have the LNet SMP
> updates in our tree. I saved notes about making sure LU-9203 lands 
> together with the future LNet SMP changes. As it stands it is safe to
> land to staging.

Thanks a lot for looking into this.  Nice to have the safety of this
change confirmed.

What do you think of:

>> > @@ -713,36 +691,46 @@ struct lu_object *lu_object_find_at(const struct 
>> > lu_env *env,
>> > * It is unnecessary to perform lookup-alloc-lookup-insert, instead,
>> > * just alloc and insert directly.
>> > *
>> > +   * If dying object is found during index search, add @waiter to the
>> > +   * site wait-queue and return ERR_PTR(-EAGAIN).
>> 
>> It seems odd to add this comment here, when it seems to describe code
>> that is being removed.
>> I can see that this comment is added by the upstream patch
>> Commit: fa14bdf6b648 ("LU-9049 obdclass: change object lookup to no wait 
>> mode")
>> but I cannot see what it refers to.
>> 

??

Am I misunderstanding something, or is that comment wrong?

Thanks,
NeilBrown


signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] staging: lustre: obdclass: change object lookup to no wait mode

[James Simmons]

> On Wed, May 02 2018, James Simmons wrote:
> 
> > From: Lai Siyao 
> >
> > Currently we set LU_OBJECT_HEARD_BANSHEE on object when we want
> > to remove object from cache, but this may lead to deadlock, because
> > when other process lookup such object, it needs to wait for this
> > object until release (done at last refcount put), while that process
> > maybe already hold an LDLM lock.
> >
> > Now that current code can handle dying object correctly, we can just
> > return such object in lookup, thus the above deadlock can be avoided.
> 
> I think one of the reasons that I didn't apply this to mainline myself
> is that "Now that" comment.  When is the "now" that it is referring to?
> Are were sure that all code in mainline "can handle dying objects
> correctly"??

So I talked to Lai and he posted the LU-9049 ticket what patches need to
land before this one. Only one patch is of concern and its for LU-9203
which doesn't apply to the staging tree since we don't have the LNet SMP
updates in our tree. I saved notes about making sure LU-9203 lands 
together with the future LNet SMP changes. As it stands it is safe to
land to staging.
 
> > Signed-off-by: Lai Siyao 
> > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-9049
> > Reviewed-on: https://review.whamcloud.com/26965
> > Reviewed-by: Alex Zhuravlev 
> > Tested-by: Cliff White 
> > Reviewed-by: Fan Yong 
> > Reviewed-by: Oleg Drokin 
> > Signed-off-by: James Simmons 
> > ---
> >  drivers/staging/lustre/lustre/include/lu_object.h  |  2 +-
> >  drivers/staging/lustre/lustre/obdclass/lu_object.c | 82 
> > +-
> >  2 files changed, 36 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/staging/lustre/lustre/include/lu_object.h 
> > b/drivers/staging/lustre/lustre/include/lu_object.h
> > index f29bbca..232063a 100644
> > --- a/drivers/staging/lustre/lustre/include/lu_object.h
> > +++ b/drivers/staging/lustre/lustre/include/lu_object.h
> > @@ -673,7 +673,7 @@ static inline void lu_object_get(struct lu_object *o)
> >  }
> >  
> >  /**
> > - * Return true of object will not be cached after last reference to it is
> > + * Return true if object will not be cached after last reference to it is
> >   * released.
> >   */
> >  static inline int lu_object_is_dying(const struct lu_object_header *h)
> > diff --git a/drivers/staging/lustre/lustre/obdclass/lu_object.c 
> > b/drivers/staging/lustre/lustre/obdclass/lu_object.c
> > index 8b507f1..9311703 100644
> > --- a/drivers/staging/lustre/lustre/obdclass/lu_object.c
> > +++ b/drivers/staging/lustre/lustre/obdclass/lu_object.c
> > @@ -589,19 +589,13 @@ static struct lu_object *htable_lookup(struct lu_site 
> > *s,
> >const struct lu_fid *f,
> >__u64 *version)
> >  {
> > -   struct cfs_hash *hs = s->ls_obj_hash;
> > struct lu_site_bkt_data *bkt;
> > struct lu_object_header *h;
> > struct hlist_node   *hnode;
> > -   __u64 ver;
> > -   wait_queue_entry_t waiter;
> > +   u64 ver = cfs_hash_bd_version_get(bd);
> >  
> > -retry:
> > -   ver = cfs_hash_bd_version_get(bd);
> > -
> > -   if (*version == ver) {
> > +   if (*version == ver)
> > return ERR_PTR(-ENOENT);
> > -   }
> >  
> > *version = ver;
> > bkt = cfs_hash_bd_extra_get(s->ls_obj_hash, bd);
> > @@ -615,31 +609,13 @@ static struct lu_object *htable_lookup(struct lu_site 
> > *s,
> > }
> >  
> > h = container_of(hnode, struct lu_object_header, loh_hash);
> > -   if (likely(!lu_object_is_dying(h))) {
> > -   cfs_hash_get(s->ls_obj_hash, hnode);
> > -   lprocfs_counter_incr(s->ls_stats, LU_SS_CACHE_HIT);
> > -   if (!list_empty(>loh_lru)) {
> > -   list_del_init(>loh_lru);
> > -   percpu_counter_dec(>ls_lru_len_counter);
> > -   }
> > -   return lu_object_top(h);
> > +   cfs_hash_get(s->ls_obj_hash, hnode);
> > +   lprocfs_counter_incr(s->ls_stats, LU_SS_CACHE_HIT);
> > +   if (!list_empty(>loh_lru)) {
> > +   list_del_init(>loh_lru);
> > +   percpu_counter_dec(>ls_lru_len_counter);
> > }
> > -
> > -   /*
> > -* Lookup found an object being destroyed this object cannot be
> > -* returned (to assure that references to dying objects are eventually
> > -* drained), and moreover, lookup has to wait until object is freed.
> > -*/
> > -
> > -   init_waitqueue_entry(, current);
> > -   add_wait_queue(>lsb_marche_funebre, );
> > -   set_current_state(TASK_UNINTERRUPTIBLE);
> > -   lprocfs_counter_incr(s->ls_stats, LU_SS_CACHE_DEATH_RACE);
> > -   cfs_hash_bd_unlock(hs, bd, 1);
> > -   schedule();
> > -   remove_wait_queue(>lsb_marche_funebre, );
> > -   cfs_hash_bd_lock(hs, bd, 1);
> > -   goto retry;
> > +   return lu_object_top(h);
> >  }
> >  
> >  /**
> > @@ 

Re: [PATCH 4/4] staging: lustre: obdclass: change object lookup to no wait mode

[Dan Carpenter]

>   /*
>* Allocate new object. This may result in rather complicated
>* operations, including fld queries, inode loading, etc.
>*/
>   o = lu_object_alloc(env, dev, f, conf);
> - if (IS_ERR(o))
> + if (unlikely(IS_ERR(o)))
>   return o;
>  

This is an unrelated and totally pointless.  likely/unlikely annotations
hurt readability, and they should only be added if it's something which
is going to show up in benchmarking.  lu_object_alloc() is already too
slow for the unlikely() to make a difference and anyway IS_ERR() has an
unlikely built in so it's duplicative...

Anyway, I understand that Intel has been ignoring kernel.org instead of
sending forwarding their patches properly so you're doing a difficult
and thankless job...  Thanks for that.  I'm sure it's frustrating to
look at these patches for you as well.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] staging: lustre: obdclass: change object lookup to no wait mode

[Greg Kroah-Hartman]

On Wed, May 02, 2018 at 02:21:48PM -0400, James Simmons wrote:
> From: Lai Siyao 
> 
> Currently we set LU_OBJECT_HEARD_BANSHEE on object when we want
> to remove object from cache, but this may lead to deadlock, because
> when other process lookup such object, it needs to wait for this
> object until release (done at last refcount put), while that process
> maybe already hold an LDLM lock.
> 
> Now that current code can handle dying object correctly, we can just
> return such object in lookup, thus the above deadlock can be avoided.
> 
> Signed-off-by: Lai Siyao 
> Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-9049
> Reviewed-on: https://review.whamcloud.com/26965
> Reviewed-by: Alex Zhuravlev 
> Tested-by: Cliff White 
> Reviewed-by: Fan Yong 
> Reviewed-by: Oleg Drokin 
> Signed-off-by: James Simmons 
> ---
>  drivers/staging/lustre/lustre/include/lu_object.h  |  2 +-
>  drivers/staging/lustre/lustre/obdclass/lu_object.c | 82 
> +-
>  2 files changed, 36 insertions(+), 48 deletions(-)

Patch does not apply to my tree :(
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] staging: lustre: obdclass: change object lookup to no wait mode

[NeilBrown]

On Wed, May 02 2018, James Simmons wrote:

> From: Lai Siyao 
>
> Currently we set LU_OBJECT_HEARD_BANSHEE on object when we want
> to remove object from cache, but this may lead to deadlock, because
> when other process lookup such object, it needs to wait for this
> object until release (done at last refcount put), while that process
> maybe already hold an LDLM lock.
>
> Now that current code can handle dying object correctly, we can just
> return such object in lookup, thus the above deadlock can be avoided.

I think one of the reasons that I didn't apply this to mainline myself
is that "Now that" comment.  When is the "now" that it is referring to?
Are were sure that all code in mainline "can handle dying objects
correctly"??


>
> Signed-off-by: Lai Siyao 
> Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-9049
> Reviewed-on: https://review.whamcloud.com/26965
> Reviewed-by: Alex Zhuravlev 
> Tested-by: Cliff White 
> Reviewed-by: Fan Yong 
> Reviewed-by: Oleg Drokin 
> Signed-off-by: James Simmons 
> ---
>  drivers/staging/lustre/lustre/include/lu_object.h  |  2 +-
>  drivers/staging/lustre/lustre/obdclass/lu_object.c | 82 
> +-
>  2 files changed, 36 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/include/lu_object.h 
> b/drivers/staging/lustre/lustre/include/lu_object.h
> index f29bbca..232063a 100644
> --- a/drivers/staging/lustre/lustre/include/lu_object.h
> +++ b/drivers/staging/lustre/lustre/include/lu_object.h
> @@ -673,7 +673,7 @@ static inline void lu_object_get(struct lu_object *o)
>  }
>  
>  /**
> - * Return true of object will not be cached after last reference to it is
> + * Return true if object will not be cached after last reference to it is
>   * released.
>   */
>  static inline int lu_object_is_dying(const struct lu_object_header *h)
> diff --git a/drivers/staging/lustre/lustre/obdclass/lu_object.c 
> b/drivers/staging/lustre/lustre/obdclass/lu_object.c
> index 8b507f1..9311703 100644
> --- a/drivers/staging/lustre/lustre/obdclass/lu_object.c
> +++ b/drivers/staging/lustre/lustre/obdclass/lu_object.c
> @@ -589,19 +589,13 @@ static struct lu_object *htable_lookup(struct lu_site 
> *s,
>  const struct lu_fid *f,
>  __u64 *version)
>  {
> - struct cfs_hash *hs = s->ls_obj_hash;
>   struct lu_site_bkt_data *bkt;
>   struct lu_object_header *h;
>   struct hlist_node   *hnode;
> - __u64 ver;
> - wait_queue_entry_t waiter;
> + u64 ver = cfs_hash_bd_version_get(bd);
>  
> -retry:
> - ver = cfs_hash_bd_version_get(bd);
> -
> - if (*version == ver) {
> + if (*version == ver)
>   return ERR_PTR(-ENOENT);
> - }
>  
>   *version = ver;
>   bkt = cfs_hash_bd_extra_get(s->ls_obj_hash, bd);
> @@ -615,31 +609,13 @@ static struct lu_object *htable_lookup(struct lu_site 
> *s,
>   }
>  
>   h = container_of(hnode, struct lu_object_header, loh_hash);
> - if (likely(!lu_object_is_dying(h))) {
> - cfs_hash_get(s->ls_obj_hash, hnode);
> - lprocfs_counter_incr(s->ls_stats, LU_SS_CACHE_HIT);
> - if (!list_empty(>loh_lru)) {
> - list_del_init(>loh_lru);
> - percpu_counter_dec(>ls_lru_len_counter);
> - }
> - return lu_object_top(h);
> + cfs_hash_get(s->ls_obj_hash, hnode);
> + lprocfs_counter_incr(s->ls_stats, LU_SS_CACHE_HIT);
> + if (!list_empty(>loh_lru)) {
> + list_del_init(>loh_lru);
> + percpu_counter_dec(>ls_lru_len_counter);
>   }
> -
> - /*
> -  * Lookup found an object being destroyed this object cannot be
> -  * returned (to assure that references to dying objects are eventually
> -  * drained), and moreover, lookup has to wait until object is freed.
> -  */
> -
> - init_waitqueue_entry(, current);
> - add_wait_queue(>lsb_marche_funebre, );
> - set_current_state(TASK_UNINTERRUPTIBLE);
> - lprocfs_counter_incr(s->ls_stats, LU_SS_CACHE_DEATH_RACE);
> - cfs_hash_bd_unlock(hs, bd, 1);
> - schedule();
> - remove_wait_queue(>lsb_marche_funebre, );
> - cfs_hash_bd_lock(hs, bd, 1);
> - goto retry;
> + return lu_object_top(h);
>  }
>  
>  /**
> @@ -680,6 +656,8 @@ static void lu_object_limit(const struct lu_env *env, 
> struct lu_device *dev)
>  }
>  
>  /**
> + * Core logic of lu_object_find*() functions.
> + *
>   * Much like lu_object_find(), but top level device of object is specifically
>   * \a dev rather than top level device of the site. This interface allows
>   * objects of different "stacking" to be created within the same site.
> @@ -713,36 +691,46 @@ struct lu_object *lu_object_find_at(const struct lu_env 
> *env,
>* It 

[PATCH 4/4] staging: lustre: obdclass: change object lookup to no wait mode

[James Simmons]

From: Lai Siyao 

Currently we set LU_OBJECT_HEARD_BANSHEE on object when we want
to remove object from cache, but this may lead to deadlock, because
when other process lookup such object, it needs to wait for this
object until release (done at last refcount put), while that process
maybe already hold an LDLM lock.

Now that current code can handle dying object correctly, we can just
return such object in lookup, thus the above deadlock can be avoided.

Signed-off-by: Lai Siyao 
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-9049
Reviewed-on: https://review.whamcloud.com/26965
Reviewed-by: Alex Zhuravlev 
Tested-by: Cliff White 
Reviewed-by: Fan Yong 
Reviewed-by: Oleg Drokin 
Signed-off-by: James Simmons 
---
 drivers/staging/lustre/lustre/include/lu_object.h  |  2 +-
 drivers/staging/lustre/lustre/obdclass/lu_object.c | 82 +-
 2 files changed, 36 insertions(+), 48 deletions(-)

diff --git a/drivers/staging/lustre/lustre/include/lu_object.h 
b/drivers/staging/lustre/lustre/include/lu_object.h
index f29bbca..232063a 100644
--- a/drivers/staging/lustre/lustre/include/lu_object.h
+++ b/drivers/staging/lustre/lustre/include/lu_object.h
@@ -673,7 +673,7 @@ static inline void lu_object_get(struct lu_object *o)
 }
 
 /**
- * Return true of object will not be cached after last reference to it is
+ * Return true if object will not be cached after last reference to it is
  * released.
  */
 static inline int lu_object_is_dying(const struct lu_object_header *h)
diff --git a/drivers/staging/lustre/lustre/obdclass/lu_object.c 
b/drivers/staging/lustre/lustre/obdclass/lu_object.c
index 8b507f1..9311703 100644
--- a/drivers/staging/lustre/lustre/obdclass/lu_object.c
+++ b/drivers/staging/lustre/lustre/obdclass/lu_object.c
@@ -589,19 +589,13 @@ static struct lu_object *htable_lookup(struct lu_site *s,
   const struct lu_fid *f,
   __u64 *version)
 {
-   struct cfs_hash *hs = s->ls_obj_hash;
struct lu_site_bkt_data *bkt;
struct lu_object_header *h;
struct hlist_node   *hnode;
-   __u64 ver;
-   wait_queue_entry_t waiter;
+   u64 ver = cfs_hash_bd_version_get(bd);
 
-retry:
-   ver = cfs_hash_bd_version_get(bd);
-
-   if (*version == ver) {
+   if (*version == ver)
return ERR_PTR(-ENOENT);
-   }
 
*version = ver;
bkt = cfs_hash_bd_extra_get(s->ls_obj_hash, bd);
@@ -615,31 +609,13 @@ static struct lu_object *htable_lookup(struct lu_site *s,
}
 
h = container_of(hnode, struct lu_object_header, loh_hash);
-   if (likely(!lu_object_is_dying(h))) {
-   cfs_hash_get(s->ls_obj_hash, hnode);
-   lprocfs_counter_incr(s->ls_stats, LU_SS_CACHE_HIT);
-   if (!list_empty(>loh_lru)) {
-   list_del_init(>loh_lru);
-   percpu_counter_dec(>ls_lru_len_counter);
-   }
-   return lu_object_top(h);
+   cfs_hash_get(s->ls_obj_hash, hnode);
+   lprocfs_counter_incr(s->ls_stats, LU_SS_CACHE_HIT);
+   if (!list_empty(>loh_lru)) {
+   list_del_init(>loh_lru);
+   percpu_counter_dec(>ls_lru_len_counter);
}
-
-   /*
-* Lookup found an object being destroyed this object cannot be
-* returned (to assure that references to dying objects are eventually
-* drained), and moreover, lookup has to wait until object is freed.
-*/
-
-   init_waitqueue_entry(, current);
-   add_wait_queue(>lsb_marche_funebre, );
-   set_current_state(TASK_UNINTERRUPTIBLE);
-   lprocfs_counter_incr(s->ls_stats, LU_SS_CACHE_DEATH_RACE);
-   cfs_hash_bd_unlock(hs, bd, 1);
-   schedule();
-   remove_wait_queue(>lsb_marche_funebre, );
-   cfs_hash_bd_lock(hs, bd, 1);
-   goto retry;
+   return lu_object_top(h);
 }
 
 /**
@@ -680,6 +656,8 @@ static void lu_object_limit(const struct lu_env *env, 
struct lu_device *dev)
 }
 
 /**
+ * Core logic of lu_object_find*() functions.
+ *
  * Much like lu_object_find(), but top level device of object is specifically
  * \a dev rather than top level device of the site. This interface allows
  * objects of different "stacking" to be created within the same site.
@@ -713,36 +691,46 @@ struct lu_object *lu_object_find_at(const struct lu_env 
*env,
 * It is unnecessary to perform lookup-alloc-lookup-insert, instead,
 * just alloc and insert directly.
 *
+* If dying object is found during index search, add @waiter to the
+* site wait-queue and return ERR_PTR(-EAGAIN).
 */
-   s  = dev->ld_site;
-   hs = s->ls_obj_hash;
+   if (conf && conf->loc_flags & LOC_F_NEW) {
+   o = lu_object_alloc(env,