Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 232499 - 'lvchange -a' with VG argument may cause "Unable" messages for hidden LVs
Summary: 'lvchange -a' with VG argument may cause "Unable" messages for hidden LVs
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 5
Classification: Red Hat
Component: lvm2
Version: 5.5
Hardware: All
OS: Linux
medium
medium
Target Milestone: beta
: ---
Assignee: Dave Wysochanski
QA Contact: Cluster QE
URL:
Whiteboard:
Depends On:
Blocks: 5.4, TechnicalNotes
TreeView+ depends on / blocked
 
Reported: 2007-03-15 19:26 UTC by Corey Marthaler
Modified: 2011-01-13 22:39 UTC (History)
10 users (show)

Fixed In Version: lvm2-2.02.73-1.el5
Doc Type: Bug Fix
Doc Text:
The lvchange command is used to change the attributes of a logical volume. Issuing the lvchange command on a volume group that contains a mirror or snapshot may result in messages similar to the following: Unable to change mirror log LV fail_secondary_mlog directly Unable to change mirror image LV fail_secondary_mimage_0 directly Unable to change mirror image LV fail_secondary_mimage_1 directly These messages can be safely ignored.
Clone Of:
Environment:
Last Closed: 2011-01-13 22:39:27 UTC
Target Upstream Version:


Attachments (Terms of Use)
strace of lvchange process (deleted)
2007-03-19 18:37 UTC, Corey Marthaler
no flags Details


Links
System ID Priority Status Summary Last Updated
Red Hat Product Errata RHBA-2011:0052 normal SHIPPED_LIVE lvm2 bug fix and enhancement update 2011-01-12 17:15:25 UTC

Description Corey Marthaler 2007-03-15 19:26:17 UTC
Description of problem:
This appears to be a recent regression, as I don't remember seeing these
messages before. If I attempt to either activate or deactivate cmirrors I get
the following messages along with error return codes, however the cmd always
ends up doing the proper thing, it just appears to fail. 

[root@link-08 ~]# lvscan
  ACTIVE            '/dev/VolGroup00/LogVol00' [72.38 GB] inherit
  ACTIVE            '/dev/VolGroup00/LogVol01' [1.94 GB] inherit
  ACTIVE            '/dev/helter_skelter/fail_secondary' [500.00 MB] inherit
[root@link-08 ~]# lvchange -an helter_skelter
  Unable to change mirror log LV fail_secondary_mlog directly
  Unable to change mirror image LV fail_secondary_mimage_0 directly
  Unable to change mirror image LV fail_secondary_mimage_1 directly
[root@link-08 ~]# lvscan
  ACTIVE            '/dev/VolGroup00/LogVol00' [72.38 GB] inherit
  ACTIVE            '/dev/VolGroup00/LogVol01' [1.94 GB] inherit
  inactive          '/dev/helter_skelter/fail_secondary' [500.00 MB] inherit
[root@link-08 ~]# lvchange -ay helter_skelter
  Unable to change mirror log LV fail_secondary_mlog directly
  Unable to change mirror image LV fail_secondary_mimage_0 directly
  Unable to change mirror image LV fail_secondary_mimage_1 directly
[root@link-08 ~]# lvscan
  ACTIVE            '/dev/VolGroup00/LogVol00' [72.38 GB] inherit
  ACTIVE            '/dev/VolGroup00/LogVol01' [1.94 GB] inherit
  ACTIVE            '/dev/helter_skelter/fail_secondary' [500.00 MB] inherit

[root@link-08 ~]# lvchange -an helter_skelter
  Unable to change mirror log LV fail_secondary_mlog directly
  Unable to change mirror image LV fail_secondary_mimage_0 directly
  Unable to change mirror image LV fail_secondary_mimage_1 directly
[root@link-08 ~]# echo $?
5
[root@link-08 ~]# lvscan
  ACTIVE            '/dev/VolGroup00/LogVol00' [72.38 GB] inherit
  ACTIVE            '/dev/VolGroup00/LogVol01' [1.94 GB] inherit
  inactive          '/dev/helter_skelter/fail_secondary' [500.00 MB] inherit

Version-Release number of selected component (if applicable):
2.6.9-50.ELsmp
cmirror-kernel-2.6.9-25.0

How reproducible:
everytime

Comment 1 Corey Marthaler 2007-03-19 18:37:26 UTC
Created attachment 150407 [details]
strace of lvchange process

Not sure if this will be of any use, but here is a trace of this issue in
action

