[PATCH] Do not count VG_GLOBAL to lock_count.

[PATCH] Do not count VG_GLOBAL to lock_count.

[Milan Broz]

Do not count VG_GLOBAL to lock_count.

Otherwise all read-only scanning will open devices RW.
because of
	flags = vg_write_lock_held() ? O_RDWR : O_RDONLY;

(And trigger udev rule because of IN_CLOSE_WRITE monitoring).

Anyway, there are many other situations when lvm opens
device RW unnecessarily...

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 lib/locking/locking.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/locking/locking.c b/lib/locking/locking.c
index 5c04b4f..482cdf0 100644
--- a/lib/locking/locking.c
+++ b/lib/locking/locking.c
@@ -189,12 +189,16 @@ void reset_locking(void)
 		_unblock_signals();
 }
 
-static void _update_vg_lock_count(uint32_t flags)
+static void _update_vg_lock_count(const char *resource, uint32_t flags)
 {
 	if ((flags & LCK_SCOPE_MASK) != LCK_VG ||
 	    (flags & LCK_CACHE))
 		return;
 
+	/* Ignore global locks */
+	if (!strcmp(resource, VG_GLOBAL))
+		return;
+
 	if ((flags & LCK_TYPE_MASK) == LCK_UNLOCK)
 		_vg_lock_count--;
 	else
@@ -356,7 +360,7 @@ static int _lock_vol(struct cmd_context *cmd, const char *resource, uint32_t fla
 								== LCK_READ);
 		}
 
-		_update_vg_lock_count(flags);
+		_update_vg_lock_count(resource, flags);
 	}
 
 	_unlock_memory(flags);

[PATCH] Do not count VG_GLOBAL to lock_count.

[Dave Wysochanski]

On Fri, 2009-07-10 at 16:10 +0200, Milan Broz wrote:
> Do not count VG_GLOBAL to lock_count.
> 

This description is misleading - it's not the count that matters its the
flag _vg_write_lock_held, and that's set based on the lock type:
	/* We don't bother to reset this until all VG locks are dropped */
	if ((flags & LCK_TYPE_MASK) == LCK_WRITE)
		_vg_write_lock_held = 1;
	else if (!_vg_lock_count)
		_vg_write_lock_held = 0;
}

Wouldn't a better fix just be changing pvscan and vgscan to use
LCK_VG_READ instead of LCK_VG_WRITE since they are not updating
metadata?  Or is there a problem with doing that?

I guess in general I don't like the idea of affecting the count.  Unless
of course we want to call "VG_GLOBAL" not really a vg lock.  But then
the naming seems inconsistent.

[PATCH] Do not count VG_GLOBAL to lock_count.

[Milan Broz]

Dave Wysochanski wrote:
> On Fri, 2009-07-10 at 16:10 +0200, Milan Broz wrote:
> Wouldn't a better fix just be changing pvscan and vgscan to use
> LCK_VG_READ instead of LCK_VG_WRITE since they are not updating
> metadata?  Or is there a problem with doing that?

The VG_LOCK thing is one hack, it is a "special" lock...

It was introduced because of cluster, to notify other nodes
about need of metadata cache changes.

Moreover, the real metadata are cached, only if VG_GLOBAL is held.

Anyway, if you want to manipulate with any VG, you have to take
anothe (real) VG locks.

I tried to use the simplest solution - ignore VG_GLOBAL the same way
how lvmcache ingores it

	if (strcmp(vgname, VG_GLOBAL))
		_vgs_locked++;

Your proposal is possible too, but it means than we need to audit all
uses.

I remember that I used some special handling in process_each_pv()
for example if LCK_VG_READ is set...

(It will probably work but I used the simplest solution here.) 
 
> I guess in general I don't like the idea of affecting the count.  Unless
> of course we want to call "VG_GLOBAL" not really a vg lock.  But then
> the naming seems inconsistent.

The whole idea of using special locks for communication between nodes
is misleading but also simple:-)

Milan