Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1587860 > unrolled thread

[RFC][PATCH 07/10] orangefs: Use RCU for destroy_inode

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC][PATCH 07/10] orangefs: Use RCU for destroy_inode Peter Zijlstra <peterz@infradead.org> - 2017-02-24 19:50 +0100
    Re: [RFC][PATCH 07/10] orangefs: Use RCU for destroy_inode Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-24 22:00 +0100
      Re: [RFC][PATCH 07/10] orangefs: Use RCU for destroy_inode Mike Marshall <hubcap@omnibond.com> - 2017-02-25 00:10 +0100
        Re: [RFC][PATCH 07/10] orangefs: Use RCU for destroy_inode Mike Marshall <hubcap@omnibond.com> - 2017-02-25 21:40 +0100
          Re: [RFC][PATCH 07/10] orangefs: Use RCU for destroy_inode Mike Marshall <hubcap@omnibond.com> - 2017-02-27 01:40 +0100
            Re: [RFC][PATCH 07/10] orangefs: Use RCU for destroy_inode Linus Torvalds <torvalds@linux-foundation.org> - 2017-02-27 02:30 +0100
            Re: [RFC][PATCH 07/10] orangefs: Use RCU for destroy_inode David Howells <dhowells@redhat.com> - 2017-02-27 10:00 +0100
              Re: [RFC][PATCH 07/10] orangefs: Use RCU for destroy_inode Mike Marshall <hubcap@omnibond.com> - 2017-02-27 15:50 +0100

#1587860 — [RFC][PATCH 07/10] orangefs: Use RCU for destroy_inode

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 fs/orangefs/super.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

--- a/fs/orangefs/super.c
+++ b/fs/orangefs/super.c
@@ -115,6 +115,13 @@ static struct inode *orangefs_alloc_inod
 	return &orangefs_inode->vfs_inode;
 }
 
