Bug 232499 - 'lvchange -a' with VG argument may cause "Unable" messages for hidden LVs
'lvchange -a' with VG argument may cause "Unable" messages for hidden LVs
Status: CLOSED ERRATA
Product: Red Hat Enterprise Linux 5
Classification: Red Hat
Component: lvm2 (Show other bugs)
5.5
All Linux
medium Severity medium
: beta
: ---
Assigned To: Dave Wysochanski
Cluster QE
:
Depends On:
Blocks: 5.4/TechnicalNotes
  Show dependency treegraph
 
Reported: 2007-03-15 15:26 EDT by Corey Marthaler
Modified: 2011-01-13 17:39 EST (History)
10 users (show)

See Also:
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.
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-01-13 17:39:27 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
strace of lvchange process (168.42 KB, text/plain)
2007-03-19 14:37 EDT, Corey Marthaler
no flags Details

  None (edit)
Description Corey Marthaler 2007-03-15 15:26:17 EDT
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 14:37:26 EDT
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 09:42:03 EDT
where you doing vgchange before?  Has lvchange always worked this way?
Comment 3 Jonathan Earl Brassow 2007-04-13 17:42:03 EDT
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 12:22:23 EDT
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 15:02:05 EDT
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 16:13:33 EDT
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 12:18:08 EDT
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 15:53:27 EDT
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 14:13:43 EDT
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 16:07:34 EDT
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 13:12:14 EDT
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 13:10:49 EDT
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 16:54:00 EDT
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 16:59:21 EDT
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 09:42:33 EST
Patch submitted to lvm-devel yesterday - some discussion of alternative approaches is ongoing.
Comment 17 Dave Wysochanski 2009-01-26 09:31:44 EST
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 09:40:05 EST
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 09:40:05 EST
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-17 21:48:02 EDT
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 19:54:17 EDT
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 12:34:00 EDT
I have a simpler fix for this and am testing / reviewing it now.
Comment 25 Dave Wysochanski 2009-10-22 17:13:42 EDT
Patch posted to lvm-devel, some review, awaiting final review / commit.
Comment 26 Dave Wysochanski 2009-12-08 17:58:23 EST
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 12:22:15 EDT
Fix has been checked in to upstream by agk and is in 2.02.63.
Comment 31 Milan Broz 2010-08-30 06:39:22 EDT
Fix in lvm2-2.02.73-1.el5.
Comment 33 Corey Marthaler 2010-11-02 17:33:51 EDT
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 17:39:27 EST
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.