[PATCH 0/3] Memory fixes

[PATCH 0/3] Memory fixes

[Zdenek Kabelac]

Originaly I wanted to post here new patch for dm_pool_splice
to join to dm_pool list together. However there is fairly simplier
fix for our memory reference problem - just fixing the vgmem release
order. (I may provide  dm_pool_splice patch - as it could be seen
as 'better' way of handling this memory problem - but as for now
it only related to vgmerge and vgsplite - it's easier this way.)

Second patch is addressing our maps mismatching - it's rather workaround,
but for now quite sufficient.

The last patch is for those who want to play with valgrind and lvm2.
There we need to disable memory size checks.

Zdenek Kabelac (3):
  Fix mem
  Set our own in/out buffers
  Valgrind updates

 lib/commands/toolcontext.c |    8 ++++++++
 lib/mm/memlock.c           |    3 +++
 libdm/mm/pool-fast.c       |   11 ++++-------
 tools/vgmerge.c            |    6 ++++--
 tools/vgsplit.c            |    6 ++++--
 5 files changed, 23 insertions(+), 11 deletions(-)

-- 
1.7.4.1

[PATCH 1/3] Fix mem

[Zdenek Kabelac]

Fix release order when some data reference are moved
between pools - as data are move 'from -> to'
we just need to ensure 'from' pool will be released
as the last.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 tools/vgmerge.c |    6 ++++--
 tools/vgsplit.c |    6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/tools/vgmerge.c b/tools/vgmerge.c
index adb00a0..e4431be 100644
--- a/tools/vgmerge.c
+++ b/tools/vgmerge.c
@@ -155,8 +155,10 @@ bad:
 		unlock_and_free_vg(cmd, vg_to, vg_name_to);
 		unlock_and_free_vg(cmd, vg_from, vg_name_from);
 	} else {
-		unlock_and_free_vg(cmd, vg_from, vg_name_from);
-		unlock_and_free_vg(cmd, vg_to, vg_name_to);
+		unlock_vg(cmd, vg_name_from);
+		unlock_vg(cmd, vg_name_to);
+		free_vg(vg_to);
+		free_vg(vg_from);
 	}
 	return r;
 }
diff --git a/tools/vgsplit.c b/tools/vgsplit.c
index b97db97..f30c335 100644
--- a/tools/vgsplit.c
+++ b/tools/vgsplit.c
@@ -491,8 +491,10 @@ bad:
 		unlock_and_free_vg(cmd, vg_to, vg_name_to);
 		unlock_and_free_vg(cmd, vg_from, vg_name_from);
 	} else {
-		unlock_and_free_vg(cmd, vg_from, vg_name_from);
-		unlock_and_free_vg(cmd, vg_to, vg_name_to);
+		unlock_vg(cmd, vg_name_from);
+		unlock_vg(cmd, vg_name_to);
+		free_vg(vg_to);
+		free_vg(vg_from);
 	}
 	return r;
 }
-- 
1.7.4.1

[PATCH 1/3] Fix mem

[Milan Broz]

On 03/25/2011 10:57 PM, Zdenek Kabelac wrote:

> -		unlock_and_free_vg(cmd, vg_from, vg_name_from);
> -		unlock_and_free_vg(cmd, vg_to, vg_name_to);
> +		unlock_vg(cmd, vg_name_from);
> +		unlock_vg(cmd, vg_name_to);
> +		free_vg(vg_to);
> +		free_vg(vg_from);

I guess there is still request to do proper deep copy...

Anyway, this is so simple and obvious for now -> ACK

Milan

[PATCH 1/3] Fix mem

[Zdenek Kabelac]

Dne 26.3.2011 10:59, Milan Broz napsal(a):
> On 03/25/2011 10:57 PM, Zdenek Kabelac wrote:
> 
>> -		unlock_and_free_vg(cmd, vg_from, vg_name_from);
>> -		unlock_and_free_vg(cmd, vg_to, vg_name_to);
>> +		unlock_vg(cmd, vg_name_from);
>> +		unlock_vg(cmd, vg_name_to);
>> +		free_vg(vg_to);
>> +		free_vg(vg_from);
> 
> I guess there is still request to do proper deep copy...
> 
> Anyway, this is so simple and obvious for now -> ACK


In fact there is even simpler version - to always unconditionally unlock and
free VGs in this order:

--
unlock_and_free_vg(cmd, vg_to, vg_name_to);
unlock_and_free_vg(cmd, vg_from, vg_name_from);
--

