(11/11) re-instantiate automatic VG recovery
PATCH 1/1 (11/11) re-instantiate automatic VG recovery
Finally!
This one restores the functionality that has been severed from the toollib, that is the automatic metadata recovery in case of running after an interrupted commit. This could do with a better naming and all, but that will be dealt with in later patches (in another series, I suppose).
All in all, this is a fairly big bundle, and hopefully it should hit CVS soon after the dust from the device-mapper merge has settled. It is definitely work in progress and there will be regressions. However, since it's now easier than ever to write tests for the testsuite, using the new tools, I would welcome this as a good opportunity to augment our suite with all the regressions we can find that the bundle has created (and even more). This is essential to improve confidence in new code and makes it a lot less hassle to change things. I have been running the branch through our resident buildbot
(http://nevrast.englab.brq.redhat.com:8010/waterfall) for a while now -- and it's pretty much green.
I would also like to thank everyone who's worked on the testsuite, it's already been of great help in development -- and I hope it will become even much more useful, as it covers more and more use-cases, bugs and, hopefully, all of our favourite regressions.
Now, I'm done for. I have been staring at diffs just a little too long by now, but I'll try to write up a summary on what still needs to be done, and some ideas on how to approach that, in not-so-distant (I hope) future.
Yours,
Petr.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvm2-lvmlib-recovery.diff
Type: text/x-diff
Size: 1924 bytes
Desc: lvmlib-recovery.diff
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20081030/d3811143/attachment.bin>
-------------- next part --------------
Signature · 6 lines
Peter Rockai | me()mornfall!net | prockai()redhat!com
http://blog.mornfall.net | http://web.mornfall.net
"In My Egotistical Opinion, most people's C programs should be
indented six feet downward and covered with dirt."
-- Blair P. Houghton on the subject of C program indentationPATCH 1/1 (11/11) re-instantiate automatic VG recovery
Quoted 12 lines · Petr Rockai
+static
vg_t*_recover_vg(structcmd_context*cmd, const char *lock, const char*vg_name,(quoted diff) +10
-0+ const char *vgid, uint32_t lock_flags ) +{ + int consistent = 1; + struct volume_group *vg; + + lock_flags &= ~LCK_TYPE_MASK; + lock_flags |= LCK_WRITE; + + unlock_vg(cmd, lock); + if (!lock_vol(cmd, lock, lock_flags))
Review
Quoted 10 lines
(quoted diff) +9
-0+ return_NULL; + + if (!(vg = vg_read_internal(cmd, vg_name, vgid, &consistent))) + return_NULL; + if (!consistent) + return_NULL; + return vg; +} + /*
A couple things:
1) Is there a reason you dropped the "if (lockingfailed()) return NULL" from your version? The original recover_vg() in toollib had this:
struct volume_group *recover_vg(struct cmd_context *cmd, const char *vgname,
uint32_t lock_type)
{
int consistent = 1;
/* Don't attempt automatic recovery without proper locking */
if (lockingfailed())
return NULL;
lock_type &= ~LCK_TYPE_MASK;
lock_type |= LCK_WRITE;2) Note that in some of the original paths (e.g. vgconvert) that called recover_vg(), there was a call to "dev_close_all()", while others did not (e.g. process_each_lv()).
3) Previous to this, in the case of inconsistent metadata, we sometimes would do recovery while others we would not. Now we're doing recovery in all cases? Is this what we want, especially with the reporting tools?
PATCH 1/1 (11/11) re-instantiate automatic VG recovery
Quoted 13 lines · Dave Wysochanski
1) Is there a reason you dropped the "if (
lockingfailed()) return NULL" from your version? The originalrecover_vg()in toollib had this:
structvolume_group*recover_vg(structcmd_context*cmd, const char *vgname,uint32_t lock_type) { int consistent = 1; /* Don't attempt automatic recovery without proper locking */ if (lockingfailed()) return NULL; lock_type &= ~LCK_TYPE_MASK; lock_type |= LCK_WRITE;
This is not a problem, since in case of locking failure, _recover_vg is never called (it is not exported from the file, and is only called in one place). Moreover, lockingfailed() is a piece of global knowledge I would love to get rid of.
Quoted 3 lines
2) Note that in some of the original paths (e.g. vgconvert) that called
recover_vg(), there was a call to "dev_close_all()", while others did not (e.g.process_each_lv()).
I believe that adding dev_close_all() could work, but it looks like a workaround of some sort, since from looking at the definition of dev_close_all, it should just scavenge open fd's that are no longer in use.
Now from reading _dev_close, it seems that we don't close devices whose refcount drop to 0, if they are part of a locked VG. This is probably where dev_close_all kicks in, and I'll note down to add it to the _recover_vg path, every time.
Quoted 4 lines
3) Previous to this, in the case of inconsistent metadata, we sometimes would do recovery while others we would not. Now we're doing recovery in all cases? Is this what we want, especially with the reporting tools?
Yes, I believe we want to do recovery every time we have write access, since the intention of this (and the "recover" bits should be renamed to reflect that) to finish incomplete transactions.
Yours,
Petr.
Signature · 6 lines
Peter Rockai | me()mornfall!net | prockai()redhat!com
http://blog.mornfall.net | http://web.mornfall.net
"In My Egotistical Opinion, most people's C programs should be
indented six feet downward and covered with dirt."
-- Blair P. Houghton on the subject of C program indentation