updates to stacking and lvconvert

Hi,

These patches do followings:

- Change default behavior of lvconvert
[ 1/11] lvconvert takes '--wait' instead of '--background'

- Add generalized upward link from LV to seg
[ 2/11] Add upward link from underlying LV to stacked seg

- Cosmetic changes for later bugfixes
[ 3/11] Move mirror log initialization functions [ 4/11] Move _find_tmp_mirror()
[ 5/11] Move removable_pvs checking

- Fix mirror log addition/removal for LV under conversion [ 6/11] Export find_tmp_mirror()
[ 7/11] lvconvert adds/removes mirror log to/from the original LV

- Fix remove_mirror_images()
[ 8/11] Allow "mirror" segment with only 1 area
[ 9/11] _init_mirror_log() to cope with activation status [10/11] Fix _remove_mirror_images() to cope with stacked mirror [11/11] Fix _remove_mirror_images() to skip AREA_LV

- Appendix: 2 test scripts for mirroring

Since the last week's patchset,

Done with the patch "2/11" of this series.

remove_mirror_images() didn't work nicely with non-empty removable_pvs.
Fixed by patches "8/11" - "11/11".

This was not a problem.

Jun'ichi Nomura, NEC Corporation of America

Remove '--background' option and add '--wait' option to lvconvert.

Recent change in lvconvert let it starts polling the conversion by default for adding mirror(s).
'--background' is an option to immediately exit from the command and do the polling in background.

The option was inherited from pvmove.
However, since lvconvert used to exit immediately and it still does for different types of conversion (e.g. linear to mirror conversion), it's more consistent and intuitive to add '--wait' option for foreground polling and do it in background by default.

Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvconvert-wait-option.patch
Type: text/x-patch
Size: 2932 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080111/38ae8e53/attachment.bin>

[14/14] lvconvert help message wasn't fixed to '--wait'
Help message was not fixed to '--wait'.

Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-lvconvert-args.patch
Type: text/x-patch
Size: 1154 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080111/4d63ff59/attachment.bin>

I'm not yet persuaded about the need for this change.

The aim is that the behaviour of the tools be consistent across the whole command set.

Waiting for the operation to complete feels more intuitive to me.

(It's the existing cases where we don't wait for the kernel to finish its initial copy that feel wrong to me.)

Alasdair

agk at redhat.com

OK. I didn't want to change the existing behavior of lvconvert. But I agree the synchronous behavior is better choice for default case.

Attached is a patch to fix the inconsistency issue in the other way. If it looks acceptable, please replace patches "1/11" and "14/14" with this. Other patches won't be affected.

By default: lvconvert waits for initial sync

Add a mirror to linear

    # lvs -o+stripes vg/lvol0; lvconvert -m+1 vg/lvol0; lvs -o+stripes vg/lvol0
      LV    VG   Attr   LSize Origin Snap%  Move Log Copy%  Convert #Str
      lvol0 vg   -wi-a- 1.00G                                          1
      vg/lvol0: Converted: 18.8%
      vg/lvol0: Converted: 39.6%
      vg/lvol0: Converted: 61.7%
      vg/lvol0: Converted: 83.6%
      vg/lvol0: Converted: 100.0%
      Logical volume lvol0 converted.
      LV    VG   Attr   LSize Origin Snap%  Move Log        Copy%  Convert #Str
      lvol0 vg   mwi-a- 1.00G                    lvol0_mlog 100.00            2

Add a mirror to mirror

    # lvs -o+stripes vg/lvol0; lvconvert -m+1 vg/lvol0; lvs -o+stripes vg/lvol0
      LV    VG   Attr   LSize Origin Snap%  Move Log        Copy%  Convert #Str
      lvol0 vg   mwi-a- 1.00G                    lvol0_mlog 100.00            2
      vg/lvol0: Converted: 53.7%
      vg/lvol0: Converted: 76.8%
      vg/lvol0: Converted: 99.8%
      vg/lvol0: Converted: 100.0%
      Logical volume lvol0 converted.
      LV    VG   Attr   LSize Origin Snap%  Move Log        Copy%  Convert #Str
      lvol0 vg   mwi-a- 1.00G                    lvol0_mlog 100.00            3

