On 25 April 2012 22:44, Miklos Szeredi <[email protected]> wrote:
> From: Miklos Szeredi <[email protected]>
>
> Use helper variable instead of path->dentry->d_inode before complete_walk().
> This will allow this code to be used in RCU mode.
What do you mean, allow it to be used?
>
> Signed-off-by: Miklos Szeredi <[email protected]>
> ---
> ?fs/namei.c | ? ?7 ++++---
> ?1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 46d4bf6..f21ddb3 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2360,15 +2360,16 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
> ? ? ? ?if (error)
> ? ? ? ? ? ? ? ?nd->flags |= LOOKUP_JUMPED;
>
> + ? ? ? inode = path->dentry->d_inode;
> ? ? ? ?error = -ENOENT;
> - ? ? ? if (!path->dentry->d_inode)
> + ? ? ? if (!inode)
> ? ? ? ? ? ? ? ?goto exit_dput;
>
> - ? ? ? if (path->dentry->d_inode->i_op->follow_link)
> + ? ? ? if (inode->i_op->follow_link)
> ? ? ? ? ? ? ? ?return NULL;
>
> ? ? ? ?path_to_nameidata(path, nd);
> - ? ? ? nd->inode = path->dentry->d_inode;
> + ? ? ? nd->inode = inode;
> ? ? ? ?/* Why this, you ask? ?_Now_ we might have grown LOOKUP_JUMPED... */
> ? ? ? ?error = complete_walk(nd);
> ? ? ? ?if (error)
In rcu-walk mode, dentry->d_inode should not be accessed at all,
outside of the core lookup code that (should) have the correct
barriers and sequence locks.
That logic should not escape into here, so I'm just not sure what
you're doing here.
This code runs in rcu-walk mode, then at the very least you need
ACCESS_ONCE to load the inode, because ->d_inode can
become NULL right after you test it for NULL (but that would likely
be a bandaid, I'm just pointing out there's raft of potential
problems).
Nick Piggin <[email protected]> writes:
> On 25 April 2012 22:44, Miklos Szeredi <[email protected]> wrote:
>> From: Miklos Szeredi <[email protected]>
>>
>> Use helper variable instead of path->dentry->d_inode before complete_walk().
>> This will allow this code to be used in RCU mode.
>
> What do you mean, allow it to be used?
I mean allow the code to be shared between RCU and non-RCU mode. See
10/16.
>
>>
>> Signed-off-by: Miklos Szeredi <[email protected]>
>> ---
>> fs/namei.c | 7 ++++---
>> 1 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/namei.c b/fs/namei.c
>> index 46d4bf6..f21ddb3 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -2360,15 +2360,16 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
>> if (error)
>> nd->flags |= LOOKUP_JUMPED;
>>
>> + inode = path->dentry->d_inode ;
>> error = -ENOENT;
>> - if (!path->dentry->d_inode)
>> + if (!inode)
>> goto exit_dput;
>>
>> - if (path->dentry->d_inode->i_op->follow_link)
>> + if (inode->i_op->follow_link)
>> return NULL;
>>
>> path_to_nameidata(path, nd);
>> - nd->inode = path->dentry->d_inode;
>> + nd->inode = inode;
>> /* Why this, you ask? _Now_ we might have grown LOOKUP_JUMPED... */
>> error = complete_walk(nd);
>> if (error)
>
> In rcu-walk mode, dentry->d_inode should not be accessed at all,
> outside of the core lookup code that (should) have the correct
> barriers and sequence locks.
>
> That logic should not escape into here, so I'm just not sure what
> you're doing here.
Right, dentry->d_inode is *not* going to be dereferenced in RCU mode.
In RCU mode it will jump to the place just after the "inode =
path->dentry->d_inode;" line. See patch 10/16.
Thanks,
Miklos
On 8 May 2012 00:28, Miklos Szeredi <[email protected]> wrote:
> Nick Piggin <[email protected]> writes:
>
>> On 25 April 2012 22:44, Miklos Szeredi <[email protected]> wrote:
>>> From: Miklos Szeredi <[email protected]>
>>>
>>> Use helper variable instead of path->dentry->d_inode before complete_walk().
>>> This will allow this code to be used in RCU mode.
>>
>> What do you mean, allow it to be used?
>
> I mean allow the code to be shared between RCU and non-RCU mode. See
> 10/16.
>
>>
>>>
>>> Signed-off-by: Miklos Szeredi <[email protected]>
>>> ---
>>> fs/namei.c | 7 ++++---
>>> 1 files changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/namei.c b/fs/namei.c
>>> index 46d4bf6..f21ddb3 100644
>>> --- a/fs/namei.c
>>> +++ b/fs/namei.c
>>> @@ -2360,15 +2360,16 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
>>> if (error)
>>> nd->flags |= LOOKUP_JUMPED;
>>>
>>> + inode = path->dentry->d_inode ;
>>> error = -ENOENT;
>>> - if (!path->dentry->d_inode)
>>> + if (!inode)
>>> goto exit_dput;
>>>
>>> - if (path->dentry->d_inode->i_op->follow_link)
>>> + if (inode->i_op->follow_link)
>>> return NULL;
>>>
>>> path_to_nameidata(path, nd);
>>> - nd->inode = path->dentry->d_inode;
>>> + nd->inode = inode;
>>> /* Why this, you ask? _Now_ we might have grown LOOKUP_JUMPED... */
>>> error = complete_walk(nd);
>>> if (error)
>>
>> In rcu-walk mode, dentry->d_inode should not be accessed at all,
>> outside of the core lookup code that (should) have the correct
>> barriers and sequence locks.
>>
>> That logic should not escape into here, so I'm just not sure what
>> you're doing here.
>
> Right, dentry->d_inode is *not* going to be dereferenced in RCU mode.
> In RCU mode it will jump to the place just after the "inode =
> path->dentry->d_inode;" line. See patch 10/16.
Hmm, OK, I admittedly didn't apply all patches and look at the result.
I wonder if that's getting too hairy... I'm sure consolidation is worthwhile
though. Perhaps comment or even a BUG_ON to ensure it is not in
rcu-walk mode, at points where you load the inode?