[PATCH] Scan also special metadata areas (like metadata/dirs) in fmt_from_vgname

[PATCH] Scan also special metadata areas (like metadata/dirs) in fmt_from_vgname

[Peter Rajnoha]

We reread labels and metadata from raw devices in fmt_from_vgname function,
but we forgot to read special metadata areas? (like dirs defined by metadata/dirs
config setting) Anyway, can someone remind me why we do scanning inside this fn
at all?

Without this patch, we will fail to find a VG when metadata/dirs is only
used and there are no other metadata areas (which is itself a strange situation,
but see also https://www.redhat.com/archives/linux-lvm/2010-November/msg00090.html)

Peter
---
 lib/cache/lvmcache.c    |   10 +++++++++-
 lib/cache/lvmcache.h    |    4 +++-
 lib/metadata/metadata.c |   14 +++++++-------
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/lib/cache/lvmcache.c b/lib/cache/lvmcache.c
index 60edcec..eebaaea 100644
--- a/lib/cache/lvmcache.c
+++ b/lib/cache/lvmcache.c
@@ -366,7 +366,9 @@ struct lvmcache_vginfo *vginfo_from_vgname(const char *vgname, const char *vgid)
 	return vginfo;
 }
 
-const struct format_type *fmt_from_vgname(const char *vgname, const char *vgid)
+const struct format_type *fmt_from_vgname(struct cmd_context *cmd,
+					  const char *vgname,
+					  const char *vgid)
 {
 	struct lvmcache_vginfo *vginfo;
 	struct lvmcache_info *info;
@@ -375,6 +377,7 @@ const struct format_type *fmt_from_vgname(const char *vgname, const char *vgid)
 	struct dm_list devs;
 	struct device_list *devl;
 	char vgid_found[ID_LEN + 1] __attribute__((aligned(8)));
+	struct format_type *fmt;
 
 	if (!(vginfo = vginfo_from_vgname(vgname, vgid)))
 		return NULL;
@@ -400,6 +403,11 @@ const struct format_type *fmt_from_vgname(const char *vgname, const char *vgid)
 		dm_free(devl);
 	}
 
+	dm_list_iterate_items(fmt, &cmd->formats) {
+		if (fmt->ops->scan && !fmt->ops->scan(fmt))
+			return NULL;
+	}
+
 	/* If vginfo changed, caller needs to rescan */
 	if (!(vginfo = vginfo_from_vgname(vgname, vgid_found)) ||
 	    strncmp(vginfo->vgid, vgid_found, ID_LEN))
diff --git a/lib/cache/lvmcache.h b/lib/cache/lvmcache.h
index 28f8541..2faec88 100644
--- a/lib/cache/lvmcache.h
+++ b/lib/cache/lvmcache.h
@@ -88,7 +88,9 @@ void lvmcache_unlock_vgname(const char *vgname);
 int lvmcache_verify_lock_order(const char *vgname);
 
 /* Queries */