Remove mirror

    # lvs -o+stripes vg/lvol0; lvconvert -m-1 vg/lvol0; lvs -o+stripes vg/lvol0
      LV    VG   Attr   LSize Origin Snap%  Move Log        Copy%  Convert #Str
      lvol0 vg   mwi-a- 1.00G                    lvol0_mlog 100.00            3
      Logical volume lvol0 converted.
      LV    VG   Attr   LSize Origin Snap%  Move Log        Copy%  Convert #Str
      lvol0 vg   mwi-a- 1.00G                    lvol0_mlog 100.00            2
    # lvs -o+stripes vg/lvol0; lvconvert -m-1 vg/lvol0; lvs -o+stripes vg/lvol0
      LV    VG   Attr   LSize Origin Snap%  Move Log        Copy%  Convert #Str
      lvol0 vg   mwi-a- 1.00G                    lvol0_mlog 100.00            2
      Logical volume lvol0 converted.
      LV    VG   Attr   LSize Origin Snap%  Move Log Copy%  Convert #Str
      lvol0 vg   -wi-a- 1.00G

With background option (-b):

Add a mirror to linear

    # lvs -o+stripes vg/lvol0; lvconvert -m+1 -b vg/lvol0; lvs -o+stripes vg/lvol0
      LV    VG   Attr   LSize Origin Snap%  Move Log Copy%  Convert #Str
      lvol0 vg   -wi-a- 1.00G                                          1
      Logical volume lvol0 converted.
      LV    VG   Attr   LSize Origin Snap%  Move Log        Copy%  Convert #Str
      lvol0 vg   mwi-a- 1.00G                    lvol0_mlog   0.59            2

Add a mirror to mirror

    # lvs -o+stripes vg/lvol0; lvconvert -m+1 -b vg/lvol0; lvs -o+stripes vg/lvol0
      LV    VG   Attr   LSize Origin Snap%  Move Log        Copy%  Convert #Str
      lvol0 vg   mwi-a- 1.00G                    lvol0_mlog  11.52            2
      LV    VG   Attr   LSize Origin Snap%  Move Log Copy%  Convert           #Str
      lvol0 vg   cwi-a- 1.00G                          0.39 lvol0_mimagetmp_2    2

Remove mirror

    # lvs -o+stripes vg/lvol0; lvconvert -m-1 -b vg/lvol0; lvs -o+stripes vg/lvol0
      LV    VG   Attr   LSize Origin Snap%  Move Log Copy%  Convert           #Str
      lvol0 vg   cwi-a- 1.00G                         17.77 lvol0_mimagetmp_2    2
      Logical volume lvol0 converted.
      LV    VG   Attr   LSize Origin Snap%  Move Log        Copy%  Convert #Str
      lvol0 vg   mwi-a- 1.00G                    lvol0_mlog  31.45            2
    # lvs -o+stripes vg/lvol0; lvconvert -m-1 -b vg/lvol0; lvs -o+stripes vg/lvol0
      LV    VG   Attr   LSize Origin Snap%  Move Log        Copy%  Convert #Str
      lvol0 vg   mwi-a- 1.00G                    lvol0_mlog  34.57            2
      Logical volume lvol0 converted.
      LV    VG   Attr   LSize Origin Snap%  Move Log Copy%  Convert #Str
      lvol0 vg   -wi-a- 1.00G                                          1

Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvconvert-wait-for-linear-mirror-conversion.patch
Type: text/x-patch
Size: 2552 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080114/39640b27/attachment.bin>

As a part of LV stacking support, this patch adds a upward link from underlying LV to LV segments using the LV.

There used to be 'seg->mirror_seg' specialized for mimage/mlog to mirrored LV relationship.
They will be replaced with the generalized link, too.

Stacking is done like this:

   +--------------------------+
   |          LV0             |
   +--------------------------+
   |SEG01|SEG02|SEG03| ...    |
   +--------------------------+
   |          LV1             |
   +--------------------------+
   |SEG11|SEG12|SEG13| ...    |
   +--------------------------+

