Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] fs: Eliminate a local variable to make the code more clear
@ 2020-07-29 15:21 Hao Lee
  2020-08-14  6:01 ` Hao Lee
  2020-09-08 13:06 ` Hao Lee
  0 siblings, 2 replies; 7+ messages in thread
From: Hao Lee @ 2020-07-29 15:21 UTC (permalink / raw)
  To: viro; +Cc: linux-fsdevel, linux-kernel, haolee.swjtu

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] fs: Eliminate a local variable to make the code more clear
  2020-07-29 15:21 [PATCH] fs: Eliminate a local variable to make the code more clear Hao Lee
@ 2020-08-14  6:01 ` Hao Lee
  2020-09-08 13:06 ` Hao Lee
  1 sibling, 0 replies; 7+ messages in thread
From: Hao Lee @ 2020-08-14  6:01 UTC (permalink / raw)
  To: viro; +Cc: linux-fsdevel, linux-kernel

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
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] fs: Eliminate a local variable to make the code more clear
  2020-07-29 15:21 [PATCH] fs: Eliminate a local variable to make the code more clear Hao Lee
  2020-08-14  6:01 ` Hao Lee
@ 2020-09-08 13:06 ` Hao Lee
  2020-09-08 18:48   ` Al Viro
  1 sibling, 1 reply; 7+ messages in thread
From: Hao Lee @ 2020-09-08 13:06 UTC (permalink / raw)
  To: viro; +Cc: linux-fsdevel, linux-kernel

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
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] fs: Eliminate a local variable to make the code more clear
  2020-09-08 13:06 ` Hao Lee
@ 2020-09-08 18:48   ` Al Viro
  2020-09-08 23:11     ` Hao Lee
  0 siblings, 1 reply; 7+ messages in thread
From: Al Viro @ 2020-09-08 18:48 UTC (permalink / raw)
  To: Hao Lee; +Cc: linux-fsdevel, linux-kernel

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?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] fs: Eliminate a local variable to make the code more clear
  2020-09-08 18:48   ` Al Viro
@ 2020-09-08 23:11     ` Hao Lee
  2020-09-09 12:54       ` Eric W. Biederman
  0 siblings, 1 reply; 7+ messages in thread
From: Hao Lee @ 2020-09-08 23:11 UTC (permalink / raw)
  To: Al Viro; +Cc: linux-fsdevel, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] fs: Eliminate a local variable to make the code more clear
  2020-09-08 23:11     ` Hao Lee
@ 2020-09-09 12:54       ` Eric W. Biederman
  2020-09-09 15:32         ` Hao Lee
  0 siblings, 1 reply; 7+ messages in thread
From: Eric W. Biederman @ 2020-09-09 12:54 UTC (permalink / raw)
  To: Hao Lee; +Cc: Al Viro, linux-fsdevel, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] fs: Eliminate a local variable to make the code more clear
  2020-09-09 12:54       ` Eric W. Biederman
@ 2020-09-09 15:32         ` Hao Lee
  0 siblings, 0 replies; 7+ messages in thread
From: Hao Lee @ 2020-09-09 15:32 UTC (permalink / raw)
  To: Eric W. Biederman; +Cc: Al Viro, linux-fsdevel, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-29 15:21 [PATCH] fs: Eliminate a local variable to make the code more clear Hao Lee
2020-08-14  6:01 ` Hao Lee
2020-09-08 13:06 ` Hao Lee
2020-09-08 18:48   ` Al Viro
2020-09-08 23:11     ` Hao Lee
2020-09-09 12:54       ` Eric W. Biederman
2020-09-09 15:32         ` Hao Lee

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git