-const struct format_type *fmt_from_vgname(const char *vgname, const char *vgid);
+const struct format_type *fmt_from_vgname(struct cmd_context *cmd,
+					  const char *vgname,
+					  const char *vgid);
 struct lvmcache_vginfo *vginfo_from_vgname(const char *vgname,
 					   const char *vgid);
 struct lvmcache_vginfo *vginfo_from_vgid(const char *vgid);
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 780b806..d09c693 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -2698,13 +2698,13 @@ static struct volume_group *_vg_read(struct cmd_context *cmd,
 
 	/* Find the vgname in the cache */
 	/* If it's not there we must do full scan to be completely sure */
-	if (!(fmt = fmt_from_vgname(vgname, vgid))) {
+	if (!(fmt = fmt_from_vgname(cmd, vgname, vgid))) {
 		lvmcache_label_scan(cmd, 0);
-		if (!(fmt = fmt_from_vgname(vgname, vgid))) {
+		if (!(fmt = fmt_from_vgname(cmd, vgname, vgid))) {
 			if (memlock())
 				return_NULL;
 			lvmcache_label_scan(cmd, 2);
-			if (!(fmt = fmt_from_vgname(vgname, vgid)))
+			if (!(fmt = fmt_from_vgname(cmd, vgname, vgid)))
 				return_NULL;
 		}
 	}
@@ -2863,7 +2863,7 @@ static struct volume_group *_vg_read(struct cmd_context *cmd,
 		if (memlock())
 			return_NULL;
 		lvmcache_label_scan(cmd, 2);
-		if (!(fmt = fmt_from_vgname(vgname, vgid)))
+		if (!(fmt = fmt_from_vgname(cmd, vgname, vgid)))
 			return_NULL;
 
 		if (precommitted && !(fmt->features & FMT_PRECOMMIT))
@@ -3788,9 +3788,9 @@ uint32_t vg_lock_newname(struct cmd_context *cmd, const char *vgname)
 
 	/* Find the vgname in the cache */
 	/* If it's not there we must do full scan to be completely sure */
-	if (!fmt_from_vgname(vgname, NULL)) {
+	if (!fmt_from_vgname(cmd, vgname, NULL)) {
 		lvmcache_label_scan(cmd, 0);
-		if (!fmt_from_vgname(vgname, NULL)) {
+		if (!fmt_from_vgname(cmd, vgname, NULL)) {
 			if (memlock()) {
 				/*
 				 * FIXME: Disallow calling this function if
@@ -3800,7 +3800,7 @@ uint32_t vg_lock_newname(struct cmd_context *cmd, const char *vgname)
 				return FAILED_LOCKING;
 			}
 			lvmcache_label_scan(cmd, 2);
-			if (!fmt_from_vgname(vgname, NULL)) {
+			if (!fmt_from_vgname(cmd, vgname, NULL)) {
 				/* vgname not found after scanning */
 				return SUCCESS;
 			}

[PATCH] Scan also special metadata areas (like metadata/dirs) in fmt_from_vgname

[Alasdair G Kergon]

On Fri, Nov 26, 2010 at 04:50:53PM +0100, Peter Rajnoha wrote:
> We reread labels and metadata from raw devices in fmt_from_vgname function,
> but we forgot to read special metadata areas? (like dirs defined by metadata/dirs
> config setting) Anyway, can someone remind me why we do scanning inside this fn
> at all?
 
We don't.

fmt_from_vgname *just* looks for the VG in the cache.  If it's not there, it
returns nothing.  If it *is* there, it rereads the PV metadata to check it's
still current.  Any scanning is the responsibility of the caller.

> Without this patch, we will fail to find a VG when metadata/dirs is only
> used and there are no other metadata areas (which is itself a strange situation,

Not at all strange.  If that's broken now, I'm afraid the fix lies elsewhere...

Alasdair

[PATCH] Scan also special metadata areas (like metadata/dirs) in fmt_from_vgname

[Alasdair G Kergon]

All that said, a hack like this might be the easiest way to fix this in the
short term until the cache handles mdas properly.

On Fri, Nov 26, 2010 at 04:50:53PM +0100, Peter Rajnoha wrote:
> @@ -400,6 +403,11 @@ const struct format_type *fmt_from_vgname(const char *vgname, const char *vgid)
>  		dm_free(devl);
>  	}
>  
> +	dm_list_iterate_items(fmt, &cmd->formats) {
> +		if (fmt->ops->scan && !fmt->ops->scan(fmt))

We can reduce the impact a bit by adding vgname as a parameter to
scan() and only running it if the vginfo got 'lost' within the
function.  (I.e. the first lookup found it, but after the label_reads()
it disappeared.)  

This type of metadata is incompatible with clustering, and memlock isn't
supported either so calling scan() here is inconsequential.


The fmt_from_vgname() triplets can also be optimised a bit:
  The last one should never need to do any label_reads because it follows
  a full scan, so it can return the answer directly from cache.
  The memlock() test before it can be skipped when there is in-filesystem
  metadata (so that the 3rd fmt_from_vgname always gets called).

So that provides an alternative fix I think: add parameter to fmt_from_vgname
telling it to skip the device checks and use that whenever it follows a
lvmcache_label_scan(cmd, 2); record whether or not in-filesystem metadata
is in use and if so avoid returning when memlock is also set.
(Treat raw the same as in-filesystem.)
(Can probably optimise that further too so it only needs to fall through the
triplet of functions the first time.)

Alasdair

[PATCH] Scan also special metadata areas (like metadata/dirs) in fmt_from_vgname

[Alasdair G Kergon]

On Tue, Nov 30, 2010 at 04:43:37AM +0000, Alasdair G Kergon wrote:
> record whether or not in-filesystem metadata
> is in use and if so avoid returning when memlock is also set.

_text_handler.scan could be changed between _text_scan and NULL
to signal this.

Alasdair

[PATCH] Scan also special metadata areas (like metadata/dirs) in fmt_from_vgname

[Alasdair G Kergon]

On Tue, Nov 30, 2010 at 04:43:37AM +0000, Alasdair G Kergon wrote:
> So that provides an alternative fix I think: add parameter to fmt_from_vgname

I've committed a fix along these lines.

Alasdair