[PATCH 1/4] libmount: mnt_resolve_path: use strcmp() only if both are canonical

[PATCH 1/4] libmount: mnt_resolve_path: use strcmp() only if both are canonical

[Eric Rannaud]

Signed-off-by: Eric Rannaud <e@nanocritical.com>
---
 libmount/src/fs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmount/src/fs.c b/libmount/src/fs.c
index cae7ce648c31..949e718f4ca5 100644
--- a/libmount/src/fs.c
+++ b/libmount/src/fs.c
@@ -1434,7 +1434,7 @@ int mnt_fs_match_target(struct libmnt_fs *fs, const char *target,
 	if (!rc && cache) {
 		/* 2) - canonicalized and non-canonicalized */
 		char *cn = mnt_resolve_path(target, cache);
-		rc = (cn && strcmp(cn, fs->target) == 0);
+		rc = (cn && mnt_fs_streq_target(fs, cn));
 
 		/* 3) - canonicalized and canonicalized */
 		if (!rc && cn && !mnt_fs_is_kernel(fs)) {
-- 
2.0.1

[PATCH 2/4] libmount: mnt_resolve_path: don't canonicalize fs->target for swap

[Eric Rannaud]

This is how mnt_table_find_target() does it. It makes sense because
@fs->target is "none" for swap and is never a sensible match for a
user-specified target.

Signed-off-by: Eric Rannaud <e@nanocritical.com>
---
 libmount/src/fs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmount/src/fs.c b/libmount/src/fs.c
index 949e718f4ca5..21ef0f7479fd 100644
--- a/libmount/src/fs.c
+++ b/libmount/src/fs.c
@@ -1437,7 +1437,7 @@ int mnt_fs_match_target(struct libmnt_fs *fs, const char *target,
 		rc = (cn && mnt_fs_streq_target(fs, cn));
 
 		/* 3) - canonicalized and canonicalized */
-		if (!rc && cn && !mnt_fs_is_kernel(fs)) {
+		if (!rc && cn && !mnt_fs_is_kernel(fs) && !mnt_fs_is_swaparea(fs)) {
 			char *tcn = mnt_resolve_path(fs->target, cache);
 			rc = (tcn && strcmp(cn, tcn) == 0);
 		}
-- 
2.0.1

[PATCH 3/4] libmount: mnt_resolve_target: tiptoe around active mount points

[Eric Rannaud]

Current code in mnt_fs_match_target() and mnt_table_find_target()
already does not canonicalize active mount points (when read from
mountinfo), because they are already canonicalized by the kernel.
Calling realpath(fs->target) on a mount point can hang -- e.g. if the
NFS server is unreachable.

This patch optionally extends this strategy to the general case, that is
when @fs does not directly come from the kernel through mountinfo (for
instance, it may have been parsed from /etc/fstab).

Given @mtab parsed from mountinfo, and if mnt_cache_set_targets(cache,
mtab) is used, then mnt_fs_match_target() and mnt_table_find_target()
check whether @fs->target is a known mount point in the cached
mountinfo, before attempting to canonicalize @fs->target, no matter
where @fs itself comes from. If found in the cached mountinfo,
@fs->target is not canonicalized.

Signed-off-by: Eric Rannaud <e@nanocritical.com>
---
 libmount/src/cache.c       | 124 ++++++++++++++++++++++++++++++++++++++-------
 libmount/src/fs.c          |   6 ++-
 libmount/src/libmount.h.in |   4 ++
 libmount/src/libmount.sym  |   2 +
 libmount/src/tab.c         |   8 +--
 5 files changed, 121 insertions(+), 23 deletions(-)

diff --git a/libmount/src/cache.c b/libmount/src/cache.c
index 2fe41b3a4e87..e10710b2c290 100644
--- a/libmount/src/cache.c
+++ b/libmount/src/cache.c
@@ -60,6 +60,8 @@ struct libmnt_cache {
 	 *    better to reuse the blkid_cache.
 	 */
 	blkid_cache		bc;
+
+	struct libmnt_table	*mtab;
 };
 
 /**
@@ -131,11 +133,27 @@ void mnt_unref_cache(struct libmnt_cache *cache)
 	if (cache) {
 		cache->refcount--;
 		/*DBG(CACHE, ul_debugobj(cache, "unref=%d", cache->refcount));*/
-		if (cache->refcount <= 0)
+		if (cache->refcount <= 0) {
+			mnt_unref_table(cache->mtab);
+
 			mnt_free_cache(cache);
+		}
 	}
 }
 
+int mnt_cache_set_targets(struct libmnt_cache *cache,
+				struct libmnt_table *mtab)
+{
+	assert(cache);
+	if (!cache)
+		return -EINVAL;
+
+	mnt_ref_table(mtab);
+	mnt_unref_table(cache->mtab);
+	cache->mtab = mtab;
+	return 0;
+}
+
 
 /* note that the @key could be the same pointer as @value */
 static int cache_add_entry(struct libmnt_cache *cache, char *key,
@@ -468,6 +486,35 @@ char *mnt_get_fstype(const char *devname, int *ambi, struct libmnt_cache *cache)
 	return type;
 }
 
+static char *canonicalize_path_and_cache(const char *path,
+						struct libmnt_cache *cache)
+{
+	char *p = NULL;
+	char *key = NULL;
+	char *value = NULL;
+
+	p = canonicalize_path(path);
+
+	if (p && cache) {
+		value = p;
+		key = strcmp(path, p) == 0 ? value : strdup(path);
+
+		if (!key || !value)
+			goto error;
+
+		if (cache_add_entry(cache, key, value,
+				MNT_CACHE_ISPATH))
+			goto error;
+	}
+
+	return p;
+error:
+	if (value != key)
+		free(value);
+	free(key);
+	return NULL;
+}
+
 /**
  * mnt_resolve_path:
  * @path: "native" path
@@ -483,8 +530,6 @@ char *mnt_get_fstype(const char *devname, int *ambi, struct libmnt_cache *cache)
 char *mnt_resolve_path(const char *path, struct libmnt_cache *cache)
 {
 	char *p = NULL;
-	char *key = NULL;
-	char *value = NULL;
 
 	/*DBG(CACHE, ul_debugobj(cache, "resolving path %s", path));*/
 
@@ -493,28 +538,71 @@ char *mnt_resolve_path(const char *path, struct libmnt_cache *cache)
 	if (cache)
 		p = (char *) cache_find_path(cache, path);
 
-	if (!p) {
-		p = canonicalize_path(path);
+	if (!p)
+		p = canonicalize_path_and_cache(path, cache);
 
-		if (p && cache) {
-			value = p;
-			key = strcmp(path, p) == 0 ? value : strdup(path);
+	return p;
+}
 
-			if (!key || !value)
-				goto error;
+/**
+ * mnt_resolve_target:
+ * @path: "native" path, a potential mount point
+ * @cache: cache for results or NULL.
+ *
+ * Like mnt_resolve_path(), unless @cache is not NULL and
+ * mnt_cache_set_targets(cache, mtab) was called: if @path is found in the
+ * cached @mtab and the matching entry was provided by the kernel, assume
+ * that @path is already canonicalized. By avoiding a call to
+ * canonicalize_path() on known mount points, there is a lower risk of
+ * stepping on a stale mount point, which can result in an application
+ * freeze. This is also faster in general, as stat(2) on a mount point is
+ * slower than on a regular file.
+ *
+ * Returns: absolute path or NULL in case of error. The result has to be
+ * deallocated by free() if @cache is NULL.
+ */
+char *mnt_resolve_target(const char *path, struct libmnt_cache *cache)
+{
+	char *p = NULL;
+	struct libmnt_iter *itr = NULL;
+	struct libmnt_fs *fs = NULL;
 
-			if (cache_add_entry(cache, key, value,
-							MNT_CACHE_ISPATH))
-				goto error;
+	/*DBG(CACHE, ul_debugobj(cache, "resolving path %s", path));*/
+
+	if (!path)
+		return NULL;
+	if (cache)
+		p = (char *) cache_find_path(cache, path);
+
+	if (cache && cache->mtab) {
+		itr = mnt_new_iter(MNT_ITER_FORWARD);
+		if (!itr)
+			goto skip_mtab;
+
+		while(mnt_table_next_fs(cache->mtab, itr, &fs) == 0) {
+			if (!mnt_fs_is_kernel(fs)
+                            || mnt_fs_is_swaparea(fs)
+                            || !mnt_fs_streq_target(fs, path))
+				continue;
+
+			p = strdup(path);
+			if (cache_add_entry(cache, p, p,
+					MNT_CACHE_ISPATH)) {
+				free(p);
+				goto done;
+			}
+			break;
 		}
 	}
 
+skip_mtab:
+	if (!p) {
+		p = canonicalize_path_and_cache(path, cache);
+	}
+
+done:
+	mnt_free_iter(itr);
 	return p;
-error:
-	if (value != key)
-		free(value);
-	free(key);
-	return NULL;
 }
 
 /**
diff --git a/libmount/src/fs.c b/libmount/src/fs.c
index 21ef0f7479fd..82541083a792 100644
--- a/libmount/src/fs.c
+++ b/libmount/src/fs.c
@@ -1413,7 +1413,9 @@ int mnt_fs_append_comment(struct libmnt_fs *fs, const char *comm)
  *	1) compare @target with @fs->target
  *	2) realpath(@target) with @fs->target
  *	3) realpath(@target) with realpath(@fs->target) if @fs is not from
- *	   /proc/self/mountinfo.
+ *	   /proc/self/mountinfo. However, if mnt_cache_set_targets(cache,
+ *	   mtab) was called, and the path @fs->target is found in @mtab,
+ *	   this comparison is not performed (see mnt_resolve_target()).
  *
  * The 2nd and 3rd attempts are not performed when @cache is NULL.
  *
@@ -1438,7 +1440,7 @@ int mnt_fs_match_target(struct libmnt_fs *fs, const char *target,
 
 		/* 3) - canonicalized and canonicalized */
 		if (!rc && cn && !mnt_fs_is_kernel(fs) && !mnt_fs_is_swaparea(fs)) {
-			char *tcn = mnt_resolve_path(fs->target, cache);
+			char *tcn = mnt_resolve_target(fs->target, cache);
 			rc = (tcn && strcmp(cn, tcn) == 0);
 		}
 	}
diff --git a/libmount/src/libmount.h.in b/libmount/src/libmount.h.in
index 08ddd659600d..ac0c790f2abe 100644
--- a/libmount/src/libmount.h.in
+++ b/libmount/src/libmount.h.in
@@ -220,6 +220,8 @@ extern void mnt_free_cache(struct libmnt_cache *cache);
 extern void mnt_ref_cache(struct libmnt_cache *cache);
 extern void mnt_unref_cache(struct libmnt_cache *cache);
 
+extern int mnt_cache_set_targets(struct libmnt_cache *cache,
+				struct libmnt_table *mtab);
 extern int mnt_cache_read_tags(struct libmnt_cache *cache, const char *devname);
 
 extern int mnt_cache_device_has_tag(struct libmnt_cache *cache,
@@ -235,6 +237,8 @@ extern char *mnt_get_fstype(const char *devname, int *ambi,
 			__ul_attribute__((warn_unused_result));
 extern char *mnt_resolve_path(const char *path, struct libmnt_cache *cache)
 			__ul_attribute__((warn_unused_result));
+extern char *mnt_resolve_target(const char *path, struct libmnt_cache *cache)
+			__ul_attribute__((warn_unused_result));
 extern char *mnt_resolve_tag(const char *token, const char *value,
 			     struct libmnt_cache *cache)
 			__ul_attribute__((warn_unused_result));
diff --git a/libmount/src/libmount.sym b/libmount/src/libmount.sym
index 088db7749434..d827c310139d 100644
--- a/libmount/src/libmount.sym
+++ b/libmount/src/libmount.sym
@@ -289,6 +289,8 @@ global:
 } MOUNT_2.23;
 
 MOUNT_2.25 {
+	mnt_cache_set_targets;
+	mnt_resolve_target;
 	mnt_table_uniq_fs;
 	mnt_tag_is_valid;
 } MOUNT_2.24;
diff --git a/libmount/src/tab.c b/libmount/src/tab.c
index 77260ab96c35..fd6a7d8e2451 100644
--- a/libmount/src/tab.c
+++ b/libmount/src/tab.c
@@ -880,8 +880,10 @@ struct libmnt_fs *mnt_table_find_mountpoint(struct libmnt_table *tb,
  *
  * Try to lookup an entry in the given tab, three iterations are possible, the first
  * with @path, the second with realpath(@path) and the third with realpath(@path)
- * against realpath(fs->target). The 2nd and 3rd iterations are not performed
- * when the @tb cache is not set (see mnt_table_set_cache()).
+ * against realpath(fs->target). The 2nd and 3rd iterations are not performed when
+ * the @tb cache is not set (see mnt_table_set_cache()). If
+ * mnt_cache_set_targets(cache, mtab) was called, the 3rd iteration skips any
+ * @fs->target found in @mtab (see mnt_resolve_target()).
  *
  * Returns: a tab entry or NULL.
  */
@@ -933,7 +935,7 @@ struct libmnt_fs *mnt_table_find_target(struct libmnt_table *tb, const char *pat
 		    || (*fs->target == '/' && *(fs->target + 1) == '\0'))
 		       continue;
 
-		p = mnt_resolve_path(fs->target, tb->cache);
+		p = mnt_resolve_target(fs->target, tb->cache);
 		/* both canonicalized, strcmp() is fine here */
 		if (p && strcmp(cn, p) == 0)
 			return fs;
-- 
2.0.1

[PATCH 4/4] findmnt: use mnt_cache_set_targets() for non-kernel table

[Eric Rannaud]

findmnt compares the user-supplied path <target> with each entry in the
parsed table. To do this comparison, libmount attempts to canonicalize
the target path of each table entry, when the entry does not originate
from the kernel (kernel supplied target paths are already
canonicalized). However, if one of these entries is an active mount
point, stat(2) or readlink(2) on the mount target path can hang (e.g.
unreachable NFS server).

If the main table is not a kernel table, we parse /proc/self/mountinfo
into a secondary table and call mnt_cache_set_targets(). This allows
libmount to check that the target path of each entry in the main table
is not an active mount point, so it can avoid canonicalizing it.

Signed-off-by: Eric Rannaud <e@nanocritical.com>
---


* This was tested on a system with a stale mount under /mnt: findmnt -s /usr
  With 2.24.2, strace shows a lstat("/mnt") (and hangs), but no lstat on active
  mount points with this change (and doesn't hang).

* mountinfo is read whenever the main table is not from the kernel (tabtype !=
  TABTYPE_KERNEL), with or without -T. I can't think of a reason to only do
  this when -T is specified.

* Karel, you mentionned this would have a performance impact on large systems,
  but I do not know how to asses it. Should there an option to turn this new
  behavior on or off?
  Isn't it true that a system with a very large number of mount points in fstab
  is also likely to have a large number of active mount points, increasing the
  chance that one of them is stale or slow to respond? With this patch, while
  you have to read mountinfo every instantiation of findmnt, I reckon that the
  average runtime would be lower as you don't need to lstat any of the active
  mountpoints.

* Should --nocanonicalize be removed after this patch?

Thanks.


 misc-utils/findmnt.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/misc-utils/findmnt.c b/misc-utils/findmnt.c
index 06fdd8f41074..37dd92a10a14 100644
--- a/misc-utils/findmnt.c
+++ b/misc-utils/findmnt.c
@@ -833,6 +833,36 @@ static struct libmnt_table *parse_tabfiles(char **files,
 	return tb;
 }
 
+/*
+ * Parses mountinfo and calls mnt_cache_set_targets(cache, mtab). Only
+ * necessary if @tb in main() was read from a non-kernel source.
+ */
+static void cache_set_targets(struct libmnt_cache *cache)
+{
+	struct libmnt_table *tb = NULL;
+	char *path = NULL;
+	int rc = 0;
+
+	tb = mnt_new_table();
+	if (!tb)
+		goto done;
+
+	path = access(_PATH_PROC_MOUNTINFO, R_OK) == 0 ?
+		_PATH_PROC_MOUNTINFO :
+		_PATH_PROC_MOUNTS;
+
+	rc = mnt_table_parse_file(tb, path);
+	if (rc)
+		goto done;
+
+	rc = mnt_cache_set_targets(cache, tb);
+	if (rc)
+		goto done;
+
+done:
+	mnt_unref_table(tb);
+}
+
 /* checks if @tb contains parent->child relations */
 static int tab_is_tree(struct libmnt_table *tb)
 {
@@ -1471,6 +1501,9 @@ int main(int argc, char *argv[])
 			goto leave;
 		}
 		mnt_table_set_cache(tb, cache);
+
+		if (tabtype != TABTYPE_KERNEL)
+			cache_set_targets(cache);
 	}
 
 	if (flags & FL_UNIQ)
-- 
2.0.1

Re: [PATCH 4/4] findmnt: use mnt_cache_set_targets() for non-kernel table

[Karel Zak]

On Thu, Jun 26, 2014 at 10:18:48PM -0700, Eric Rannaud wrote:

> * This was tested on a system with a stale mount under /mnt: findmnt -s /usr
>   With 2.24.2, strace shows a lstat("/mnt") (and hangs), but no lstat on active
>   mount points with this change (and doesn't hang).
> 
> * mountinfo is read whenever the main table is not from the kernel (tabtype !=
>   TABTYPE_KERNEL), with or without -T. I can't think of a reason to only do
>   this when -T is specified.

 Yep, it seems fine.

> * Karel, you mentionned this would have a performance impact on large systems,
>   but I do not know how to asses it. Should there an option to turn this new
>   behavior on or off?

 I have talked about your original idea to parse mtab always in
 canonicalize_path() when someone asking for any path canonicalization. The 
 solution with the cache is better because we parse the file only once.

> * Should --nocanonicalize be removed after this patch?

 I think it makes sense to keep it there, at least for testing we have
 a way how to turn all the cache stuff off. We have the same option
 for mount(8) just for cases when you can be sure that all is already
 canonical.


 Good work, the patches look good, I'll merge it next week. I'll try
 to use it also for mnt_context_* function in libmount, maybe we can
 avoid unnecessary canonicalization in mount/umount too. 

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 1/4] libmount: mnt_resolve_path: use strcmp() only if both are canonical

[Karel Zak]

On Thu, Jun 26, 2014 at 10:17:16PM -0700, Eric Rannaud wrote:
> Signed-off-by: Eric Rannaud <e@nanocritical.com>
> ---
>  libmount/src/fs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

 Applied, thanks.

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 2/4] libmount: mnt_resolve_path: don't canonicalize fs->target for swap

[Karel Zak]

On Thu, Jun 26, 2014 at 10:17:17PM -0700, Eric Rannaud wrote:
>  libmount/src/fs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

 Applied, thanks.

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 3/4] libmount: mnt_resolve_target: tiptoe around active mount points

[Karel Zak]

On Thu, Jun 26, 2014 at 10:17:18PM -0700, Eric Rannaud wrote:
>  libmount/src/cache.c       | 124 ++++++++++++++++++++++++++++++++++++++-------
>  libmount/src/fs.c          |   6 ++-
>  libmount/src/libmount.h.in |   4 ++
>  libmount/src/libmount.sym  |   2 +
>  libmount/src/tab.c         |   8 +--
>  5 files changed, 121 insertions(+), 23 deletions(-)

 Applied with small changes, thanks.

> +int mnt_cache_set_targets(struct libmnt_cache *cache,
> +				struct libmnt_table *mtab)

 added docs.

 [...]

> +char *mnt_resolve_target(const char *path, struct libmnt_cache *cache)
> +{
> +	char *p = NULL;
> +	struct libmnt_iter *itr = NULL;
> +	struct libmnt_fs *fs = NULL;

 added fallback to mnt_reolve_path() if no cache or cache->mtab no
 specified.

> +	if (!path)
> +		return NULL;
> +	if (cache)
> +		p = (char *) cache_find_path(cache, path);
> +
> +	if (cache && cache->mtab) {
> +		itr = mnt_new_iter(MNT_ITER_FORWARD);
> +		if (!itr)
> +			goto skip_mtab;

 within libmount we don't allocate iterators, just use "struct
 libmnt_iter" (no pointer) and mnt_reset_iter().

 [...]

> +> diff --git a/libmount/src/fs.c b/libmount/src/fs.c
> index 21ef0f7479fd..82541083a792 100644
> --- a/libmount/src/fs.c
> +++ b/libmount/src/fs.c
> @@ -1413,7 +1413,9 @@ int mnt_fs_append_comment(struct libmnt_fs *fs, const char *comm)
>   *	1) compare @target with @fs->target
>   *	2) realpath(@target) with @fs->target
>   *	3) realpath(@target) with realpath(@fs->target) if @fs is not from
> - *	   /proc/self/mountinfo.
> + *	   /proc/self/mountinfo. However, if mnt_cache_set_targets(cache,
> + *	   mtab) was called, and the path @fs->target is found in @mtab,
> + *	   this comparison is not performed (see mnt_resolve_target()).
>   *
>   * The 2nd and 3rd attempts are not performed when @cache is NULL.
>   *

 I did small change here, it seems that we can use  mnt_resolve_target() also
 for 2nd attempt to avoid canonicalization of the requested path. It
 means that for example:

     findmnt --fstab --target /mnt/nfs

 where "/mnt/nfs" is mounted NFS mounpoint will not canonicalize the
 path from fstab (@fs->target) as well as from command line (@target).

 I have added a debug message to canonicalize_path_and_cache(), so
 from

    LIBMOUNT_DEBUG=0xffff ./findmnt ...

 it's pretty obvious when we call realpath().

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 4/4] findmnt: use mnt_cache_set_targets() for non-kernel table

[Karel Zak]

On Thu, Jun 26, 2014 at 10:18:48PM -0700, Eric Rannaud wrote:
> diff --git a/misc-utils/findmnt.c b/misc-utils/findmnt.c
> index 06fdd8f41074..37dd92a10a14 100644
> --- a/misc-utils/findmnt.c
> +++ b/misc-utils/findmnt.c

 Applied, thanks.

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com