Comment 2 Jonathan Earl Brassow 2007-04-10 13:42:03 UTC
where you doing vgchange before?  Has lvchange always worked this way?


Comment 3 Jonathan Earl Brassow 2007-04-13 21:42:03 UTC
yeah, I think you have your commands mixed up.

[root@neo-04 ~]# lvchange -ay vg
  Unable to change mirror log LV lv_mlog directly
  Unable to change mirror image LV lv_mimage_0 directly
  Unable to change mirror image LV lv_mimage_1 directly
[root@neo-04 ~]# vgchange -an vg
  0 logical volume(s) in volume group "vg" now active

Perhaps we should be ensuring that a volume group is not a valid argument to
lvchange?


Comment 4 Corey Marthaler 2007-04-16 16:22:23 UTC
What is confusing, is that this works for lvremove, and is even in the man page:

       Remove all logical volumes in volume group vg00:
            lvremove vg00

So I just figured it was supported with lvchange as well, and oddly enough, it
does work as expected if you discount the errors. I would think we'd want to
keep the behavior between commands similar.

Comment 6 Corey Marthaler 2007-04-18 19:02:05 UTC
Also note, that this works with linears and stripes:

[root@link-04 ~]# lvcreate -L 1G vg2
  Logical volume "lvol0" created
[root@link-04 ~]# lvcreate -L 1G vg2
  Logical volume "lvol1" created
[root@link-04 ~]# lvcreate -L 1G vg2
  Logical volume "lvol2" created
[root@link-04 ~]# lvcreate -L 1G vg2
  Logical volume "lvol3" created
[root@link-04 ~]# lvchange -an vg2
[root@link-04 ~]# lvs
  LV       VG         Attr   LSize  Origin Snap%  Move Log Copy%
  LogVol00 VolGroup00 -wi-ao 72.44G
  LogVol01 VolGroup00 -wi-ao  1.94G
  lvol0    vg2        -wi---  1.00G
  lvol1    vg2        -wi---  1.00G
  lvol2    vg2        -wi---  1.00G
  lvol3    vg2        -wi---  1.00G


Comment 7 Jonathan Earl Brassow 2007-04-18 20:13:33 UTC
yes, it will work for linear and stripe, because there are no "invisible"
volumes (i.e. those that only list with '-a').  Try single machine snapshot, for
example.


Comment 8 Dave Wysochanski 2007-05-21 16:18:08 UTC
Jon, is this fairly straightforward?  How about detect whether a vg is on the
cmdline in process_each_lv, add a flags field to struct cmd_context, set
VGS_ON_CMDLINE flag in this field and check for that in lvchange_single() to
suppress the messages?

Comment 9 Dave Wysochanski 2007-09-11 19:53:27 UTC
Not sure about this one at this point.  Might want to change the iterator to
only iterate over visible LVs but need to check all callers and think about
alternatives.

