On Tue, Sep 08, 2020 at 01:06:56PM +0000, Hao Lee wrote:
> ping
>
> On Wed, Jul 29, 2020 at 03:21:28PM +0000, Hao Lee wrote:
> > The dentry local variable is introduced in 'commit 84d17192d2afd ("get
> > rid of full-hash scan on detaching vfsmounts")' to reduce the length of
> > some long statements for example
> > mutex_lock(&path->dentry->d_inode->i_mutex). We have already used
> > inode_lock(dentry->d_inode) to do the same thing now, and its length is
> > acceptable. Furthermore, it seems not concise that assign path->dentry
> > to local variable dentry in the statement before goto. So, this function
> > would be more clear if we eliminate the local variable dentry.
How does it make the function more clear? More specifically, what
analysis of behaviour is simplified by that?
On Tue, Sep 08, 2020 at 07:48:57PM +0100, Al Viro wrote:
> On Tue, Sep 08, 2020 at 01:06:56PM +0000, Hao Lee wrote:
> > ping
> >
> > On Wed, Jul 29, 2020 at 03:21:28PM +0000, Hao Lee wrote:
> > > The dentry local variable is introduced in 'commit 84d17192d2afd ("get
> > > rid of full-hash scan on detaching vfsmounts")' to reduce the length of
> > > some long statements for example
> > > mutex_lock(&path->dentry->d_inode->i_mutex). We have already used
> > > inode_lock(dentry->d_inode) to do the same thing now, and its length is
> > > acceptable. Furthermore, it seems not concise that assign path->dentry
> > > to local variable dentry in the statement before goto. So, this function
> > > would be more clear if we eliminate the local variable dentry.
>
> How does it make the function more clear? More specifically, what
> analysis of behaviour is simplified by that?
When I first read this function, it takes me a few seconds to think
about if the local variable dentry is always equal to path->dentry and
want to know if it has special purpose. This local variable may confuse
other people too, so I think it would be better to eliminate it.
Thanks,
Hao Lee
Hao Lee <[email protected]> writes:
> On Tue, Sep 08, 2020 at 07:48:57PM +0100, Al Viro wrote:
>> On Tue, Sep 08, 2020 at 01:06:56PM +0000, Hao Lee wrote:
>> > ping
>> >
>> > On Wed, Jul 29, 2020 at 03:21:28PM +0000, Hao Lee wrote:
>> > > The dentry local variable is introduced in 'commit 84d17192d2afd ("get
>> > > rid of full-hash scan on detaching vfsmounts")' to reduce the length of
>> > > some long statements for example
>> > > mutex_lock(&path->dentry->d_inode->i_mutex). We have already used
>> > > inode_lock(dentry->d_inode) to do the same thing now, and its length is
>> > > acceptable. Furthermore, it seems not concise that assign path->dentry
>> > > to local variable dentry in the statement before goto. So, this function
>> > > would be more clear if we eliminate the local variable dentry.
>>
>> How does it make the function more clear? More specifically, what
>> analysis of behaviour is simplified by that?
>
> When I first read this function, it takes me a few seconds to think
> about if the local variable dentry is always equal to path->dentry and
> want to know if it has special purpose. This local variable may confuse
> other people too, so I think it would be better to eliminate it.
I tend to have the opposite reaction. I read your patch and wonder
why path->dentry needs to be reread what is changing path that I can not see.
my back.
Now for clarity it would probably help to do something like:
diff --git a/fs/namespace.c b/fs/namespace.c
index bae0e95b3713..430f3b4785e3 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2206,7 +2206,7 @@ static struct mountpoint *lock_mount(struct path *path)
return mp;
}
namespace_unlock();
- inode_unlock(path->dentry->d_inode);
+ inode_unlock(dentry->d_inode);
path_put(path);
path->mnt = mnt;
dentry = path->dentry = dget(mnt->mnt_root);
So at least the inode_lock and inode_unlock are properly paired.
At first glance inode_unlock using path->dentry instead of dentry
appears to be an oversight in 84d17192d2af ("get rid of full-hash scan
on detaching vfsmounts").
Eric
On Wed, Sep 09, 2020 at 07:54:44AM -0500, Eric W. Biederman wrote:
> Hao Lee <[email protected]> writes:
>
> > On Tue, Sep 08, 2020 at 07:48:57PM +0100, Al Viro wrote:
> >> On Tue, Sep 08, 2020 at 01:06:56PM +0000, Hao Lee wrote:
> >> > ping
> >> >
> >> > On Wed, Jul 29, 2020 at 03:21:28PM +0000, Hao Lee wrote:
> >> > > The dentry local variable is introduced in 'commit 84d17192d2afd ("get
> >> > > rid of full-hash scan on detaching vfsmounts")' to reduce the length of
> >> > > some long statements for example
> >> > > mutex_lock(&path->dentry->d_inode->i_mutex). We have already used
> >> > > inode_lock(dentry->d_inode) to do the same thing now, and its length is
> >> > > acceptable. Furthermore, it seems not concise that assign path->dentry
> >> > > to local variable dentry in the statement before goto. So, this function
> >> > > would be more clear if we eliminate the local variable dentry.
> >>
> >> How does it make the function more clear? More specifically, what
> >> analysis of behaviour is simplified by that?
> >
> > When I first read this function, it takes me a few seconds to think
> > about if the local variable dentry is always equal to path->dentry and
> > want to know if it has special purpose. This local variable may confuse
> > other people too, so I think it would be better to eliminate it.
>
> I tend to have the opposite reaction. I read your patch and wonder
> why path->dentry needs to be reread what is changing path that I can not see.
> my back.
If I understand correctly, accessing path->dentry->d_inode needs one
more instruction than accessing dentry->d_inode, so the original code is
more efficient.
>
> Now for clarity it would probably help to do something like:
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index bae0e95b3713..430f3b4785e3 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -2206,7 +2206,7 @@ static struct mountpoint *lock_mount(struct path *path)
> return mp;
> }
> namespace_unlock();
> - inode_unlock(path->dentry->d_inode);
> + inode_unlock(dentry->d_inode);
> path_put(path);
> path->mnt = mnt;
> dentry = path->dentry = dget(mnt->mnt_root);
>
>
> So at least the inode_lock and inode_unlock are properly paired.
>
> At first glance inode_unlock using path->dentry instead of dentry
> appears to be an oversight in 84d17192d2af ("get rid of full-hash scan
> on detaching vfsmounts").
I think I have understood why we use the local variable dentry. Thanks.
Regards,
Hao Lee
>
>
> Eric