Memory fixes

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

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.

---
 tools/vgmerge.c |    6 ++++--
 tools/vgsplit.c |    6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

index adb00a0..e4431be 100644
@@ -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;
 }

index b97db97..f30c335 100644
@@ -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

Dne 26.3.2011 10:59, Milan Broz napsal(a):

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

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.

---
 lib/commands/toolcontext.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

index 30c8fc6..be2ba5c 100644
@@ -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

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

Dne 26.3.2011 11:09, Milan Broz napsal(a):

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

Dne 26.3.2011 13:53, Zdenek Kabelac napsal(a):

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>

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.

---
 lib/mm/memlock.c     |    3 +++
 libdm/mm/pool-fast.c |   11 ++++-------
 2 files changed, 7 insertions(+), 7 deletions(-)

index 3e472bd..0e8f585 100644
@@ -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",

index 377ad99..b651449 100644
@@ -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