Re: [PATCH] DM RAID: Add message/status support for changing sync action

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 2986 bytes --]

On Fri, 15 Mar 2013 15:48:23 -0500 Jonathan Brassow <jbrassow@redhat.com>
wrote:

> DM RAID:  Add message/status support for changing sync action
> 
> This patch adds a message interface to dm-raid to allow the user to more
> finely control the sync actions being performed by the MD driver.  This
> gives the user the ability to initiate "check" and "repair" (i.e. scrubbing).
> Two additional fields have been added to the status output to provide more
> information about the type of sync action occurring and the results of those
> actions, specifically: <sync_action> and <mismatch_cnt>.  This is essentially
> the device-mapper way of doing what MD controls through the 'sync_action'
> sysfs file and shows through the 'mismatch_cnt' sysfs file.
> 
> This is a first patch and I still have two concerns:
> 1) in 'raid_messages', reap_sync_thread() is not called when "frozen" or
>    "idle" is issued like it would be in  md.c:action_store() because it is
>    not available to dm-raid.c.  This hasn't prevented the thread from
>    shutting down properly in my tests, but there may be something I'm
>    not considering.

I think the only real need for the reap_sync_thread() call is to make the
change synchronous.  i.e. after "echo frozen > sync_action" completes, you
know that the array actually is frozen, rather that it should be frozen very
soon.  This can be important for code which wants to set 'frozen', then
manipulate the array in a way that would be prevented if it was still
undergoing a resync/recovery/check/etc.

I'm don't know how much I actually depend on that property, but it sounds
like a good one to have.
I would probably be OK to export reap_sync_thread() so that dm-raid can use
it.


> 2) (and this is more of an LVM problem)  The "in-sync ratio" field of the
>    status output has generally been used to determine the progress of the
>    initial synchronization or of rebuilding a device.  "check" and "repair"
>    have not been available options until now.  So, if this ratio also
>    shows the progress of a "check", older versions of LVM would seem to
>    indicate (through the 'sync%' field) that the array is not in-sync, when
>    in fact it is.  Thus, this change might be perceived as a break to
>    backwards compatability.  (However, the old LVM tools would not be able
>    to /issue/ a "check" command and should never run into this.)  I don't
>    see this as a problem for this patch, but I thought it should be
>    brought up and considered.   Obviously, code in LVM will need to be
>    changed to adjust to this - perhaps to show the different types of sync
>    actions that might be occurring.

I see your point.  I don't have any opinion on whether it is a real problem
or just a theoretical one though.

In general, the patch looks good - particular as it includes documentation
changes!  When you get a very you are happy with I'll apply it.

Thanks,
NeilBrown



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]