Re: [PATCH 00/24] Clustered MD RAID1

[NeilBrown <neilb@suse.de>]

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

On Thu, 18 Dec 2014 10:14:57 -0600 Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:

> Hello,
> 
> This is an attempt to make MD-RAID cluster-aware. The advantage of
> redundancy can help highly available systems to improve uptime.
> Currently, the implementation is limited to RAID1 but with further work
> (and some positive feedback), we could extend this to other compatible
> RAID scenarios.
> 
> The design document (first patch) is pretty descriptive of how
> the md has been made cluster-aware and how DLM is used to safeguard data
> and communication.
> 
> This work requires some patches to the mdadm tool [1]
> 
> A quick howto:
> 
> 1. With your corosync/pacemaker based cluster running execute:
> # mdadm --create md0 --bitmap=clustered --raid-devices=2 --level=mirror --assume-clean /dev/sda /dev/sdb
> 
> 2. On other nodes, issue:
> # mdadm --assemble md0 /dev/sda /dev/sdb
> 
> References:
> [1] mdadm tool changes: https://github.com/goldwynr/mdadm branch:cluster-md
> [2] Patches against stable 3.14: https://github.com/goldwynr/linux branch: cluster-md-devel
> 
> Regards,
> 


hi Goldwyn,
 thanks for these - and sorry for the long delay.   Lots of leave over
 southern summer, and the lots of email etc to deal with.

This patch set is very close and I am tempted to just apply it and then
fix things up with subsequent patches.  In order to allow that, could you
please:
  - rebase against current upstream
  - fix the checkpatch.pl errors and warnings.
    The "WARNING: line over 80 characters" are often a judgement call
    so I'm not particularly worried about those.  Most, if not all, of
    the others should be followed just to have consistent layout.

Then I'll queue them up for 3.21, providing I don't find anything that would
hurt non-cluster usage ....
 On that topic: why initialise rv to -EINVAL in "metadata_update sends
 message...".  That looks wrong.

I noticed that a number of times a patch will revert something that a
previous patch added.  It would be much nicer to fold these changes back into
the original patch.  Often this is just extra blank lines, but occasionally
variable names are changed (md -> mddev).  It should be given the final name
when introduced.  Every chunk in every patch should be directly relevant to
that patch.

Some other issues, that could possibly be fixed up afterwards:

 - Is a clustername 64 bytes or 63 bytes?  I would have thought 64,
   but the use of strlcpy make is 63 plus a nul.  Is that really what is
   wanted?

 - Based on https://lkml.org/lkml/2012/10/23/580 it might be good to add
   "default n" to Kconfig, and possible add a WARN() if anyone tries to use
   the code.

 - I'm a bit concerned about the behaviour on node failure.
   When a node fails, two things must happen w.r.t the bits in that node's
   bitmap.
   1/ The corresponding regions of the array need to be resynced.  You do have
      code to do this.
   2/ Other nodes must avoid read-balancing on those regions until the
      resync has completed.  

   You do have code for this second bit, but it looks wrong.  It avoids
   read-balancing if ->area_resyncing().  That isn't sufficient.
   The "area_resyncing" is always (I assume) a relatively small region of
   the array which will be completely resynced quite quickly.  It must be
   because writes are blocked to this area.  However the region in which
   we must disable re-balancing can be much larger.  It covers *all* bits
   that are set in any unsynced bitmap.  So it isn't just the area that is
   currently being synced, but all areas that will be synced.

 - I think md_reload_sb() might be too simple.  It probably should check that
   nothing serious has changed.  The "mddev->raid_disks = 0" look suspicious.
   I'll have to think about this a bit more.

That's all I can see for now.  I'll have another look once I have it all in my tree.

Thanks,
NeilBrown

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]