This simplifies and shortens the code a bit ;). 'vg_to' is always released
before 'vg_from' - if we make this a documented rule in the source code - we
currently avoid the need of deep copy (It would quite complicate our code, to
add such 'deep' copy code to every target - as we are not in C++ - it's quite
ugly task to do).  As long as we only move object pointers from 'vg_from' to
'vg_to' it's safe - and we may even avoid partial 'deep' copy of some object
there - if we can count with the right free_vg() order.
(Also implementing of deep-copy only for very occasionally used vgmerge and
vgsplit currently sound like a bit to complex).

AFAIK - for deadlock-less locking - only the locking order is important,
unlocking could happen in any order as long as all locks are always released
before the next usage - so current unlock ordering is not needed.

Zdenek

[PATCH 2/3] Set our own in/out buffers

[Zdenek Kabelac]

Memory lock from critical_section is now being kept over the critical
section - mallopt() should ensure, that mmap is not used for allocation,
and we preallocate some memory to be able to satisfy some small
alloc request. However when glibc needs buffers for line buffering of
input and output buffers - it allocates these buffers in such way it
adds memory page for each such buffer and size of unlock memory check will
mismatch by 1 or 2 pages.

This happens when we print or read lines without '\n' so these buffers
are used. To avoid this extra allocation, use setvbuf to set these
bufffers ahead.

FIXME: Bigger fix is needs to avoid quering users while holding VG
locks, as such question might take quite some time....
Thus should in effect unlock memory before displaying such message.
But some more work needs to be done for this - so hack for now.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/commands/toolcontext.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/lib/commands/toolcontext.c b/lib/commands/toolcontext.c
index 30c8fc6..be2ba5c 100644
--- a/lib/commands/toolcontext.c
+++ b/lib/commands/toolcontext.c
@@ -1129,6 +1129,8 @@ static void _init_globals(struct cmd_context *cmd)
 struct cmd_context *create_toolcontext(unsigned is_long_lived,
 				       const char *system_dir)
 {
+    	static char inbuf[4096];
+	static char outbuf[4096];
 	struct cmd_context *cmd;
 
 #ifdef M_MMAP_MAX
@@ -1159,6 +1161,12 @@ struct cmd_context *create_toolcontext(unsigned is_long_lived,
 	dm_list_init(&cmd->tags);
 	dm_list_init(&cmd->config_files);
 
+        /* Set in/out line buffers before glibc */
+	if (setvbuf(stdin, inbuf, _IOLBF, sizeof(inbuf)) != 0)
+		log_sys_error("setvbuf", "in");
+	if (setvbuf(stdout, outbuf, _IOLBF, sizeof(outbuf)) != 0)
+		log_sys_error("setvbuf", "out");
+
 	/* FIXME Make this configurable? */
 	reset_lvm_errno(1);
 
-- 
1.7.4.1

[PATCH 2/3] Set our own in/out buffers

[Milan Broz]

On 03/25/2011 10:57 PM, Zdenek Kabelac wrote:
> Memory lock from critical_section is now being kept over the critical
> section - mallopt() should ensure, that mmap is not used for allocation,
> and we preallocate some memory to be able to satisfy some small
> alloc request. However when glibc needs buffers for line buffering of
> input and output buffers - it allocates these buffers in such way it
> adds memory page for each such buffer and size of unlock memory check will
> mismatch by 1 or 2 pages.

ack in principle, these warnings appears quite often on various places
and distract people form real problems.

> +    	static char inbuf[4096];
> +	static char outbuf[4096];

Because we are handling stdin/stdout, I think this call is common
for glibc, right?

Shouldn't this be initialized only once (do we support multiple command
contexts, IOW reentrant code here)? Is create_toolcontext() the right
place for it?

Milan

[PATCH 2/3] Set our own in/out buffers

[Zdenek Kabelac]

Dne 26.3.2011 11:09, Milan Broz napsal(a):
> On 03/25/2011 10:57 PM, Zdenek Kabelac wrote:
>> Memory lock from critical_section is now being kept over the critical
>> section - mallopt() should ensure, that mmap is not used for allocation,
>> and we preallocate some memory to be able to satisfy some small
>> alloc request. However when glibc needs buffers for line buffering of
>> input and output buffers - it allocates these buffers in such way it
>> adds memory page for each such buffer and size of unlock memory check will
>> mismatch by 1 or 2 pages.
> 
> ack in principle, these warnings appears quite often on various places
> and distract people form real problems.
> 
>> +    	static char inbuf[4096];
>> +	static char outbuf[4096];
> 
> Because we are handling stdin/stdout, I think this call is common
> for glibc, right?
> 
> Shouldn't this be initialized only once (do we support multiple command
> contexts, IOW reentrant code here)? Is create_toolcontext() the right
> place for it?

AFAIK lvm2 code is quite far far away from being reentrant - thus I'd not say
there could be any new problem with this. However if the lvm2api user will set
his own buffering it will get overwritten.

