[PATCH] fs: Eliminate a local variable to make the code more clear

[PATCH] fs: Eliminate a local variable to make the code more clear

[Hao Lee]

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.

The function logic is not changed.

Signed-off-by: Hao Lee <haolee.swjtu@gmail.com>
---
 fs/namespace.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 4a0f600a3328..fcb93586fcc9 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2187,20 +2187,19 @@ static int attach_recursive_mnt(struct mount *source_mnt,
 static struct mountpoint *lock_mount(struct path *path)
 {
 	struct vfsmount *mnt;
-	struct dentry *dentry = path->dentry;
 retry:
-	inode_lock(dentry->d_inode);
-	if (unlikely(cant_mount(dentry))) {
-		inode_unlock(dentry->d_inode);
+	inode_lock(path->dentry->d_inode);
+	if (unlikely(cant_mount(path->dentry))) {
+		inode_unlock(path->dentry->d_inode);
 		return ERR_PTR(-ENOENT);
 	}
 	namespace_lock();
 	mnt = lookup_mnt(path);
 	if (likely(!mnt)) {
-		struct mountpoint *mp = get_mountpoint(dentry);
+		struct mountpoint *mp = get_mountpoint(path->dentry);
 		if (IS_ERR(mp)) {
 			namespace_unlock();
-			inode_unlock(dentry->d_inode);
+			inode_unlock(path->dentry->d_inode);
 			return mp;
 		}
 		return mp;
@@ -2209,7 +2208,7 @@ static struct mountpoint *lock_mount(struct path *path)
 	inode_unlock(path->dentry->d_inode);
 	path_put(path);
 	path->mnt = mnt;
-	dentry = path->dentry = dget(mnt->mnt_root);
+	path->dentry = dget(mnt->mnt_root);
 	goto retry;
 }
 
-- 
2.24.1

Re: [PATCH] fs: Eliminate a local variable to make the code more clear

[Hao Lee]

Ping

On Wed, Jul 29, 2020 at 11:21 PM Hao Lee <haolee.swjtu@gmail.com> 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.
>
> The function logic is not changed.
>
> Signed-off-by: Hao Lee <haolee.swjtu@gmail.com>
> ---
>  fs/namespace.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 4a0f600a3328..fcb93586fcc9 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -2187,20 +2187,19 @@ static int attach_recursive_mnt(struct mount *source_mnt,
>  static struct mountpoint *lock_mount(struct path *path)
>  {
>         struct vfsmount *mnt;
> -       struct dentry *dentry = path->dentry;
>  retry:
> -       inode_lock(dentry->d_inode);
> -       if (unlikely(cant_mount(dentry))) {
> -               inode_unlock(dentry->d_inode);
> +       inode_lock(path->dentry->d_inode);
> +       if (unlikely(cant_mount(path->dentry))) {
> +               inode_unlock(path->dentry->d_inode);
>                 return ERR_PTR(-ENOENT);
>         }
>         namespace_lock();
>         mnt = lookup_mnt(path);
>         if (likely(!mnt)) {
> -               struct mountpoint *mp = get_mountpoint(dentry);
> +               struct mountpoint *mp = get_mountpoint(path->dentry);
>                 if (IS_ERR(mp)) {
>                         namespace_unlock();
> -                       inode_unlock(dentry->d_inode);
> +                       inode_unlock(path->dentry->d_inode);
>                         return mp;
>                 }
>                 return mp;
> @@ -2209,7 +2208,7 @@ static struct mountpoint *lock_mount(struct path *path)
>         inode_unlock(path->dentry->d_inode);
>         path_put(path);
>         path->mnt = mnt;
> -       dentry = path->dentry = dget(mnt->mnt_root);
> +       path->dentry = dget(mnt->mnt_root);
>         goto retry;
>  }
>
> --
> 2.24.1
>

Re: [PATCH] fs: Eliminate a local variable to make the code more clear

[Hao Lee]

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.
> 
> The function logic is not changed.
> 
> Signed-off-by: Hao Lee <haolee.swjtu@gmail.com>
> ---
>  fs/namespace.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 4a0f600a3328..fcb93586fcc9 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -2187,20 +2187,19 @@ static int attach_recursive_mnt(struct mount *source_mnt,
>  static struct mountpoint *lock_mount(struct path *path)
>  {
>  	struct vfsmount *mnt;
> -	struct dentry *dentry = path->dentry;
>  retry:
> -	inode_lock(dentry->d_inode);
> -	if (unlikely(cant_mount(dentry))) {
> -		inode_unlock(dentry->d_inode);
> +	inode_lock(path->dentry->d_inode);
> +	if (unlikely(cant_mount(path->dentry))) {
> +		inode_unlock(path->dentry->d_inode);
>  		return ERR_PTR(-ENOENT);
>  	}
>  	namespace_lock();
>  	mnt = lookup_mnt(path);
>  	if (likely(!mnt)) {
> -		struct mountpoint *mp = get_mountpoint(dentry);
> +		struct mountpoint *mp = get_mountpoint(path->dentry);
>  		if (IS_ERR(mp)) {
>  			namespace_unlock();
> -			inode_unlock(dentry->d_inode);
> +			inode_unlock(path->dentry->d_inode);
>  			return mp;
>  		}
>  		return mp;
> @@ -2209,7 +2208,7 @@ static struct mountpoint *lock_mount(struct path *path)
>  	inode_unlock(path->dentry->d_inode);
>  	path_put(path);
>  	path->mnt = mnt;
> -	dentry = path->dentry = dget(mnt->mnt_root);
> +	path->dentry = dget(mnt->mnt_root);
>  	goto retry;
>  }
>  
> -- 
> 2.24.1
> 

Re: [PATCH] fs: Eliminate a local variable to make the code more clear

[Al Viro]

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?

Re: [PATCH] fs: Eliminate a local variable to make the code more clear

[Hao Lee]

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

Re: [PATCH] fs: Eliminate a local variable to make the code more clear

[Eric W. Biederman]

Hao Lee <haolee.swjtu@gmail.com> 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

Re: [PATCH] fs: Eliminate a local variable to make the code more clear

[Hao Lee]

On Wed, Sep 09, 2020 at 07:54:44AM -0500, Eric W. Biederman wrote:
> Hao Lee <haolee.swjtu@gmail.com> 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