VG share v2

PATCH New Zdenek Kabelac 2 participants 20 messages Mar 22, 2011 – Mar 24, 2011

Share & tools

Zdenek Kabelac

author

2011-03-22 21:21 UTC #0 lore raw

cover VG share v2

Second version of VG sharing patch.
Still rather in 'proposal' form as some part of code are not fully finished (pool-debug code is missing, usage of posix_memalign). If someone notices some weak points - let me know.

Patchset add support for locking and unlocking memory pool. Such pool has volatile memory area attached.
VG sharing is made in way the only 1 user could work with such VG. It may optionaly modify few small parts (only uin64_t is supported) which are reverted before next reuse.

2 methods to catch writes are implemented -
mprotect - memory consuming, and creates immediate segfault when
expected change of pool content happens. But it's fast and give exact location of such writer.

pool_crc - simple hash crc checksum - gives notice that after multiple
uses is not in its original state - doesn't segfault, but it's hard to detect such writer. And checksumming consumes some CPU.

Pool volatile area uses very simle 'exception pair values' - if there would be a lot of them - more efficent code would be needed. Also support for storing more differnet types would add some complexity. Both these extension do not seem to be needed with current code.

Patch set - as is - passes valgrind tests.

Zdenek Kabelac (6):
Pool locking code
Code move vg_mark_partial up in stack
Share VG multiple times
Use dm_pool_set_uint64
lv_postorder using dm_pool_set_uint64
lv_postorder unlock and lock

 lib/activate/activate.c       |    5 +-
 lib/cache/lvmcache.c          |  111 ++++++++++++++++++-
 lib/cache/lvmcache.h          |    3 +
 lib/metadata/lv.h             |    5 +-
 lib/metadata/metadata.c       |   58 ++++++++--
 lib/metadata/snapshot_manip.c |   15 ++-
 lib/metadata/vg.h             |    2 +
 libdm/libdevmapper.h          |   11 ++
 libdm/mm/pool-debug.c         |    9 ++
 libdm/mm/pool-fast.c          |   74 ++++++++++++-
 libdm/mm/pool.c               |  246 ++++++++++++++++++++++++++++++++++++++++-
 11 files changed, 517 insertions(+), 22 deletions(-)

Signature · 1 line

1.7.4.1

Zdenek Kabelac

author

2011-03-22 21:21 UTC #1 lore raw

PATCH 1/6 Pool locking code

Adding specific functions to lock and unlock pool, to modify specific uint64_t*, and to revert all modified uint64_t back.

Current API is the result of several iterations I've made.

To avoid some more rewrites in case this API would still require some changes - only pool-fast are currently fully updated for this API.

2 ways to debug code:
mprotect - quite fast all the time - but requires more memory and
currently it is using posix_memalign - this would be later modified to use dm_malloc() and align internally.

crc - checksum of locked pool - implemented via quick simple hash.
But even this gets quite slow when the pool has bigger size - thus it's not very effiecient to check with every restore, so the check is made only when reused VG is released.

Probably needs some disscusion where to move futher...

Signed-off-by Zdenek Kabelac <[email protected]>

---
 libdm/libdevmapper.h  |   11 ++
 libdm/mm/pool-debug.c |    9 ++
 libdm/mm/pool-fast.c  |   74 ++++++++++++++-
 libdm/mm/pool.c       |  246 ++++++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 337 insertions(+), 3 deletions(-)

libdm/libdevmapper.h +11 -0

index 88bddb1..a0bed97 100644
@@ -594,6 +594,17 @@ void *dm_pool_alloc_aligned(struct dm_pool *p, size_t s, unsigned alignment);
 void dm_pool_empty(struct dm_pool *p);
 void dm_pool_free(struct dm_pool *p, void *ptr);
 
+/* prevent modification of pool */
+int dm_pool_locked(struct dm_pool *p);
+int dm_pool_lock(struct dm_pool *p, unsigned count)
+	__attribute__((__warn_unused_result__));
+int dm_pool_unlock(struct dm_pool *p)
+	__attribute__((__warn_unused_result__));
+int dm_pool_set_uint64(struct dm_pool *p, uint64_t *addr, uint64_t new_value)
+	__attribute__((__warn_unused_result__));
+int dm_pool_restore(struct dm_pool *p, int check_crc)
+	__attribute__((__warn_unused_result__));
+
 /*
  * Object building routines:
  *

libdm/mm/pool-debug.c +9 -0

index fd931e4..206dd84 100644
@@ -33,6 +33,7 @@ struct dm_pool {
 	struct dm_list list;
 	const char *name;
 	void *orig_pool;	/* to pair it with first allocation call */
+	unsigned locked;
 
 	int begun;
 	struct block *object;

