[PATCH] Cleanup partial failure of activation

[PATCH] Cleanup partial failure of activation

[Jun'ichi Nomura]

Hi,

If activate_lv() fails, callers won't call deactivate_lv().
However, activate_lv() may succeeded activation partially.

For example, in this case: (with the latest 2.02.29-cvs)
  # dmsetup create --notable vg-lv0_mimage_0
  # lvcreate -l1 -m1 -nlv0 vg

lvcreate fails. "lv0", "lv0_mimage_0", "lv0_mimage_1" and "lv0_mlog"
are once created but removed from the VG metadata. That's ok.
However, maps of vg-lv0 and vg-lv0_mlog remain in kernel.

Attached patch deactivates a partially activated LV so that
callers of activate_lv() don't need to care about partial failure.

Thanks,
-- 
Jun'ichi Nomura, NEC Corporation of America
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cleanup-partial-activation-failure.patch
Type: text/x-patch
Size: 506 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20071121/5f942175/attachment.bin>

[PATCH] Cleanup partial failure of activation

[Alasdair G Kergon]

On Wed, Nov 21, 2007 at 11:00:03AM -0500, Jun'ichi Nomura wrote:
> If activate_lv() fails, callers won't call deactivate_lv().
> However, activate_lv() may succeeded activation partially.
 
> Attached patch deactivates a partially activated LV so that
> callers of activate_lv() don't need to care about partial failure.

But could the deactivation call deactivate something that was active *before*
it was called?  Are you sure this will always accurately return things to the
state they were in prior to the failed call, and not deactivate something that
was previously active?

Alasdair
-- 
agk at redhat.com

[PATCH] Cleanup partial failure of activation

[Jun'ichi Nomura]

Alasdair G Kergon wrote:
> On Wed, Nov 21, 2007 at 11:00:03AM -0500, Jun'ichi Nomura wrote:
>> If activate_lv() fails, callers won't call deactivate_lv().
>> However, activate_lv() may succeeded activation partially.
>  
>> Attached patch deactivates a partially activated LV so that
>> callers of activate_lv() don't need to care about partial failure.
> 
> But could the deactivation call deactivate something that was active *before*
> it was called?  Are you sure this will always accurately return things to the
> state they were in prior to the failed call, and not deactivate something that
> was previously active?

Currently, mirror, snapshot and pvmove are only stacking-type LVs.
They don't allow partial activation.
# Is this assumption correct?

If the above is correct, the tree is either activated or deactivated
as a whole. So, if activation failed, deactivating the LV would
return it to the state before activation is called.


And if, in future, partial activation is allowed, I think the
current activation implementation can't handle this case anyway:
  1. activate partial tree
  2. activate other tree on top of the partial tree
  ..
  3. deactivate the other tree
  <but don't want to deactivate the partial tree>

I'm not whether there actually is such a use case.

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

[PATCH] Cleanup partial failure of activation

[Alasdair G Kergon]

On Wed, Nov 21, 2007 at 09:16:44PM -0500, Jun'ichi Nomura wrote:
> Currently, mirror, snapshot and pvmove are only stacking-type LVs.
> They don't allow partial activation.
> # Is this assumption correct?
> 
> If the above is correct, the tree is either activated or deactivated
> as a whole. So, if activation failed, deactivating the LV would
> return it to the state before activation is called.
 
Have all the code paths been audited?

The general "self-correcting" philosophy throughout the activation code is to
ensure the LV is in a known final state when they return success, and to cope
with a broad range of initial states.

This patch would represent a change in the semantics of the functions,
reducing the range of initial states handled sensibly.

Look at the lv_resume code path, for example.  Wouldn't it be more sensible to
revert to the previous table on failure than to attempt to remove the device?

I don't believe this problem can be resolved with such a tiny patch.

There are two approaches:
  1) have proper reversion code inside the guts of the library activation code
  2) let the caller, which possesses knowledge about the *sequence* of steps
making up the operation, handle reversion sensibly

I expect a mixture of the two - mostly (2) in the places we need it now (quick
and easy and a few cases already done), but eventually more use of (1) which is
technically a better solution (but harder).

Alasdair
-- 
agk at redhat.com

[PATCH] Cleanup partial failure of activation

[Jun'ichi Nomura]

Alasdair G Kergon wrote:
> I don't believe this problem can be resolved with such a tiny patch.
> 
> There are two approaches:
>   1) have proper reversion code inside the guts of the library activation code
>   2) let the caller, which possesses knowledge about the *sequence* of steps
> making up the operation, handle reversion sensibly
> 
> I expect a mixture of the two - mostly (2) in the places we need it now (quick
> and easy and a few cases already done), but eventually more use of (1) which is
> technically a better solution (but harder).

Thanks for the explanation.
I see the point.

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