(This figure might be over simplified.
The LV-LV relationship is not necessarily 1:1. The point is they are stacked as lv-seg-lv-seg-...)

Downward links:
- lv-to-seg (e.g. LV0 -> SEG0x, LV1 -> SEG1x) => lv->segments
- seg-to-lv (e.g. SEG0x -> LV1) => seg_lv(seg, s)
Upward links:
- lv-to-seg (e.g. LV1 -> SEG0x) => N/A
- seg-to-lv (e.g. SEG0x -> LV0, SEG1x -> LV1) => seg->lv

This patch adds lv-to-seg upward link, lv->segs_using_this_lv.

The patch also replaces seg->mirror_seg with this.
mirror_seg was seg-to-seg link, i.e. with the above example, SEG1x -> SEG0x. However, it's been used only when LV0 has 1 segment.
So the conversion is straight-forward.

For segment areas, the linking/unlinkg of the LV to the seg is automatically done with set_lv_segment_area_lv()/release_lv_segment_area(). For other relation ships (e.g. log_lv), the linking/unlinking should done explicitly using lv_add_user_seg()/lv_remove_user_seg().

find_parent_for_layer() is removed.

Thanks,

Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: stacked-lv-generalized-upward-link.patch
Type: text/x-patch
Size: 18230 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080111/fc84daea/attachment.bin>

I'm very sorry. There were some bugs sneaked in the patchst and I missed them by running tests on unclean tree.

I'm posting 3 additional patches.
[12/14] Do not make extra call to lv_add_user_seg() [13/14] lvconvert should check first_seg(original_lv) [14/14] lvconvert help message wasn't fixed to '--wait'

This patch is:

[12/14] Do not make extra call to lv_add_user_seg()
Without this, lv->segs_using_this_lv will have duplicated entry and find_mirror_seg() will fail. VG validator will detect it and so most commands will not work.

Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-upward-link.patch
Type: text/x-patch
Size: 861 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080111/a3c5ff1f/attachment.bin>

The picture is confusing:
There is no 1:1 mapping from SEG0x to SEG1x as the picture suggests. One upper LV segment may use an arbitrary number of LV segments below it. This is not like the lv segment to pv segment mappings.
- Whereas PV segments can be split arbitrarily to maintain the simple mappings, LV segments *cannot* in general be split on demand. (This of the complexity involved in splitting a snapshot segment, for example!)

where s is the index of the area (stripe)

Yes.

We need to rename that - 'user_seg' could mean something else.

Alasdair

agk at redhat.com

/*
+ * Add/remove upward link from underlying LV to the segment using it
+ */
+int lv_add_user_seg(struct logical_volume *lv, struct lv_segment *seg); +int lv_remove_user_seg(struct logical_volume *lv, struct lv_segment *seg);

need to rename - too ambiguous

