* [PATCH] Enable use of cached metadata for pvs & pvdisplay
@ 2009-04-06 11:26 Milan Broz
2009-04-07 3:04 ` Takahiro Yasui
2009-04-07 11:04 ` [PATCH v2] " Milan Broz
0 siblings, 2 replies; 5+ messages in thread
From: Milan Broz @ 2009-04-06 11:26 UTC (permalink / raw)
To: lvm-devel
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);
}
}
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH] Enable use of cached metadata for pvs & pvdisplay
2009-04-06 11:26 [PATCH] Enable use of cached metadata for pvs & pvdisplay Milan Broz
@ 2009-04-07 3:04 ` Takahiro Yasui
2009-04-07 11:04 ` [PATCH v2] " Milan Broz
1 sibling, 0 replies; 5+ messages in thread
From: Takahiro Yasui @ 2009-04-07 3:04 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] Enable use of cached metadata for pvs & pvdisplay
2009-04-06 11:26 [PATCH] Enable use of cached metadata for pvs & pvdisplay Milan Broz
2009-04-07 3:04 ` Takahiro Yasui
@ 2009-04-07 11:04 ` Milan Broz
2009-04-08 6:09 ` Takahiro Yasui
1 sibling, 1 reply; 5+ messages in thread
From: Milan Broz @ 2009-04-07 11:04 UTC (permalink / raw)
To: lvm-devel
(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;
}
/*
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v2] Enable use of cached metadata for pvs & pvdisplay
2009-04-07 11:04 ` [PATCH v2] " Milan Broz
@ 2009-04-08 6:09 ` Takahiro Yasui
2009-04-08 10:50 ` Milan Broz
0 siblings, 1 reply; 5+ messages in thread
From: Takahiro Yasui @ 2009-04-08 6:09 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] Enable use of cached metadata for pvs & pvdisplay
2009-04-08 6:09 ` Takahiro Yasui
@ 2009-04-08 10:50 ` Milan Broz
0 siblings, 0 replies; 5+ messages in thread
From: Milan Broz @ 2009-04-08 10:50 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-04-08 10:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-04-06 11:26 [PATCH] Enable use of cached metadata for pvs & pvdisplay Milan Broz
2009-04-07 3:04 ` Takahiro Yasui
2009-04-07 11:04 ` [PATCH v2] " Milan Broz
2009-04-08 6:09 ` Takahiro Yasui
2009-04-08 10:50 ` Milan Broz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.