@@ -81,6 +82,8 @@ static void _free_blocks(struct dm_pool *p, struct block *b)
 {
 	struct block *n;
 
+	assert(!p->locked);
+
 	while (b) {
 		p->stats.bytes -= b->size;
 		p->stats.blocks_allocated--;

@@ -94,6 +97,8 @@ static void _free_blocks(struct dm_pool *p, struct block *b)
 
 static void _pool_stats(struct dm_pool *p, const char *action)
 {
+	assert(!p->locked);
+
 #ifdef DEBUG_POOL
 	log_debug("%s mempool %s: %u/%u bytes, %u/%u blocks, "
 		  "%u allocations)", action, p->name, p->stats.bytes,

@@ -119,6 +124,8 @@ void *dm_pool_alloc(struct dm_pool *p, size_t s)
 
 static void _append_block(struct dm_pool *p, struct block *b)
 {
+	assert(!p->locked);
+
 	if (p->tail) {
 		p->tail->next = b;
 		p->tail = b;

@@ -226,6 +233,8 @@ int dm_pool_grow_object(struct dm_pool *p, const void *extra, size_t delta)
 	struct block *new;
 	size_t new_size;
 
+	assert(!p->locked);
+
 	if (!delta)
 		delta = strlen(extra);

libdm/mm/pool-fast.c +72 -2

116 lines not on this page · raw message

libdm/mm/pool.c +245 -1

264 lines not on this page · raw message

Signature · 1 line

1.7.4.1

Joe Thornber

2011-03-23 14:03 UTC #2 lore raw

PATCH 1/6 Pool locking code

Quoted 4 lines · Zdenek Kabelac

Adding specific functions to lock and unlock pool, to modify specific uint64_t*, and to revert all modified uint64_t back.
libdm/libdevmapper.h +1 -0
+/* prevent modification of pool */

libdm/libdevmapper.h void *dm_pool_alloc_aligned(struct dm_pool *p, size_t s, unsigned alignment); · line 607

Quoted 10 lines

libdm/libdevmapper.h +10 -0

+int dm_pool_locked(struct dm_pool *p);
+int dm_pool_lock(struct dm_pool *p, unsigned count)
+	__attribute__((__warn_unused_result__));
+int dm_pool_unlock(struct dm_pool *p)
+	__attribute__((__warn_unused_result__));
+int dm_pool_set_uint64(struct dm_pool *p, uint64_t *addr, uint64_t new_value)
+	__attribute__((__warn_unused_result__));
+int dm_pool_restore(struct dm_pool *p, int check_crc)
+	__attribute__((__warn_unused_result__));
+

Hi Kabi,

I like what you're trying to do here. But I don't really like the above interface, particularly the dm_pool_set_uint64() method which is going to be ugly to use.

Ideally I guess I'd prefer something like:

        struct dm_pool_snapshot;
        
        struct dm_pool_snapshot *dm_pool_take_snapshot(struct dm_pool *p);
        void dm_pool_destroy_snapshot(struct dm_pool_snapshot *snap);
        int dm_pool_compare(struct dm_pool *p, struct dm_pool_snapshot *snap);
        int dm_pool_restore_snapshot(struct dm_pool *p, struct dm_pool_snapshot *snap, int release_subsequently_allocated_data);

You'd use the compare function within assertions to check nobody has changed a pool unexpectedly. There's no concept of a locked pool, you don't have to change the existing pool code to check if you're locked. Also you don't need a special interface for changing data within a locked pool.

The simple way of implementing the above is to just make a copy of the pool data within the snapshot. Which of course is a very slow op. So could you give me an idea of the size of a typical pool when you are locking it? How frequently do you lock?

- Joe

Zdenek Kabelac

author

2011-03-23 14:43 UTC #3 lore raw

PATCH 1/6 Pool locking code

Dne 23.3.2011 15:03, Joe Thornber napsal(a):

Quoted 37 lines · Zdenek Kabelac

Quoted 4 lines
Adding specific functions to lock and unlock pool, to modify specific uint64_t*, and to revert all modified uint64_t back.
libdm/libdevmapper.h +1 -0
+/* prevent modification of pool */
libdm/libdevmapper.h void *dm_pool_alloc_aligned(struct dm_pool *p, size_t s, unsigned alignment); · line 607
Quoted 10 lines
libdm/libdevmapper.h +10 -0
+int dm_pool_locked(struct dm_pool *p);
+int dm_pool_lock(struct dm_pool *p, unsigned count)
+	__attribute__((__warn_unused_result__));
+int dm_pool_unlock(struct dm_pool *p)
+	__attribute__((__warn_unused_result__));
+int dm_pool_set_uint64(struct dm_pool *p, uint64_t *addr, uint64_t new_value)
+	__attribute__((__warn_unused_result__));
+int dm_pool_restore(struct dm_pool *p, int check_crc)
+	__attribute__((__warn_unused_result__));
+
Hi Kabi,
I like what you're trying to do here. But I don't really like the above interface, particularly the dm_pool_set_uint64() method which is going to be ugly to use.
Ideally I guess I'd prefer something like:
        struct dm_pool_snapshot;
        
        struct dm_pool_snapshot *dm_pool_take_snapshot(struct dm_pool *p);
        void dm_pool_destroy_snapshot(struct dm_pool_snapshot *snap);
        int dm_pool_compare(struct dm_pool *p, struct dm_pool_snapshot *snap);
        int dm_pool_restore_snapshot(struct dm_pool *p, struct dm_pool_snapshot *snap, int release_subsequently_allocated_data);
You'd use the compare function within assertions to check nobody has changed a pool unexpectedly. There's no concept of a locked pool, you don't have to change the existing pool code to check if you're locked. Also you don't need a special interface for changing data within a locked pool.

Quoted 5 lines

The simple way of implementing the above is to just make a copy of the pool data within the snapshot. Which of course is a very slow op. So could you give me an idea of the size of a typical pool when you are locking it? How frequently do you lock?

It's been my first idea - to copy all chunks to separate buffer - but the VG mempool size for large VGs is actually quite big - thus doing always a snapshot isn't performance optimal. I'm doing my tests with ~7000LV - and it would be quite noticable to copy all the pool chunks with each use of it.

That's why I came with keeping only few separate values - which is way more efficient in this case.

As you can see in patch 4,5,6 only very few modifications are being made to the pool itself - so the restore is usually only for few uin64_t in exception area.

I'm aware the API isn't the best - but it seems the most efficient I could think of for now.

We may probably trade of the speed with mirroring whole mempool structure. But I wanted to show - that only few stucture members are usually modified during the lifetime of VG structure (thought yes - using the knowledge of the _lv_postoder() where I intentionally disable the locking - as then I'd get status of each LV modified - not very efficient with current exception store)

So basically - lock is done when the VG is parsed for the first time. (See Patch 3) - during the whole live of this shared VG (except for _lv_postoder()) - it's kept locked. Thus with mprotect-ing enabled - it's impossible to write to this pool without using dm_pool_set() as you get immediately segfault. It gets unlocked when it's being released to free vginfo cache - or when it's droped because RW usage is needed.

It would be nice to have 'COW' in user space - but have no idea how to implement this.

Zdenek

Joe Thornber

2011-03-23 14:53 UTC #4 lore raw

PATCH 1/6 Pool locking code

Quoted 4 lines · Zdenek Kabelac

It's been my first idea - to copy all chunks to separate buffer - but the VG mempool size for large VGs is actually quite big - thus doing always a snapshot isn't performance optimal. I'm doing my tests with ~7000LV - and it would be quite noticable to copy all the pool chunks with each use of it.

Can you define 'quite big' please?

- Joe

Zdenek Kabelac

author

2011-03-23 14:54 UTC #5 lore raw

PATCH 1/6 Pool locking code

In reply to Joe Thornber

Dne 23.3.2011 15:03, Joe Thornber napsal(a):

Quoted 37 lines · Zdenek Kabelac

Quoted 4 lines
Adding specific functions to lock and unlock pool, to modify specific uint64_t*, and to revert all modified uint64_t back.
libdm/libdevmapper.h +1 -0
+/* prevent modification of pool */
libdm/libdevmapper.h void *dm_pool_alloc_aligned(struct dm_pool *p, size_t s, unsigned alignment); · line 607
Quoted 10 lines
libdm/libdevmapper.h +10 -0
+int dm_pool_locked(struct dm_pool *p);
+int dm_pool_lock(struct dm_pool *p, unsigned count)
+	__attribute__((__warn_unused_result__));
+int dm_pool_unlock(struct dm_pool *p)
+	__attribute__((__warn_unused_result__));
+int dm_pool_set_uint64(struct dm_pool *p, uint64_t *addr, uint64_t new_value)
+	__attribute__((__warn_unused_result__));
+int dm_pool_restore(struct dm_pool *p, int check_crc)
+	__attribute__((__warn_unused_result__));
+
Hi Kabi,
I like what you're trying to do here. But I don't really like the above interface, particularly the dm_pool_set_uint64() method which is going to be ugly to use.
Ideally I guess I'd prefer something like:
        struct dm_pool_snapshot;
        
        struct dm_pool_snapshot *dm_pool_take_snapshot(struct dm_pool *p);
        void dm_pool_destroy_snapshot(struct dm_pool_snapshot *snap);
        int dm_pool_compare(struct dm_pool *p, struct dm_pool_snapshot *snap);
        int dm_pool_restore_snapshot(struct dm_pool *p, struct dm_pool_snapshot *snap, int release_subsequently_allocated_data);
You'd use the compare function within assertions to check nobody has changed a pool unexpectedly. There's no concept of a locked pool, you don't have to change the existing pool code to check if you're locked. Also you don't need a special interface for changing data within a locked pool.

Quoted 5 lines

The simple way of implementing the above is to just make a copy of the pool data within the snapshot. Which of course is a very slow op. So could you give me an idea of the size of a typical pool when you are locking it? How frequently do you lock?

Forget to answer the size question - >8MB is allocated by the VG mempool when set of simple linear 7200LVs on 1 PV are in use.

Also that's why I mention in my patchset that doing even quick crc hash checksum is quite slow - I except full memcpy would be even slower - and I do that only with finally pool unlock).

Zdenek

Joe Thornber

2011-03-23 15:13 UTC #6 lore raw

PATCH 1/6 Pool locking code

Quoted 3 lines · Zdenek Kabelac

Forget to answer the size question - >8MB is allocated by the VG mempool when
set of simple linear 7200LVs on 1 PV are in use.

So I think each snapshot/restore of a VG pool of that size is going to take ~0.1 seconds. So now my question is how many times are we expecting to happen during a typical tool invocation? Your previous reply suggested it happens just once?

- Joe

Zdenek Kabelac

author

2011-03-23 15:57 UTC #7 lore raw

PATCH 1/6 Pool locking code

Dne 23.3.2011 16:13, Joe Thornber napsal(a):

Quoted 9 lines · Zdenek Kabelac

Quoted 3 lines
Forget to answer the size question - >8MB is allocated by the VG mempool when
set of simple linear 7200LVs on 1 PV are in use.
So I think each snapshot/restore of a VG pool of that size is going to take ~0.1 seconds. So now my question is how many times are we expecting to happen during a typical tool invocation? Your previous reply suggested it happens just once?

If there would be just 1 vg read for 7200 activate call - obviously I'd not need to play with this dm_pool locking :)

So there is 7200 vg reads - which will use this one locked pool with my patch set:

To give some time info here
(using my unoptimized gcc debug build on C2D 2.2GHz - but with few more patches applied than current upstream)

Deactivate 7200LV (7200 already active)
real 1m16.140s
user 0m5.686s
sys 0m31.410s

Deactivate 7200LV (non active)
real 0m2.121s
user 0m1.083s
sys 0m1.032s

Activate 7200 (non active)
real 2m19.324s
user 0m6.503s
sys 1m30.785s

Activate 7200 (7200 already active)
real 0m10.945s
user 0m1.422s
sys 0m9.502s

Zdenek

Zdenek Kabelac

author

2011-03-23 16:33 UTC #8 lore raw

PATCH 1/6 Pool locking code

Dne 23.3.2011 16:57, Zdenek Kabelac napsal(a):

Quoted 38 lines

Dne 23.3.2011 16:13, Joe Thornber napsal(a):
Quoted 9 lines · Zdenek Kabelac
Quoted 3 lines
Forget to answer the size question - >8MB is allocated by the VG mempool when
set of simple linear 7200LVs on 1 PV are in use.
So I think each snapshot/restore of a VG pool of that size is going to take ~0.1 seconds. So now my question is how many times are we expecting to happen during a typical tool invocation? Your previous reply suggested it happens just once?
If there would be just 1 vg read for 7200 activate call - obviously I'd not need to play with this dm_pool locking :)
So there is 7200 vg reads - which will use this one locked pool with my patch set:
To give some time info here
(using my unoptimized gcc debug build on C2D 2.2GHz - but with few more patches applied than current upstream)
Deactivate 7200LV (7200 already active)
real 1m16.140s
user 0m5.686s
sys 0m31.410s
Deactivate 7200LV (non active)
real 0m2.121s
user 0m1.083s
sys 0m1.032s
Activate 7200 (non active)
real 2m19.324s
user 0m6.503s
sys 1m30.785s

Quoted 5 lines

Activate 7200 (7200 already active)
real 0m10.945s
user 0m1.422s
sys 0m9.502s

I think good example could be this - when I disable mprotect-ing in my patchset - and enable crc checking with each dm_pool_restore (which should be similar to memcpy) (otherwise crc is checked only with final pool unlock)

Deactivate 7200LV
real 1m45.353s
user 0m38.128s
sys 0m27.395s

Deactivate 7200LV
real 0m36.948s
user 0m35.284s
sys 0m1.642s

Activate 7200 (non active)
real 2m31.065s
user 0m37.740s
sys 1m18.481s

(here could be observed - thet mprotecting takes 12s)

Activate 7200 (7200 already active)
real 0m52.188s
user 0m35.570s
sys 0m16.563s

So I'd estimate like 33seconds would be wasted.

And to complete the picture - crc only with unlock - and without pool mprotect-ing - assuming this would be normal work.

Deactivate 7200LV
real 1m9.989s
user 0m4.584s
sys 0m26.882s

Deactivate 7200LV
real 0m2.119s
user 0m1.087s
sys 0m1.025s

Activate 7200 (non active)
real 2m11.363s
user 0m4.696s
sys 1m15.393s

Activate 7200 (7200 already active)
real 0m11.127s
user 0m1.392s
sys 0m9.669s

Zdenek

Zdenek Kabelac

author

2011-03-23 17:29 UTC #9 lore raw

PATCH 1/6 Pool locking code

Dne 23.3.2011 17:33, Zdenek Kabelac napsal(a):

Quoted 95 lines

Dne 23.3.2011 16:57, Zdenek Kabelac napsal(a):
Quoted 38 lines
Dne 23.3.2011 16:13, Joe Thornber napsal(a):
Quoted 9 lines · Zdenek Kabelac
Quoted 3 lines
Forget to answer the size question - >8MB is allocated by the VG mempool when
set of simple linear 7200LVs on 1 PV are in use.
So I think each snapshot/restore of a VG pool of that size is going to take ~0.1 seconds. So now my question is how many times are we expecting to happen during a typical tool invocation? Your previous reply suggested it happens just once?
If there would be just 1 vg read for 7200 activate call - obviously I'd not need to play with this dm_pool locking :)
So there is 7200 vg reads - which will use this one locked pool with my patch set:
To give some time info here
(using my unoptimized gcc debug build on C2D 2.2GHz - but with few more patches applied than current upstream)
Deactivate 7200LV (7200 already active)
real 1m16.140s
user 0m5.686s
sys 0m31.410s
Deactivate 7200LV (non active)
real 0m2.121s
user 0m1.083s
sys 0m1.032s
Activate 7200 (non active)
real 2m19.324s
user 0m6.503s
sys 1m30.785s
Quoted 5 lines
Activate 7200 (7200 already active)
real 0m10.945s
user 0m1.422s
sys 0m9.502s
I think good example could be this - when I disable mprotect-ing in my patchset - and enable crc checking with each dm_pool_restore (which should be similar to memcpy) (otherwise crc is checked only with final pool unlock)
Deactivate 7200LV
real 1m45.353s
user 0m38.128s
sys 0m27.395s
Deactivate 7200LV
real 0m36.948s
user 0m35.284s
sys 0m1.642s
Activate 7200 (non active)
real 2m31.065s
user 0m37.740s
sys 1m18.481s
(here could be observed - thet mprotecting takes 12s)
Activate 7200 (7200 already active)
real 0m52.188s
user 0m35.570s
sys 0m16.563s
So I'd estimate like 33seconds would be wasted.
--
And to complete the picture - crc only with unlock - and without pool mprotect-ing - assuming this would be normal work.
Deactivate 7200LV
real 1m9.989s
user 0m4.584s
sys 0m26.882s
Deactivate 7200LV
real 0m2.119s
user 0m1.087s
sys 0m1.025s
Activate 7200 (non active)
real 2m11.363s
user 0m4.696s
sys 1m15.393s

Quoted 5 lines

Activate 7200 (7200 already active)
real 0m11.127s
user 0m1.392s
sys 0m9.669s

And as the implementation of memcpy instead of crc was rather very simple - here are exact numbers with memcpy (memcpy being somewhat faster then crc):

Deactivate 7200LV (7200 already active)
real 1m32.725s
user 0m24.828s
sys 0m27.213s

Deactivate 7200LV (non active)
real 0m20.486s
user 0m18.239s
sys 0m2.236s

Activate 7200 (non active)
real 2m33.219s
user 0m24.777s
sys 1m19.996s

Activate 7200 (7200 already active)
real 0m36.627s
user 0m18.474s
sys 0m18.065s

Zdenek

Joe Thornber

2011-03-23 18:41 UTC #10 lore raw

PATCH 1/6 Pool locking code

In reply to Zdenek Kabelac

Quoted 6 lines · Zdenek Kabelac

If there would be just 1 vg read for 7200 activate call - obviously I'd not
need to play with this dm_pool locking :)
So there is 7200 vg reads - which will use this one locked pool with my patch set:

ok, so I'm playing catch-up; This locking functionality is debug only. In the near term the set/restore api wont be needed at all, at which point that interface can be removed. We should leave the locking side for regression tests.

So I'll ACK the patch.

- Joe

Zdenek Kabelac

author

2011-03-23 19:01 UTC #11 lore raw

PATCH 1/6 Pool locking code

Dne 23.3.2011 19:41, Joe Thornber napsal(a):

Quoted 5 lines · Zdenek Kabelac

Quoted 4 lines
If there would be just 1 vg read for 7200 activate call - obviously I'd not
need to play with this dm_pool locking :)

Quoted 7 lines

Quoted 2 lines
So there is 7200 vg reads - which will use this one locked pool with my patch set:
ok, so I'm playing catch-up; This locking functionality is debug only. In the near term the set/restore api wont be needed at all, at which point that interface can be removed. We should leave the locking side for regression tests.

Yes - the locking has rather 'debug' semantic to catch writes to vg mempool.

But I'm not sure what do you mean here by 'set/restore api won't be needed' ?

As currently I need to restore the content of the modified part of mempool to return always the same memory like it would be created 1st time. (Unless you mean here, that you would rather change the code which modifies lv/vg->status - so these flag would not need to be restored - which would be a nice option - but would certainly require bigger modification of the code.)

Zdenek

Joe Thornber

2011-03-24 12:21 UTC #12 lore raw

PATCH 1/6 Pool locking code

Quoted 6 lines · Zdenek Kabelac

Yes - the locking has rather 'debug' semantic to catch writes to vg mempool.
But I'm not sure what do you mean here by 'set/restore api won't be needed' ?

I had a quick chat about this with Alasdair yesterday. The plan is to change the code that updates the VGs so they don't touch the VG data in the pool, but instead hold those small changes off to one side.

So the steps you seem to be following are:

i) debug lock/unlock api for pool to identify mutating code. (Useful in perpetuity for regression tests.)
ii) set/restore api for pool (temporary).
iii) update the mutating code to use set.
iv) gradually get rid of the use of set/restore on a case by case basis.
v) remove the set/restore api

