LVM2 tools/lvmcmdlib.c lib/mm/memlock.h lib/mm ...

LVM2 tools/lvmcmdlib.c lib/mm/memlock.h lib/mm ...

[mornfall]

CVSROOT:	/cvs/lvm2
Module name:	LVM2
Changes by:	mornfall at sourceware.org	2009-11-19 01:11:57

Modified files:
	tools          : lvmcmdlib.c 
	lib/mm         : memlock.h memlock.c 

Log message:
	Fix another bug in memlock handling, this time the "global" dmeventd memlock
	was preventing device scans in lvconvert --repair running from inside dmeventd.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/tools/lvmcmdlib.c.diff?cvsroot=lvm2&r1=1.9&r2=1.10
http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/lib/mm/memlock.h.diff?cvsroot=lvm2&r1=1.4&r2=1.5
http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/lib/mm/memlock.c.diff?cvsroot=lvm2&r1=1.13&r2=1.14

--- LVM2/tools/lvmcmdlib.c	2009/02/22 22:11:59	1.9
+++ LVM2/tools/lvmcmdlib.c	2009/11/19 01:11:57	1.10
@@ -82,9 +82,9 @@
 	/* FIXME Temporary - move to libdevmapper */
 	ret = ECMD_PROCESSED;
 	if (!strcmp(cmdline, "_memlock_inc"))
-		memlock_inc();
+		memlock_inc_daemon();
 	else if (!strcmp(cmdline, "_memlock_dec"))
-		memlock_dec();
+		memlock_dec_daemon();
 	else
 		ret = lvm_run_command(cmd, argc, argv);
 
--- LVM2/lib/mm/memlock.h	2007/08/20 20:55:27	1.4
+++ LVM2/lib/mm/memlock.h	2009/11/19 01:11:57	1.5
@@ -20,6 +20,8 @@
 
 void memlock_inc(void);
 void memlock_dec(void);
+void memlock_inc_daemon(void);
+void memlock_dec_daemon(void);
 int memlock(void);
 void memlock_init(struct cmd_context *cmd);
 
--- LVM2/lib/mm/memlock.c	2009/11/18 18:22:32	1.13
+++ LVM2/lib/mm/memlock.c	2009/11/19 01:11:57	1.14
@@ -53,6 +53,7 @@
 
 static void *_malloc_mem = NULL;
 static int _memlock_count = 0;
+static int _memlock_count_daemon = 0;
 static int _priority;
 static int _default_priority;
 
@@ -123,22 +124,61 @@
 			  strerror(errno));
 }
 
+static void _lock_mem_if_needed(void) {
+	if ((_memlock_count + _memlock_count_daemon) == 1)
+		_lock_mem();
+}
+
+static void _unlock_mem_if_possible(void) {
+	if ((_memlock_count + _memlock_count_daemon) == 0)
+		_unlock_mem();
+}
+
 void memlock_inc(void)
 {
-	if (!_memlock_count++)
-		_lock_mem();
+	++_memlock_count;
+	_lock_mem_if_needed();
 	log_debug("memlock_count inc to %d", _memlock_count);
 }
 
 void memlock_dec(void)
 {
-	if (_memlock_count && (!--_memlock_count))
-		_unlock_mem();
-	log_debug("memlock_count dec to %d", _memlock_count);
-	if (_memlock_count < 0)
+	if (!_memlock_count)
 		log_error("Internal error: _memlock_count has dropped below 0.");
+	--_memlock_count;
+	_unlock_mem_if_possible();
+	log_debug("memlock_count dec to %d", _memlock_count);
 }
 
