[PATCH lvconvert 0/11] Additional fixes and changes to lvconvert restructure

[PATCH lvconvert 0/11] Additional fixes and changes to lvconvert restructure

[Jun'ichi Nomura]

These patchset includes bugfixes and feature updates to
the recently committed lvconvert changes.

Bug fixes
[ 1/11] Correct 'const' for _lv_mimage_in_sync()
[ 2/11] Don't start polldaemon if conversion failed in early stage
[ 3/11] Multiple fixes about to-corelog conversion
[ 4/11] Fix possible mirror image corruption 
[ 5/11] Don't insert temporary layer for corelog mirror
[ 6/11] init_mirror_in_sync(0) for corelog mirror addition
[ 7/11] Fix semantic consistency of mirrorlog option

Feature updates
[ 8/11] Add internal 'CONVERTING' flag for LVs under conversion
[ 9/11] Add a new attr field char for the CONVERTING LV
[10/11] Warn if lvconvert needs activation to complete conversion
[11/11] Start lvconvert polldaemon on activation (like pvmove)

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.
  - Review and fix remove_mirror_images() so that 'removable_pvs'
    filtering works on stacked mirror.
  - Review the suspend code path. I think 'noflush' should be
    avoided for lower-level mirrors. Otherwise the upper-level mirror
    can't suspend.
  - 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'

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

[PATCH lvconvert 1/11] Correct 'const' for _lv_mimage_in_sync()

[Jun'ichi Nomura]

The patch removes this warning:

report/report.c:315: warning: passing argument 1 of '_lv_mimage_in_sync' discards qualifiers from pointer target type

Thanks,
-- 
Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-const-correctness-for-lv_mimage_in_sync.patch
Type: text/x-patch
Size: 597 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080104/4e2c5353/attachment.bin>

[PATCH lvconvert 2/11] Don't start polldaemon if conversion failed in early stage

[Jun'ichi Nomura]

If lvconvert fails in the early stage, we shouldn't invoke
polldaemon.

Thanks,
-- 
Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-dont-poll-if-failed.patch
Type: text/x-patch
Size: 517 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080104/2e852642/attachment.bin>

[PATCH lvconvert 3/11] Multiple fixes about to-corelog conversion

[Jun'ichi Nomura]

_remove_mirror_images() is the core function used for both
mirror image removal and mirror log removal.
However, it has some bugs and to-corelog conversion didn't work
at all.

The patches fixes them at once.
Please see the patch header for detailed information about
each bug.

Thanks,
-- 
Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-corelog-convert.patch
Type: text/x-patch
Size: 4452 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080104/2f648174/attachment.bin>

[PATCH lvconvert 4/11] Fix possible mirror image corruption

[Jun'ichi Nomura]

collapse_mirrored_lv() is a function to move mimage LVs from
temporary sync layer to the original LV after sync completion.
However, the code did resume_lv between the removal of mimage LVs
and addition of them. As a result, it's possible to make the
mimage LVs out-of-sync while mirror log says in-sync.

Thanks,
-- 
Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-atomic-remove-and-merge.patch
Type: text/x-patch
Size: 5279 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080104/e6a4d298/attachment.bin>

[PATCH lvconvert 5/11] Don't insert temporary layer for corelog mirror

[Jun'ichi Nomura]

lvconvert inserts a temporary layer so that it can sync newly added
mirrors while keeping the original mirror in-sync.
However, LVM2 manages in-sync status for corelog mirror by
the global _mirror_in_sync variable.
So the insertion just duplicates the sync effort.

If the original mirror is corelog, we can just add mimage LV to it
and restart sync.

Thanks,
-- 
Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-dont-insert-tmplayer-for-corelog-mirror.patch
Type: text/x-patch
Size: 1834 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080104/ebdb32d8/attachment.bin>

[PATCH lvconvert 6/11] init_mirror_in_sync(0) for corelog mirror addition

[Jun'ichi Nomura]

Since corelog mirror is controlled by the global _mirror_in_sync,
lv_add_mirrors() should explicitly reset it when adding mirrors
with corelog.
Otherwise, it might be set 'in-sync' in some other places and
skip sync process.

Thanks,
-- 
Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-init-sync-status-for-corelog.patch
Type: text/x-patch
Size: 844 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080104/c58e3f54/attachment.bin>

[PATCH lvconvert 7/11] Fix semantic consistency of mirrorlog option

[Jun'ichi Nomura]

'--mirrorlog'/'--corelog' options of lvconvert are used to
add/remove disklog of mirrored LV.
However, it had different meanings for "adding mirrors to mirror",
i.e. the options specify whether to use disklog for conversion.
The patch corrects it.

-- 
Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-semantic-consistency-of-mirrorlog-option.patch
Type: text/x-patch
Size: 2103 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080104/53355b99/attachment.bin>

[PATCH lvconvert 8/11] Add internal 'CONVERTING' flag for LVs under conversion

[Jun'ichi Nomura]

Add 'CONVERTING' flag for LVs under conversion.
Reporting code and daemon restarting code will use it.

Thanks,
-- 
Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvconvert-add-flag.patch
Type: text/x-patch
Size: 2571 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080104/6377cdc4/attachment.bin>

[PATCH lvconvert 9/11] Add a new attr field char for the CONVERTING LV

[Jun'ichi Nomura]

The patch changes the reporting format to show the conversion
structure.

# lvs -o+convert_lv vg
  LV    VG   Attr   LSize Origin Snap%  Move Log Copy%  Convert          
  lvol0 vg   cwi--- 1.00G                               lvol0_mimagetmp_2

'c' in the 'Attr' field means 'LV under conversion'.
'Convert' field points to the temporary LV for the conversion.

Might be better to add a new char for Attr field than
using volume-type char.

Thanks,
-- 
Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvconvert-add-attr-char-for-converting.patch
Type: text/x-patch
Size: 3974 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080104/ed53e075/attachment.bin>

[PATCH lvconvert 10/11] Warn if lvconvert needs activation to complete conversion

[Jun'ichi Nomura]

lvconvert requires activation of the LV, when it adds mirrors to
already mirrored LV. The patch makes it to print a message when
the LV was not activated.
(Alternative may force activation on lvconvert.)

Thanks,
-- 
Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvconvert-check-active.patch
Type: text/x-patch
Size: 974 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080104/b3f50ed3/attachment.bin>

[PATCH lvconvert 11/11] Start lvconvert polldaemon on activation (like pvmove)

[Jun'ichi Nomura]

polldaemon should be invoked on activation
if the LV is under conversion.

Thanks,
-- 
Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvconvert-start-polling-on-activation.patch
Type: text/x-patch
Size: 2390 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20080104/c729f9a8/attachment.bin>

[PATCH lvconvert 0/11] Additional fixes and changes to lvconvert restructure

[Alasdair G Kergon]

On Fri, Jan 04, 2008 at 09:55:20AM -0500, Jun'ichi Nomura wrote:
> Bug fixes
> [ 1/11] Correct 'const' for _lv_mimage_in_sync()
> [ 2/11] Don't start polldaemon if conversion failed in early stage
> [ 3/11] Multiple fixes about to-corelog conversion
> [ 4/11] Fix possible mirror image corruption 
> [ 5/11] Don't insert temporary layer for corelog mirror
> [ 6/11] init_mirror_in_sync(0) for corelog mirror addition
> [ 7/11] Fix semantic consistency of mirrorlog option
> 
> Feature updates
> [ 8/11] Add internal 'CONVERTING' flag for LVs under conversion
> [ 9/11] Add a new attr field char for the CONVERTING LV
> [10/11] Warn if lvconvert needs activation to complete conversion

Not sure here - should we perform the activation immediately?

> [11/11] Start lvconvert polldaemon on activation (like pvmove)
 
All applied, thanks.

Alasdair
-- 
agk at redhat.com

[PATCH lvconvert 0/11] Additional fixes and changes to lvconvert restructure

[Jun'ichi Nomura]

Alasdair G Kergon wrote:
> On Fri, Jan 04, 2008 at 09:55:20AM -0500, Jun'ichi Nomura wrote:
>> [10/11] Warn if lvconvert needs activation to complete conversion
> 
> Not sure here - should we perform the activation immediately?

I think it's ok to leave it for the users decision.
IMO, if the LV is not active when lvconvert is executed,
there is likely a reason the LV is not activated.

It followed the way linear-to-mirror conversion used to work.
If linear inactive LV is converted to mirror, syncing doesn't
start until it's activated.
(With that regard, maybe I should have let lvconvert take
  '--wait' rather than '--background' and return immediately
 by default.)

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

[PATCH lvconvert 0/11] Additional fixes and changes to lvconvert restructure

[Jun'ichi Nomura]

Jun'ichi Nomura wrote:
> Other things I would like to work on next are:
..
>   - Review and fix remove_mirror_images() so that 'removable_pvs'
>     filtering works on stacked mirror.

For remove_mirror_images() to properly work with stacked
mirror, I'm going to allow "mirror" segment with 1 area.

'removable_pvs' is used for selectively removing mimage LVs.
Especially, it's important for mirror reconfiguration
after device failure.
However, the current code can't handle the case where
"_mimagetmp" LV turns to become single.

I've been thinking about this situation:

  1 mirror is added to 2-way mirrored LV

      lv0
        +-- lv0_mimagetmp_2
        |      +-- lv0_mimage_0
        |      +-- lv0_mimage_1
        |      +-- lv0_mlog
        +-- lv0_mimage_2

  lv0_mimage_2 is still syncing.
  lv0_mlog contains sync status between lv0_mimage_0 and
  lv0_mimage_1.

If lv0_mimage_1 is removed, what should we do?

What the current code will do is:

      lv0
        +-- lv0_mimagetmp_2
        +-- lv0_mimage_2

Of course, it's not correct.

IMO, first, the lv0_mimage_1 is removed and lv0_mlog is
set to mark in-sync,

      lv0
        +-- lv0_mimagetmp_2
        |      +-- lv0_mimage_0
        |      +-- lv0_mlog
        +-- lv0_mimage_2

then, polldaemon converts it after sync completion,

      lv0
        +-- lv0_mimage_0
        +-- lv0_mimage_2
        +-- lv0_mlog

So in the intermediate state, "mirror" segment will have
only 1 area. There's already a code related to pvmove that
uses "linear" target for "mirror" segment.
It's not enough to convert to "linear" segment because
that makes mirror parameters (log LV, region size, etc.) lost.

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

Suspend is done top-down. So 'noflush' should not be a problem.

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