There is an argument that set/restore shouldn't ever go into HEAD, but is instead something solely useful to you while you do this work. But since this work is going to take some time I see no problem with this going in on the understanding that it comes out again as soon as possible.

- Joe

Zdenek Kabelac

author

2011-03-24 12:45 UTC #13 lore raw

PATCH 1/6 Pool locking code

Dne 24.3.2011 13:21, Joe Thornber napsal(a):

Quoted 21 lines · Zdenek Kabelac

Quoted 6 lines
Yes - the locking has rather 'debug' semantic to catch writes to vg mempool.
But I'm not sure what do you mean here by 'set/restore api won't be needed' ?
I had a quick chat about this with Alasdair yesterday. The plan is to change the code that updates the VGs so they don't touch the VG data in the pool, but instead hold those small changes off to one side.
So the steps you seem to be following are:
i) debug lock/unlock api for pool to identify mutating code. (Useful in perpetuity for regression tests.)
ii) set/restore api for pool (temporary).
iii) update the mutating code to use set.
iv) gradually get rid of the use of set/restore on a case by case basis.
v) remove the set/restore api

Quoted 5 lines

There is an argument that set/restore shouldn't ever go into HEAD, but is instead something solely useful to you while you do this work. But since this work is going to take some time I see no problem with this going in on the understanding that it comes out again as soon as possible.

