[PATCH] A different implementation of --ignorelockingfailure.

[PATCH] A different implementation of --ignorelockingfailure.

[Petr Rockai]

Hi,

two ends are addressed by this patch: getting rid of some global state and also
restricting --ignorelockingfailure to only allow what it is supposed to allow
(instead of a few hacked-in checks at some places and our belief that the users
will be well-behaved).

Yours,
   Petr.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: ignorelockingfailure.diff
Type: text/x-diff
Size: 6281 bytes
Desc: ignorelockingfailure.diff
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090122/709daf42/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] A different implementation of --ignorelockingfailure.

[Petr Rockai]

Petr Rockai <prockai@redhat.com> writes:
> diff -rN -u -p old-lvmlib/man/lvm.conf.5 new-lvmlib/man/lvm.conf.5
> --- old-lvmlib/man/lvm.conf.5	2009-01-22 11:14:00.054777458 +0100
> +++ new-lvmlib/man/lvm.conf.5	2009-01-22 11:14:00.218780704 +0100
> @@ -1,4 +1,4 @@
> -.TH LVM.CONF 5 "LVM TOOLS" "Sistina Software UK" \" -*- nroff -*-
> +.TH LVM.CONF 5 "LVM TOOLS 2.02.44-cvs (2008-11-19)" "Sistina Software UK" \" -*- nroff -*-
>  .SH NAME
>  lvm.conf \- Configuration file for LVM2
>  .SH SYNOPSIS
Please ignore this hunk. It crept in by mistake, the file is autogenerated.

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

[PATCH] A different implementation of --ignorelockingfailure.

[Dave Wysochanski]

Nice attempt at cleanup.

Not sure about this patch but I do have comments.
1. Why call it 'boottime_locking' - why not readonly_locking which is
clearer?
2. If you make this change you'll also need to modify lvchange.c to take
a READ lock if the request is for an availability change (vgchange has a
conditional).*  Otherwise, lvchange will fail when before it would
succeed.
3. Introducing boottime/readonly as a "type" of locking seems out of
place with the current set of locking types - file operations, cluster
operations.

* Then again, lvchange and vgchange seem to have different code for
changing availability of an LV (?) so there may be a subtle reason
lvchange always takes a WRITE lock.


On Thu, 2009-01-22 at 11:53 +0100, Petr Rockai wrote:
> Hi,
> 
> two ends are addressed by this patch: getting rid of some global state and also
> restricting --ignorelockingfailure to only allow what it is supposed to allow
> (instead of a few hacked-in checks at some places and our belief that the users
> will be well-behaved).
> 
> Yours,
>    Petr.
> 
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel

[PATCH] A different implementation of --ignorelockingfailure.

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:
> 1. Why call it 'boottime_locking' - why not readonly_locking which is
> clearer?
boottime is to show clearly that it should never be used elsewhere than
boottime. It's *not* readonly locking. It's locking that will grant read access
*without taking any locks*. Ie. inherently dangerous. Under normal operation,
this is a locking violation that may lead to corrupt metadata reads.

> 2. If you make this change you'll also need to modify lvchange.c to take
> a READ lock if the request is for an availability change (vgchange has a
> conditional).*  Otherwise, lvchange will fail when before it would
> succeed.
Oh. Ok, I will need to investigate that. From a glance, it seems like a simple
omission where lvchange doesn't check that it doesn't need a write lock in some
cases. I'll amend my patch with a fix for that and resend.

Will also see if I can come up with some test for the suite...

> 3. Introducing boottime/readonly as a "type" of locking seems out of
> place with the current set of locking types - file operations, cluster
> operations.
There's also "nolocking" as a locking type, so a boottime one that is
restriction of nolocking seems in line to me.

> * Then again, lvchange and vgchange seem to have different code for
> changing availability of an LV (?) so there may be a subtle reason
> lvchange always takes a WRITE lock.
It doesn't seem so (see above). It however might be related to the pvmove_poll
and lvconvert_poll bits that are invoked by lvchange -a y (but not vgchange -a
y). This discrepancy alone looks a little odd at best. However, the polldaemons
do their own locking. This is not really a reason for lvchange requiring a
write lock. What does happen is though that the polldaemons inherit the locking
type, so if anyone runs lvchange -a y --ignorelockingfailure, they might be in
for serious trouble if pvmove (or lvconvert maybe) was in progress, since they
will get unlocked metadata writes on a live system that way.

However, at least on Debian, the initscripts don't use lvchange at all, they
just use vgchange -a y --ignorelockingfailure and I imagine this is the case
with most distributions? In which case, we don't really need to care about
lvchange all that much. (Also note that this patch fixes the above corruption
bug induced by lvchange -a y --ignorelockingfailure, since it will disallow the
polldaemon to write metadata, and it should fail in some reasonable controlled
fashion. Well, it will disallow lvchange -a y altogether, but that can be fixed
by not requiring write lock for the activation itself...)

Thanks for review,
   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

[PATCH] A different implementation of --ignorelockingfailure.

[Milan Broz]

Petr Rockai wrote:
> two ends are addressed by this patch: getting rid of some global state and also
> restricting --ignorelockingfailure to only allow what it is supposed to allow
> (instead of a few hacked-in checks at some places and our belief that the users
> will be well-behaved).

Acked-by: Milan Broz <mbroz@redhat.com>

--ignorelockingfailure should be used only to activate base volumes during boot,
(e.g. when /var/lock/lvm is read-only, locking is clustered but we just starting
clvmd or /var is on activated volumes itself etc)


Tested-by: Milan Broz <mbroz@redhat.com>

Some warnings (both are trivial)

locking/no_locking.c: In function '_boottime_lock_resource':
locking/no_locking.c:73: warning: suggest parentheses around comparison in operand of &

tools/toollib.c:1134: undefined reference to `lockingfailed'


If anyone want write test script for testsuite

- a) commands with locking_type = 1, /etc/lvm/lock mounted read only
- b) locking type 3 without clvmd running

 without --ignorelockingfailure it must fail,
 with that parm only vgchange, lvchange -a y/n should work

- transition from clustered->local->ignorefailure still works
(test combination of fallback_to_clustered_locking, fallback_to_local_locking)

Milan
--
mbroz at redhat.com