* [PATCH] Avoid scanning all pvs in the system if pvcreating on a device with mdas.
@ 2010-03-16 22:33 Dave Wysochanski
2010-03-17 10:17 ` Petr Rockai
0 siblings, 1 reply; 6+ messages in thread
From: Dave Wysochanski @ 2010-03-16 22:33 UTC (permalink / raw)
To: lvm-devel
Other areas of the code check whether there are mdas on an orphan pv before
doing the expensive scan of the system. This patch adds this check to pvcreate
as well, and so will avoid the unnecessary scan if pvcreate on a device that
is an orphan PV.
Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
lib/metadata/metadata.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 2cab0b6..fff7a61 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -1254,11 +1254,14 @@ static int pvcreate_check(struct cmd_context *cmd, const char *name,
struct device *dev;
uint64_t md_superblock, swap_signature;
int wipe_md, wipe_swap;
+ struct dm_list mdas;
+
+ dm_list_init(&mdas);
/* FIXME Check partition type is LVM unless --force is given */
/* Is there a pv here already? */
- pv = pv_read(cmd, name, NULL, NULL, 0, 0);
+ pv = pv_read(cmd, name, &mdas, NULL, 0, 0);
/*
* If a PV has no MDAs it may appear to be an orphan until the
@@ -1266,7 +1269,7 @@ static int pvcreate_check(struct cmd_context *cmd, const char *name,
* this means checking every VG by scanning every PV on the
* system.
*/
- if (pv && is_orphan(pv)) {
+ if (pv && is_orphan(pv) && !dm_list_size(&mdas)) {
if (!scan_vgs_for_pvs(cmd))
return_0;
pv = pv_read(cmd, name, NULL, NULL, 0, 0);
--
1.6.0.6
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] Avoid scanning all pvs in the system if pvcreating on a device with mdas.
2010-03-16 22:33 [PATCH] Avoid scanning all pvs in the system if pvcreating on a device with mdas Dave Wysochanski
@ 2010-03-17 10:17 ` Petr Rockai
2010-03-17 17:32 ` Dave Wysochanski
2010-03-18 1:05 ` [PATCH] Avoid scanning all pvs in the system if pvcreating " Alasdair G Kergon
0 siblings, 2 replies; 6+ messages in thread
From: Petr Rockai @ 2010-03-17 10:17 UTC (permalink / raw)
To: lvm-devel
Hi,
Dave Wysochanski <dwysocha@redhat.com> writes:
> Other areas of the code check whether there are mdas on an orphan pv before
> doing the expensive scan of the system. This patch adds this check to pvcreate
> as well, and so will avoid the unnecessary scan if pvcreate on a device that
> is an orphan PV.
Looks OK to me. As far as I can tell, it should never happen that the
metadata is out-of-date and we overwrite a non-orphan PV mistakenly.
This would require that the metadata on the PV claim this is an orphan
but a newer copy of the metadata elsewhere claims this is part of a
VG. That would mean that vgextend (or similar) failed to update the
metadata on the new PV, which would presumably lead to overall vgextend
failure and no new metadata on the pre-existing PVs either. So this
should be safe.
> diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
> index 2cab0b6..fff7a61 100644
> --- a/lib/metadata/metadata.c
> +++ b/lib/metadata/metadata.c
> @@ -1254,11 +1254,14 @@ static int pvcreate_check(struct cmd_context *cmd, const char *name,
> struct device *dev;
> uint64_t md_superblock, swap_signature;
> int wipe_md, wipe_swap;
> + struct dm_list mdas;
> +
> + dm_list_init(&mdas);
>
> /* FIXME Check partition type is LVM unless --force is given */
>
> /* Is there a pv here already? */
> - pv = pv_read(cmd, name, NULL, NULL, 0, 0);
> + pv = pv_read(cmd, name, &mdas, NULL, 0, 0);
>
> /*
> * If a PV has no MDAs it may appear to be an orphan until the
> @@ -1266,7 +1269,7 @@ static int pvcreate_check(struct cmd_context *cmd, const char *name,
> * this means checking every VG by scanning every PV on the
> * system.
> */
> - if (pv && is_orphan(pv)) {
> + if (pv && is_orphan(pv) && !dm_list_size(&mdas)) {
> if (!scan_vgs_for_pvs(cmd))
> return_0;
> pv = pv_read(cmd, name, NULL, NULL, 0, 0);
ACK.
Yours,
Petr.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Avoid scanning all pvs in the system if pvcreating on a device with mdas.
2010-03-17 10:17 ` Petr Rockai
@ 2010-03-17 17:32 ` Dave Wysochanski
2010-03-17 17:59 ` [PATCH] Avoid scanning all pvs in the system if operating " Dave Wysochanski
2010-03-18 1:05 ` [PATCH] Avoid scanning all pvs in the system if pvcreating " Alasdair G Kergon
1 sibling, 1 reply; 6+ messages in thread
From: Dave Wysochanski @ 2010-03-17 17:32 UTC (permalink / raw)
To: lvm-devel
On Wed, 2010-03-17 at 11:17 +0100, Petr Rockai wrote:
> Hi,
>
> Dave Wysochanski <dwysocha@redhat.com> writes:
> > Other areas of the code check whether there are mdas on an orphan pv before
> > doing the expensive scan of the system. This patch adds this check to pvcreate
> > as well, and so will avoid the unnecessary scan if pvcreate on a device that
> > is an orphan PV.
> Looks OK to me. As far as I can tell, it should never happen that the
> metadata is out-of-date and we overwrite a non-orphan PV mistakenly.
>
> This would require that the metadata on the PV claim this is an orphan
> but a newer copy of the metadata elsewhere claims this is part of a
> VG. That would mean that vgextend (or similar) failed to update the
> metadata on the new PV, which would presumably lead to overall vgextend
> failure and no new metadata on the pre-existing PVs either. So this
> should be safe.
>
Ok - turns out I should have searched more carefully as there are more
instances where we could save the scanning.
I'll work on an updated patch.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Avoid scanning all pvs in the system if operating on a device with mdas.
2010-03-17 17:32 ` Dave Wysochanski
@ 2010-03-17 17:59 ` Dave Wysochanski
2010-03-18 0:47 ` Alasdair G Kergon
0 siblings, 1 reply; 6+ messages in thread
From: Dave Wysochanski @ 2010-03-17 17:59 UTC (permalink / raw)
To: lvm-devel
When we pv_read() a device that has an orphan vgname, we might need to scan
the system to be sure this is true. However, if the PV has mdas, there's
no way possible for it to have an orphan vgname unless it is a true orphan.
Some areas of the code were optimized to take advantage of this fact, while
others were not (we would still do the expensive scan if a device had mdas
but had an orphan VG).
This patch unifies the code so that every place we are operating on such
a PV, we skip the expensive scan if there are mdas.
Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
lib/metadata/metadata.c | 13 +++++++++----
tools/toollib.c | 8 ++++++--
2 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 4060ca2..020e897 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -1251,11 +1251,14 @@ static int pvcreate_check(struct cmd_context *cmd, const char *name,
struct device *dev;
uint64_t md_superblock, swap_signature;
int wipe_md, wipe_swap;
+ struct dm_list mdas;
+
+ dm_list_init(&mdas);
/* FIXME Check partition type is LVM unless --force is given */
/* Is there a pv here already? */
- pv = pv_read(cmd, name, NULL, NULL, 0, 0);
+ pv = pv_read(cmd, name, &mdas, NULL, 0, 0);
/*
* If a PV has no MDAs it may appear to be an orphan until the
@@ -1263,7 +1266,7 @@ static int pvcreate_check(struct cmd_context *cmd, const char *name,
* this means checking every VG by scanning every PV on the
* system.
*/
- if (pv && is_orphan(pv)) {
+ if (pv && is_orphan(pv) && !dm_list_size(&mdas)) {
if (!scan_vgs_for_pvs(cmd))
return_0;
pv = pv_read(cmd, name, NULL, NULL, 0, 0);
@@ -1747,14 +1750,16 @@ struct physical_volume *find_pv_by_name(struct cmd_context *cmd,
static struct physical_volume *_find_pv_by_name(struct cmd_context *cmd,
const char *pv_name)
{
+ struct dm_list mdas;
struct physical_volume *pv;
- if (!(pv = _pv_read(cmd, cmd->mem, pv_name, NULL, NULL, 1, 0))) {
+ dm_list_init(&mdas);
+ if (!(pv = _pv_read(cmd, cmd->mem, pv_name, &mdas, NULL, 1, 0))) {
log_error("Physical volume %s not found", pv_name);
return NULL;
}
- if (is_orphan_vg(pv->vg_name)) {
+ if (is_orphan_vg(pv->vg_name) && !dm_list_size(&mdas)) {
/* If a PV has no MDAs - need to search all VGs for it */
if (!scan_vgs_for_pvs(cmd))
return_NULL;
diff --git a/tools/toollib.c b/tools/toollib.c
index 0d3fe0a..efe870c 100644
--- a/tools/toollib.c
+++ b/tools/toollib.c
@@ -642,6 +642,7 @@ int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
struct str_list *sll;
char *tagname;
int scanned = 0;
+ struct dm_list mdas;
dm_list_init(&tags);
@@ -682,7 +683,9 @@ int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
}
pv = pvl->pv;
} else {
- if (!(pv = pv_read(cmd, argv[opt], NULL,
+
+ dm_list_init(&mdas);
+ if (!(pv = pv_read(cmd, argv[opt], &mdas,
NULL, 1, scan_label_only))) {
log_error("Failed to read physical "
"volume \"%s\"", argv[opt]);
@@ -697,7 +700,8 @@ int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
* means checking every VG by scanning every
* PV on the system.
*/
- if (!scanned && is_orphan(pv)) {
+ if (!scanned && is_orphan(pv) &&
+ !dm_list_size(&mdas)) {
if (!scan_label_only &&
!scan_vgs_for_pvs(cmd)) {
stack;
--
1.6.0.6
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] Avoid scanning all pvs in the system if operating on a device with mdas.
2010-03-17 17:59 ` [PATCH] Avoid scanning all pvs in the system if operating " Dave Wysochanski
@ 2010-03-18 0:47 ` Alasdair G Kergon
0 siblings, 0 replies; 6+ messages in thread
From: Alasdair G Kergon @ 2010-03-18 0:47 UTC (permalink / raw)
To: lvm-devel
On Wed, Mar 17, 2010 at 01:59:02PM -0400, Dave Wysochanski wrote:
> When we pv_read() a device that has an orphan vgname, we might need to scan
> the system to be sure this is true. However, if the PV has mdas, there's
> no way possible for it to have an orphan vgname unless it is a true orphan.
> Some areas of the code were optimized to take advantage of this fact, while
> others were not (we would still do the expensive scan if a device had mdas
> but had an orphan VG).
> This patch unifies the code so that every place we are operating on such
> a PV, we skip the expensive scan if there are mdas.
Makes sense.
Ack.
Alasdair
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Avoid scanning all pvs in the system if pvcreating on a device with mdas.
2010-03-17 10:17 ` Petr Rockai
2010-03-17 17:32 ` Dave Wysochanski
@ 2010-03-18 1:05 ` Alasdair G Kergon
1 sibling, 0 replies; 6+ messages in thread
From: Alasdair G Kergon @ 2010-03-18 1:05 UTC (permalink / raw)
To: lvm-devel
On Wed, Mar 17, 2010 at 11:17:12AM +0100, Peter Rockai wrote:
> Looks OK to me. As far as I can tell, it should never happen that the
> metadata is out-of-date and we overwrite a non-orphan PV mistakenly.
> This would require that the metadata on the PV claim this is an orphan
> but a newer copy of the metadata elsewhere claims this is part of a
> VG. That would mean that vgextend (or similar) failed to update the
> metadata on the new PV, which would presumably lead to overall vgextend
> failure and no new metadata on the pre-existing PVs either. So this
> should be safe.
There is actually a failure mode when this can happen.
The VG metadata is considered committed when the *first* vg_commit
occurs. If the process dies at that point, the next vg_read of the VG
does automatic recovery which completes writing the correct VG metadata
to the remaining devices including the newly-added one.
So the optimisation in this patch assumes that if a process doing a
vgextend died at exactly the wrong point, there would be a vg_read
before this pv* code. With a reboot or any sort of display command
a sysadmin would want to run after failure to check the state of
the system, this would happen.
So I'm not particularly concerned about that failure mode, and the logic
may change anyway when mdas are allowed to be empty.
Alasdair
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-03-18 1:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-03-16 22:33 [PATCH] Avoid scanning all pvs in the system if pvcreating on a device with mdas Dave Wysochanski
2010-03-17 10:17 ` Petr Rockai
2010-03-17 17:32 ` Dave Wysochanski
2010-03-17 17:59 ` [PATCH] Avoid scanning all pvs in the system if operating " Dave Wysochanski
2010-03-18 0:47 ` Alasdair G Kergon
2010-03-18 1:05 ` [PATCH] Avoid scanning all pvs in the system if pvcreating " Alasdair G Kergon
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.