[RFC][PATCH] never scan a device which is using the error target

[RFC][PATCH] never scan a device which is using the error target

[Mike Snitzer]

A merged snapshot's DM device is made to use the "error" target as part
of lvm's transaction to merge a snapshot.  This snapshot merge use-case
aside, any device using the error target shouldn't be scanned.

NOTE: I'm not using an ignore_suspended_devices() check like other
target checks in device_is_usable() -- its not clear to me what such a
check achieves -- but it clearly doesn't work for my needs seeing as the
default for lvm.conf's ignore_suspended_devices is 0.  Not sure what
commit dd5d9aa6 is up to.. but its devoid of relevant comments.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 lib/activate/dev_manager.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/lib/activate/dev_manager.c b/lib/activate/dev_manager.c
index 7981f22..aab0c9a 100644
--- a/lib/activate/dev_manager.c
+++ b/lib/activate/dev_manager.c
@@ -190,6 +190,12 @@ int device_is_usable(struct device *dev)
 				  dev_name(dev), name);
 			goto out;
 		}
+
+		if (target_type && !strcmp(target_type, "error")) {
+			log_debug("%s: Error device %s not usable.",
+				  dev_name(dev), name);
+			goto out;
+		}
 	} while (next);
 
 	/* FIXME Also check dependencies? */

[RFC][PATCH] never scan a device which is using the error target

[Milan Broz]

On 10/24/2010 02:55 AM, Mike Snitzer wrote:
> A merged snapshot's DM device is made to use the "error" target as part
> of lvm's transaction to merge a snapshot.  This snapshot merge use-case
> aside, any device using the error target shouldn't be scanned.
> 
> NOTE: I'm not using an ignore_suspended_devices() check like other
> target checks in device_is_usable() -- its not clear to me what such a

IIRC scan for mirrors, snapshots etc is OK.
But during mirror repair or conversion some devices in the middle of stack
can be suspended, so the ignore_suspended_devices() says when it is safe to
scan it. (see that it is set during conversions etc)
IMHO it is quite hack (mainly for clusters where one node runs the conversion
or repair and other just follows the state).

For error device it makes no sense - it is unusable all the time:-)

> +		if (target_type && !strcmp(target_type, "error")) {

Do we have some #define for the error target name?

Anyway, ack.

Milan

[RFC][PATCH] never scan a device which is using the error target

[Mike Snitzer]

On Sun, Oct 24 2010 at 12:43pm -0400,
Milan Broz <mbroz@redhat.com> wrote:

> On 10/24/2010 02:55 AM, Mike Snitzer wrote:
> > A merged snapshot's DM device is made to use the "error" target as part
> > of lvm's transaction to merge a snapshot.  This snapshot merge use-case
> > aside, any device using the error target shouldn't be scanned.
> > 
> > NOTE: I'm not using an ignore_suspended_devices() check like other
> > target checks in device_is_usable() -- its not clear to me what such a
> 
> IIRC scan for mirrors, snapshots etc is OK.
> But during mirror repair or conversion some devices in the middle of stack
> can be suspended, so the ignore_suspended_devices() says when it is safe to
> scan it. (see that it is set during conversions etc)
> IMHO it is quite hack (mainly for clusters where one node runs the conversion
> or repair and other just follows the state).

OK, thanks for the info.

> For error device it makes no sense - it is unusable all the time:-)

Right.

> > +		if (target_type && !strcmp(target_type, "error")) {
> 
> Do we have some #define for the error target name?

I don't think we have a #define for any target names.  Target names are
open coded strings throughout the lvm2 codebase.  Certainly a candidate
for cleanup.

> Anyway, ack.

Thanks,
Mike

[RFC][PATCH] never scan a device which is using the error target

[Alasdair G Kergon]

On Sat, Oct 23, 2010 at 08:55:15PM -0400, Mike Snitzer wrote:
> any device using the error target shouldn't be scanned.
 
I disagree with that:)
Scanning an error target does not cause the system to lock up so, in general,
there's no reason not to scan it.
(Remember that the error target might only be used in 1 out of hundreds
of table lines and that loop - as you'd expect for the current uses - skips the
device if *any* target matches.)

The defining characteristic here is surely that the device is no longer
meant to be available for mounting - no longer visible, in other words.
So you may (for now) need a more specific test that's limited
to snapshot merge.

Alasdair

[RFC][PATCH] never scan a device which is using the error target

[Mike Snitzer]

On Sun, Oct 24 2010 at  9:00pm -0400,
Alasdair G Kergon <agk@redhat.com> wrote:

> On Sat, Oct 23, 2010 at 08:55:15PM -0400, Mike Snitzer wrote:
> > any device using the error target shouldn't be scanned.
>  
> I disagree with that:)
> Scanning an error target does not cause the system to lock up so, in general,
> there's no reason not to scan it.
> (Remember that the error target might only be used in 1 out of hundreds
> of table lines and that loop - as you'd expect for the current uses - skips the
> device if *any* target matches.)
> 
> The defining characteristic here is surely that the device is no longer
> meant to be available for mounting - no longer visible, in other words.
> So you may (for now) need a more specific test that's limited
> to snapshot merge.

What about skipping a dm device whose table only has a single error
target?

Anyway... guess I need to back this change out and have another look.

Mike

[RFC][PATCH] never scan a device which is using the error target

[Alasdair G Kergon]

On Sun, Oct 24, 2010 at 09:23:04PM -0400, Mike Snitzer wrote:
> What about skipping a dm device whose table only has a single error
> target?
 
That's OK.

Alasdair

[RFC][PATCH] never scan a device which is using the error target

[Milan Broz]

On 10/25/2010 03:00 AM, Alasdair G Kergon wrote:

> So you may (for now) need a more specific test that's limited
> to snapshot merge.

ok. But with this approach we will have just code full of strange
exceptions.

I thought about "error" device as replacement for "failed" device,
so with this definition it should not be scanned or used.

But we are now apparently using error device as some padding
in intermediate state...

Just ad idea: should not be here special alias for it?
Like "failed" target + "error" target?

The logic "what if error is just one line in table" - the logic
is wrong there already - what if mirror is just part of table?
Now is excluded completely.

Also "suspended" state is state for LUKS device after luksSuspend,
(with volume key wiped) should I add another special exception for that?
...

Isn't there some better approach? Last 2 or 3 deadlocks with
clusters were caused by this code too, seems like candidate for rewrite:)

Milan

[RFC][PATCH] never scan a device which is using the error target

[Alasdair G Kergon]

See the FIXMEs in the code.
With mirrors it already excludes more devs that it actually needs to.

And of course you know what I'd say the right answer is:-)
The entire scanning logic should be outside lvm...

Alasdair