Comment 10 Dave Wysochanski 2008-04-14 18:13:43 UTC
On the surface this sounds easy but the way the code is structured it does not
appear simple to come up with a good fix (at least I'm not sure about it).

Checked the history on this one - this was devel_ack'd a year ago before I took
it.  I should have put it back to '?' for devel_ack for 4.7.  I will take a look
for an hour or so but it may be too late to get in the proper fix for 4.7.  



Comment 11 Dave Wysochanski 2008-04-14 20:07:34 UTC
There's a bit of code in process_each_lv_in_vg() to skip over snapshots:
	list_iterate_items(lvl, &vg->lvs) {
		if (lvl->lv->status & SNAPSHOT)
			continue;


We could restrict this code to only iterate through visible LVs and skip
internal ones.  However, there's a significant number of callers of this code so
I'm unsure about it at this point.  I started an analysis but it will take some
time.

The other alternative is to add a simple flag somewhere indicating whether or
not to process internal LVs.  We can pass this flag into process_each_lv()
and/or process_each_lv_in_vg() depending on the caller.  I'm not confident this
is the right long-term solution though.

At this point my gut feel is to defer this to 4.8 or following.

Comment 12 RHEL Product and Program Management 2008-09-05 17:12:14 UTC
This request was evaluated by Red Hat Product Management for inclusion in a Red
Hat Enterprise Linux maintenance release.  Product Management has requested
further review of this request by Red Hat Engineering, for potential
inclusion in a Red Hat Enterprise Linux Update release for currently deployed
products.  This request is not yet committed for inclusion in an Update
release.

Comment 13 Dave Wysochanski 2008-09-24 17:10:49 UTC
Right now I'm trying to determine whether a simple patch like this is safe:
diff --git a/tools/toollib.c b/tools/toollib.c
index 4d11b9b..0de7670 100644
--- a/tools/toollib.c
+++ b/tools/toollib.c
@@ -178,6 +178,11 @@ int process_each_lv_in_vg(struct cmd_context *cmd,
                if (lvl->lv->status & SNAPSHOT)
                        continue;
 
+#if 1
+               if (!lv_is_visible(lvl->lv))
+                       continue;
+#endif
+
                /* Should we process this LV? */
                if (process_all)
                        process_lv = 1;


It is is not clear whether the iterator functions expect to process hidden LVs or not.  I'm going through each of the callers to try to determine the risk of this change.  Good news is this change does pass nightly tests ok, and most of the callers do not expect to process hidden LVs.  I think at least one caller (vgmknodes) expects to process hidden LVs and there are a lot of code paths.

Key problem is: When a tool receives a VG on the cmdline and uses the iterator functions (process_each_lv or process_each_lv_in_vg), does the "process_single" function expect to be called for hidden LVs?  If not, we should make this clear in the iterator functions.

Comment 14 Dave Wysochanski 2008-09-24 20:54:00 UTC
Right now I'm trying to determine whether a simple patch like this is safe:
diff --git a/tools/toollib.c b/tools/toollib.c
index 4d11b9b..0de7670 100644
--- a/tools/toollib.c
+++ b/tools/toollib.c
@@ -178,6 +178,11 @@ int process_each_lv_in_vg(struct cmd_context *cmd,
                if (lvl->lv->status & SNAPSHOT)
                        continue;
 
+#if 1
+               if (!lv_is_visible(lvl->lv))
+                       continue;
+#endif
+
                /* Should we process this LV? */
                if (process_all)
                        process_lv = 1;


It is is not clear whether the iterator functions expect to process hidden LVs or not.  I'm going through each of the callers to try to determine the risk of this change.  Good news is this change does pass nightly tests ok, and most of the callers do not expect to process hidden LVs.  I think at least one caller (vgmknodes) expects to process hidden LVs and there are a lot of code paths.

Key problem is: When a tool receives a VG on the cmdline and uses the iterator functions (process_each_lv or process_each_lv_in_vg), does the "process_single" function expect to be called for hidden LVs?  If not, we should make this clear in the iterator functions.

Comment 15 Dave Wysochanski 2008-09-24 20:59:21 UTC
Looks like bz somehow duplicated my last entry, but lost my latest update -
I'll try to summarize again.

I think I have enough to devel_ack this.  The above mentioned solution will not
work because some tools assume hidden LVs will be processed while others do
not.

The solution I'm leaning towards is to add a flag argument to pass to
process_each_lv and through to process_each_lv_in_vg, and a specific flag like
this:
/*
 * Indicates whether we should process only visible LVs when looping
 * through a list of LVs on a VG.
 * NOTE: This does not apply if a hidden/non-visible LV is given on the
 * commandline for a tool, only if another option is given (e.g. a VG) that
 * implies a list of LVs and the tools need to make a decision.
 */
#define PROCESS_ONLY_VISIBLE_LVS 0x1

In process_each_lv_in_vg we have a loop where we're iterating through the LVs
in a VG.  In that loop we have various conditions we're checking for to
determine whether to process the current LV.  To resolve this bz, I can just
add another condtion like this:
/*
 * If no LVs given on the cmdline, and this is not a visible LV,
 * then skip the LV if we were told to only process visible ones.
 */
if (!lvargs_supplied && 
    !lv_is_visible(lv) &&
    (flags & PROCESS_ONLY_VISIBLE_LVS))
  continue;

This will skip over processing the LV if no LVs were given on the commandline,
it is a hidden LV, and the flag is set.

Comment 16 Dave Wysochanski 2009-01-21 14:42:33 UTC
Patch submitted to lvm-devel yesterday - some discussion of alternative approaches is ongoing.

Comment 17 Dave Wysochanski 2009-01-26 14:31:44 UTC
Proposed release note:

Issuing lvchange with a volume group parameter, where the volume group contains a mirror or snapshot may result in messages similar to the following:
# lvchange -ay helter_skelter
  Unable to change mirror log LV fail_secondary_mlog directly
  Unable to change mirror image LV fail_secondary_mimage_0 directly
  Unable to change mirror image LV fail_secondary_mimage_1 directly

These messages are benign and can be ignored.

Comment 18 Tom Coughlan 2009-01-26 14:40:05 UTC
We have decided that the risk associated with this fix is not worth the benefit at this point in the life of RHEL 4. Instead, we will defer the fix to RHEL 5, and add a release note to 4.8 explaining the issue.

Comment 19 Tom Coughlan 2009-01-26 14:40:05 UTC
Release note added. If any revisions are required, please set the 
"requires_release_notes" flag to "?" and edit the "Release Notes" field accordingly.
All revisions will be proofread by the Engineering Content Services team.

New Contents:
Issuing lvchange with a volume group parameter, where the volume group contains
a mirror or snapshot may result in messages similar to the following:
# lvchange -ay helter_skelter
  Unable to change mirror log LV fail_secondary_mlog directly
  Unable to change mirror image LV fail_secondary_mimage_0 directly
  Unable to change mirror image LV fail_secondary_mimage_1 directly

These messages are benign and can be ignored.

Comment 20 Dave Wysochanski 2009-03-18 01:48:02 UTC
There are still some patches pending that involve the interator code.  For now let's add the release note to 5.4 and we'll see if we can get the code fixed in 5.5

Comment 22 Ryan Lerch 2009-06-02 23:54:17 UTC
Release note updated. If any revisions are required, please set the 
"requires_release_notes"  flag to "?" and edit the "Release Notes" field accordingly.
All revisions will be proofread by the Engineering Content Services team.

Diffed Contents:
@@ -1,8 +1,7 @@
-Issuing lvchange with a volume group parameter, where the volume group contains
+The lvchange command is used to change the attributes of a logical volume. Issuing the lvchange command on a volume group that contains
 a mirror or snapshot may result in messages similar to the following:
-# lvchange -ay helter_skelter
   Unable to change mirror log LV fail_secondary_mlog directly
   Unable to change mirror image LV fail_secondary_mimage_0 directly
   Unable to change mirror image LV fail_secondary_mimage_1 directly
 
-These messages are benign and can be ignored.+These messages can be safely ignored.

Comment 24 Dave Wysochanski 2009-10-14 16:34:00 UTC
I have a simpler fix for this and am testing / reviewing it now.

Comment 25 Dave Wysochanski 2009-10-22 21:13:42 UTC
Patch posted to lvm-devel, some review, awaiting final review / commit.

Comment 26 Dave Wysochanski 2009-12-08 22:58:23 UTC
Still working on another round of this patchset after some review.  This fix is blocked behind an assumption I'd like to make - we only take visible / top level LVs as commandline arguments.  Need some improvements to vg_validate to ensure we never actually commit hidden/incomplete LVs to disk and subsequently might need to lvremove, etc.  Need to make sure I cover all possible LV types and as I recall I ran into some problems deciphering / verifying a few of these cases (virtual origins I think were a problem).

Comment 29 Dave Wysochanski 2010-05-10 16:22:15 UTC
Fix has been checked in to upstream by agk and is in 2.02.63.

Comment 31 Milan Broz 2010-08-30 10:39:22 UTC
Fix in lvm2-2.02.73-1.el5.

Comment 33 Corey Marthaler 2010-11-02 21:33:51 UTC
Fix verified in lvm2-2.02.74-1.el5.

[root@taft-01 ~]# lvs -a -o +devices
  LV                VG         Attr   LSize   Log         Copy%  Convert Devices
  mirror            taft       mwi-a- 500.00M mirror_mlog 100.00         mirror_mimage_0(0),mirror_mimage_1(0)
  [mirror_mimage_0] taft       iwi-ao 500.00M                            /dev/sdb1(0)
  [mirror_mimage_1] taft       iwi-ao 500.00M                            /dev/sdc1(0)
  [mirror_mlog]     taft       lwi-ao   4.00M                            /dev/sdh1(0)
[root@taft-01 ~]# lvscan
  ACTIVE            '/dev/taft/mirror' [500.00 MB] inherit
[root@taft-01 ~]# lvchange -an taft
[root@taft-01 ~]# echo $?
0
[root@taft-01 ~]# lvscan
  inactive          '/dev/taft/mirror' [500.00 MB] inherit
[root@taft-01 ~]# lvchange -ay taft
[root@taft-01 ~]# lvscan
  ACTIVE            '/dev/taft/mirror' [500.00 MB] inherit

Comment 35 errata-xmlrpc 2011-01-13 22:39:27 UTC
An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHBA-2011-0052.html


Note You need to log in before you can comment on or make changes to this bug.