[PATCH 0/11] updates to stacking and lvconvert

[PATCH 0/11] updates to stacking and lvconvert

[Jun'ichi Nomura]

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,

> Other things I would like to work on next are:
>   - Generalization of seg->mirrored_seg as upward link
>     from LV to seg(s) which use the LV.

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

>   - Review and fix remove_mirror_images() so that 'removable_pvs'
>     filtering works on stacked mirror.

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

>   - Review the suspend code path. I think 'noflush' should be
>     avoided for lower-level mirrors. Otherwise the upper-level mirror
>     can't suspend.

This was not a problem.

>   - Remaining patches from the last post:
>     https://www.redhat.com/archives/lvm-devel/2007-December/msg00045.html
>      [11/13] Add 'lv_mirrors' field to reporting functions
>      [12/13] Remove hardcoded "_mlog" name from deptree construction
>      [13/13] Allow disk logs for temporary resync mirror
>   - Creating a script for 'make check'

-- 
Jun'ichi Nomura, NEC Corporation of America

[PATCH 1/11] lvconvert takes '--wait' instead of '--background'

[Jun'ichi Nomura]

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>

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

[Jun'ichi Nomura]

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>

[PATCH 3/11] Move mirror log initialization functions

[Jun'ichi Nomura]

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>

[PATCH 4/11] Move _find_tmp_mirror()

[Jun'ichi Nomura]

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>

[PATCH 5/11] Move removable_pvs checking

[Jun'ichi Nomura]

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>

[PATCH 6/11] Export find_tmp_mirror()

[Jun'ichi Nomura]

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>

[PATCH 7/11] lvconvert adds/removes mirror log to/from the original LV

[Jun'ichi Nomura]

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>

[PATCH 8/11] Allow "mirror" segment with only 1 area

[Jun'ichi Nomura]

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>

[PATCH 9/11] _init_mirror_log() to cope with activation status

[Jun'ichi Nomura]

_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>

[PATCH 10/11] Fix _remove_mirror_images() to cope with stacked mirror

[Jun'ichi Nomura]

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>

[PATCH 11/11] Fix _remove_mirror_images() to skip AREA_LV

[Jun'ichi Nomura]

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>

[Test case 1/2] Basic mirrored LV testing

[Jun'ichi Nomura]

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>

[Test case 2/2] lvconvert mirror-addition testing

[Jun'ichi Nomura]

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>

[PATCH 12/14] Do not make extra call to lv_add_user_seg()

[Jun'ichi Nomura]

Jun'ichi Nomura wrote:
> As a part of LV stacking support, this patch
> adds a upward link from underlying LV to LV segments
> using the LV.

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>

[PATCH 13/14] lvconvert should check first_seg(original_lv)

[Jun'ichi Nomura]

Jun'ichi Nomura wrote:
> If mirror log addition/removal was requested during conversion,
> lvconvert should manipulate on the original LV, not the top-level LV.

[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>

[PATCH 14/14] lvconvert help message wasn't fixed to '--wait'

[Jun'ichi Nomura]

Jun'ichi Nomura wrote:
> Remove '--background' option and add '--wait' option to lvconvert.

[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>

[PATCH 1/11] lvconvert takes '--wait' instead of '--background'

[Alasdair G Kergon]

On Fri, Jan 11, 2008 at 06:43:56PM -0500, Jun'ichi Nomura wrote:
> Remove '--background' option and add '--wait' option to lvconvert.
 
> 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.
 
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

[PATCH 1/11] lvconvert takes '--wait' instead of '--background'

[Jun'ichi Nomura]

Alasdair G Kergon wrote:
> On Fri, Jan 11, 2008 at 06:43:56PM -0500, Jun'ichi Nomura wrote:
>> Remove '--background' option and add '--wait' option to lvconvert.
>  
>> 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.
>  
> 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.

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>

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

[Alasdair G Kergon]

On Fri, Jan 11, 2008 at 06:44:50PM -0500, Jun'ichi Nomura wrote:
> 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-...)
 
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!)

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

where s is the index of the area (stripe)

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

Yes.

> 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().
 
We need to rename that - 'user_seg' could mean something else.

Alasdair
-- 
agk at redhat.com

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

[Alasdair G Kergon]

 /*
+ * 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

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

[Jun'ichi Nomura]

Alasdair,

thanks for the comments.

Alasdair G Kergon wrote:
>  /*
> + * 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

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)

> +/*
> + * 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?

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

> @@ -110,8 +110,8 @@ int check_lv_segments(struct logical_vol
>  		}
>  
> Where is this new list validated?

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

Thanks,
-- 
Jun'ichi Nomura, NEC Corporation of America

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

[Alasdair G Kergon]

On Tue, Jan 15, 2008 at 11:39:13AM -0500, Jun'ichi Nomura wrote:
> 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)

Not sure.  Still thinking.
 
> > +struct lv_segment *seg_using_lv(struct logical_volume *lv)

> Returning NULL is valid if the list is empty.

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?

> > Where is this new list validated?
> Some validation were included in the tests using find_mirror_seg().
> I'll add generic ones.
 
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

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

[Jun'ichi Nomura]

Alasdair G Kergon wrote:
> On Tue, Jan 15, 2008 at 11:39:13AM -0500, Jun'ichi Nomura wrote:
>>> +struct lv_segment *seg_using_lv(struct logical_volume *lv)
> 
>> Returning NULL is valid if the list is empty.
> 
> 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.

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

> 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 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);
  }

>>> Where is this new list validated?
>> Some validation were included in the tests using find_mirror_seg().
>> I'll add generic ones.
>  
> 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.

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

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

[Jun'ichi Nomura]

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>

[Test case 2/2] lvconvert mirror-addition testing

[Jun'ichi Nomura]

Jun'ichi Nomura wrote:
> This is a test script for new lvconvert features
> related to adding mirror(s) to already mirrored LV.

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>

[PATCH 13/14] lvconvert should check first_seg(original_lv)

[Jun'ichi Nomura]

Jun'ichi Nomura wrote:
> Jun'ichi Nomura wrote:
>> If mirror log addition/removal was requested during conversion,
>> lvconvert should manipulate on the original LV, not the top-level LV.
> 
> [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.

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>