Bug 232499

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

where you doing vgchange before?  Has lvchange always worked this way?

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?

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.

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

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.

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?

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.

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.  

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.

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.

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.

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.

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.

Patch submitted to lvm-devel yesterday - some discussion of alternative approaches is ongoing.

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.

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.

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.

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

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.

I have a simpler fix for this and am testing / reviewing it now.

Patch posted to lvm-devel, some review, awaiting final review / commit.

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

Fix has been checked in to upstream by agk and is in 2.02.63.

Fix in lvm2-2.02.73-1.el5.

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

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