So another option would be to move it into lvm_main - but I'm not sure,
how to resolve lvm2api users - but I guess we may set some limits for these
users - anyway - I think we have not even resolved yet, how is the memory
locking supposed to work for them...

The real fix is to avoid sending text lines without '\n' while the memory is
locked - as we do this while queering some questions - once we will do this
outside VG locks - we may probably again switch to default glib settings.
Until this is fixed - this patch avoids distracting warnings.

Zdenek

[PATCH 2/3] Set our own in/out buffers

[Zdenek Kabelac]

Dne 26.3.2011 13:53, Zdenek Kabelac napsal(a):
> Dne 26.3.2011 11:09, Milan Broz napsal(a):
>> On 03/25/2011 10:57 PM, Zdenek Kabelac wrote:
>>> Memory lock from critical_section is now being kept over the critical
>>> section - mallopt() should ensure, that mmap is not used for allocation,
>>> and we preallocate some memory to be able to satisfy some small
>>> alloc request. However when glibc needs buffers for line buffering of
>>> input and output buffers - it allocates these buffers in such way it
>>> adds memory page for each such buffer and size of unlock memory check will
>>> mismatch by 1 or 2 pages.
>>
>> ack in principle, these warnings appears quite often on various places
>> and distract people form real problems.
>>
>>> +    	static char inbuf[4096];
>>> +	static char outbuf[4096];
>>
>> Because we are handling stdin/stdout, I think this call is common
>> for glibc, right?
>>
>> Shouldn't this be initialized only once (do we support multiple command
>> contexts, IOW reentrant code here)? Is create_toolcontext() the right
>> place for it?
> 
> AFAIK lvm2 code is quite far far away from being reentrant - thus I'd not say
> there could be any new problem with this. However if the lvm2api user will set
> his own buffering it will get overwritten.
> 
> So another option would be to move it into lvm_main - but I'm not sure,
> how to resolve lvm2api users - but I guess we may set some limits for these
> users - anyway - I think we have not even resolved yet, how is the memory
> locking supposed to work for them...
> 
> The real fix is to avoid sending text lines without '\n' while the memory is
> locked - as we do this while queering some questions - once we will do this
> outside VG locks - we may probably again switch to default glib settings.
> Until this is fixed - this patch avoids distracting warnings.
> 

Here is updated version  based on review, which enables this only for liblvm
and lvm2api and does not modify glibc settings in CLVMD and reverts glibc
default behavior when context is destroyed.

Zdenek

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-Set-our-own-in-out-buffers.patch
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20110404/f9afac97/attachment.ksh>

[PATCH 3/3] Valgrind updates

[Zdenek Kabelac]

Avoid locking sum with valgrind compilation.

Make memory unaccessible in valgrind for dm_pool_abadon_object.

Valgrind hinting should not be needed in _free_chunk for dm_free.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/mm/memlock.c     |    3 +++
 libdm/mm/pool-fast.c |   11 ++++-------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/mm/memlock.c b/lib/mm/memlock.c
index 3e472bd..0e8f585 100644
--- a/lib/mm/memlock.c
+++ b/lib/mm/memlock.c
@@ -195,6 +195,9 @@ static int _maps_line(const struct config_node *cn, lvmlock_t lock,
 		}
 	}
 
+#ifdef VALGRIND_POOL
+	sz -= sz;  /* memory sum doesn't work with valgrind */
+#endif
 	*mstats += sz;
 	log_debug("%s %10ldKiB %12lx - %12lx %c%c%c%c%s",
 		  (lock == LVM_MLOCK) ? "mlock" : "munlock",
diff --git a/libdm/mm/pool-fast.c b/libdm/mm/pool-fast.c
index 377ad99..b651449 100644
--- a/libdm/mm/pool-fast.c
+++ b/libdm/mm/pool-fast.c
@@ -238,6 +238,9 @@ void *dm_pool_end_object(struct dm_pool *p)
 
 void dm_pool_abandon_object(struct dm_pool *p)
 {
+#ifdef VALGRIND_POOL
+	VALGRIND_MAKE_MEM_NOACCESS(p->chunk, p->object_len);
+#endif
 	p->object_len = 0;
 	p->object_alignment = DEFAULT_ALIGNMENT;
 }
@@ -278,11 +281,5 @@ static struct chunk *_new_chunk(struct dm_pool *p, size_t s)
 
 static void _free_chunk(struct chunk *c)
 {
-	if (c) {
-#ifdef VALGRIND_POOL
-		VALGRIND_MAKE_MEM_UNDEFINED(c, c->end - (char *) c);
-#endif
-
-		dm_free(c);
-	}
+	dm_free(c);
 }
-- 
1.7.4.1