[PATCH] (11/11) re-instantiate automatic VG recovery

[PATCH] (11/11) re-instantiate automatic VG recovery

[Petr Rockai]

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 --------------

-- 
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

[PATCH] (11/11) re-instantiate automatic VG recovery

[Dave Wysochanski]

On Thu, 2008-10-30 at 19:36 +0100, Petr Rockai wrote:
> +static vg_t *_recover_vg(struct cmd_context *cmd, const char *lock,
> const char *vg_name,
> +                        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))
> +               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] (11/11) re-instantiate automatic VG recovery

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:
> 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;

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.

> 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.

> 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.

-- 
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