Yeah - I've been already thinking about this -
I could probably make even API for dm_pool_set & restore purely internal - so only lvm would know about it - by copying internal structures.

So we would not need to expose this in public libdm API for a while.

And I've been also thinking about changing the places which are modifying 'read-only' VG structure members - but at this moment it's somewhat bigger patch.

My idea is - to use for this variable 'offset' inside volume_group structure, and to access it for read/write - one must deference it through array access over the exception store area (I'd stay with uint64_t data)

Depends on whether we accept this direction.

As for now it seems there is not so many accesses - so it looks like hopefully 'relatively' small amount of code needs to be changed.

If we would agree on the way we access them - it's probably easy to write before it hits upstream - to avoid hassles with internal structures.

Zdenek

Zdenek Kabelac

author

2011-03-24 13:50 UTC #14 lore raw

PATCH 1/6 Pool locking code

Dne 24.3.2011 13:45, Zdenek Kabelac napsal(a):

Quoted 43 lines

Dne 24.3.2011 13:21, Joe Thornber napsal(a):
Quoted 21 lines · Zdenek Kabelac
Quoted 6 lines
Yes - the locking has rather 'debug' semantic to catch writes to vg mempool.
But I'm not sure what do you mean here by 'set/restore api won't be needed' ?
I had a quick chat about this with Alasdair yesterday. The plan is to change the code that updates the VGs so they don't touch the VG data in the pool, but instead hold those small changes off to one side.
So the steps you seem to be following are:
i) debug lock/unlock api for pool to identify mutating code. (Useful in perpetuity for regression tests.)
ii) set/restore api for pool (temporary).
iii) update the mutating code to use set.
iv) gradually get rid of the use of set/restore on a case by case basis.
v) remove the set/restore api
Quoted 5 lines
There is an argument that set/restore shouldn't ever go into HEAD, but is instead something solely useful to you while you do this work. But since this work is going to take some time I see no problem with this going in on the understanding that it comes out again as soon as possible.
Yeah - I've been already thinking about this -
I could probably make even API for dm_pool_set & restore purely internal - so only lvm would know about it - by copying internal structures.
So we would not need to expose this in public libdm API for a while.
And I've been also thinking about changing the places which are modifying 'read-only' VG structure members - but at this moment it's somewhat bigger patch.
My idea is - to use for this variable 'offset' inside volume_group structure, and to access it for read/write - one must deference it through array access over the exception store area (I'd stay with uint64_t data)

Quoted 8 lines

Depends on whether we accept this direction.
As for now it seems there is not so many accesses - so it looks like hopefully 'relatively' small amount of code needs to be changed.
If we would agree on the way we access them - it's probably easy to write before it hits upstream - to avoid hassles with internal structures.

I should still mention here - that typical use-case of locked VG is to operate on very limited set of LVs (for just linears - it's == 1)
- thus very short list of exceptions is usually restored (if some at all).

So the idea of restoring all fields of say 3 or more 8byte values of 7000LVs would be surely more expensive (though it's obviously less then 8MB)

Also the code which would work on references instead of direct structure members would look less fancy than it's now (As now only 'write operation' requires 'wrapper').

So I think it's good to well specify what we really want to achieve - and what are we going to sacrifice for what.
(I know the current API isn't the best - but as I said - it's been already couple iteration on my own harddrive)

Zdenek

Zdenek Kabelac

author

2011-03-22 21:21 UTC #15 lore raw

PATCH 2/6 Code move vg_mark_partial up in stack

In reply to Zdenek Kabelac

It's useful to keep the partial flag cached - so just move the call for vg_mark_partil_lvs into lvmcache.c

This patch should not present any functional change.

Signed-off-by Zdenek Kabelac <[email protected]>

---
 lib/cache/lvmcache.c    |    7 +++++++
 lib/metadata/metadata.c |    5 -----
 2 files changed, 7 insertions(+), 5 deletions(-)

lib/cache/lvmcache.c +7 -0

index 037fc4a..d0de34f 100644
@@ -639,6 +639,7 @@ struct volume_group *lvmcache_get_vg(const char *vgid, unsigned precommitted)
 	struct volume_group *vg = NULL;
 	struct format_instance *fid;
 	struct format_instance_ctx fic;
+	int vg_missing;
 
 	if (!vgid || !(vginfo = vginfo_from_vgid(vgid)) || !vginfo->vgmetadata)
 		return NULL;

@@ -678,6 +679,12 @@ struct volume_group *lvmcache_get_vg(const char *vgid, unsigned precommitted)
 	if (!(vg = import_vg_from_config_tree(vginfo->cft, fid)))
 		goto_bad;
 
+	if ((vg_missing = vg_missing_pv_count(vg))) {
+		log_verbose("There are %d physical volumes missing.",
+			    vg_missing);
+		vg_mark_partial_lvs(vg);
+	}
+
 	log_debug("Using cached %smetadata for VG %s.",
 		  vginfo->precommitted ? "pre-committed" : "", vginfo->vgname);

lib/metadata/metadata.c +0 -5

index f4d4671..b6312a0 100644
@@ -2819,11 +2819,6 @@ static struct volume_group *_vg_read(struct cmd_context *cmd,
 		else	/* Inconsistent but we can't repair it */
 			correct_vg->status &= ~INCONSISTENT_VG;
 
-		if (vg_missing_pv_count(correct_vg)) {
-			log_verbose("There are %d physical volumes missing.",
-				    vg_missing_pv_count(correct_vg));
-			vg_mark_partial_lvs(correct_vg);
-		}
 		return correct_vg;
 	} else {
 		free_vg(correct_vg);

Signature · 1 line

1.7.4.1

Zdenek Kabelac

author

2011-03-22 21:21 UTC #16 lore raw

PATCH 3/6 Share VG multiple times

In reply to Zdenek Kabelac

Saves time of the creation of the same volume_group structure.

Structure vginfo is extended with linked list of vg_pool_list elements.

It tracks the number of reuses of every cached VG. As especially clvmd and few other situation requires more then 1 parsed VG - so using this vg_pool implementation.

Sharing is not used when VG is opened RW recover mode.
As there is far more case when sharing is possible - use rather specific lvmcache_drop_vg() call to drop parsed VG from cache - such VG is then unlocked moved away from the vg pool and it is fully modifiable.

Signed-off-by Zdenek Kabelac <[email protected]>

---
 lib/cache/lvmcache.c    |  104 +++++++++++++++++++++++++++++++++++++++++++++-
 lib/cache/lvmcache.h    |    3 +
 lib/metadata/metadata.c |   20 ++++++++-
 lib/metadata/vg.h       |    2 +
 4 files changed, 124 insertions(+), 5 deletions(-)

lib/cache/lvmcache.c +101 -3

index d0de34f..9ee009d 100644
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2001-2004 Sistina Software, Inc. All rights reserved.
- * Copyright (C) 2004-2008 Red Hat, Inc. All rights reserved.
+ * Copyright (C) 2004-2011 Red Hat, Inc. All rights reserved.
  *
  * This file is part of LVM2.
  *

@@ -38,6 +38,13 @@ static int _has_scanned = 0;
 static int _vgs_locked = 0;
 static int _vg_global_lock_held = 0;	/* Global lock held when cache wiped? */
 
+struct vg_pool_list {
+	struct dm_list list;    /* vg_pool list */
+	struct volume_group *vg;
+	int in_use;
+	unsigned use_count;	/* Reuse counter for statistics */
+};
+
 int lvmcache_init(void)
 {
 	/*

@@ -76,6 +83,10 @@ int lvmcache_init(void)
 /* Volume Group metadata cache functions */
 static void _free_cached_vgmetadata(struct lvmcache_vginfo *vginfo)
 {
+	struct vg_pool_list *vgpl, *tvgpl;
+	struct volume_group *vg;
+	int in_use;
+
 	if (!vginfo || !vginfo->vgmetadata)
 		return;

@@ -89,6 +100,17 @@ static void _free_cached_vgmetadata(struct lvmcache_vginfo *vginfo)
 		vginfo->cft = NULL;
 	}
 
+	dm_list_iterate_items_safe(vgpl, tvgpl, &vginfo->vg_pool) {
+		vg = vgpl->vg;
+		in_use = vgpl->in_use;
+		if (!lvmcache_drop_vg(vg))
+			stack;
+		if (in_use)
+			log_debug("Unpooling referenced VG:%p %s", vg, vg->name);
+		else
+			free_vg(vg);
+	}
+
 	log_debug("Metadata cache: VG %s wiped.", vginfo->vgname);
 }

@@ -637,6 +659,7 @@ struct volume_group *lvmcache_get_vg(const char *vgid, unsigned precommitted)
 {
 	struct lvmcache_vginfo *vginfo;
 	struct volume_group *vg = NULL;
+	struct vg_pool_list *vgpl = NULL;
 	struct format_instance *fid;
 	struct format_instance_ctx fic;
 	int vg_missing;

@@ -663,12 +686,32 @@ struct volume_group *lvmcache_get_vg(const char *vgid, unsigned precommitted)
 	    (!precommitted && vginfo->precommitted && !critical_section()))
 		return NULL;
 
+	/* Try to find some usable VG in pool - short list is expected */
+	dm_list_iterate_items(vgpl, &vginfo->vg_pool)  {
+		if (!vgpl->in_use) {
+			if (!dm_pool_restore(vgpl->vg->vgmem, 0))
+				/* Should not happen, but ignore such VG */
+				stack;
+			else {
+				vg = vgpl->vg;
+				vgpl->in_use = 1;
+				vgpl->use_count++;
+				goto out;
+			}
+		}
+	}
+
 	fic.type = FMT_INSTANCE_VG | FMT_INSTANCE_MDAS | FMT_INSTANCE_AUX_MDAS;
 	fic.context.vg_ref.vg_name = vginfo->vgname;
 	fic.context.vg_ref.vg_id = vgid;
 	if (!(fid = vginfo->fmt->ops->create_instance(vginfo->fmt, &fic)))
 		return_NULL;
 
+	if (!(vgpl = dm_zalloc(sizeof(*vgpl)))) {
+		log_error("Failed to allocate memory for pool list.");
+		goto bad;
+	}
+
 	/* Build config tree from vgmetadata, if not yet cached */
 	if (!vginfo->cft &&
 	    !(vginfo->cft =

@@ -685,17 +728,71 @@ struct volume_group *lvmcache_get_vg(const char *vgid, unsigned precommitted)
 		vg_mark_partial_lvs(vg);
 	}
 
-	log_debug("Using cached %smetadata for VG %s.",
-		  vginfo->precommitted ? "pre-committed" : "", vginfo->vgname);
+	/* Add to VGs pool */
+	vg->vg_pool = vgpl;
+	vgpl->vg = vg;
+	vgpl->in_use = 1; /* Also for goto_bad fail path */
+	dm_list_add(&vginfo->vg_pool, &vgpl->list);
+	log_debug("Add VG:%p %s to the vginfo %p vg_pool (%d).",
+		  vg, vg->name, vginfo, dm_list_size(&vginfo->vg_pool));
+	if (!dm_pool_lock(vg->vgmem, 8 + dm_list_size(&vg->lvs)))
+		goto_bad;
+out:
+	log_debug("Using cached (%u) %smetadata for VG %s.", vgpl->use_count,
+		  vginfo->precommitted ? "pre-committed " : "", vginfo->vgname);
 
 	return vg;
 
 bad:
 	free_vg(vg);
 	_free_cached_vgmetadata(vginfo);
+	dm_free(vgpl);
 	return NULL;
 }
 
+/**
+ * Put back VG as reusable in vg_pool list
+ */
+int lvmcache_put_vg(struct volume_group *vg)
+{
+	if (!vg->vg_pool || !vg->vg_pool->in_use)
+		return 0;
+
+	log_debug("Putting back to cache list VG:%p %s (reused:%d).",
+		  vg, vg->name, vg->vg_pool->use_count);
+	vg->vg_pool->in_use = 0;
+	return 1;
+}
+
+/**
+ * Drop VG from the vginfo vg_pool list so it can be used
+ * for modification.
+ */
+int lvmcache_drop_vg(struct volume_group *vg)
+{
+	if (!vg->vg_pool)
+		return 0;
+
+	log_debug("Dropping VG:%p %s from cache (reused:%d).",
+		  vg, vg->name, vg->vg_pool->use_count);
+
+	/* Debug perform crc check */
+	if (!vg->vg_pool->in_use &&
+	    vg->vg_pool->use_count &&
+	    !dm_pool_restore(vg->vgmem, 1))
+		/* Should not happen, but ignore such VG */
+		stack;
+
+	if (!dm_pool_unlock(vg->vgmem))
+		stack;
+
+	dm_list_del(&vg->vg_pool->list);
+	dm_free(vg->vg_pool);
+	vg->vg_pool = NULL;
+
+	return 1;
+}
+
 struct dm_list *lvmcache_get_vgids(struct cmd_context *cmd,
 				   int include_internal)
 {

@@ -1117,6 +1214,7 @@ static int _lvmcache_update_vgname(struct lvmcache_info *info,
 			return 0;
 		}
 		dm_list_init(&vginfo->infos);
+		dm_list_init(&vginfo->vg_pool);
 
 		/*
 		 * If we're scanning and there's an invalidated entry, remove it.

lib/cache/lvmcache.h +3 -0

index f13cad2..6360f30 100644
@@ -50,6 +50,7 @@ struct lvmcache_vginfo {
 	char *vgmetadata;	/* Copy of VG metadata as format_text string */
 	struct config_tree *cft; /* Config tree created from vgmetadata */
 				/* Lifetime is directly tied to vgmetadata */
+	struct dm_list vg_pool; /* List of cached parsed struct volume_group */
 	unsigned precommitted;	/* Is vgmetadata live or precommitted? */
 };

@@ -122,6 +123,8 @@ struct dm_list *lvmcache_get_pvids(struct cmd_context *cmd, const char *vgname,
 
 /* Returns cached volume group metadata. */
 struct volume_group *lvmcache_get_vg(const char *vgid, unsigned precommitted);
+int lvmcache_put_vg(struct volume_group *vg);
+int lvmcache_drop_vg(struct volume_group *vg);
 void lvmcache_drop_metadata(const char *vgname, int drop_precommitted);
 void lvmcache_commit_metadata(const char *vgname);

lib/metadata/metadata.c +18 -2

index b6312a0..e466821 100644
@@ -855,6 +855,12 @@ static struct volume_group *_vg_make_handle(struct cmd_context *cmd,
 					    struct volume_group *vg,
 					    uint32_t failure)
 {
+	if (vg && vg->vg_pool && (failure != SUCCESS)) {
+		/* Avoid overwrite of shared VG structure */
+		free_vg(vg);
+		vg = NULL;
+	}
+
 	if (!vg && !(vg = alloc_vg("vg_make_handle", cmd, NULL)))
 		return_NULL;

@@ -2816,8 +2822,11 @@ static struct volume_group *_vg_read(struct cmd_context *cmd,
 	    (use_precommitted || !*consistent || !(correct_vg->status & INCONSISTENT_VG))) {
 		if (!(correct_vg->status & INCONSISTENT_VG))
 			*consistent = 1;
-		else	/* Inconsistent but we can't repair it */
+		else { 
+			lvmcache_drop_vg(correct_vg);
+			/* Inconsistent but we can't repair it */
 			correct_vg->status &= ~INCONSISTENT_VG;
+		}
 
 		return correct_vg;
 	} else {

@@ -3243,6 +3252,11 @@ void free_vg(struct volume_group *vg)
 	if (!vg)
 		return;
 
+	log_debug("Releasing VG:%p %s.", vg, vg->name);
+
+	if (lvmcache_put_vg(vg))
+		return;
+
 	dm_list_iterate_items(pvl, &vg->pvs)
 		pvl->pv->fid->fmt->ops->destroy_instance(pvl->pv->fid);

@@ -3789,7 +3803,9 @@ static struct volume_group *_recover_vg(struct cmd_context *cmd,
 		return_NULL;
 	}
 
-	return (struct volume_group *)vg;
+	lvmcache_drop_vg(vg);
+
+	return vg;
 }
 
 /*

lib/metadata/vg.h +2 -0

index 2cc6047..e153c76 100644
@@ -20,6 +20,7 @@ struct dm_pool;
 struct format_instance;
 struct dm_list;
 struct id;
+struct vg_pool_list;
 
 typedef enum {
 	ALLOC_INVALID,

@@ -35,6 +36,7 @@ struct volume_group {
 	struct cmd_context *cmd;
 	struct dm_pool *vgmem;
 	struct format_instance *fid;
+	struct vg_pool_list *vg_pool; /* Backward reference to lvmcache */
 	struct dm_list *cmd_vgs;/* List of wanted/locked and opened VGs */
 	uint32_t cmd_missing_vgs;/* Flag marks missing VG */
 	uint32_t seqno;		/* Metadata sequence number */

Signature · 1 line

1.7.4.1

Zdenek Kabelac

author

2011-03-22 21:21 UTC #17 lore raw

PATCH 4/6 Use dm_pool_set_uint64

In reply to Zdenek Kabelac

Use the new dm_pool API call for the update of content of the locked shared VG mempool. Save the original value of the VG into an extra allocated volatile area. Currently only simple preallocated memory area is used - with expectation there should not be too many updates here.

Otherwisee more effective algorithm would be needed to get better perfomance. (most probably some resizable heap??)

Original code is kept in comments to make it more obvious what it is supposed to do.

Note: (snapshot is kept 64bit aligned in union)

Signed-off-by Zdenek Kabelac <[email protected]>

---
 lib/activate/activate.c       |    5 ++++-
 lib/metadata/lv.h             |    5 ++++-
 lib/metadata/snapshot_manip.c |   15 ++++++++++++---
 3 files changed, 20 insertions(+), 5 deletions(-)

lib/activate/activate.c +4 -1

index 1ec518d..4d257f1 100644
@@ -1411,7 +1411,10 @@ static int _lv_activate(struct cmd_context *cmd, const char *lvid_s,
 	lv_calculate_readahead(lv, NULL);
 
 	if (exclusive)
-		lv->status |= ACTIVATE_EXCL;
+		/* lv->status |= ACTIVATE_EXCL */
+		if (!dm_pool_set_uint64(lv->vg->vgmem, &lv->status,
+					lv->status | ACTIVATE_EXCL))
+			goto_out;
 
 	critical_section_inc(cmd);
 	if (!(r = _lv_activate_lv(lv, 0)))

lib/metadata/lv.h +4 -1

index 1c3640b..66db813 100644
@@ -38,7 +38,10 @@ struct logical_volume {
 
 	uint32_t origin_count;
 	struct dm_list snapshot_segs;
-	struct lv_segment *snapshot;
+	union {
+		struct lv_segment *snapshot;
+		uint64_t snapshot_64;	/* Keep the size 64bit on 32bit arch */
+	};
 
 	struct replicator_device *rdevice;/* For replicator-devs, rimages, slogs - reference to rdevice */
 	struct dm_list rsites;	/* For replicators - all sites */

lib/metadata/snapshot_manip.c +12 -3

index cb5df6b..0dc8620 100644
@@ -130,9 +130,18 @@ void init_snapshot_merge(struct lv_segment *cow_seg,
 void clear_snapshot_merge(struct logical_volume *origin)
 {
 	/* clear merge attributes */
-	origin->snapshot->status &= ~MERGING;
-	origin->snapshot = NULL;
-	origin->status &= ~MERGING;
+
+	/* origin->snapshot->status &= ~MERGING */
+	/* origin->snapshot = NULL */
+	/* origin->status &= ~MERGING */
+	if (!dm_pool_set_uint64(origin->vg->vgmem,
+				&origin->snapshot->status,
+				origin->snapshot->status & ~MERGING) ||
+	    !dm_pool_set_uint64(origin->vg->vgmem,
+				&origin->snapshot_64, 0) ||
+	    !dm_pool_set_uint64(origin->vg->vgmem, &origin->status,
+				origin->status & ~MERGING))
+		stack; /* FIXME: change to return error value */
 }
 
 int vg_add_snapshot(struct logical_volume *origin,

Signature · 1 line

1.7.4.1

Zdenek Kabelac

author

2011-03-22 21:21 UTC #18 lore raw

PATCH 5/6 lv_postorder using dm_pool_set_uint64

In reply to Zdenek Kabelac

We may use either this code to safe&restore lv_postorder and keep the structure locked.

Next patch is optimization for this one - it unlocks VG during postorder processing (thus it equals to the original setting).

When memory pool is unlocked - this patch presents just very small overhead and potentially helps to better debugging if there ever would done something more complex then current lv_postoder code which could be easily checked there are no other side-effects.

More comments in following patch...

Signed-off-by Zdenek Kabelac <[email protected]>

---
 lib/metadata/metadata.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

lib/metadata/metadata.c +13 -4

index e466821..cec9c3c 100644
@@ -2023,7 +2023,10 @@ static int _lv_postorder_cleanup(struct logical_volume *lv, void *data)
 {
 	if (!(lv->status & POSTORDER_FLAG))
 		return 1;
-	lv->status &= ~POSTORDER_FLAG;
+	/* lv->status &= ~POSTORDER_FLAG */
+	if (!dm_pool_set_uint64(lv->vg->vgmem, &lv->status,
+				lv->status & ~POSTORDER_FLAG))
+                return_0;
 
 	if (!_lv_each_dependency(lv, _lv_postorder_cleanup, data))
 		return_0;

@@ -2047,7 +2050,10 @@ static int _lv_postorder_visit(struct logical_volume *lv,
 		return 1;
 	if (lv->status & POSTORDER_OPEN_FLAG)
 		return 1; // a data structure loop has closed...
-	lv->status |= POSTORDER_OPEN_FLAG;
+	/* lv->status |= POSTORDER_OPEN_FLAG */
+	if (!dm_pool_set_uint64(lv->vg->vgmem, &lv->status,
+				lv->status | POSTORDER_OPEN_FLAG))
+                return_0;
 
 	baton.fn = fn;
 	baton.data = data;

@@ -2056,8 +2062,11 @@ static int _lv_postorder_visit(struct logical_volume *lv,
 	if (r)
 		r = fn(lv, data);
 
-	lv->status &= ~POSTORDER_OPEN_FLAG;
-	lv->status |= POSTORDER_FLAG;
+	/* lv->status &= ~POSTORDER_OPEN_FLAG */
+	/* lv->status |= POSTORDER_FLAG */
+	if (!dm_pool_set_uint64(lv->vg->vgmem, &lv->status,
+				(lv->status & ~POSTORDER_OPEN_FLAG) | POSTORDER_FLAG))
+		return_0;
 
 	return r;
 }

Signature · 1 line

1.7.4.1

Zdenek Kabelac

author

2011-03-22 21:22 UTC #19 lore raw

PATCH 6/6 lv_postorder unlock and lock

In reply to Zdenek Kabelac

Instead of unlocking and locking for every changed struct member do it once when entering and leaving function.

Currently lv_postoder() does not modify other part of vg structure then status flags of each LV with flags that are also removed. However using standard dm_pool_set_uint64 would require to implement more efective data structure for exception stor - as it is not needed for now - avoid complexity and use this very simple workaround.

Signed-off-by Zdenek Kabelac <[email protected]>

---
 lib/metadata/metadata.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

lib/metadata/metadata.c +16 -0

index cec9c3c..85bcb2d 100644
@@ -2083,8 +2083,17 @@ static int _lv_postorder(struct logical_volume *lv,
 			       void *data)
 {
 	int r;
+	int lck = dm_pool_locked(lv->vg->vgmem);
+
+	if (lck && !dm_pool_unlock(lv->vg->vgmem))
+		return_0;
+
 	r = _lv_postorder_visit(lv, fn, data);
 	_lv_postorder_cleanup(lv, 0);
+
+	if (lck && !dm_pool_lock(lv->vg->vgmem, 0))
+		return_0;
+
 	return r;
 }

@@ -2098,6 +2107,10 @@ static int _lv_postorder_vg(struct volume_group *vg,
 {
 	struct lv_list *lvl;
 	int r = 1;
+	int lck = dm_pool_locked(vg->vgmem);
+
+	if (lck && !dm_pool_unlock(vg->vgmem))
+		return_0;
 
 	dm_list_iterate_items(lvl, &vg->lvs)
 		if (!_lv_postorder_visit(lvl->lv, fn, data)) {

@@ -2108,6 +2121,9 @@ static int _lv_postorder_vg(struct volume_group *vg,
 	dm_list_iterate_items(lvl, &vg->lvs)
 		_lv_postorder_cleanup(lvl->lv, 0);
 
+	if (lck && !dm_pool_lock(vg->vgmem, 0))
+		return_0;
+
 	return r;
 }

Signature · 1 line

1.7.4.1