Re: [RFC] [PATCH] lvm2: mirroredlog support

[malahal@us.ibm.com]

Jonathan Brassow [jbrassow@redhat.com] wrote:
> Looking through the code before applying your patch, it seems that someone 
> has already thought about this issue - even if it hasn't been implemented.  
> For example, already in the top most function used to create mirrors, 
> 'lv_add_mirrors', we see a 'log_count' parameter.  That parameter can be 
> traced down through 'add_mirror_images/log', '_set_up_mirror_log', and even 
> the allocation functions.  In fact, the first comment in 'add_mirror_log' 
> is /* Unimplemented features */, followed by a check to see if 'log_count > 
> 1'.  Your patch seems to ignore 'log_count' and create new parameters (like 
> mirroredlog), which seem unnecessary to me...

Yes, I saw the log_count but not sure if I can use that for my purpose.
You can have multiple logs (reserved/standby mode implementations)
without the logs themselves mirrored.

> I don't understand why any 
> of the new parameters to the functions are necessary, can you explain?  [I 
> can see new parameters for '_create_mirror_log' though, as it doesn't seem 
> to maintain the 'log_count' parameter - but you didn't do work in that 
> function.]

_set_up_mirror_log() needs those extra parameters while calling
lv_extend() in the patch.

> You also seem to have violated the allocation policies by ignoring the line 
> of work that has been done up to '_set_up_mirror_log' by simply calling 
> 'add_mirror_images'.  This works, but it is oversimplified, I think.  You 
> can see this is incorrect by simply testing:
> prompt> lvcreate -m1 -L 500M -n lv vg /dev/sd[xy]1  # will fail because 
> there are only two devices
> prompt> lvcreate ... --mirroredlog /dev/sd[xy]1 # should fail, but succeeds 
> due to ignoring previous allocation work
> You may wish to push your enhancements into '_[create | init]_mirror_log'?

Thank you for spotting it. I will look into your suggestion.

> Additionally, the 'log_count' parameter is more general than 'mirroredlog' 
> and can support more log types.  Consider the following:
> --mirrorlog core => (log_count = 0)
> --mirrorlog disk => (log_count = 1)
> --mirrorlog redundant => (log_count = 2)
> --mirrorlog fully_redundant => (log_count = # of mirror legs)
> You are looking to add "redundant" (you can call it "mirrored" if you 
> like), but if we use 'log_count' in a general way, we get "fully_redundant" 
> (almost) for free.

The current patch actually creates fully_redundant log based what you
described.

If we are not going to use log_count for anything else other than
creating "mirrored" logs, I can use it. I remember Heinz implementing a
log device per mirror leg but the logs not not mirrored at all.

Alasdair, any comments?

Thanks, Malahal.