| Message ID | 20170224162044.333662852@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > } > > /* > >
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); >> } >> >> /* >> >>
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); >>> } >>> >>> /* >>> >>>
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); >>>> } >>>> >>>> /* >>>> >>>>
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
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
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
--- 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); } /*
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- fs/orangefs/super.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)