| Message ID | 20200224170142.5604-1-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfs: Rename __is_local_mountpoint to is_local_mountpoint | expand |
On Mon, Feb 24, 2020 at 07:01:42PM +0200, Nikolay Borisov wrote: > Current is_local_mountpoint is a simple wrapper with added d_mountpoint > check. However, the same check is the first thing which > __is_local_mountpoint performs. So remove the wrapper and promote the > private helper to is_local_mountpoint. No semantics changes. NAK. "No semantics changes" does not cut it - inline helper that checks some unlikely condition and calls an out-of-line version is a fairly common pattern, with legitimate uses. It *may* be unwarranted here, but you need more serious analysis than that. I'm not saying that the patch is wrong, but you'll also need to explain why removing the check from __is_local_mountpoint() (and marking the condition unlikely in the wrapper) would be worse than what you propose.
On 24.02.20 г. 19:18 ч., Al Viro wrote: > On Mon, Feb 24, 2020 at 07:01:42PM +0200, Nikolay Borisov wrote: >> Current is_local_mountpoint is a simple wrapper with added d_mountpoint >> check. However, the same check is the first thing which >> __is_local_mountpoint performs. So remove the wrapper and promote the >> private helper to is_local_mountpoint. No semantics changes. > > NAK. "No semantics changes" does not cut it - inline helper that checks > some unlikely condition and calls an out-of-line version is a fairly > common pattern, with legitimate uses. It *may* be unwarranted here, > but you need more serious analysis than that. I'm not saying that > the patch is wrong, but you'll also need to explain why removing the > check from __is_local_mountpoint() (and marking the condition unlikely > in the wrapper) would be worse than what you propose. > My main motivation was to collapse the number of functions so it's easier to see what's going on and removing duplicate check. If you are going to accept the alternative version you proposed I'm fine sending it.
diff --git a/fs/mount.h b/fs/mount.h index 711a4093e475..bdbd2ad79209 100644 --- a/fs/mount.h +++ b/fs/mount.h @@ -140,14 +140,7 @@ struct proc_mounts { extern const struct seq_operations mounts_op; -extern bool __is_local_mountpoint(struct dentry *dentry); -static inline bool is_local_mountpoint(struct dentry *dentry) -{ - if (!d_mountpoint(dentry)) - return false; - - return __is_local_mountpoint(dentry); -} +extern bool is_local_mountpoint(struct dentry *dentry); static inline bool is_anon_ns(struct mnt_namespace *ns) { diff --git a/fs/namespace.c b/fs/namespace.c index 85b5f7bea82e..6d1f00849ac6 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -649,7 +649,7 @@ struct vfsmount *lookup_mnt(const struct path *path) } /* - * __is_local_mountpoint - Test to see if dentry is a mountpoint in the + * is_local_mountpoint - Test to see if dentry is a mountpoint in the * current mount namespace. * * The common case is dentries are not mountpoints at all and that @@ -663,7 +663,7 @@ struct vfsmount *lookup_mnt(const struct path *path) * namespace not just a mount that happens to have some specified * parent mount. */ -bool __is_local_mountpoint(struct dentry *dentry) +bool is_local_mountpoint(struct dentry *dentry) { struct mnt_namespace *ns = current->nsproxy->mnt_ns; struct mount *mnt;
Current is_local_mountpoint is a simple wrapper with added d_mountpoint check. However, the same check is the first thing which __is_local_mountpoint performs. So remove the wrapper and promote the private helper to is_local_mountpoint. No semantics changes. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/mount.h | 9 +-------- fs/namespace.c | 4 ++-- 2 files changed, 3 insertions(+), 10 deletions(-)