+/*
+ * The memlock_*_daemon functions will force the mlockall() call that we need
+ * to stay in memory, but they will have no effect on device scans (unlike
+ * normal memlock_inc and memlock_dec). Memory is kept locked as long as either
+ * of memlock or memlock_daemon is in effect.
+ */
+
+void memlock_inc_daemon(void)
+{
+	++_memlock_count_daemon;
+	_lock_mem_if_needed();
+	log_debug("memlock_count_daemon inc to %d", _memlock_count_daemon);
+}
+
+void memlock_dec_daemon(void)
+{
+	if (!_memlock_count_daemon)
+		log_error("Internal error: _memlock_count_daemon has dropped below 0.");
+	--_memlock_count_daemon;
+	_unlock_mem_if_possible();
+	log_debug("memlock_count_daemon dec to %d", _memlock_count_daemon);
+}
+
+/*
+ * This disregards the daemon (dmeventd) locks, since we use memlock() to check
+ * whether it is safe to run a device scan, which would normally coincide with
+ * !memlock() -- but the daemon global memory lock breaks this assumption, so
+ * we do not take those into account here.
+ */
 int memlock(void)
 {
 	return _memlock_count;

LVM2 tools/lvmcmdlib.c lib/mm/memlock.h lib/mm ...

[Marian Csontos]

On 11/19/2009 02:11 AM, mornfall at sourceware.org wrote:
> CVSROOT:	/cvs/lvm2
> Module name:	LVM2
> Changes by:	mornfall at sourceware.org	2009-11-19 01:11:57
>
> Modified files:
> 	tools          : lvmcmdlib.c
> 	lib/mm         : memlock.h memlock.c
>
> Log message:
> 	Fix another bug in memlock handling, this time the "global" dmeventd memlock
> 	was preventing device scans in lvconvert --repair running from inside dmeventd.
>
> Patches:
> http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/tools/lvmcmdlib.c.diff?cvsroot=lvm2&r1=1.9&r2=1.10
> http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/lib/mm/memlock.h.diff?cvsroot=lvm2&r1=1.4&r2=1.5
> http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/lib/mm/memlock.c.diff?cvsroot=lvm2&r1=1.13&r2=1.14
>
> --- LVM2/tools/lvmcmdlib.c	2009/02/22 22:11:59	1.9
> +++ LVM2/tools/lvmcmdlib.c	2009/11/19 01:11:57	1.10
> @@ -82,9 +82,9 @@
>   	/* FIXME Temporary - move to libdevmapper */
>   	ret = ECMD_PROCESSED;
>   	if (!strcmp(cmdline, "_memlock_inc"))
> -		memlock_inc();
> +		memlock_inc_daemon();
>   	else if (!strcmp(cmdline, "_memlock_dec"))
> -		memlock_dec();
> +		memlock_dec_daemon();
>   	else
>   		ret = lvm_run_command(cmd, argc, argv);
>
> --- LVM2/lib/mm/memlock.h	2007/08/20 20:55:27	1.4
> +++ LVM2/lib/mm/memlock.h	2009/11/19 01:11:57	1.5
> @@ -20,6 +20,8 @@
>
>   void memlock_inc(void);
>   void memlock_dec(void);
> +void memlock_inc_daemon(void);
> +void memlock_dec_daemon(void);
>   int memlock(void);
>   void memlock_init(struct cmd_context *cmd);
>
> --- LVM2/lib/mm/memlock.c	2009/11/18 18:22:32	1.13
> +++ LVM2/lib/mm/memlock.c	2009/11/19 01:11:57	1.14
> @@ -53,6 +53,7 @@
>
>   static void *_malloc_mem = NULL;
>   static int _memlock_count = 0;
> +static int _memlock_count_daemon = 0;
>   static int _priority;
>   static int _default_priority;
>
> @@ -123,22 +124,61 @@
>   			  strerror(errno));
>   }
>
> +static void _lock_mem_if_needed(void) {
> +	if ((_memlock_count + _memlock_count_daemon) == 1)
> +		_lock_mem();
> +}
> +
> +static void _unlock_mem_if_possible(void) {
> +	if ((_memlock_count + _memlock_count_daemon) == 0)
> +		_unlock_mem();
> +}
> +
>   void memlock_inc(void)
>   {
> -	if (!_memlock_count++)
> -		_lock_mem();
> +	++_memlock_count;
> +	_lock_mem_if_needed();
>   	log_debug("memlock_count inc to %d", _memlock_count);
>   }
>
>   void memlock_dec(void)
>   {
> -	if (_memlock_count&&  (!--_memlock_count))
> -		_unlock_mem();
> -	log_debug("memlock_count dec to %d", _memlock_count);
> -	if (_memlock_count<  0)
> +	if (!_memlock_count)
>   		log_error("Internal error: _memlock_count has dropped below 0.");
>    
Why not to zero _memlock_count here (and _memlock_count_daemon below)?
IMO, simple log_error is not enough. Though I understand this should not 
happen under any conditions, the Murphy's Law says it will happen. And 
when it happens...

...dropping below zero, will result in subsequent 
memlock_inc/memloc_inc_daemon having no effect. (Q: How serious is this 
condition? Could it result in data corruption?)

On the other hand, if it were zeroed, the possible deadlock could be the 
only result.
However, this could happen only when memory is unlocked before it is locked.

Cheers,

-- Marian

> +	--_memlock_count;
> +	_unlock_mem_if_possible();
> +	log_debug("memlock_count dec to %d", _memlock_count);
>   }
>
> +/*
> + * The memlock_*_daemon functions will force the mlockall() call that we need
> + * to stay in memory, but they will have no effect on device scans (unlike
> + * normal memlock_inc and memlock_dec). Memory is kept locked as long as either
> + * of memlock or memlock_daemon is in effect.
> + */
>    
Q: It does not work as proposed now. Does the "will" mean it will once 
implemented?
> +
> +void memlock_inc_daemon(void)
> +{
> +	++_memlock_count_daemon;
> +	_lock_mem_if_needed();
> +	log_debug("memlock_count_daemon inc to %d", _memlock_count_daemon);
> +}
> +
> +void memlock_dec_daemon(void)
> +{
> +	if (!_memlock_count_daemon)
> +		log_error("Internal error: _memlock_count_daemon has dropped below 0.");
>    
...and zero _memlock_count_daemon on error.
> +	--_memlock_count_daemon;
> +	_unlock_mem_if_possible();
> +	log_debug("memlock_count_daemon dec to %d", _memlock_count_daemon);
> +}
> +
> +/*
> + * This disregards the daemon (dmeventd) locks, since we use memlock() to check
> + * whether it is safe to run a device scan, which would normally coincide with
> + * !memlock() -- but the daemon global memory lock breaks this assumption, so
> + * we do not take those into account here.
> + */
>   int memlock(void)
>   {
>   	return _memlock_count;
>
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel
>    

LVM2 tools/lvmcmdlib.c lib/mm/memlock.h lib/mm ...

[Petr Rockai]

Marian Csontos <mcsontos@redhat.com> writes:
> Why not to zero _memlock_count here (and _memlock_count_daemon below)?
> IMO, simple log_error is not enough. Though I understand this should not happen
> under any conditions, the Murphy's Law says it will happen. And when it
> happens...
>
> ...dropping below zero, will result in subsequent memlock_inc/memloc_inc_daemon
> having no effect. (Q: How serious is this condition? Could it result in data
> corruption?)
When the value is out of sync once, there's no really good way to
recover. Too high will prevent scans, too low will cause deadlocks, the
result always being non-functional code.

> On the other hand, if it were zeroed, the possible deadlock could be
> the only result.  However, this could happen only when memory is
> unlocked before it is locked.
See above.

>> +/*
>> + * The memlock_*_daemon functions will force the mlockall() call that we need
>> + * to stay in memory, but they will have no effect on device scans (unlike
>> + * normal memlock_inc and memlock_dec). Memory is kept locked as long as either
>> + * of memlock or memlock_daemon is in effect.
>> + */
>>    
> Q: It does not work as proposed now. Does the "will" mean it will once
> implemented?
Why not? As far as I can tell, this works as advertised, and testing
confirms that.

Yours,
   Petr.

LVM2 tools/lvmcmdlib.c lib/mm/memlock.h lib/mm ...

[Marian Csontos]

On 11/19/2009 12:59 PM, Petr Rockai wrote:
> Marian Csontos<mcsontos@redhat.com>  writes:
>    
>> Why not to zero _memlock_count here (and _memlock_count_daemon below)?
>> IMO, simple log_error is not enough. Though I understand this should not happen
>> under any conditions, the Murphy's Law says it will happen. And when it
>> happens...
>>
>> ...dropping below zero, will result in subsequent memlock_inc/memloc_inc_daemon
>> having no effect. (Q: How serious is this condition? Could it result in data
>> corruption?)
>>      
> When the value is out of sync once, there's no really good way to
> recover. Too high will prevent scans, too low will cause deadlocks, the
> result always being non-functional code.
>    
If it is non functional scans and not data corruption as I had thought, 
then it is safer to leave as is.
>> On the other hand, if it were zeroed, the possible deadlock could be
>> the only result.  However, this could happen only when memory is
>> unlocked before it is locked.
>>      
> See above.
>
>    
>>> +/*
>>> + * The memlock_*_daemon functions will force the mlockall() call that we need
>>> + * to stay in memory, but they will have no effect on device scans (unlike
>>> + * normal memlock_inc and memlock_dec). Memory is kept locked as long as either
>>> + * of memlock or memlock_daemon is in effect.
>>> + */
>>>
>>>        
>> Q: It does not work as proposed now. Does the "will" mean it will once
>> implemented?
>>      
> Why not? As far as I can tell, this works as advertised, and testing
> confirms that.
>    
Oh, I see. It must be the memlock function, what affects device scans.
I apologize.

I noticed some failures while looking at nevrast/waterfall, and in my 
zeal reviewed changes what might be causing troubles. Of course the 
changes were reviewed and tested carefully before, thus it is just my 
reasoning what's wrong here and the mistake is elsewhere.

Cheers,

-- Marian

> Yours,
>     Petr.
>
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel
>    

LVM2 tools/lvmcmdlib.c lib/mm/memlock.h lib/mm ...

[Marian Csontos]

On 11/19/2009 02:03 PM, Marian Csontos wrote:
> On 11/19/2009 12:59 PM, Petr Rockai wrote:
>> Marian Csontos<mcsontos@redhat.com>  writes:
>>> Why not to zero _memlock_count here (and _memlock_count_daemon below)?
>>> IMO, simple log_error is not enough. Though I understand this should 
>>> not happen
>>> under any conditions, the Murphy's Law says it will happen. And when it
>>> happens...
>>>
>>> ...dropping below zero, will result in subsequent 
>>> memlock_inc/memloc_inc_daemon
>>> having no effect. (Q: How serious is this condition? Could it result 
>>> in data
>>> corruption?)
>> When the value is out of sync once, there's no really good way to
>> recover. Too high will prevent scans, too low will cause deadlocks, the
>> result always being non-functional code.
> If it is non functional scans and not data corruption as I had 
> thought, then it is safer to leave as is.
However, after better looking at the code, I am prone to think the 
current solution might lead to deadlock and not to non-functional scans 
(and that's what you have said too):

When one of _memlock_count* variables is 0 and the other one -1, the 
subsequent memlock_inc* will have no effect and thus would not prevent 
pages from being swapped out. Is not it this what is causing the 
deadlocks we are talking about?

If it is two bad solutions to choose from, which one is worse, deadlock 
or broken scan?

Could broken scan lead to data corruption, or would this affect VG 
administration tools only?

What are the consequences of deadlock we are talking about? Is it frozen 
system, or just frozen LVM user-space? I reckon it is system wide death, 
and thus much worse solution than not-working userspace tools, but still 
better than data corruption.

I see the tests are now fixed, so this is now, hopefully, hypothetical 
problem...

>>> On the other hand, if it were zeroed, the possible deadlock could be
>>> the only result.  However, this could happen only when memory is
>>> unlocked before it is locked.
>> See above.
>>
>>>> +/*
>>>> + * The memlock_*_daemon functions will force the mlockall() call 
>>>> that we need
>>>> + * to stay in memory, but they will have no effect on device scans 
>>>> (unlike
>>>> + * normal memlock_inc and memlock_dec). Memory is kept locked as 
>>>> long as either
>>>> + * of memlock or memlock_daemon is in effect.
>>>> + */
>>>>
>>> Q: It does not work as proposed now. Does the "will" mean it will once
>>> implemented?
>> Why not? As far as I can tell, this works as advertised, and testing
>> confirms that.
> Oh, I see. It must be the memlock function, what affects device scans.
> I apologize.
>
> I noticed some failures while looking at nevrast/waterfall, and in my 
> zeal reviewed changes what might be causing troubles. Of course the 
> changes were reviewed and tested carefully before, thus it is just my 
> reasoning what's wrong here and the mistake is elsewhere.
>
> Cheers,
>
> -- Marian
>
>> Yours,
>>     Petr.
>>
>> -- 
>> lvm-devel mailing list
>> lvm-devel at redhat.com
>> https://www.redhat.com/mailman/listinfo/lvm-devel
>
> -- 
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel