[PATCH] Enable use of cached metadata for pvs & pvdisplay

[PATCH] Enable use of cached metadata for pvs & pvdisplay

[Milan Broz]

Enable use of cached metadata for pvs & pvdisplay.

Currently PV commands, which performs full device scan, repeatly
re-reads PVs and scans for all devices.

Without vg private mempool this behaviour leads to OOM for large VG.

This patch allows using internal metadata cache for pvs & pvdisplay,
so the commands scan the PVs only once.
(We have to use VG_GLOBAL otherwise cache is invalidated on every
VG unlock in process_single PV call.)

(this is kind of temporary solution...)

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 tools/commands.h  |    4 ++--
 tools/pvdisplay.c |   13 ++++++++++++-
 tools/reporter.c  |    2 +-
 tools/toollib.c   |   33 ++++++++++++++++++++++++---------
 4 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/tools/commands.h b/tools/commands.h
index da202c0..cd83d5e 100644
--- a/tools/commands.h
+++ b/tools/commands.h
@@ -497,7 +497,7 @@ xx(pvdata,
 
 xx(pvdisplay,
    "Display various attributes of physical volume(s)",
-   0,
+   CACHE_VGMETADATA,
    "pvdisplay\n"
    "\t[-c|--colon]\n"
    "\t[-d|--debug]\n"
@@ -571,7 +571,7 @@ xx(pvremove,
 
 xx(pvs,
    "Display information about physical volumes",
-   0,
+   CACHE_VGMETADATA,
    "pvs" "\n"
    "\t[--aligned]\n"
    "\t[-a|--all]\n"
diff --git a/tools/pvdisplay.c b/tools/pvdisplay.c
index 406e631..086a2da 100644
--- a/tools/pvdisplay.c
+++ b/tools/pvdisplay.c
@@ -91,6 +91,8 @@ out:
 
 int pvdisplay(struct cmd_context *cmd, int argc, char **argv)
 {
+	int r;
+
 	if (arg_count(cmd, columns_ARG)) {
 		if (arg_count(cmd, colon_ARG) || arg_count(cmd, maps_ARG) ||
 		    arg_count(cmd, short_ARG)) {
@@ -113,6 +115,15 @@ int pvdisplay(struct cmd_context *cmd, int argc, char **argv)
 		return EINVALID_CMD_LINE;
 	}
 
-	return process_each_pv(cmd, argc, argv, NULL, LCK_VG_READ, 0, NULL,
+	if (!lock_vol(cmd, VG_GLOBAL, LCK_VG_READ)) {
+		log_error("Unable to obtain global lock.");
+		return ECMD_FAILED;
+	}
+
+	r = process_each_pv(cmd, argc, argv, NULL, LCK_VG_READ, 0, NULL,
 			       _pvdisplay_single);
+
+	unlock_vg(cmd, VG_GLOBAL);
+
+	return r;
 }
diff --git a/tools/reporter.c b/tools/reporter.c
index 9d03f58..aa15af6 100644
--- a/tools/reporter.c
+++ b/tools/reporter.c
@@ -435,5 +435,5 @@ int pvs(struct cmd_context *cmd, int argc, char **argv)
 	else
 		type = LABEL;
 
-	return _report(cmd, argc, argv, type);
+	return  _report(cmd, argc, argv, type);
 }
diff --git a/tools/toollib.c b/tools/toollib.c
index 72d08ca..fef0761 100644
--- a/tools/toollib.c
+++ b/tools/toollib.c
@@ -611,15 +611,16 @@ static int _process_all_devs(struct cmd_context *cmd, void *handle,
 	int ret_max = ECMD_PROCESSED;
 	int ret = 0;
 
-	if (!scan_vgs_for_pvs(cmd)) {
-		stack;
+	if (!lock_vol(cmd, VG_GLOBAL, LCK_VG_READ)) {
+		log_error("Unable to obtain global lock.");
 		return ECMD_FAILED;
 	}
 
-	if (!(iter = dev_iter_create(cmd->filter, 1))) {
-		log_error("dev_iter creation failed");
-		return ECMD_FAILED;
-	}
+	if (!scan_vgs_for_pvs(cmd))
+		goto_bad;
+
+	if (!(iter = dev_iter_create(cmd->filter, 1)))
+		goto_bad;
 
 	while ((dev = dev_iter_get(iter))) {
 		if (!(pv = pv_read(cmd, dev_name(dev), NULL, NULL, 0, 0))) {
@@ -638,8 +639,12 @@ static int _process_all_devs(struct cmd_context *cmd, void *handle,
 	}
 
 	dev_iter_destroy(iter);
-
+	
+	unlock_vg(cmd, VG_GLOBAL);
 	return ret_max;
+bad:
+	unlock_vg(cmd, VG_GLOBAL);
+	return ECMD_FAILED;
 }
 
 int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
@@ -791,17 +796,27 @@ int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
 				return ret_max;
 		} else {
 			log_verbose("Scanning for physical volume names");
-			if (!(pvslist = get_pvs(cmd)))
+			if (!lock_vol(cmd, VG_GLOBAL, LCK_VG_READ)) {
+				log_error("Unable to obtain global lock.");
 				return ECMD_FAILED;
+			}
+
+			if (!(pvslist = get_pvs(cmd))) {
+				unlock_vg(cmd, VG_GLOBAL);
+				return ECMD_FAILED;
+			}
 
 			dm_list_iterate_items(pvl, pvslist) {
 				ret = process_single(cmd, NULL, pvl->pv,
 						     handle);
 				if (ret > ret_max)
 					ret_max = ret;
-				if (sigint_caught())
+				if (sigint_caught()) {
+					unlock_vg(cmd, VG_GLOBAL);
 					return ret_max;
+				}
 			}
+			unlock_vg(cmd, VG_GLOBAL);
 		}
 	}
 

[PATCH] Enable use of cached metadata for pvs & pvdisplay

[Takahiro Yasui]

Hi Milan,

> @@ -435,5 +435,5 @@ int pvs(struct cmd_context *cmd, int argc, char **argv)
...
> -	return _report(cmd, argc, argv, type);
> +	return  _report(cmd, argc, argv, type);

> @@ -638,8 +639,12 @@ static int _process_all_devs(struct cmd_context *cmd, void *handle,
...
>  	dev_iter_destroy(iter);
> -
> +	

Those two lines are unnecessary change?


By the way, I tested this patch in my environment at the point of
the number of I/Os, and I found that I/Os were reduced as expected.

Tested-by: Takahiro Yasui <tyasui@redhat.com>

Thanks,
Taka

[PATCH v2] Enable use of cached metadata for pvs & pvdisplay

[Milan Broz]

(reposted with fixed nested locking and use it for all pv reports)

Enable use of cached metadata for pvs & pvdisplay.

Currently PV commands, which performs full device scan, repeatly
re-reads PVs and scans for all devices.

Without vg private mempool this behaviour leads to OOM for large VG.

This patch allows using internal metadata cache for pvs & pvdisplay,
so the commands scan the PVs only once.
(We have to use VG_GLOBAL otherwise cache is invalidated on every
VG unlock in process_single PV call.)

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 tools/commands.h |    4 ++--
 tools/toollib.c  |   30 ++++++++++++++++++++++--------
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/tools/commands.h b/tools/commands.h
index da202c0..cd83d5e 100644
--- a/tools/commands.h
+++ b/tools/commands.h
@@ -497,7 +497,7 @@ xx(pvdata,
 
 xx(pvdisplay,
    "Display various attributes of physical volume(s)",
-   0,
+   CACHE_VGMETADATA,
    "pvdisplay\n"
    "\t[-c|--colon]\n"
    "\t[-d|--debug]\n"
@@ -571,7 +571,7 @@ xx(pvremove,
 
 xx(pvs,
    "Display information about physical volumes",
-   0,
+   CACHE_VGMETADATA,
    "pvs" "\n"
    "\t[--aligned]\n"
    "\t[-a|--all]\n"
diff --git a/tools/toollib.c b/tools/toollib.c
index 1a2fd01..8f6b0e1 100644
--- a/tools/toollib.c
+++ b/tools/toollib.c
@@ -631,6 +631,7 @@ int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
 	int opt = 0;
 	int ret_max = ECMD_PROCESSED;
 	int ret = 0;
+	int lock_global = lock_type != LCK_NONE;
 
 	struct pv_list *pvl;
 	struct physical_volume *pv;
@@ -643,6 +644,11 @@ int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
 
 	dm_list_init(&tags);
 
+	if (lock_global && !lock_vol(cmd, VG_GLOBAL, lock_type)) {
+		log_error("Unable to obtain global lock.");
+		return ECMD_FAILED;
+	}
+
 	if (argc) {
 		log_verbose("Using physical volume(s) on command line");
 		for (; opt < argc; opt++) {
@@ -660,7 +666,7 @@ int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
 						  dm_pool_strdup(cmd->mem,
 							      tagname))) {
 					log_error("strlist allocation failed");
-					return ECMD_FAILED;
+					goto bad;
 				}
 				continue;
 			}
@@ -714,7 +720,7 @@ int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
 			if (ret > ret_max)
 				ret_max = ret;
 			if (sigint_caught())
-				return ret_max;
+				goto out;
 		}
 		if (!dm_list_empty(&tags) && (vgnames = get_vgnames(cmd, 0)) &&
 			   !dm_list_empty(vgnames)) {
@@ -748,7 +754,7 @@ int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
 				if (ret > ret_max)
 					ret_max = ret;
 				if (sigint_caught())
-					return ret_max;
+					goto out;
 			}
 		}
 	} else {
@@ -760,17 +766,18 @@ int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
 			if (ret > ret_max)
 				ret_max = ret;
 			if (sigint_caught())
-				return ret_max;
+				goto out;
 		} else if (arg_count(cmd, all_ARG)) {
 			ret = _process_all_devs(cmd, handle, process_single);
 			if (ret > ret_max)
 				ret_max = ret;
 			if (sigint_caught())
-				return ret_max;
+				goto out;
 		} else {
 			log_verbose("Scanning for physical volume names");
+
 			if (!(pvslist = get_pvs(cmd)))
-				return ECMD_FAILED;
+				goto bad;
 
 			dm_list_iterate_items(pvl, pvslist) {
 				ret = process_single(cmd, NULL, pvl->pv,
@@ -778,12 +785,19 @@ int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
 				if (ret > ret_max)
 					ret_max = ret;
 				if (sigint_caught())
-					return ret_max;
+					goto bad;
 			}
 		}
 	}
-
+out:
+	if (lock_global)
+		unlock_vg(cmd, VG_GLOBAL);
 	return ret_max;
+bad:
+	if (lock_global)
+		unlock_vg(cmd, VG_GLOBAL);
+
+	return ECMD_FAILED;
 }
 
 /*

[PATCH v2] Enable use of cached metadata for pvs & pvdisplay

[Takahiro Yasui]

Hi Milan,

> @@ -778,12 +785,19 @@ int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
>  				if (ret > ret_max)
>  					ret_max = ret;
>  				if (sigint_caught())
> -					return ret_max;
> +					goto bad;

In the above line, "goto bad" is typo of "goto out"?

Thanks,
Taka

[PATCH v2] Enable use of cached metadata for pvs & pvdisplay

[Milan Broz]

Takahiro Yasui wrote:
>> @@ -778,12 +785,19 @@ int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
>>  				if (ret > ret_max)
>>  					ret_max = ret;
>>  				if (sigint_caught())
>> -					return ret_max;
>> +					goto bad;
> 
> In the above line, "goto bad" is typo of "goto out"?

hm. you are right, I thought that signal interruption should return ECMD_FAIL but apparently
reporting code do not use that.

We should probably define something like ECMD_INTERRUPTED and use that... but it is unrelated problem.


If Alasdair acks that, I fix it in real commit.

Milan