| Message ID | 20200729151740.GA3430@haolee.github.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: Eliminate a local variable to make the code more clear | expand |
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 >
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 >
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 <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
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
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; }
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(-)