+/*
+ * This is a function specialized for the common case where there is
+ * only one segment which uses the LV.
+ * e.g. the LV is a layer inserted by insert_layer_for_lv().
+ *
+ * In general, walk through lv->segs_using_this_lv.
+ */
+struct lv_segment *seg_using_lv(struct logical_volume *lv) +{
+ struct seg_list *sl;
+
+ if (list_size(&lv->segs_using_this_lv) != 1)
+ return NULL;

Is it ever valid to return here and not have an error?

+ if (!lv_add_user_seg(log_lv, seg))

Needs a clearer name.

@@ -110,8 +110,8 @@ int check_lv_segments(struct logical_vol
}

Where is this new list validated?

Alasdair

agk at redhat.com

Alasdair,

thanks for the comments.

How about this?
add_seg_to_segs_using_this_lv(lv, seg)
remove_seg_from_segs_using_this_lv(lv, seg)

Or this?
add_parent_seg(lv, seg)
remove_parent_seg(lv, seg)

Returning NULL is valid if the list is empty.
But it should print error if the list has more than 1 items.

Some validation were included in the tests using find_mirror_seg(). I'll add generic ones.

Thanks,

Jun'ichi Nomura, NEC Corporation of America

Not sure. Still thinking.

What I mean is: why would the list be empty if that function
is being called? If the list was known to be empty, you
wouldn't be in a code path that called the function, surely?
IOW it's a coding error if 'return NULL' is hit.
Or does the function have a secondary use to find out whether
the LV is at the top of the stack?
Is it better split into two? First an is_ function to find out
if the segment is the type expected with a single segment; then the function to give details of the one-and-only segment?

I mean the validation in the vg_write() path must have independent validity checks so we never write out the metadata if there's an internal inconsistency in it. IOW it must be done somewhere under vg_validate(). IOW the pointers must be validated in both directions:
all existing list elements are correct (and not duplicated), and no elements are missing from the list.

Alasdair

agk at redhat.com

Ah, I understand now.
If it returns NULL, there should be internal error in LVM2 code.

I don't think we need to split it.
If a code calls seg_using_lv(lv), it knows the lv should have one-and-only user (upper-level) segment.
If the lv doesn't, it's a coding error.

Maybe it's handy to have a wrapper for !list_empty(&lv->segs_using_this_lv) to check if the LV is top-level or not.

  int is_top_of_the_stack(lv) {
    return list_empty(lv->segs_using_this_lv);

}

I'll add that type of validation check.

Duplication is currently allowed since 2 areas in a segment can be on the same LV.

Jun'ichi Nomura, NEC Corporation of America

Attached is a revised version of the patch in the subject, including a fix posted as "12/14" fix-upward-link.patch. Other patches in the patchset won't be affected.

Changes since the last post:
* Modified the picture in the patch header
* Add reference count to 'struct seg_list'
* Add validation checks in check_lv_segments()
* Don't add link from mimage to mirror LV in mirror fixup code
* Add _remove_mirror_log() to properly unlink mirror seg

    from log_lv->segs_using_this_lv, instead of just setting
    seg->log_lv = NULL

* Change _lv_split_segment() to use set_lv_segment_area_lv() instead of directly modifying seg_lv(seg, s)
* Add error messages in find_mirror_seg()
* Add error messages in seg_using_lv()
* Rename lv_{add,remove}_user_seg() and seg_using_lv()

Following issues may need more discussion:

- Namings of functions to add/remove seg to/from lv->segs_using_this_lv.
For now, I just changed them to descriptive names
- add_seg_to_segs_using_this_lv()
- remove_seg_from_segs_using_this_lv()

- About seg_using_lv().
Maybe adding is_top_of_the_stack(lv) is better if we would like to check it in future.
For now, seg_using_lv() is really a specialized function to check if it has one and only segment and to get it. So I didn't change it except for additional error message and renaming to a descriptive name to characterize its specialty.

- Duplication check of lv->segs_using_this_lv.
In theory, it's possible that a LV has multiple references from a single segment. (e.g. multiple areas refer the same LV) So I added reference count to 'struct seg_list' and the validator matches it with actual references as well as checking duplications of entries.

Thanks,

Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: stacked-lv-generalized-upward-link.patch
Type: text/x-patch
Size: 23460 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080115/8b3d8ab9/attachment.bin>

Move mirror log initialization functions to the earlier part of the file. No functional change.

_init_mirror_log() will be used by _remove_mirror_images(), when it wants to reset the log contents for reconfigured mirror.

Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cosmetic-move-log-function-to-head.patch
Type: text/x-patch
Size: 7475 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080111/daa659a5/attachment.bin>

Move _find_tmp_mirror() for the use by remove_mirror_images(). No functional change.

Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cosmetic-move-find_tmp_mirror-to-head.patch
Type: text/x-patch
Size: 1582 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080111/a1da17f2/attachment.bin>

Move 'removable_pvs' checking from _remove_mirror_images() to a new function. No functional change.

To improve readability.

Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cosmetic-move-removable_pvs-checking.patch
Type: text/x-patch
Size: 2819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080111/016363b3/attachment.bin>

Export find_tmp_mirror() for use by lvconvert to
walk down the converting LV stack.

Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: export-find_tmp_mirror.patch
Type: text/x-patch
Size: 1620 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080111/34e8d2ed/attachment.bin>

If mirror log addition/removal was requested during conversion, lvconvert should manipulate on the original LV, not the top-level LV.

Use find_tmp_mirror() to find out the original LV.

Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvconvert-add-remove-log-original-lv.patch
Type: text/x-patch
Size: 2429 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080111/6f8ff7b6/attachment.bin>

[13/14] lvconvert should check first_seg(original_lv)
lvconvert tries to add/remove log for the original LV. So it should check the original LV's segment for the current log type.
However, it checks the existing of the log for the top-level LV.

Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-lvconvert-on-original-lv.patch
Type: text/x-patch
Size: 1031 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080111/8e870dd9/attachment.bin>

The patch was incomplete.
The same change is needed for log-only addition/removal.

Thanks,

Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvconvert-mod-log-to-orig-lv.patch
Type: text/x-patch
Size: 850 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080116/813083c7/attachment.bin>

Allow "mirror" segment with only 1 area.

As a transitional state, "mirror" segment with only 1 area makes sense for keeping mirror specific parameters (log_lv, region_size) in metadata.

The segment handler already has a code to add "linear" target line for table. (see MIRR_DISABLED)

Codes to get copy percent have to consider the segment is in-sync if area_count is 1.

Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: allow-1way-mirror-seg.patch
Type: text/x-patch
Size: 1990 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080111/c7d0c283/attachment.bin>

_init_mirror_log() is a function to initialize mirror log contents with necessary activation and cleanups.
The function was originaly made for log_lv, which is just created and not active.

For the log (re-)initialization, it's nice if the function takes care of the current activation status and the intent of the caller for the failure case.

Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: init_mirror_log-cope-with-existing-active-log.patch
Type: text/x-patch
Size: 3184 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080111/d7de6533/attachment.bin>

remove_mirror_images() has a feature to select mimage LV(s) to be removed (with 'removable_pvs' param).
e.g. 'lvconvert -m-1 vg/lvol1 /dev/sdc /dev/sdd' removes a mirror image
that doesn't use PVs other than /dev/sdc and /dev/sdd.

But it doesn't work properly if the LV is stacked.
- Even if no LVs were removed, remove_mirror_images() should walk down the stack and retry because it may have LVs matching 'removable_pvs'. To allow that, _remove_mirror_images() should return how many LVs are removed and distinguish failure vs. 0-removal.
- If the mimage is a temporary one ("*_mimagetmp_<n>"), don't remove.
- If a temporary LV becomes single area as a result of removal, it should still be a "mirror" to keep log_lv and region_size. collapse_mirrored_lv() will take care of reconfiguration.

Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-remove_mirror_images-for-removable_pvs.patch
Type: text/x-patch
Size: 6599 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080111/b8215336/attachment.bin>

Do not remove mirror image on AREA_LV.

If the mirror image LV consists of AREA_LV, current version of remove_mirror_images() always remove it ignoring removable_pvs constraint.
That's not correct.

The FIXME comment suggests recursing down the AREA_LV. However, it might not be sufficient if we can have LVs with different structure.

Currently, we won't hit this check.
So just leave it for safety side.

Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-remove_mirror_images-dont-remove-stacked-mirror.patch
Type: text/x-patch
Size: 1048 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080111/4e8342fa/attachment.bin>

This is a test script for basic mirrored LV operations. Not yet comprehensive but can prevent trivial regressions.

Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: t-mirror-basic.sh
Type: application/x-sh
Size: 7365 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080111/339a892b/attachment.sh>

This is a test script for new lvconvert features related to adding mirror(s) to already mirrored LV.

Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: t-mirror-lvconvert.sh
Type: application/x-sh
Size: 10163 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080111/1d3fbe20/attachment.sh>

Updated test script for lvconvert without '--wait' option.

Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: t-mirror-lvconvert.sh
Type: application/x-sh
Size: 10115 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080115/a9555d39/attachment.sh>