+static void orangefs_i_callback(struct rcu_head *head)
+{
+	struct inode *inode = container_of(head, struct inode, i_rcu);
+	struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
+	kmem_cache_free(orangefs_inode_cache, orangefs_inode);
+}
+
 static void orangefs_destroy_inode(struct inode *inode)
 {
 	struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
@@ -123,7 +130,7 @@ static void orangefs_destroy_inode(struc
 			"%s: deallocated %p destroying inode %pU\n",
 			__func__, orangefs_inode, get_khandle_from_ino(inode));
 
-	kmem_cache_free(orangefs_inode_cache, orangefs_inode);
+	call_rcu(&inode->i_rcu, orangefs_i_callback);
 }
 
 /*

[toc] | [next] | [standalone]

#1587937

That, AFAICS, fixes a real bug.  Applied, and it needs Cc:stable as well.


> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  fs/orangefs/super.c |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> --- a/fs/orangefs/super.c
> +++ b/fs/orangefs/super.c
> @@ -115,6 +115,13 @@ static struct inode *orangefs_alloc_inod
>  	return &orangefs_inode->vfs_inode;
>  }
>  
> +static void orangefs_i_callback(struct rcu_head *head)
> +{
> +	struct inode *inode = container_of(head, struct inode, i_rcu);
> +	struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
> +	kmem_cache_free(orangefs_inode_cache, orangefs_inode);
> +}
> +
>  static void orangefs_destroy_inode(struct inode *inode)
>  {
>  	struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
> @@ -123,7 +130,7 @@ static void orangefs_destroy_inode(struc
>  			"%s: deallocated %p destroying inode %pU\n",
>  			__func__, orangefs_inode, get_khandle_from_ino(inode));
>  
> -	kmem_cache_free(orangefs_inode_cache, orangefs_inode);
> +	call_rcu(&inode->i_rcu, orangefs_i_callback);
>  }
>  
>  /*
> 
> 

[toc] | [prev] | [next] | [standalone]

#1587996

Thanks Al... I was going to try and evaluate that patch next
week, now all I have to do is test it <g> ...

-Mike

On Fri, Feb 24, 2017 at 3:52 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> That, AFAICS, fixes a real bug.  Applied, and it needs Cc:stable as well.
>
>
>> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>> ---
>>  fs/orangefs/super.c |    9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> --- a/fs/orangefs/super.c
>> +++ b/fs/orangefs/super.c
>> @@ -115,6 +115,13 @@ static struct inode *orangefs_alloc_inod
>>       return &orangefs_inode->vfs_inode;
>>  }
>>
>> +static void orangefs_i_callback(struct rcu_head *head)
>> +{
>> +     struct inode *inode = container_of(head, struct inode, i_rcu);
>> +     struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
>> +     kmem_cache_free(orangefs_inode_cache, orangefs_inode);
>> +}
>> +
>>  static void orangefs_destroy_inode(struct inode *inode)
>>  {
>>       struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
>> @@ -123,7 +130,7 @@ static void orangefs_destroy_inode(struc
>>                       "%s: deallocated %p destroying inode %pU\n",
>>                       __func__, orangefs_inode, get_khandle_from_ino(inode));
>>
>> -     kmem_cache_free(orangefs_inode_cache, orangefs_inode);
>> +     call_rcu(&inode->i_rcu, orangefs_i_callback);
>>  }
>>
>>  /*
>>
>>

[toc] | [prev] | [next] | [standalone]

#1588220

After looking through the code and seeing how some other filesystems
use call_rcu, it seems that call_rcu has to do with consistency and
waiting for stuff to complete before returning an object to the slab cache,
whereas we were just calling kmem_cache_free without worrying about that
kind of stuff...

Is that a "close enough" description of the error that is being
fixed here?

-Mike

On Fri, Feb 24, 2017 at 6:00 PM, Mike Marshall <hubcap@omnibond.com> wrote:
> Thanks Al... I was going to try and evaluate that patch next
> week, now all I have to do is test it <g> ...
>
> -Mike
>
> On Fri, Feb 24, 2017 at 3:52 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> That, AFAICS, fixes a real bug.  Applied, and it needs Cc:stable as well.
>>
>>
>>> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>>> ---
>>>  fs/orangefs/super.c |    9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> --- a/fs/orangefs/super.c
>>> +++ b/fs/orangefs/super.c
>>> @@ -115,6 +115,13 @@ static struct inode *orangefs_alloc_inod
>>>       return &orangefs_inode->vfs_inode;
>>>  }
>>>
>>> +static void orangefs_i_callback(struct rcu_head *head)
>>> +{
>>> +     struct inode *inode = container_of(head, struct inode, i_rcu);
>>> +     struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
>>> +     kmem_cache_free(orangefs_inode_cache, orangefs_inode);
>>> +}
>>> +
>>>  static void orangefs_destroy_inode(struct inode *inode)
>>>  {
>>>       struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
>>> @@ -123,7 +130,7 @@ static void orangefs_destroy_inode(struc
>>>                       "%s: deallocated %p destroying inode %pU\n",
>>>                       __func__, orangefs_inode, get_khandle_from_ino(inode));
>>>
>>> -     kmem_cache_free(orangefs_inode_cache, orangefs_inode);
>>> +     call_rcu(&inode->i_rcu, orangefs_i_callback);
>>>  }
>>>
>>>  /*
>>>
>>>

[toc] | [prev] | [next] | [standalone]

#1588430

Since Orangefs uses ref-walk, not rcu-walk, this patch with call_rcu
has seemed weird to me.

Using the git log, I searched back to where it seems to me call_rcu was
added, a giant patch from 2005 by David Howells which includes tons of
source and a large amount of documentation.

It seems that the call back function in call_rcu is used to destroy objects
only after readers are known to be finished, the mechanism by which
the readers are known to be finished is described in the documentation.

Perhaps I shouldn't think that Orangefs doesn't use rcu-walk, rather
that it switches to ref-walk from rcu-walk when d_revalidate returns
ECHILD (which it does right away).

-Mike

On Sat, Feb 25, 2017 at 3:31 PM, Mike Marshall <hubcap@omnibond.com> wrote:
> After looking through the code and seeing how some other filesystems
> use call_rcu, it seems that call_rcu has to do with consistency and
> waiting for stuff to complete before returning an object to the slab cache,
> whereas we were just calling kmem_cache_free without worrying about that
> kind of stuff...
>
> Is that a "close enough" description of the error that is being
> fixed here?
>
> -Mike
>
> On Fri, Feb 24, 2017 at 6:00 PM, Mike Marshall <hubcap@omnibond.com> wrote:
>> Thanks Al... I was going to try and evaluate that patch next
>> week, now all I have to do is test it <g> ...
>>
>> -Mike
>>
>> On Fri, Feb 24, 2017 at 3:52 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>>> That, AFAICS, fixes a real bug.  Applied, and it needs Cc:stable as well.
>>>
>>>
>>>> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>>>> ---
>>>>  fs/orangefs/super.c |    9 ++++++++-
>>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> --- a/fs/orangefs/super.c
>>>> +++ b/fs/orangefs/super.c
>>>> @@ -115,6 +115,13 @@ static struct inode *orangefs_alloc_inod
>>>>       return &orangefs_inode->vfs_inode;
>>>>  }
>>>>
>>>> +static void orangefs_i_callback(struct rcu_head *head)
>>>> +{
>>>> +     struct inode *inode = container_of(head, struct inode, i_rcu);
>>>> +     struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
>>>> +     kmem_cache_free(orangefs_inode_cache, orangefs_inode);
>>>> +}
>>>> +
>>>>  static void orangefs_destroy_inode(struct inode *inode)
>>>>  {
>>>>       struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
>>>> @@ -123,7 +130,7 @@ static void orangefs_destroy_inode(struc
>>>>                       "%s: deallocated %p destroying inode %pU\n",
>>>>                       __func__, orangefs_inode, get_khandle_from_ino(inode));
>>>>
>>>> -     kmem_cache_free(orangefs_inode_cache, orangefs_inode);
>>>> +     call_rcu(&inode->i_rcu, orangefs_i_callback);
>>>>  }
>>>>
>>>>  /*
>>>>
>>>>

[toc] | [prev] | [next] | [standalone]

#1588464

On Sun, Feb 26, 2017 at 4:34 PM, Mike Marshall <hubcap@omnibond.com> wrote:
> Since Orangefs uses ref-walk, not rcu-walk, this patch with call_rcu
> has seemed weird to me.

Even if orangefs never really allows RCU walking, the VFS layer will
look up dentries - and look at their inodes - from RCU. It will then
call into the filesystem

So even if orangefs always returned ECHILD (it doesn't - it has a
timeout) - we'd be following the dentry inode pointer in jus a RCU
read region.

           Linus

[toc] | [prev] | [next] | [standalone]

#1588553

Mike Marshall <hubcap@omnibond.com> wrote:

> Using the git log, I searched back to where it seems to me call_rcu was
> added, a giant patch from 2005 by David Howells which includes tons of
> source and a large amount of documentation.

I'm pretty sure Paul McKenney added call_rcu(), not me.

David

[toc] | [prev] | [next] | [standalone]

#1588694

Hi... sorry if I got the attribution wrong... the commit I studied
was:

commit 76d8aeabfeb1c42641a81c44280177b9a08670d8
Author: David Howells <dhowells@redhat.com>
Date:   Thu Jun 23 22:00:49 2005 -0700

It was huge <g> ...

-Mike


On Mon, Feb 27, 2017 at 3:44 AM, David Howells <dhowells@redhat.com> wrote:
> Mike Marshall <hubcap@omnibond.com> wrote:
>
>> Using the git log, I searched back to where it seems to me call_rcu was
>> added, a giant patch from 2005 by David Howells which includes tons of
>> source and a large amount of documentation.
>
> I'm pretty sure Paul McKenney added call_rcu(), not me.
>
> David

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web