Re: [PATCH] fs: Eliminate a local variable to make the code more clear
On Wed, Sep 09, 2020 at 07:54:44AM -0500, Eric W. Biederman wrote: > Hao Lee writes: > > > On Tue, Sep 08, 2020 at 07:48:57PM +0100, Al Viro wrote: > >> On Tue, Sep 08, 2020 at 01:06:56PM +, Hao Lee wrote: > >> > ping > >> > > >> > On Wed, Jul 29, 2020 at 03:21:28PM +, 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(>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
Re: [PATCH] fs: Eliminate a local variable to make the code more clear
Hao Lee writes: > On Tue, Sep 08, 2020 at 07:48:57PM +0100, Al Viro wrote: >> On Tue, Sep 08, 2020 at 01:06:56PM +, Hao Lee wrote: >> > ping >> > >> > On Wed, Jul 29, 2020 at 03:21:28PM +, 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(>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
On Tue, Sep 08, 2020 at 07:48:57PM +0100, Al Viro wrote: > On Tue, Sep 08, 2020 at 01:06:56PM +, Hao Lee wrote: > > ping > > > > On Wed, Jul 29, 2020 at 03:21:28PM +, 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(>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
ping On Wed, Jul 29, 2020 at 03:21:28PM +, 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(>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 > --- > 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
On Tue, Sep 08, 2020 at 01:06:56PM +, Hao Lee wrote: > ping > > On Wed, Jul 29, 2020 at 03:21:28PM +, 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(>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
Ping On Wed, Jul 29, 2020 at 11:21 PM 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(>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 > --- > 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 >