[Patch 00/17] Autorebuild

[Patch 00/17] Autorebuild

[Czarnowska, Anna]

This is updated series of patches forming autorebuild functionality in mdadm monitor based on new policy code.

Autorebuild Monitoring application:
Autorebuild monitor is part of monitor application (mdadm -F). In the current code of mdadm monitor autorebuild feature was based on spare group assignment in mdadm.conf file and worked only for native metadata. This code has been retained for compatibility with old config format.
The new autorebuild implementation works also for external metadata types. It uses the concept of domains in mdadm.conf introduced by Neil Brown. 
Monitoring application periodically checks the state of MD active arrays and triggers a rebuild if there are eligible spare disks in other arrays/containers.
Degraded arrays are checked one by one. If there is a spare disk in other array/container that matches the domain of the degraded array and the domain action allows for spare sharing the spare is moved using existing Manage_subdevs function. If the addition fails, the spare device is moved back to the original container and next potential spare is tried. The process is repeated until all arrays are checked and the process is put into a sleep state for a configured period.

New option --no-sharing has been added to Monitor mode to be able to run monitoring only (without moving spares). This is recommended when many instances of monitor are to be run on the same set of devices.
Spare sharing is allowed in only one instance of Monitor running with --scan option. User is still able to start Monitoring functions in multiple instances without --scan option.

The autorebuild build-in assumptions are:
1\spares are shared between the arrays of the same metadata 2\spares are moved only from containers/volumes that are not degraded 3\spares are moved to containers/volumes lacking a *good* spare (size)

Anna Czarnowska
Przemyslaw Hawrylewicz-Czarnowski
Marcin Labun

0001-added-path-path_id-to-give-the-information-on-the-pa.patch
0002-Update-of-udev-rules-to-support-IMSM-devices.patch
0003-extension-of-IncrementalRemove-to-store-location-pat.patch
0004-Incremental-for-bare-disks-implementation-of-spare-s.patch
0005-Util-get-device-size-from-id.patch
0006-Monitor-set-err-on-arrays-not-in-mdstat.patch
0007-Monitor-spare-group-based-spare-sharing-moved-to-sep.patch
0008-mdadm-added-no-sharing-option-for-Monitor-mode.patch
0009-Monitor-avoid-skipping-checks-on-external-arays.patch
0010-Monitor-include-containers-in-scan-mode.patch
0011-Monitor-link-containers-with-subarrays-in-statelist.patch
0012-imsm-create-mdinfo-list-of-disks-in-a-container-from.patch
0013-Monitor-autorebuild-functionality-added.patch
0014-Monitor-Respect-policy-in-auto-rebuild-in-mdadm-moni.patch
0015-Monitor-more-accurate-size-check-when-looking-for-sp.patch
0016-IMSM-Fix-problem-in-mdmon-monitor-of-using-removed-d.patch
0017-Policy-is-aware-of-metadata-disk-s-controller-domain.patch

Incremental.c      |  230 +++++++++++++++---
Makefile           |    3 +
Monitor.c          |  691 ++++++++++++++++++++++++++++++++++++++++++++--------
ReadMe.c           |    4 +
managemon.c        |   38 +++
mdadm.c            |   29 ++-
mdadm.h            |   49 ++++-
policy.c           |  134 +++++++++-
super-intel.c      |  274 ++++++++++++++++++---
udev-md-raid.rules |    7 +-
util.c             |   23 ++
11 files changed, 1290 insertions(+), 192 deletions(-)
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
z siedziba w Gdansku
ul. Slowackiego 173
80-298 Gdansk

Sad Rejonowy Gdansk Polnoc w Gdansku, 
VII Wydzial Gospodarczy Krajowego Rejestru Sadowego, 
numer KRS 101882

NIP 957-07-52-316
Kapital zakladowy 200.000 zl

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [Patch 00/17] Autorebuild

[Neil Brown]

On Fri, 29 Oct 2010 15:13:40 +0100
"Czarnowska, Anna" <anna.czarnowska@intel.com> wrote:

> This is updated series of patches forming autorebuild functionality in mdadm monitor based on new policy code.

I Anna and all,

I have decided that the best way forward is for me to apply all of your
patches and fix them up on the way.  It turned out there was quite a few
changes that I wanted to make that I only discovered while examining the
patches very closely, so this seems to be a very useful exercise.

I haven't finished yet, but you can see my current state - which is up to
about patch 11 in your series - in my devel-3.2 branch.

Probably the most significant change so far is the interpretation of
action=spare-same-slot.  I have defined that in a way that is *more*
permissive than action=spare, where you had it less permissive.

The comment for that change set is below.
The new list of policy actions is:

enum policy_action {
	act_default,
	act_include,
	act_re_add,
	act_spare,	/* This only applies to bare devices */
	act_spare_same_slot, /* this allows non-bare devices,
			      * but only if recent removal */
	act_force_spare, /* this allow non-bare devices in any case */
	act_err
};

I'll hopefully get the rest of your patches done tomorrow and then I might
start on Adam... and then on the changes that I want to make.
If you can do some testing once enough code is in place, that would be great.
Thanks,
NeilBrown

commit a2191ce8af4aa178d62df759fab47ef4dc8e6f67
Author: NeilBrown <neilb@suse.de>
Date:   Wed Nov 17 12:46:35 2010 +1100

    Add action=spare-same-slot policy.
    
    When "mdadm -I" is given a device with no metadata, mdadm tries to add
    it as a 'spare' somewhere based on policy.
    
    This patch changes the behaviour in two ways:
    
    1/ If the device is at a 'path' where a previous device was removed
      from an array or container, then we preferentially add the spare to
      that array or container.
    
    2/ Previously only 'bare' devices were considered for adding as
      spares.  Now if action=spare-same-slot is active, we will add
      non-bare devices, but *only* if the path was previously in use
      for some array, and the device will only be added to that array.
    
    Based on code
      From: Przemyslaw Czarnowski <przemyslaw.hawrylewicz.czarnowski@intel.com>
    
    Signed-off-by: Przemyslaw Czarnowski <przemyslaw.hawrylewicz.czarnowski@intel.com>
    Signed-off-by: NeilBrown <neilb@suse.de>


> 
> Autorebuild Monitoring application:
> Autorebuild monitor is part of monitor application (mdadm -F). In the current code of mdadm monitor autorebuild feature was based on spare group assignment in mdadm.conf file and worked only for native metadata. This code has been retained for compatibility with old config format.
> The new autorebuild implementation works also for external metadata types. It uses the concept of domains in mdadm.conf introduced by Neil Brown. 
> Monitoring application periodically checks the state of MD active arrays and triggers a rebuild if there are eligible spare disks in other arrays/containers.
> Degraded arrays are checked one by one. If there is a spare disk in other array/container that matches the domain of the degraded array and the domain action allows for spare sharing the spare is moved using existing Manage_subdevs function. If the addition fails, the spare device is moved back to the original container and next potential spare is tried. The process is repeated until all arrays are checked and the process is put into a sleep state for a configured period.
> 
> New option --no-sharing has been added to Monitor mode to be able to run monitoring only (without moving spares). This is recommended when many instances of monitor are to be run on the same set of devices.
> Spare sharing is allowed in only one instance of Monitor running with --scan option. User is still able to start Monitoring functions in multiple instances without --scan option.
> 
> The autorebuild build-in assumptions are:
> 1\spares are shared between the arrays of the same metadata 2\spares are moved only from containers/volumes that are not degraded 3\spares are moved to containers/volumes lacking a *good* spare (size)
> 
> Anna Czarnowska
> Przemyslaw Hawrylewicz-Czarnowski
> Marcin Labun
> 
> 0001-added-path-path_id-to-give-the-information-on-the-pa.patch
> 0002-Update-of-udev-rules-to-support-IMSM-devices.patch
> 0003-extension-of-IncrementalRemove-to-store-location-pat.patch
> 0004-Incremental-for-bare-disks-implementation-of-spare-s.patch
> 0005-Util-get-device-size-from-id.patch
> 0006-Monitor-set-err-on-arrays-not-in-mdstat.patch
> 0007-Monitor-spare-group-based-spare-sharing-moved-to-sep.patch
> 0008-mdadm-added-no-sharing-option-for-Monitor-mode.patch
> 0009-Monitor-avoid-skipping-checks-on-external-arays.patch
> 0010-Monitor-include-containers-in-scan-mode.patch
> 0011-Monitor-link-containers-with-subarrays-in-statelist.patch
> 0012-imsm-create-mdinfo-list-of-disks-in-a-container-from.patch
> 0013-Monitor-autorebuild-functionality-added.patch
> 0014-Monitor-Respect-policy-in-auto-rebuild-in-mdadm-moni.patch
> 0015-Monitor-more-accurate-size-check-when-looking-for-sp.patch
> 0016-IMSM-Fix-problem-in-mdmon-monitor-of-using-removed-d.patch
> 0017-Policy-is-aware-of-metadata-disk-s-controller-domain.patch
> 
> Incremental.c      |  230 +++++++++++++++---
> Makefile           |    3 +
> Monitor.c          |  691 ++++++++++++++++++++++++++++++++++++++++++++--------
> ReadMe.c           |    4 +
> managemon.c        |   38 +++
> mdadm.c            |   29 ++-
> mdadm.h            |   49 ++++-
> policy.c           |  134 +++++++++-
> super-intel.c      |  274 ++++++++++++++++++---
> udev-md-raid.rules |    7 +-
> util.c             |   23 ++
> 11 files changed, 1290 insertions(+), 192 deletions(-)
> ---------------------------------------------------------------------
> Intel Technology Poland sp. z o.o.
> z siedziba w Gdansku
> ul. Slowackiego 173
> 80-298 Gdansk
> 
> Sad Rejonowy Gdansk Polnoc w Gdansku, 
> VII Wydzial Gospodarczy Krajowego Rejestru Sadowego, 
> numer KRS 101882
> 
> NIP 957-07-52-316
> Kapital zakladowy 200.000 zl
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.

RE: [Patch 00/17] Autorebuild

[Labun, Marcin]

> -----Original Message-----
> From: Neil Brown [mailto:neilb@suse.de]
> Sent: Wednesday, November 17, 2010 11:23 AM
> To: Czarnowska, Anna
> Cc: linux-raid@vger.kernel.org; Neubauer, Wojciech; Williams, Dan J;
> Ciechanowski, Ed; Labun, Marcin; Hawrylewicz Czarnowski, Przemyslaw
> Subject: Re: [Patch 00/17] Autorebuild
> 
> 
> I'll hopefully get the rest of your patches done tomorrow and then I
> might
> start on Adam... and then on the changes that I want to make.
> If you can do some testing once enough code is in place, that would be
> great.
We will start testing of hot-plug and are waiting for auto-rebuild staff integrated on your devel branch do run the rest of tests.
Marcin

RE: [Patch 00/17] Autorebuild

[Czarnowska, Anna]

Hi Neil,
I started testing new code today. Just the Incremental part.

There are few problems:
1. Cookie file is cleared before it is read so spare-same-slot can't work. It should be just open for reading. (probably a typo)
2. Container uuid instead of subarray uuid is written in cookie file, so for ddf it may not be clear which subarray used the slot.
3. Incremental fail does not work for external metadata. Przemek's original patch did fail the disk in subarrays. Now Manage_subdevs tries to fail a disk in container while subarray is expected. Do you intend to change Manage_subdevs to take a container?
4. With spare-same-slot when there is a cookie and disk has no metadata then we probably shouldn't look at domains. Just add.

This is all for now.
Regards
Anna


> -----Original Message-----
> From: Neil Brown [mailto:neilb@suse.de]
> Sent: Wednesday, November 17, 2010 11:23 AM
> To: Czarnowska, Anna
> Cc: linux-raid@vger.kernel.org; Neubauer, Wojciech; Williams, Dan J;
> Ciechanowski, Ed; Labun, Marcin; Hawrylewicz Czarnowski, Przemyslaw
> Subject: Re: [Patch 00/17] Autorebuild
> 
> On Fri, 29 Oct 2010 15:13:40 +0100
> "Czarnowska, Anna" <anna.czarnowska@intel.com> wrote:
> 
> > This is updated series of patches forming autorebuild functionality
> in mdadm monitor based on new policy code.
> 
> I Anna and all,
> 
> I have decided that the best way forward is for me to apply all of your
> patches and fix them up on the way.  It turned out there was quite a
> few
> changes that I wanted to make that I only discovered while examining
> the
> patches very closely, so this seems to be a very useful exercise.
> 
> I haven't finished yet, but you can see my current state - which is up
> to
> about patch 11 in your series - in my devel-3.2 branch.
> 
> Probably the most significant change so far is the interpretation of
> action=spare-same-slot.  I have defined that in a way that is *more*
> permissive than action=spare, where you had it less permissive.
> 
> The comment for that change set is below.
> The new list of policy actions is:
> 
> enum policy_action {
> 	act_default,
> 	act_include,
> 	act_re_add,
> 	act_spare,	/* This only applies to bare devices */
> 	act_spare_same_slot, /* this allows non-bare devices,
> 			      * but only if recent removal */
> 	act_force_spare, /* this allow non-bare devices in any case */
> 	act_err
> };
> 
> I'll hopefully get the rest of your patches done tomorrow and then I
> might
> start on Adam... and then on the changes that I want to make.
> If you can do some testing once enough code is in place, that would be
> great.
> Thanks,
> NeilBrown
> 
> commit a2191ce8af4aa178d62df759fab47ef4dc8e6f67
> Author: NeilBrown <neilb@suse.de>
> Date:   Wed Nov 17 12:46:35 2010 +1100
> 
>     Add action=spare-same-slot policy.
> 
>     When "mdadm -I" is given a device with no metadata, mdadm tries to
> add
>     it as a 'spare' somewhere based on policy.
> 
>     This patch changes the behaviour in two ways:
> 
>     1/ If the device is at a 'path' where a previous device was removed
>       from an array or container, then we preferentially add the spare
> to
>       that array or container.
> 
>     2/ Previously only 'bare' devices were considered for adding as
>       spares.  Now if action=spare-same-slot is active, we will add
>       non-bare devices, but *only* if the path was previously in use
>       for some array, and the device will only be added to that array.
> 
>     Based on code
>       From: Przemyslaw Czarnowski
> <przemyslaw.hawrylewicz.czarnowski@intel.com>
> 
>     Signed-off-by: Przemyslaw Czarnowski
> <przemyslaw.hawrylewicz.czarnowski@intel.com>
>     Signed-off-by: NeilBrown <neilb@suse.de>
> 
> 
> >
> > Autorebuild Monitoring application:
> > Autorebuild monitor is part of monitor application (mdadm -F). In the
> current code of mdadm monitor autorebuild feature was based on spare
> group assignment in mdadm.conf file and worked only for native
> metadata. This code has been retained for compatibility with old config
> format.
> > The new autorebuild implementation works also for external metadata
> types. It uses the concept of domains in mdadm.conf introduced by Neil
> Brown.
> > Monitoring application periodically checks the state of MD active
> arrays and triggers a rebuild if there are eligible spare disks in
> other arrays/containers.
> > Degraded arrays are checked one by one. If there is a spare disk in
> other array/container that matches the domain of the degraded array and
> the domain action allows for spare sharing the spare is moved using
> existing Manage_subdevs function. If the addition fails, the spare
> device is moved back to the original container and next potential spare
> is tried. The process is repeated until all arrays are checked and the
> process is put into a sleep state for a configured period.
> >
> > New option --no-sharing has been added to Monitor mode to be able to
> run monitoring only (without moving spares). This is recommended when
> many instances of monitor are to be run on the same set of devices.
> > Spare sharing is allowed in only one instance of Monitor running with
> --scan option. User is still able to start Monitoring functions in
> multiple instances without --scan option.
> >
> > The autorebuild build-in assumptions are:
> > 1\spares are shared between the arrays of the same metadata 2\spares
> are moved only from containers/volumes that are not degraded 3\spares
> are moved to containers/volumes lacking a *good* spare (size)
> >
> > Anna Czarnowska
> > Przemyslaw Hawrylewicz-Czarnowski
> > Marcin Labun
> >
> > 0001-added-path-path_id-to-give-the-information-on-the-pa.patch
> > 0002-Update-of-udev-rules-to-support-IMSM-devices.patch
> > 0003-extension-of-IncrementalRemove-to-store-location-pat.patch
> > 0004-Incremental-for-bare-disks-implementation-of-spare-s.patch
> > 0005-Util-get-device-size-from-id.patch
> > 0006-Monitor-set-err-on-arrays-not-in-mdstat.patch
> > 0007-Monitor-spare-group-based-spare-sharing-moved-to-sep.patch
> > 0008-mdadm-added-no-sharing-option-for-Monitor-mode.patch
> > 0009-Monitor-avoid-skipping-checks-on-external-arays.patch
> > 0010-Monitor-include-containers-in-scan-mode.patch
> > 0011-Monitor-link-containers-with-subarrays-in-statelist.patch
> > 0012-imsm-create-mdinfo-list-of-disks-in-a-container-from.patch
> > 0013-Monitor-autorebuild-functionality-added.patch
> > 0014-Monitor-Respect-policy-in-auto-rebuild-in-mdadm-moni.patch
> > 0015-Monitor-more-accurate-size-check-when-looking-for-sp.patch
> > 0016-IMSM-Fix-problem-in-mdmon-monitor-of-using-removed-d.patch
> > 0017-Policy-is-aware-of-metadata-disk-s-controller-domain.patch
> >
> > Incremental.c      |  230 +++++++++++++++---
> > Makefile           |    3 +
> > Monitor.c          |  691
> ++++++++++++++++++++++++++++++++++++++++++++--------
> > ReadMe.c           |    4 +
> > managemon.c        |   38 +++
> > mdadm.c            |   29 ++-
> > mdadm.h            |   49 ++++-
> > policy.c           |  134 +++++++++-
> > super-intel.c      |  274 ++++++++++++++++++---
> > udev-md-raid.rules |    7 +-
> > util.c             |   23 ++
> > 11 files changed, 1290 insertions(+), 192 deletions(-)
> > ---------------------------------------------------------------------
> > Intel Technology Poland sp. z o.o.
> > z siedziba w Gdansku
> > ul. Slowackiego 173
> > 80-298 Gdansk
> >
> > Sad Rejonowy Gdansk Polnoc w Gdansku,
> > VII Wydzial Gospodarczy Krajowego Rejestru Sadowego,
> > numer KRS 101882
> >
> > NIP 957-07-52-316
> > Kapital zakladowy 200.000 zl
> >
> > This e-mail and any attachments may contain confidential material for
> > the sole use of the intended recipient(s). Any review or distribution
> > by others is strictly prohibited. If you are not the intended
> > recipient, please contact the sender and delete all copies.

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
z siedziba w Gdansku
ul. Slowackiego 173
80-298 Gdansk

Sad Rejonowy Gdansk Polnoc w Gdansku, 
VII Wydzial Gospodarczy Krajowego Rejestru Sadowego, 
numer KRS 101882

NIP 957-07-52-316
Kapital zakladowy 200.000 zl

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Devel 3.2 branch issues

[Czarnowska, Anna]

Hi Neil,
Our validation team have reported problems with assembly on the devel 3.2 branch.
I have verified that currently it is not possible to assemble any array.

Patch: Assemble - avoid including wayward devices
It does not affect native metadata but breaks assembly of external arrays.
Only one disk is assembled for any raid level. 

After patch: super_by_fd: return subarray info explicitly
Assembly becomes much slower.

Patch: Assemble: small cleanup of error checking
Breaks assembly for all metadata types. Nothing assembles after it is applied.

These are just early modifications of Assemble.c. The impact of further changes
can't be verified at the moment. 
Are you aware of the above issues? This is stopping our further validation.
I also mentioned issues with Incremental in previous mail.
When are you planning to submit the rest of modified autorebuild code?

Regards
Anna

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
z siedziba w Gdansku
ul. Slowackiego 173
80-298 Gdansk

Sad Rejonowy Gdansk Polnoc w Gdansku, 
VII Wydzial Gospodarczy Krajowego Rejestru Sadowego, 
numer KRS 101882

NIP 957-07-52-316
Kapital zakladowy 200.000 zl

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: Devel 3.2 branch issues

[Neil Brown]

On Fri, 19 Nov 2010 12:43:20 +0000
"Czarnowska, Anna" <anna.czarnowska@intel.com> wrote:

> Hi Neil,
> Our validation team have reported problems with assembly on the devel 3.2 branch.
> I have verified that currently it is not possible to assemble any array.
> 
> Patch: Assemble - avoid including wayward devices
> It does not affect native metadata but breaks assembly of external arrays.
> Only one disk is assembled for any raid level. 

Thanks - fixed.

> 
> After patch: super_by_fd: return subarray info explicitly
> Assembly becomes much slower.

Yep .. I was calling 'strcpy' with a NULL as the source - bad.
Fixed, though a subsequent patch removed the strcpy anyway.


> 
> Patch: Assemble: small cleanup of error checking
> Breaks assembly for all metadata types. Nothing assembles after it is applied.
> 

I'm not sure this is true, but the test/03* tests of assembly certainly fail.
I've fixed that.  Thanks.


> These are just early modifications of Assemble.c. The impact of further changes
> can't be verified at the moment. 
> Are you aware of the above issues? This is stopping our further validation.
> I also mentioned issues with Incremental in previous mail.
> When are you planning to submit the rest of modified autorebuild code?

Shortly .. 

by the way, some of the changes in you of the patches you sent have not been
included in any form.  They include:

- the getinfo_super_disks method.  I couldn't see why you need this.  All the
  info about the state of the arrays should already be available.
  If there is something that you need that we don't have, please explain and
  we can see how best to add it back in.

- min_active_disk_size_in_array.  I don't think the minimum current size is
  really a good guide.  I've kept the code for letting the metadata handler
  check the size, but anything beyond that should be done with domains I
  think.
  E.g have a domain '2G-or-greater' which is assigned to all 2G or greater
  devices.  Then anything smaller will automatically be excluded from arrays
  with those devices.

- The remove_from_super method.  As Dan pointed out there seems to be
  something wrong there so I chose to just leave it out for now.  If you
  could explain again what is needed, we can find the best way to add that
  functionality.

Thanks,
NeilBrown

RE: Devel 3.2 branch issues

[Labun, Marcin]

> - the getinfo_super_disks method.  I couldn't see why you need this.
> All the
>   info about the state of the arrays should already be available.
>   If there is something that you need that we don't have, please
> explain and
>   we can see how best to add it back in.

For external metadata we have added a metadata handler to get a disk state (a spare or not a spare) based on current metadata state on disk.
Ioctl(GET_DISK_INFO) does not have a disk state info for containers (returns 0 - so we don't know if it is a spare or a failed disk).
We know that a disk is an array member based on check its state in the array.

Since Monitor code on devel-3.2 does not have calls to getinfo_super_disks method, 
auto-rebuild grabs the first disk in container and tries to move it to a degraded one (without a successes), and the first one happens to be array member and have state = 0 (a good spare).
Like all disks in container. 

There is also a fatal in pol_add, when trying to update policy rules with NULL spare_group:
@@ -732,7 +738,8 @@ static int move_spare(struct state *from, struct state *to,
 				continue;
 
 			pol = devnum_policy(from->devid[d]);
-			pol_add(&pol, pol_domain, from->spare_group, NULL);
+			if (from->spare_group)
+				pol_add(&pol, pol_domain, from->spare_group, NULL);

We will send you our FT integrated with your mdadm test suit in a couple of days. 

Marcin

Re: Devel 3.2 branch issues

[Dan Williams]

On 11/22/2010 9:18 AM, Labun, Marcin wrote:
>> - the getinfo_super_disks method.  I couldn't see why you need this.
>> All the
>>    info about the state of the arrays should already be available.
>>    If there is something that you need that we don't have, please
>> explain and
>>    we can see how best to add it back in.
>
> For external metadata we have added a metadata handler to get a disk state (a spare or not a spare) based on current metadata state on disk.
> Ioctl(GET_DISK_INFO) does not have a disk state info for containers (returns 0 - so we don't know if it is a spare or a failed disk).
> We know that a disk is an array member based on check its state in the array.

I'm still catching up on the devel-3.2 getinfo/load_super reworks, but I 
think this info would probably fit into the new 'map' parameter of 
getinfo_super().  Spares can be indicated as 'working' in the map.  I.e. 
if map returns 1 for a container member and that disk is not currently 
in use in a subarray then we can assume it is a spare at the container 
level.

Alternatively we could just return 2 in the map to indicate spare, but I 
think in the locations we care about we already know that it is not 
currently in use in a subarray.

--
Dan

RE: Devel 3.2 branch issues

[Labun, Marcin]

This is fixed version of original patch for the problem of imsm using spare disk 
that has been removed from a container. In previous patch  there was a problem of
releasing spare structures too early for spares that has been a part of volume (mdadm -f) - fixed.
As for Dan's comment, 

> Since we do not update the metadata can we just lazily queue an modified
> imsm_delete() update the next time we call activate_spare() and find the spare removed?  That way it is just garbage collection without this new infrastructure that gives the appearance we are writing metadata when removing a spare.

In fact, we are not updating metadata when removing a spare. I am reusing current imsm communication method between mdmon threads that already is used to do much more than just metadata updates. 
In your proposal we would still need to send info to  monitor that a disk shall not longer be used in a container.

Marcin


From 49406f135843a6bc2d2d28a34f8e8647fcced4d0 Mon Sep 17 00:00:00 2001
From: Marcin Labun <marcin.labun@intel.com>
Date: Wed, 17 Nov 2010 00:09:02 +0100
Subject: [PATCH] IMSM: Fix problem in mdmon monitor of using removed disk from in imsm container.

Manager thread shall pass the information to monitor thread (mdmon)
that some devices are removed from container. Otherwise, monitor (mdmon)
might use such devices (spares) to rebuild the array that has gone degraded.

This problem happens for imsm containers, since a list of the container disks
is maintained in intel_super structure. When array goes degraded, the list is
searched to find a spare disks to start rebuild.
Without this fix the rebuild could be stared on the spare device that was
a member of the container, but has been removed from it.

New super type function handler has been introduced to prepare metadata
format specific information about removed devices.
int (*remove_from_super)(struct supertype *st, mdu_disk_info_t *dinfo,
                         int fd);
The message prepared in remove_from_super is later processed
by proceess_update handler in monitor thread.

Signed-off-by: Marcin Labun <marcin.labun@intel.com>
---
 managemon.c   |   38 +++++++++++++
 mdadm.h       |    7 ++-
 super-intel.c |  173 +++++++++++++++++++++++++++++++++++++++++++++++----------
 3 files changed, 187 insertions(+), 31 deletions(-)

diff --git a/managemon.c b/managemon.c
index 8915522..93b130a 100644
--- a/managemon.c
+++ b/managemon.c
@@ -297,6 +297,43 @@ static void add_disk_to_container(struct supertype *st, struct mdinfo *sd)
 	st->update_tail = NULL;
 }
 
+/*
+ * Create and queue update structure about the removed disks.
+ * The update is prepared by super type handler and passed to the monitor
+ * thread.
+ */
+static void remove_disk_from_container(struct supertype *st, struct mdinfo *sd)
+{
+	int dfd;
+	char nm[20];
+	struct metadata_update *update = NULL;
+	mdu_disk_info_t dk = {
+		.number = -1,
+		.major = sd->disk.major,
+		.minor = sd->disk.minor,
+		.raid_disk = -1,
+		.state = 0,
+	};
+	/* nothing to do if super type handler does not support
+	 * remove disk primitive
+	 */
+	if (!st->ss->remove_from_super)
+		return;
+	dprintf("%s: remove %d:%d to container\n",
+		__func__, sd->disk.major, sd->disk.minor);
+
+	sprintf(nm, "%d:%d", sd->disk.major, sd->disk.minor);
+	dfd = dev_open(nm, O_RDWR);
+	if (dfd < 0)
+		return;
+
+	st->update_tail = &update;
+	st->ss->remove_from_super(st, &dk, dfd);
+	st->ss->write_init_super(st);
+	queue_metadata_update(update);
+	st->update_tail = NULL;
+}
+
 static void manage_container(struct mdstat_ent *mdstat,
 			     struct supertype *container)
 {
@@ -334,6 +371,7 @@ static void manage_container(struct mdstat_ent *mdstat,
 			if (!found) {
 				cd = *cdp;
 				*cdp = (*cdp)->next;
+				remove_disk_from_container(container, cd);
 				free(cd);
 			} else
 				cdp = &(*cdp)->next;
diff --git a/mdadm.h b/mdadm.h
index 2d1db36..6309a62 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -596,7 +596,12 @@ extern struct superswitch {
 	 * when hot-adding a spare.
 	 */
 	int (*add_to_super)(struct supertype *st, mdu_disk_info_t *dinfo,
-			     int fd, char *devname);
+			    int fd, char *devname);
+	/* update the metadata to delete a device,
+	 * when hot-removing a spare.
+	 */
+	int (*remove_from_super)(struct supertype *st, mdu_disk_info_t *dinfo,
+				 int fd);
 
 	/* Write metadata to one device when fixing problems or adding
 	 * a new device.
diff --git a/super-intel.c b/super-intel.c
index 9b4ad19..ac168e8 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -233,6 +233,10 @@ struct intel_dev {
 	unsigned index;
 };
 
+enum action {
+  DISK_REMOVE = 0,
+  DISK_ADD
+};
 /* internal representation of IMSM metadata */
 struct intel_super {
 	union {
@@ -258,8 +262,10 @@ struct intel_super {
 		int extent_cnt;
 		struct extent *e; /* for determining freespace @ create */
 		int raiddisk; /* slot to fill in autolayout */
+		enum action action;
 	} *disks;
-	struct dl *add; /* list of disks to add while mdmon active */
+	struct dl *disk_mgmt_list; /* list of disks to add/remove while mdmon
+				      active */
 	struct dl *missing; /* disks removed while we weren't looking */
 	struct bbm_log *bbm_log;
 	const char *hba; /* device path of the raid controller for this metadata */
@@ -285,6 +291,7 @@ enum imsm_update_type {
 	update_kill_array,
 	update_rename_array,
 	update_add_disk,
+	update_add_remove_disk
 };
 
 struct imsm_update_activate_spare {
@@ -316,7 +323,7 @@ struct imsm_update_rename_array {
 	int dev_idx;
 };
 
-struct imsm_update_add_disk {
+struct imsm_update_add_remove_disk {
 	enum imsm_update_type type;
 };
 
@@ -2428,6 +2435,7 @@ static void __free_imsm_disk(struct dl *d)
 	free(d);
 
 }
+
 static void free_imsm_disks(struct intel_super *super)
 {
 	struct dl *d;
@@ -3393,6 +3401,7 @@ static int add_to_super_imsm(struct supertype *st, mdu_disk_info_t *dk,
 	dd->devname = devname ? strdup(devname) : NULL;
 	dd->fd = fd;
 	dd->e = NULL;
+	dd->action = DISK_ADD;
 	rv = imsm_read_serial(fd, devname, dd->serial);
 	if (rv) {
 		fprintf(stderr,
@@ -3412,8 +3421,8 @@ static int add_to_super_imsm(struct supertype *st, mdu_disk_info_t *dk,
 		dd->disk.scsi_id = __cpu_to_le32(0);
 
 	if (st->update_tail) {
-		dd->next = super->add;
-		super->add = dd;
+		dd->next = super->disk_mgmt_list;
+		super->disk_mgmt_list = dd;
 	} else {
 		dd->next = super->disks;
 		super->disks = dd;
@@ -3422,6 +3431,45 @@ static int add_to_super_imsm(struct supertype *st, mdu_disk_info_t *dk,
 	return 0;
 }
 
+
+static int remove_from_super_imsm(struct supertype *st, mdu_disk_info_t *dk,
+				  int fd)
+{
+	struct intel_super *super = st->sb;
+	struct dl *dd;
+
+	/* remove from super works only in mdmon - for communication
+	 * manager - monitor. Check if communication memory buffer
+	 * is prepared.
+	 */
+	if (!st->update_tail) {
+		fprintf(stderr,
+			Name ": %s shall be used in mdmon context only"
+			"(line %d).\n", __func__, __LINE__);
+		return 1;
+	}
+	dd = malloc(sizeof(*dd));
+	if (!dd) {
+		fprintf(stderr,
+			Name ": malloc failed %s:%d.\n", __func__, __LINE__);
+		return 1;
+	}
+	memset(dd, 0, sizeof(*dd));
+	dd->major = dk->major;
+	dd->minor = dk->minor;
+	dd->index = -1;
+	dd->fd = fd;
+	dd->disk.status = SPARE_DISK;
+	dd->action = DISK_REMOVE;
+
+	if (st->update_tail) {
+		dd->next = super->disk_mgmt_list;
+		super->disk_mgmt_list = dd;
+	}
+
+	return 0;
+}
+
 static int store_imsm_mpb(int fd, struct imsm_super *mpb);
 
 static union {
@@ -3574,13 +3622,13 @@ static int create_array(struct supertype *st, int dev_idx)
 	return 0;
 }
 
-static int _add_disk(struct supertype *st)
+static int mgmt_disk(struct supertype *st)
 {
 	struct intel_super *super = st->sb;
 	size_t len;
-	struct imsm_update_add_disk *u;
+	struct imsm_update_add_remove_disk *u;
 
-	if (!super->add)
+	if (!super->disk_mgmt_list)
 		return 0;
 
 	len = sizeof(*u);
@@ -3591,7 +3639,7 @@ static int _add_disk(struct supertype *st)
 		return 1;
 	}
 
-	u->type = update_add_disk;
+	u->type = update_add_remove_disk;
 	append_metadata_update(st, u, len);
 
 	return 0;
@@ -3613,10 +3661,10 @@ static int write_init_super_imsm(struct supertype *st)
 
 		/* determine if we are creating a volume or adding a disk */
 		if (current_vol < 0) {
-			/* in the add disk case we are running in mdmon
-			 * context, so don't close fd's
+			/* in the mgmt (add/remove) disk case we are running
+			 * in mdmon context, so don't close fd's
 			 */
-			return _add_disk(st);
+			return mgmt_disk(st);
 		} else
 			rv = create_array(st, current_vol);
 
@@ -4873,10 +4921,9 @@ static int store_imsm_mpb(int fd, struct imsm_super *mpb)
 static void imsm_sync_metadata(struct supertype *container)
 {
 	struct intel_super *super = container->sb;
-
+	dprintf("sync metadata: %d\n", super->updates_pending);
 	if (!super->updates_pending)
 		return;
-
 	write_super_imsm(super, 0);
 
 	super->updates_pending = 0;
@@ -5165,8 +5212,80 @@ static int disks_overlap(struct intel_super *super, int idx, struct imsm_update_
 	return 0;
 }
 
+
+static struct dl *get_disk_super(struct intel_super *super, int major, int minor)
+{
+	struct dl *dl = NULL;
+	for (dl = super->disks; dl; dl = dl->next)
+		if ((dl->major == major) &&  (dl->minor == minor))
+			return dl;
+	return NULL;
+}
+
+static int remove_disk_super(struct intel_super *super, int major, int minor)
+{
+	struct dl *prev = NULL;
+	struct dl *dl;
+
+	prev = NULL;
+	for (dl = super->disks; dl; dl = dl->next) {
+		if ((dl->major == major) && (dl->minor == minor)) {
+			/* remove */
+			if (prev)
+				prev->next = dl->next;
+			else
+				super->disks = dl->next;
+			dl->next = NULL;
+			__free_imsm_disk(dl);
+			dprintf("%s: removed %x:%x\n",
+				__func__, major, minor);
+			break;
+		}
+		prev = dl;
+	}
+	return 0;
+}
+
 static void imsm_delete(struct intel_super *super, struct dl **dlp, unsigned index);
 
+static int add_remove_disk_update(struct intel_super *super)
+{
+	int check_degraded = 0;
+	struct dl *disk = NULL;
+	/* add/remove some spares to/from the metadata/contrainer */
+	while (super->disk_mgmt_list) {
+		struct dl *disk_cfg;
+
+		disk_cfg = super->disk_mgmt_list;
+		super->disk_mgmt_list = disk_cfg->next;
+		disk_cfg->next = NULL;
+
+		if (disk_cfg->action == DISK_ADD) {
+			disk_cfg->next = super->disks;
+			super->disks = disk_cfg;
+			check_degraded = 1;
+			dprintf("%s: added %x:%x\n",
+				__func__, disk_cfg->major,
+				disk_cfg->minor);
+		} else if (disk_cfg->action == DISK_REMOVE) {
+			dprintf("Disk remove action processed: %x.%x\n",
+				disk_cfg->major, disk_cfg->minor);
+			disk = get_disk_super(super,
+					     disk_cfg->major,
+					     disk_cfg->minor);
+			/* remove spare disks only */
+			if (disk->index == -1) {
+				remove_disk_super(super,  
+						  disk_cfg->major,
+						  disk_cfg->minor);
+			}
+			/* release allocate disk structure */
+			__free_imsm_disk(disk_cfg);
+		}
+	}
+	return check_degraded;
+}
+
 static void imsm_process_update(struct supertype *st,
 			        struct metadata_update *update)
 {
@@ -5476,31 +5595,24 @@ static void imsm_process_update(struct supertype *st,
 		super->updates_pending++;
 		break;
 	}
-	case update_add_disk:
-
+	case update_add_remove_disk: {
 		/* we may be able to repair some arrays if disks are
-		 * being added */
-		if (super->add) {
+		 * being added, check teh status of add_remove_disk
+		 * if discs has been added.
+		 */
+		if (add_remove_disk_update(super)) {
 			struct active_array *a;
 
 			super->updates_pending++;
- 			for (a = st->arrays; a; a = a->next)
+			for (a = st->arrays; a; a = a->next)
 				a->check_degraded = 1;
 		}
-		/* add some spares to the metadata */
-		while (super->add) {
-			struct dl *al;
-
-			al = super->add;
-			super->add = al->next;
-			al->next = super->disks;
-			super->disks = al;
-			dprintf("%s: added %x:%x\n",
-				__func__, al->major, al->minor);
-		}
-
 		break;
 	}
+	default:
+		fprintf(stderr, "error: unsuported process update type:"
+			"(type: %d)\n",	type);
+	}
 }
 
 static void imsm_prepare_update(struct supertype *st,
@@ -5685,6 +5797,7 @@ struct superswitch super_imsm = {
 	.write_init_super = write_init_super_imsm,
 	.validate_geometry = validate_geometry_imsm,
 	.add_to_super	= add_to_super_imsm,
+	.remove_from_super	= remove_from_super_imsm,
 	.detail_platform = detail_platform_imsm,
 	.kill_subarray = kill_subarray_imsm,
 	.update_subarray = update_subarray_imsm,
-- 
1.6.4.2

Autorebuild, new dynamic udev rules for hot-plugs

[Hawrylewicz Czarnowski, Przemyslaw]

Hi,

I would like to present another patch for our autorebuild tree (it should be applied on the top of Autorebuild series, as devel-3.2 is not stable yet). Dan Williams proposed to reduce overhead associated with passing each hot-plugged block device to mdadm, using just the devices described in policies via mdadm.conf file.
See patch below for details, comments are as always very welcome.

Date: Thu, 18 Nov 2010 01:19:52 +0100
Subject: [PATCH] New dynamic hot-plug udev rules for policies

When introducing policies, new hot-plug rules were added to support
bare disks. Mdadm was started for each hot plugged block device
to determine if it could be used as spare or as a replacement member for
degraded array.
This patch introduces limitation of range of devices that are handled
by mdadm. It limits them to the ones specified in domains associated 
with the actions: spare-same-port, spare and spare-force.
In order to enable hot-plug for bare disks one must update udev rules
with command

	mdadm --activate-domains

After mdadm.conf is changed one is obliged to re-run
"mdadm --activate-domains" command in order to bring the system
configuration up to date.
All hot-plugged disks containing metadata are still handled by existing
rules.

Note: this patch is just a proposition to minimize overhead of using mdadm for
each plugged block device. If accepted, it will be incorporated in
previous implementation of hot-plug for bare disks.

Signed-off-by: Przemyslaw Czarnowski <przemyslaw.hawrylewicz.czarnowski@intel.com>
---
 Makefile           |    2 +
 ReadMe.c           |    1 +
 mdadm.c            |    4 ++
 mdadm.h            |    9 +++-
 policy.c           |  137 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 udev-md-raid.rules |    5 +-
 6 files changed, 154 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 2b88818..eeae8e1 100644
--- a/Makefile
+++ b/Makefile
@@ -73,9 +73,11 @@ MAP_FILE = map
 MDMON_DIR = /dev/.mdadm
 # place for autoreplace cookies
 FAILED_SLOTS_DIR = /dev/.mdadm/failed-slots
+UDEV_RULES_DIR = $(DESTDIR)/lib/udev/rules.d
 DIRFLAGS = -DMAP_DIR=\"$(MAP_DIR)\" -DMAP_FILE=\"$(MAP_FILE)\"
 DIRFLAGS += -DMDMON_DIR=\"$(MDMON_DIR)\"
 DIRFLAGS += -DFAILED_SLOTS_DIR=\"$(FAILED_SLOTS_DIR)\"
+DIRFLAGS += -DUDEV_RULES_DIR=\"$(UDEV_RULES_DIR)\"
 CFLAGS = $(CWFLAGS) $(CXFLAGS) -DSendmail=\""$(MAILCMD)"\" $(CONFFILEFLAGS) $(DIRFLAGS)
 
 # The glibc TLS ABI requires applications that call clone(2) to set up
diff --git a/ReadMe.c b/ReadMe.c
index 54a1998..f17b8a1 100644
--- a/ReadMe.c
+++ b/ReadMe.c
@@ -110,6 +110,7 @@ struct option long_options[] = {
     {"detail-platform", 0, 0, DetailPlatform},
     {"kill-subarray", 1, 0, KillSubarray},
     {"update-subarray", 1, 0, UpdateSubarray},
+    {"activate-domains", 0, 0, ActivateDomains},
 
     /* synonyms */
     {"monitor",   0, 0, 'F'},
diff --git a/mdadm.c b/mdadm.c
index c9a172a..b5403cf 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -228,6 +228,7 @@ int main(int argc, char *argv[])
 				}
 				subarray = optarg;
 			}
+		case ActivateDomains:
 		case 'K': if (!mode) newmode = MISC; break;
 		case NoSharing: newmode = MONITOR; break;
 		}
@@ -841,6 +842,7 @@ int main(int argc, char *argv[])
 		case O(MISC, DetailPlatform):
 		case O(MISC, KillSubarray):
 		case O(MISC, UpdateSubarray):
+		case O(MISC, ActivateDomains):
 			if (devmode && devmode != opt &&
 			    (devmode == 'E' || (opt == 'E' && devmode != 'Q'))) {
 				fprintf(stderr, Name ": --examine/-E cannot be given with ");
@@ -1421,6 +1423,8 @@ int main(int argc, char *argv[])
 						free_mdstat(ms);
 					} while (!last && err);
 					if (err) rv |= 1;
+				} else if (devmode == ActivateDomains) {
+					rv = Activate_Domains();
 				} else {
 					fprintf(stderr, Name ": No devices given.\n");
 					exit(2);
diff --git a/mdadm.h b/mdadm.h
index 9b4a1a8..171aa69 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -101,6 +101,11 @@ extern __off64_t lseek64 __P ((int __fd, __off64_t __offset, int __whence));
 #define FAILED_SLOTS_DIR "/dev/.mdadm/failed-slots"
 #endif /* FAILED_SLOTS */
 
+/* UDEV_RULES_DIR is the place, where udev holds its rules */
+#ifndef UDEV_RULES_DIR
+#define UDEV_RULES_DIR "/lib/udev/rules.d"
+#endif /* UDEV_RULES_DIR */
+
 #include	"md_u.h"
 #include	"md_p.h"
 #include	"bitmap.h"
@@ -289,7 +294,8 @@ enum special_options {
 	KillSubarray,
 	UpdateSubarray, /* 16 */
 	IncrementalPath,
-	NoSharing
+	NoSharing,
+	ActivateDomains
 };
 
 /* structures read from config file */
@@ -971,6 +977,7 @@ extern int CreateBitmap(char *filename, int force, char uuid[16],
 			unsigned long long array_size,
 			int major);
 extern int ExamineBitmap(char *filename, int brief, struct supertype *st);
+extern int Activate_Domains(void);
 extern int bitmap_update_uuid(int fd, int *uuid, int swap);
 extern unsigned long bitmap_sectors(struct bitmap_super_s *bsb);
 
diff --git a/policy.c b/policy.c
index daee52a..e4c53d5 100644
--- a/policy.c
+++ b/policy.c
@@ -753,3 +753,140 @@ void domain_free(struct domainlist *dl)
 		free(head);
 	}
 }
+
+/* invocation of udev rule file */
+char udev_template_start[] =
+"# do not edit this file, it will be overwritten on update\n"
+"\n"
+"SUBSYSTEM!=\"block\", GOTO=\"md_autorebuild_end\"\n"
+"\n"
+"ENV{ID_FS_TYPE}==\"linux_raid_member\", GOTO=\"md_autorebuild_end\"\n"
+"ENV{ID_FS_TYPE}==\"isw_raid_member\", GOTO=\"md_autorebuild_end\"\n"
+"\n";
+
+/* ending of udev rule file */
+char udev_template_end[] =
+"\n"
+"LABEL=\"md_autorebuild_end\"\n"
+"\n";
+
+/* find rule named rule_type and return its value */
+char *find_rule(struct rule *rule, char *rule_type)
+{
+	while (rule) {
+		if (rule->name == rule_type)
+			return rule->value;
+
+		rule = rule->next;
+	}
+	return NULL;
+}
+
+#define UDEV_RULE_FORMAT \
+"ACTION==\"add\", KERNEL!=\"md*\" ENV{ID_PATH}==\"%s\" " \
+"RUN+=\"/sbin/mdadm --incremental $env{DEVNAME}\", " \
+"GOTO=\"md_autorebuild_end\"\n" \
+
+/* Write rule in the rule file. Use format from UDEV_RULE_FORMAT */
+int write_rule(struct rule *rule, int fd)
+{
+	char line[1024];
+	char *r = find_rule(rule, rule_path);
+	if (!r)
+		return -1;
+
+	snprintf(line, sizeof(line) - 1, UDEV_RULE_FORMAT, r);
+	return write(fd, line, strlen(line));
+}
+
+/* Generate single entry in udev rule basing on POLICY line found in config
+ * file. Take only those with paths, only first occurrence if paths are equal
+ * and if actions supports handling of spares (>=act_spare_same_slot)
+ */
+int generate_entries(int fd)
+{
+	struct pol_rule *loop, *dup;
+	char *loop_value, *dup_value;
+	int duplicate;
+	int written = 0;
+
+	for (loop = config_rules; loop; loop = loop->next) {
+		if (loop->type != rule_policy)
+			continue;
+		duplicate = 0;
+
+		/* only policies with paths and with actions supporting
+		 * bare disks are considered */
+		loop_value = find_rule(loop->rule, pol_act);
+		if (!loop_value || map_act(loop_value) < act_spare_same_slot)
+			continue;
+		loop_value = find_rule(loop->rule, rule_path);
+		if (!loop_value)
+			continue;
+		for (dup = config_rules; dup != loop; dup = dup->next) {
+			if (dup->type != rule_policy)
+				continue;
+			dup_value = find_rule(dup->rule, pol_act);
+			if (!dup_value || map_act(dup_value) < act_spare_same_slot)
+				continue;
+			dup_value = find_rule(dup->rule, rule_path);
+			if (!dup_value)
+				continue;
+			if (strcmp(loop_value, dup_value) == 0) {
+				duplicate = 1;
+				break;
+			}
+		}
+
+		/* not a dup or first occurrence */
+		if (!duplicate) {
+			if (write_rule(loop->rule, fd) == -1)
+				return 0;
+			written++;
+		}
+	}
+	return written;
+}
+
+#define AR_UDEV_RULE_FILE UDEV_RULES_DIR "/63-md-raid-autorebuild.rules"
+
+/* Activate_Domains routine creates dynamic udev rules used to handle
+ * hot-plug events for bare devices (and making them spares)
+ */
+int Activate_Domains(void)
+{
+	int fd = 0;
+	int rv;
+
+	fd = creat(AR_UDEV_RULE_FILE,
+		S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+	if (fd == -1)
+		return 1;
+
+	/* write static invocation */
+	if (write(fd, udev_template_start,
+		sizeof(udev_template_start) - 1) == -1) {
+		close(fd);
+		unlink(AR_UDEV_RULE_FILE);
+		return 1;
+	}
+
+	/* iterate, if none created or error occurred, remove file */
+	rv = generate_entries(fd);
+	if (rv <= 0) {
+		close(fd);
+		unlink(AR_UDEV_RULE_FILE);
+		return rv == -1 ? -1 : 0;
+	}
+
+	/* write ending */
+	if (write(fd, udev_template_end, sizeof(udev_template_end) - 1) == -1) {
+		close(fd);
+		unlink(AR_UDEV_RULE_FILE);
+		return 1;
+	}
+
+	close(fd);
+
+	return 0;
+}
diff --git a/udev-md-raid.rules b/udev-md-raid.rules
index 36dd51e..11057bb 100644
--- a/udev-md-raid.rules
+++ b/udev-md-raid.rules
@@ -3,11 +3,10 @@
 SUBSYSTEM!="block", GOTO="md_end"
 
 # handle potential components of arrays
+ENV{ID_FS_TYPE}=="linux_raid_member", ACTION=="add", RUN+="/sbin/mdadm --incremental $env{DEVNAME}"
 ENV{ID_FS_TYPE}=="linux_raid_member", ACTION=="remove", RUN+="/sbin/mdadm -If $name --path $env{ID_PATH}"
+ENV{ID_FS_TYPE}=="isw_raid_member", ACTION=="add", RUN+="/sbin/mdadm --incremental $env{DEVNAME}"
 ENV{ID_FS_TYPE}=="isw_raid_member", ACTION=="remove", RUN+="/sbin/mdadm -If $name --path $env{ID_PATH}"
-# try incremental for each block device and not md. Most cases should not create
-# unnecessary overhead, as no "heavy" disk operations are performed
-ACTION=="add", KERNEL!="md*", RUN+="/sbin/mdadm --incremental $env{DEVNAME}"
 
 # handle md arrays
 ACTION!="add|change", GOTO="md_end"
-- 
1.7.1

Re: Autorebuild, new dynamic udev rules for hot-plugs

[Neil Brown]

On Fri, 19 Nov 2010 15:12:19 +0000
"Hawrylewicz Czarnowski, Przemyslaw"
<przemyslaw.hawrylewicz.czarnowski@intel.com> wrote:

> Hi,
> 
> I would like to present another patch for our autorebuild tree (it should be applied on the top of Autorebuild series, as devel-3.2 is not stable yet). Dan Williams proposed to reduce overhead associated with passing each hot-plugged block device to mdadm, using just the devices described in policies via mdadm.conf file.
> See patch below for details, comments are as always very welcome.

I am in general in favour of this approach.
It has the benefit that it can very easily be not-used if there turn out to
be problems with it.

Four comments.

1/ I wouldn't write a file in /lib/udev/rules.d/
  I think it should be written to "/dev/.udev/rules.d/"
  which is referred to as the "temporary rules directory"
  in the udev documentation.

2/ I would be good to process the type=disk or type=part part of the
   policy into the rules file as well.

3/ I'm not very comfortable with hard-coding the name of the
   file to be created in the rules.d directory.  Maybe usage could be
      --activate-domains=63-md-whatever

4/ I don't think it is good to have an incomplete file in rules.d that udev
   might accidentally read.  We should create the file with a name with a
   leading '.' (assuming udev ignores those, I haven't checked) and then
   rename it after it has been completely written.

Other than that, it looks pretty good.

Thanks,
NeilBrown

> 
> Date: Thu, 18 Nov 2010 01:19:52 +0100
> Subject: [PATCH] New dynamic hot-plug udev rules for policies
> 
> When introducing policies, new hot-plug rules were added to support
> bare disks. Mdadm was started for each hot plugged block device
> to determine if it could be used as spare or as a replacement member for
> degraded array.
> This patch introduces limitation of range of devices that are handled
> by mdadm. It limits them to the ones specified in domains associated 
> with the actions: spare-same-port, spare and spare-force.
> In order to enable hot-plug for bare disks one must update udev rules
> with command
> 
> 	mdadm --activate-domains
> 
> After mdadm.conf is changed one is obliged to re-run
> "mdadm --activate-domains" command in order to bring the system
> configuration up to date.
> All hot-plugged disks containing metadata are still handled by existing
> rules.
> 
> Note: this patch is just a proposition to minimize overhead of using mdadm for
> each plugged block device. If accepted, it will be incorporated in
> previous implementation of hot-plug for bare disks.
> 
> Signed-off-by: Przemyslaw Czarnowski <przemyslaw.hawrylewicz.czarnowski@intel.com>
> ---
>  Makefile           |    2 +
>  ReadMe.c           |    1 +
>  mdadm.c            |    4 ++
>  mdadm.h            |    9 +++-
>  policy.c           |  137 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  udev-md-raid.rules |    5 +-
>  6 files changed, 154 insertions(+), 4 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 2b88818..eeae8e1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -73,9 +73,11 @@ MAP_FILE = map
>  MDMON_DIR = /dev/.mdadm
>  # place for autoreplace cookies
>  FAILED_SLOTS_DIR = /dev/.mdadm/failed-slots
> +UDEV_RULES_DIR = $(DESTDIR)/lib/udev/rules.d
>  DIRFLAGS = -DMAP_DIR=\"$(MAP_DIR)\" -DMAP_FILE=\"$(MAP_FILE)\"
>  DIRFLAGS += -DMDMON_DIR=\"$(MDMON_DIR)\"
>  DIRFLAGS += -DFAILED_SLOTS_DIR=\"$(FAILED_SLOTS_DIR)\"
> +DIRFLAGS += -DUDEV_RULES_DIR=\"$(UDEV_RULES_DIR)\"
>  CFLAGS = $(CWFLAGS) $(CXFLAGS) -DSendmail=\""$(MAILCMD)"\" $(CONFFILEFLAGS) $(DIRFLAGS)
>  
>  # The glibc TLS ABI requires applications that call clone(2) to set up
> diff --git a/ReadMe.c b/ReadMe.c
> index 54a1998..f17b8a1 100644
> --- a/ReadMe.c
> +++ b/ReadMe.c
> @@ -110,6 +110,7 @@ struct option long_options[] = {
>      {"detail-platform", 0, 0, DetailPlatform},
>      {"kill-subarray", 1, 0, KillSubarray},
>      {"update-subarray", 1, 0, UpdateSubarray},
> +    {"activate-domains", 0, 0, ActivateDomains},
>  
>      /* synonyms */
>      {"monitor",   0, 0, 'F'},
> diff --git a/mdadm.c b/mdadm.c
> index c9a172a..b5403cf 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -228,6 +228,7 @@ int main(int argc, char *argv[])
>  				}
>  				subarray = optarg;
>  			}
> +		case ActivateDomains:
>  		case 'K': if (!mode) newmode = MISC; break;
>  		case NoSharing: newmode = MONITOR; break;
>  		}
> @@ -841,6 +842,7 @@ int main(int argc, char *argv[])
>  		case O(MISC, DetailPlatform):
>  		case O(MISC, KillSubarray):
>  		case O(MISC, UpdateSubarray):
> +		case O(MISC, ActivateDomains):
>  			if (devmode && devmode != opt &&
>  			    (devmode == 'E' || (opt == 'E' && devmode != 'Q'))) {
>  				fprintf(stderr, Name ": --examine/-E cannot be given with ");
> @@ -1421,6 +1423,8 @@ int main(int argc, char *argv[])
>  						free_mdstat(ms);
>  					} while (!last && err);
>  					if (err) rv |= 1;
> +				} else if (devmode == ActivateDomains) {
> +					rv = Activate_Domains();
>  				} else {
>  					fprintf(stderr, Name ": No devices given.\n");
>  					exit(2);
> diff --git a/mdadm.h b/mdadm.h
> index 9b4a1a8..171aa69 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -101,6 +101,11 @@ extern __off64_t lseek64 __P ((int __fd, __off64_t __offset, int __whence));
>  #define FAILED_SLOTS_DIR "/dev/.mdadm/failed-slots"
>  #endif /* FAILED_SLOTS */
>  
> +/* UDEV_RULES_DIR is the place, where udev holds its rules */
> +#ifndef UDEV_RULES_DIR
> +#define UDEV_RULES_DIR "/lib/udev/rules.d"
> +#endif /* UDEV_RULES_DIR */
> +
>  #include	"md_u.h"
>  #include	"md_p.h"
>  #include	"bitmap.h"
> @@ -289,7 +294,8 @@ enum special_options {
>  	KillSubarray,
>  	UpdateSubarray, /* 16 */
>  	IncrementalPath,
> -	NoSharing
> +	NoSharing,
> +	ActivateDomains
>  };
>  
>  /* structures read from config file */
> @@ -971,6 +977,7 @@ extern int CreateBitmap(char *filename, int force, char uuid[16],
>  			unsigned long long array_size,
>  			int major);
>  extern int ExamineBitmap(char *filename, int brief, struct supertype *st);
> +extern int Activate_Domains(void);
>  extern int bitmap_update_uuid(int fd, int *uuid, int swap);
>  extern unsigned long bitmap_sectors(struct bitmap_super_s *bsb);
>  
> diff --git a/policy.c b/policy.c
> index daee52a..e4c53d5 100644
> --- a/policy.c
> +++ b/policy.c
> @@ -753,3 +753,140 @@ void domain_free(struct domainlist *dl)
>  		free(head);
>  	}
>  }
> +
> +/* invocation of udev rule file */
> +char udev_template_start[] =
> +"# do not edit this file, it will be overwritten on update\n"
> +"\n"
> +"SUBSYSTEM!=\"block\", GOTO=\"md_autorebuild_end\"\n"
> +"\n"
> +"ENV{ID_FS_TYPE}==\"linux_raid_member\", GOTO=\"md_autorebuild_end\"\n"
> +"ENV{ID_FS_TYPE}==\"isw_raid_member\", GOTO=\"md_autorebuild_end\"\n"
> +"\n";
> +
> +/* ending of udev rule file */
> +char udev_template_end[] =
> +"\n"
> +"LABEL=\"md_autorebuild_end\"\n"
> +"\n";
> +
> +/* find rule named rule_type and return its value */
> +char *find_rule(struct rule *rule, char *rule_type)
> +{
> +	while (rule) {
> +		if (rule->name == rule_type)
> +			return rule->value;
> +
> +		rule = rule->next;
> +	}
> +	return NULL;
> +}
> +
> +#define UDEV_RULE_FORMAT \
> +"ACTION==\"add\", KERNEL!=\"md*\" ENV{ID_PATH}==\"%s\" " \
> +"RUN+=\"/sbin/mdadm --incremental $env{DEVNAME}\", " \
> +"GOTO=\"md_autorebuild_end\"\n" \
> +
> +/* Write rule in the rule file. Use format from UDEV_RULE_FORMAT */
> +int write_rule(struct rule *rule, int fd)
> +{
> +	char line[1024];
> +	char *r = find_rule(rule, rule_path);
> +	if (!r)
> +		return -1;
> +
> +	snprintf(line, sizeof(line) - 1, UDEV_RULE_FORMAT, r);
> +	return write(fd, line, strlen(line));
> +}
> +
> +/* Generate single entry in udev rule basing on POLICY line found in config
> + * file. Take only those with paths, only first occurrence if paths are equal
> + * and if actions supports handling of spares (>=act_spare_same_slot)
> + */
> +int generate_entries(int fd)
> +{
> +	struct pol_rule *loop, *dup;
> +	char *loop_value, *dup_value;
> +	int duplicate;
> +	int written = 0;
> +
> +	for (loop = config_rules; loop; loop = loop->next) {
> +		if (loop->type != rule_policy)
> +			continue;
> +		duplicate = 0;
> +
> +		/* only policies with paths and with actions supporting
> +		 * bare disks are considered */
> +		loop_value = find_rule(loop->rule, pol_act);
> +		if (!loop_value || map_act(loop_value) < act_spare_same_slot)
> +			continue;
> +		loop_value = find_rule(loop->rule, rule_path);
> +		if (!loop_value)
> +			continue;
> +		for (dup = config_rules; dup != loop; dup = dup->next) {
> +			if (dup->type != rule_policy)
> +				continue;
> +			dup_value = find_rule(dup->rule, pol_act);
> +			if (!dup_value || map_act(dup_value) < act_spare_same_slot)
> +				continue;
> +			dup_value = find_rule(dup->rule, rule_path);
> +			if (!dup_value)
> +				continue;
> +			if (strcmp(loop_value, dup_value) == 0) {
> +				duplicate = 1;
> +				break;
> +			}
> +		}
> +
> +		/* not a dup or first occurrence */
> +		if (!duplicate) {
> +			if (write_rule(loop->rule, fd) == -1)
> +				return 0;
> +			written++;
> +		}
> +	}
> +	return written;
> +}
> +
> +#define AR_UDEV_RULE_FILE UDEV_RULES_DIR "/63-md-raid-autorebuild.rules"
> +
> +/* Activate_Domains routine creates dynamic udev rules used to handle
> + * hot-plug events for bare devices (and making them spares)
> + */
> +int Activate_Domains(void)
> +{
> +	int fd = 0;
> +	int rv;
> +
> +	fd = creat(AR_UDEV_RULE_FILE,
> +		S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
> +	if (fd == -1)
> +		return 1;
> +
> +	/* write static invocation */
> +	if (write(fd, udev_template_start,
> +		sizeof(udev_template_start) - 1) == -1) {
> +		close(fd);
> +		unlink(AR_UDEV_RULE_FILE);
> +		return 1;
> +	}
> +
> +	/* iterate, if none created or error occurred, remove file */
> +	rv = generate_entries(fd);
> +	if (rv <= 0) {
> +		close(fd);
> +		unlink(AR_UDEV_RULE_FILE);
> +		return rv == -1 ? -1 : 0;
> +	}
> +
> +	/* write ending */
> +	if (write(fd, udev_template_end, sizeof(udev_template_end) - 1) == -1) {
> +		close(fd);
> +		unlink(AR_UDEV_RULE_FILE);
> +		return 1;
> +	}
> +
> +	close(fd);
> +
> +	return 0;
> +}
> diff --git a/udev-md-raid.rules b/udev-md-raid.rules
> index 36dd51e..11057bb 100644
> --- a/udev-md-raid.rules
> +++ b/udev-md-raid.rules
> @@ -3,11 +3,10 @@
>  SUBSYSTEM!="block", GOTO="md_end"
>  
>  # handle potential components of arrays
> +ENV{ID_FS_TYPE}=="linux_raid_member", ACTION=="add", RUN+="/sbin/mdadm --incremental $env{DEVNAME}"
>  ENV{ID_FS_TYPE}=="linux_raid_member", ACTION=="remove", RUN+="/sbin/mdadm -If $name --path $env{ID_PATH}"
> +ENV{ID_FS_TYPE}=="isw_raid_member", ACTION=="add", RUN+="/sbin/mdadm --incremental $env{DEVNAME}"
>  ENV{ID_FS_TYPE}=="isw_raid_member", ACTION=="remove", RUN+="/sbin/mdadm -If $name --path $env{ID_PATH}"
> -# try incremental for each block device and not md. Most cases should not create
> -# unnecessary overhead, as no "heavy" disk operations are performed
> -ACTION=="add", KERNEL!="md*", RUN+="/sbin/mdadm --incremental $env{DEVNAME}"
>  
>  # handle md arrays
>  ACTION!="add|change", GOTO="md_end"

RE: Autorebuild, new dynamic udev rules for hot-plugs

[Hawrylewicz Czarnowski, Przemyslaw]

Hi,

> -----Original Message-----
> From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
> owner@vger.kernel.org] On Behalf Of Neil Brown
> Sent: Monday, November 22, 2010 6:02 AM
> To: Hawrylewicz Czarnowski, Przemyslaw
> Cc: linux-raid@vger.kernel.org; Neubauer, Wojciech; Williams, Dan J;
> Ciechanowski, Ed; Labun, Marcin; Czarnowska, Anna
> Subject: Re: Autorebuild, new dynamic udev rules for hot-plugs
> 
> On Fri, 19 Nov 2010 15:12:19 +0000
> "Hawrylewicz Czarnowski, Przemyslaw"
> <przemyslaw.hawrylewicz.czarnowski@intel.com> wrote:
> 
> > Hi,
> >
> > I would like to present another patch for our autorebuild tree (it should
> be applied on the top of Autorebuild series, as devel-3.2 is not stable
> yet). Dan Williams proposed to reduce overhead associated with passing each
> hot-plugged block device to mdadm, using just the devices described in
> policies via mdadm.conf file.
> > See patch below for details, comments are as always very welcome.
> 
> I am in general in favour of this approach.
> It has the benefit that it can very easily be not-used if there turn out to
> be problems with it.
> 
> Four comments.
> 
> 1/ I wouldn't write a file in /lib/udev/rules.d/
>   I think it should be written to "/dev/.udev/rules.d/"
>   which is referred to as the "temporary rules directory"
>   in the udev documentation.
I am not sure if it is what we are looking for. Temporary means they disappear after reboot. It is OK as cold-plug does not need support for bare disks (or maybe I am wrong?). But in such case, one who wants to use autorebuild should invoke mdadm --activate-domains for example in /etc/init.d/local.boot or somewhere else. Second idea here is to use ActivateDomain() when one starts monitor with autorebuild enabled. Which one? I would prefer to leave it as it was written initially (considering comment #4). Then, if one removes policies from config, invoking --activate-domains should reset/remove rules (but see #3)

> 
> 2/ I would be good to process the type=disk or type=part part of the
>    policy into the rules file as well.
OK

> 
> 3/ I'm not very comfortable with hard-coding the name of the
>    file to be created in the rules.d directory.  Maybe usage could be
>       --activate-domains=63-md-whatever
Good idea, but only if we store our rules in /dev/.udev/rules.d. Otherwise it would be difficult to maintain all generated rules and remove the old ones... I would leave default if not given by user, but one can pass any file name.

> 
> 4/ I don't think it is good to have an incomplete file in rules.d that udev
>    might accidentally read.  We should create the file with a name with a
>    leading '.' (assuming udev ignores those, I haven't checked) and then
>    rename it after it has been completely written.
You're right. In theory, such partial udev rules are excluded when udev can't interpret them properly. I have looked into udev's sources and found that it looks for "*.rules" files. All other file extensions are ignored. Files with leading dots are also omitted. I would prefer to create <name>.temp file and then rename it into <name>.rules.

> 
> Other than that, it looks pretty good.
Great

> 
> Thanks,
> NeilBrown
> 
> >
> > Date: Thu, 18 Nov 2010 01:19:52 +0100
> > Subject: [PATCH] New dynamic hot-plug udev rules for policies
> >
> > When introducing policies, new hot-plug rules were added to support
> > bare disks. Mdadm was started for each hot plugged block device
> > to determine if it could be used as spare or as a replacement member for
> > degraded array.
> > This patch introduces limitation of range of devices that are handled
> > by mdadm. It limits them to the ones specified in domains associated
> > with the actions: spare-same-port, spare and spare-force.
> > In order to enable hot-plug for bare disks one must update udev rules
> > with command
> >
> > 	mdadm --activate-domains
> >
> > After mdadm.conf is changed one is obliged to re-run
> > "mdadm --activate-domains" command in order to bring the system
> > configuration up to date.
> > All hot-plugged disks containing metadata are still handled by existing
> > rules.
> >
> > Note: this patch is just a proposition to minimize overhead of using
> mdadm for
> > each plugged block device. If accepted, it will be incorporated in
> > previous implementation of hot-plug for bare disks.
> >
> > Signed-off-by: Przemyslaw Czarnowski
> <przemyslaw.hawrylewicz.czarnowski@intel.com>
> > ---
> >  Makefile           |    2 +
> >  ReadMe.c           |    1 +
> >  mdadm.c            |    4 ++
> >  mdadm.h            |    9 +++-
> >  policy.c           |  137
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  udev-md-raid.rules |    5 +-
> >  6 files changed, 154 insertions(+), 4 deletions(-)
> >
[cut]

Re: Autorebuild, new dynamic udev rules for hot-plugs

[Dan Williams]

On 11/22/2010 3:50 PM, Hawrylewicz Czarnowski, Przemyslaw wrote:
>> Four comments.
>>
>> 1/ I wouldn't write a file in /lib/udev/rules.d/
>>    I think it should be written to "/dev/.udev/rules.d/"
>>    which is referred to as the "temporary rules directory"
>>    in the udev documentation.
> I am not sure if it is what we are looking for. Temporary means they disappear after reboot. It is OK as cold-plug does not need support for bare disks (or maybe I am wrong?). But in such case, one who wants to use autorebuild should invoke mdadm --activate-domains for example in /etc/init.d/local.boot or somewhere else. Second idea here is to use ActivateDomain() when one starts monitor with autorebuild enabled. Which one? I would prefer to leave it as it was written initially (considering comment #4). Then, if one removes policies from config, invoking --activate-domains should reset/remove rules (but see #3)

The intent was always to have this be something reinitialized at boot. 
Putting these in the temporary rule directory also precludes them from 
being added to the initramfs where they are not needed / potentially 
confusing.

The other intent was to only match the pci paths for the controllers we 
cared about.  That does not appear to be a part of this patch.

>
>>
>> 2/ I would be good to process the type=disk or type=part part of the
>>     policy into the rules file as well.
> OK
>
>>
>> 3/ I'm not very comfortable with hard-coding the name of the
>>     file to be created in the rules.d directory.  Maybe usage could be
>>        --activate-domains=63-md-whatever
> Good idea, but only if we store our rules in /dev/.udev/rules.d. Otherwise it would be difficult to maintain all generated rules and remove the old ones... I would leave default if not given by user, but one can pass any file name.

The issue is that this namespace belongs to the distro and since they 
need to modify initscripts to turn this feature on might as well dump 
the entirety of the naming responsibility to the user.

>> 4/ I don't think it is good to have an incomplete file in rules.d that udev
>>     might accidentally read.  We should create the file with a name with a
>>     leading '.' (assuming udev ignores those, I haven't checked) and then
>>     rename it after it has been completely written.
> You're right. In theory, such partial udev rules are excluded when udev can't interpret them properly. I have looked into udev's sources and found that it looks for "*.rules" files. All other file extensions are ignored. Files with leading dots are also omitted. I would prefer to create<name>.temp file and then rename it into<name>.rules.

There must be an existing convention for this sort of the thing, if so 
let's not invent another one.

--
Dan

Re: Autorebuild, new dynamic udev rules for hot-plugs

[Neil Brown]

On Mon, 22 Nov 2010 16:11:41 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> On 11/22/2010 3:50 PM, Hawrylewicz Czarnowski, Przemyslaw wrote:
> >> Four comments.
> >>
> >> 1/ I wouldn't write a file in /lib/udev/rules.d/
> >>    I think it should be written to "/dev/.udev/rules.d/"
> >>    which is referred to as the "temporary rules directory"
> >>    in the udev documentation.
> > I am not sure if it is what we are looking for. Temporary means they disappear after reboot. It is OK as cold-plug does not need support for bare disks (or maybe I am wrong?). But in such case, one who wants to use autorebuild should invoke mdadm --activate-domains for example in /etc/init.d/local.boot or somewhere else. Second idea here is to use ActivateDomain() when one starts monitor with autorebuild enabled. Which one? I would prefer to leave it as it was written initially (considering comment #4). Then, if one removes policies from config, invoking --activate-domains should reset/remove rules (but see #3)
> 
> The intent was always to have this be something reinitialized at boot. 
> Putting these in the temporary rule directory also precludes them from 
> being added to the initramfs where they are not needed / potentially 
> confusing.
> 
> The other intent was to only match the pci paths for the controllers we 
> cared about.  That does not appear to be a part of this patch.

Can you define "we cared about".  Don't we care about everything listed in
mdadm.conf??


> 
> >
> >>
> >> 2/ I would be good to process the type=disk or type=part part of the
> >>     policy into the rules file as well.
> > OK
> >
> >>
> >> 3/ I'm not very comfortable with hard-coding the name of the
> >>     file to be created in the rules.d directory.  Maybe usage could be
> >>        --activate-domains=63-md-whatever
> > Good idea, but only if we store our rules in /dev/.udev/rules.d. Otherwise it would be difficult to maintain all generated rules and remove the old ones... I would leave default if not given by user, but one can pass any file name.
> 
> The issue is that this namespace belongs to the distro and since they 
> need to modify initscripts to turn this feature on might as well dump 
> the entirety of the naming responsibility to the user.
> 
> >> 4/ I don't think it is good to have an incomplete file in rules.d that udev
> >>     might accidentally read.  We should create the file with a name with a
> >>     leading '.' (assuming udev ignores those, I haven't checked) and then
> >>     rename it after it has been completely written.
> > You're right. In theory, such partial udev rules are excluded when udev can't interpret them properly. I have looked into udev's sources and found that it looks for "*.rules" files. All other file extensions are ignored. Files with leading dots are also omitted. I would prefer to create<name>.temp file and then rename it into<name>.rules.
> 
> There must be an existing convention for this sort of the thing, if so 
> let's not invent another one.

We could avoid both these issues by just writing the new rules file to stdout.
When when the init script gets it wrong, it isn't our fault :-)

But I don't really like that.  At least there should be a simple and uniform
way to propagate any mdadm.conf changes into udev.

Maybe the name of the rules file should be given in mdadm.conf, and e.g.
  mdadm --check-config
would report any syntax errors, report any inconsistencies with current
arrays, and update the udev file if necessary..

Maybe leave that for 3.2.1, and just support '--activate-domains=filename'
for now.

???

NeilBrown


> 
> --
> Dan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Autorebuild, new dynamic udev rules for hot-plugs

[Dan Williams]

On 11/22/2010 5:17 PM, Neil Brown wrote:
> On Mon, 22 Nov 2010 16:11:41 -0800
> Dan Williams<dan.j.williams@intel.com>  wrote:
>
>> On 11/22/2010 3:50 PM, Hawrylewicz Czarnowski, Przemyslaw wrote:
>>>> Four comments.
>>>>
>>>> 1/ I wouldn't write a file in /lib/udev/rules.d/
>>>>     I think it should be written to "/dev/.udev/rules.d/"
>>>>     which is referred to as the "temporary rules directory"
>>>>     in the udev documentation.
>>> I am not sure if it is what we are looking for. Temporary means they disappear after reboot. It is OK as cold-plug does not need support for bare disks (or maybe I am wrong?). But in such case, one who wants to use autorebuild should invoke mdadm --activate-domains for example in /etc/init.d/local.boot or somewhere else. Second idea here is to use ActivateDomain() when one starts monitor with autorebuild enabled. Which one? I would prefer to leave it as it was written initially (considering comment #4). Then, if one removes policies from config, invoking --activate-domains should reset/remove rules (but see #3)
>>
>> The intent was always to have this be something reinitialized at boot.
>> Putting these in the temporary rule directory also precludes them from
>> being added to the initramfs where they are not needed / potentially
>> confusing.
>>
>> The other intent was to only match the pci paths for the controllers we
>> cared about.  That does not appear to be a part of this patch.
>
> Can you define "we cared about".  Don't we care about everything listed in
> mdadm.conf??

A hot plug event outside of ahci (in raid mode), or the upcoming isci 
driver needs to be ignored and an error thrown on activate if we can 
unambiguously determine that the domain defines firmware unreachable 
devices.

The IMSM_NO_PLATFORM debug environment variable can override this 
behavior, or in the ahci case you can run in raid disabled mode.  I need 
to check if the same raid disabled case holds for isci.

>
> We could avoid both these issues by just writing the new rules file to stdout.
> When when the init script gets it wrong, it isn't our fault :-)
>
> But I don't really like that.  At least there should be a simple and uniform
> way to propagate any mdadm.conf changes into udev.
>
> Maybe the name of the rules file should be given in mdadm.conf, and e.g.
>    mdadm --check-config
> would report any syntax errors, report any inconsistencies with current
> arrays, and update the udev file if necessary..
>
> Maybe leave that for 3.2.1, and just support '--activate-domains=filename'
> for now.
>
> ???

A more generic mdadm.conf checker sounds like a good idea in general.

--
Dan

Re: Autorebuild, new dynamic udev rules for hot-plugs

[Neil Brown]

On Mon, 22 Nov 2010 21:04:07 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> On 11/22/2010 5:17 PM, Neil Brown wrote:
> > On Mon, 22 Nov 2010 16:11:41 -0800
> > Dan Williams<dan.j.williams@intel.com>  wrote:
> >
> >> On 11/22/2010 3:50 PM, Hawrylewicz Czarnowski, Przemyslaw wrote:
> >>>> Four comments.
> >>>>
> >>>> 1/ I wouldn't write a file in /lib/udev/rules.d/
> >>>>     I think it should be written to "/dev/.udev/rules.d/"
> >>>>     which is referred to as the "temporary rules directory"
> >>>>     in the udev documentation.
> >>> I am not sure if it is what we are looking for. Temporary means they disappear after reboot. It is OK as cold-plug does not need support for bare disks (or maybe I am wrong?). But in such case, one who wants to use autorebuild should invoke mdadm --activate-domains for example in /etc/init.d/local.boot or somewhere else. Second idea here is to use ActivateDomain() when one starts monitor with autorebuild enabled. Which one? I would prefer to leave it as it was written initially (considering comment #4). Then, if one removes policies from config, invoking --activate-domains should reset/remove rules (but see #3)
> >>
> >> The intent was always to have this be something reinitialized at boot.
> >> Putting these in the temporary rule directory also precludes them from
> >> being added to the initramfs where they are not needed / potentially
> >> confusing.
> >>
> >> The other intent was to only match the pci paths for the controllers we
> >> cared about.  That does not appear to be a part of this patch.
> >
> > Can you define "we cared about".  Don't we care about everything listed in
> > mdadm.conf??
> 
> A hot plug event outside of ahci (in raid mode), or the upcoming isci 
> driver needs to be ignored and an error thrown on activate if we can 
> unambiguously determine that the domain defines firmware unreachable 
> devices.


I can agree that a hot plug event for a non-firmware-reachable device should
not cause that device to be added to an imsm array.  But the domain setting
should stop that happening already.

I don't agree (as I *think* you are saying) that hot plug events on such
devices should be completely ignored by mdadm.  But as I am very surprised
that you would say that, I suspect I'm misunderstanding.


NeilBrown




> 
> The IMSM_NO_PLATFORM debug environment variable can override this 
> behavior, or in the ahci case you can run in raid disabled mode.  I need 
> to check if the same raid disabled case holds for isci.
> 
> >
> > We could avoid both these issues by just writing the new rules file to stdout.
> > When when the init script gets it wrong, it isn't our fault :-)
> >
> > But I don't really like that.  At least there should be a simple and uniform
> > way to propagate any mdadm.conf changes into udev.
> >
> > Maybe the name of the rules file should be given in mdadm.conf, and e.g.
> >    mdadm --check-config
> > would report any syntax errors, report any inconsistencies with current
> > arrays, and update the udev file if necessary..
> >
> > Maybe leave that for 3.2.1, and just support '--activate-domains=filename'
> > for now.
> >
> > ???
> 
> A more generic mdadm.conf checker sounds like a good idea in general.
> 
> --
> Dan

Re: Autorebuild, new dynamic udev rules for hot-plugs

[Dan Williams]

On 11/22/2010 9:27 PM, Neil Brown wrote:
> On Mon, 22 Nov 2010 21:04:07 -0800
> Dan Williams<dan.j.williams@intel.com>  wrote:
>
>> On 11/22/2010 5:17 PM, Neil Brown wrote:
>>> On Mon, 22 Nov 2010 16:11:41 -0800
>>> Dan Williams<dan.j.williams@intel.com>   wrote:
>>>
>>>> On 11/22/2010 3:50 PM, Hawrylewicz Czarnowski, Przemyslaw wrote:
>>>>>> Four comments.
>>>>>>
>>>>>> 1/ I wouldn't write a file in /lib/udev/rules.d/
>>>>>>      I think it should be written to "/dev/.udev/rules.d/"
>>>>>>      which is referred to as the "temporary rules directory"
>>>>>>      in the udev documentation.
>>>>> I am not sure if it is what we are looking for. Temporary means they disappear after reboot. It is OK as cold-plug does not need support for bare disks (or maybe I am wrong?). But in such case, one who wants to use autorebuild should invoke mdadm --activate-domains for example in /etc/init.d/local.boot or somewhere else. Second idea here is to use ActivateDomain() when one starts monitor with autorebuild enabled. Which one? I would prefer to leave it as it was written initially (considering comment #4). Then, if one removes policies from config, invoking --activate-domains should reset/remove rules (but see #3)
>>>>
>>>> The intent was always to have this be something reinitialized at boot.
>>>> Putting these in the temporary rule directory also precludes them from
>>>> being added to the initramfs where they are not needed / potentially
>>>> confusing.
>>>>
>>>> The other intent was to only match the pci paths for the controllers we
>>>> cared about.  That does not appear to be a part of this patch.
>>>
>>> Can you define "we cared about".  Don't we care about everything listed in
>>> mdadm.conf??
>>
>> A hot plug event outside of ahci (in raid mode), or the upcoming isci
>> driver needs to be ignored and an error thrown on activate if we can
>> unambiguously determine that the domain defines firmware unreachable
>> devices.
>
>
> I can agree that a hot plug event for a non-firmware-reachable device should
> not cause that device to be added to an imsm array.  But the domain setting
> should stop that happening already.
>
> I don't agree (as I *think* you are saying) that hot plug events on such
> devices should be completely ignored by mdadm.  But as I am very surprised
> that you would say that, I suspect I'm misunderstanding.

Well, this comes back to the idea that the user need not be burdened 
with figuring out the pci-device or sas-domain-topology paths of a raid 
controller, especially if that controller changes paths from boot-to-boot.

I see your point that if we define a maximal domain the 
controller/firmware constraints can be applied late to handle the 
clarification.  But if everything is interrogated in this fashion then 
what is the point of dynamically limiting the hot plug path set?  Do we 
need to require explicit definition of a default catch-all domain for 
out-of-firmware-bounds devices?  Now I think I'm the one that is 
misunderstanding :-)

--
Dan

RE: Autorebuild, new dynamic udev rules for hot-plugs

[Hawrylewicz Czarnowski, Przemyslaw]

> -----Original Message-----
> From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
> owner@vger.kernel.org] On Behalf Of Neil Brown
> Sent: Tuesday, November 23, 2010 2:17 AM
> To: Williams, Dan J
> Cc: Hawrylewicz Czarnowski, Przemyslaw; linux-raid@vger.kernel.org;
> Neubauer, Wojciech; Ciechanowski, Ed; Labun, Marcin; Czarnowska, Anna
> Subject: Re: Autorebuild, new dynamic udev rules for hot-plugs
> 
> On Mon, 22 Nov 2010 16:11:41 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > On 11/22/2010 3:50 PM, Hawrylewicz Czarnowski, Przemyslaw wrote:
> > >> Four comments.
> > >>
> > >> 1/ I wouldn't write a file in /lib/udev/rules.d/
> > >>    I think it should be written to "/dev/.udev/rules.d/"
> > >>    which is referred to as the "temporary rules directory"
> > >>    in the udev documentation.
> > > I am not sure if it is what we are looking for. Temporary means they
> disappear after reboot. It is OK as cold-plug does not need support for
> bare disks (or maybe I am wrong?). But in such case, one who wants to use
> autorebuild should invoke mdadm --activate-domains for example in
> /etc/init.d/local.boot or somewhere else. Second idea here is to use
> ActivateDomain() when one starts monitor with autorebuild enabled. Which
> one? I would prefer to leave it as it was written initially (considering
> comment #4). Then, if one removes policies from config, invoking --
> activate-domains should reset/remove rules (but see #3)
> >
> > The intent was always to have this be something reinitialized at boot.
> > Putting these in the temporary rule directory also precludes them from
> > being added to the initramfs where they are not needed / potentially
> > confusing.
> >
> > The other intent was to only match the pci paths for the controllers we
> > cared about.  That does not appear to be a part of this patch.
> 
> Can you define "we cared about".  Don't we care about everything listed in
> mdadm.conf??
> 
> 
> >
> > >
> > >>
> > >> 2/ I would be good to process the type=disk or type=part part of the
> > >>     policy into the rules file as well.
> > > OK
> > >
> > >>
> > >> 3/ I'm not very comfortable with hard-coding the name of the
> > >>     file to be created in the rules.d directory.  Maybe usage could be
> > >>        --activate-domains=63-md-whatever
> > > Good idea, but only if we store our rules in /dev/.udev/rules.d.
> Otherwise it would be difficult to maintain all generated rules and remove
> the old ones... I would leave default if not given by user, but one can
> pass any file name.
> >
> > The issue is that this namespace belongs to the distro and since they
> > need to modify initscripts to turn this feature on might as well dump
> > the entirety of the naming responsibility to the user.
> >
> > >> 4/ I don't think it is good to have an incomplete file in rules.d that
> udev
> > >>     might accidentally read.  We should create the file with a name
> with a
> > >>     leading '.' (assuming udev ignores those, I haven't checked) and
> then
> > >>     rename it after it has been completely written.
> > > You're right. In theory, such partial udev rules are excluded when udev
> can't interpret them properly. I have looked into udev's sources and found
> that it looks for "*.rules" files. All other file extensions are ignored.
> Files with leading dots are also omitted. I would prefer to
> create<name>.temp file and then rename it into<name>.rules.
> >
> > There must be an existing convention for this sort of the thing, if so
> > let's not invent another one.
I haven't found anything similar. Just mountall, but it writes single line in one "shot"... Both options with extension or with leading dot will work.

> 
> We could avoid both these issues by just writing the new rules file to
> stdout.
> When when the init script gets it wrong, it isn't our fault :-)
I like that idea at this stage. Later on we might develop better solution (see below)

> 
> But I don't really like that.  At least there should be a simple and
> uniform
> way to propagate any mdadm.conf changes into udev.
> 
> Maybe the name of the rules file should be given in mdadm.conf, and e.g.
>   mdadm --check-config
> would report any syntax errors, report any inconsistencies with current
> arrays, and update the udev file if necessary..
> 
> Maybe leave that for 3.2.1, and just support '--activate-domains=filename'
> for now.
Let me extend this thought a little. As I mentioned above I like the idea of writing rule to stdout. Or if somebody wants to pass a file name, just write the file in current directory - similar to the way one creates mdadm.conf with mdadm --examine (but with small improvement:).

But the general problem is to find "simple and uniform way". Something distro-independent. Rules should be prepared once, right after config file is finished. Fire and forget:)
We need to handle hot/cold-plug events for action=spare, and hot-plug events for actions=spare-same-slot. Considering we use temporary udev rules directory they need to be regenerated each reboot, putting attention at the moment when we have to do it so we handle all cases. What options then?

As a last resort, maybe just a note in man pages with possibilities, leaving implementation to user/admin?

> 
> ???
> 
> NeilBrown
> 
> 

RE: Autorebuild, new dynamic udev rules for hot-plugs

[Hawrylewicz Czarnowski, Przemyslaw]

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

> -----Original Message-----
> From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
> owner@vger.kernel.org] On Behalf Of Hawrylewicz Czarnowski, Przemyslaw
> Sent: Tuesday, November 23, 2010 6:02 PM
> To: Neil Brown
> Cc: linux-raid@vger.kernel.org; Neubauer, Wojciech; Ciechanowski, Ed;
> Labun, Marcin; Czarnowska, Anna; Williams, Dan J
> Subject: RE: Autorebuild, new dynamic udev rules for hot-plugs
> 
> > -----Original Message-----
> > From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
> > owner@vger.kernel.org] On Behalf Of Neil Brown
> > Sent: Tuesday, November 23, 2010 2:17 AM
> > To: Williams, Dan J
> > Cc: Hawrylewicz Czarnowski, Przemyslaw; linux-raid@vger.kernel.org;
> > Neubauer, Wojciech; Ciechanowski, Ed; Labun, Marcin; Czarnowska, Anna
> > Subject: Re: Autorebuild, new dynamic udev rules for hot-plugs
> >
> > On Mon, 22 Nov 2010 16:11:41 -0800
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > > On 11/22/2010 3:50 PM, Hawrylewicz Czarnowski, Przemyslaw wrote:
> > > >> Four comments.
> > > >>
> > > >> 1/ I wouldn't write a file in /lib/udev/rules.d/
> > > >>    I think it should be written to "/dev/.udev/rules.d/"
> > > >>    which is referred to as the "temporary rules directory"
> > > >>    in the udev documentation.
> > > > I am not sure if it is what we are looking for. Temporary means they
> > disappear after reboot. It is OK as cold-plug does not need support for
> > bare disks (or maybe I am wrong?). But in such case, one who wants to use
> > autorebuild should invoke mdadm --activate-domains for example in
> > /etc/init.d/local.boot or somewhere else. Second idea here is to use
> > ActivateDomain() when one starts monitor with autorebuild enabled. Which
> > one? I would prefer to leave it as it was written initially (considering
> > comment #4). Then, if one removes policies from config, invoking --
> > activate-domains should reset/remove rules (but see #3)
> > >
> > > The intent was always to have this be something reinitialized at boot.
> > > Putting these in the temporary rule directory also precludes them from
> > > being added to the initramfs where they are not needed / potentially
> > > confusing.
> > >
> > > The other intent was to only match the pci paths for the controllers we
> > > cared about.  That does not appear to be a part of this patch.
> >
> > Can you define "we cared about".  Don't we care about everything listed
> in
> > mdadm.conf??
> >
> >
> > >
> > > >
> > > >>
> > > >> 2/ I would be good to process the type=disk or type=part part of the
> > > >>     policy into the rules file as well.
> > > > OK
> > > >
> > > >>
> > > >> 3/ I'm not very comfortable with hard-coding the name of the
> > > >>     file to be created in the rules.d directory.  Maybe usage could
> be
> > > >>        --activate-domains=63-md-whatever
> > > > Good idea, but only if we store our rules in /dev/.udev/rules.d.
> > Otherwise it would be difficult to maintain all generated rules and
> remove
> > the old ones... I would leave default if not given by user, but one can
> > pass any file name.
> > >
> > > The issue is that this namespace belongs to the distro and since they
> > > need to modify initscripts to turn this feature on might as well dump
> > > the entirety of the naming responsibility to the user.
> > >
> > > >> 4/ I don't think it is good to have an incomplete file in rules.d
> that
> > udev
> > > >>     might accidentally read.  We should create the file with a name
> > with a
> > > >>     leading '.' (assuming udev ignores those, I haven't checked) and
> > then
> > > >>     rename it after it has been completely written.
> > > > You're right. In theory, such partial udev rules are excluded when
> udev
> > can't interpret them properly. I have looked into udev's sources and
> found
> > that it looks for "*.rules" files. All other file extensions are ignored.
> > Files with leading dots are also omitted. I would prefer to
> > create<name>.temp file and then rename it into<name>.rules.
> > >
> > > There must be an existing convention for this sort of the thing, if so
> > > let's not invent another one.
> I haven't found anything similar. Just mountall, but it writes single line
> in one "shot"... Both options with extension or with leading dot will work.
> 
> >
> > We could avoid both these issues by just writing the new rules file to
> > stdout.
> > When when the init script gets it wrong, it isn't our fault :-)
> I like that idea at this stage. Later on we might develop better solution
> (see below)
> 
> >
> > But I don't really like that.  At least there should be a simple and
> > uniform
> > way to propagate any mdadm.conf changes into udev.
> >
> > Maybe the name of the rules file should be given in mdadm.conf, and e.g.
> >   mdadm --check-config
> > would report any syntax errors, report any inconsistencies with current
> > arrays, and update the udev file if necessary..
> >
> > Maybe leave that for 3.2.1, and just support '--activate-
> domains=filename'
> > for now.
> Let me extend this thought a little. As I mentioned above I like the idea
> of writing rule to stdout. Or if somebody wants to pass a file name, just
> write the file in current directory - similar to the way one creates
> mdadm.conf with mdadm --examine (but with small improvement:).
Replying to my last post I would like to present new patch based on the above scheme.
I also want to raise the discussion again, as this thread is dead for a while...

Please comment.

> But the general problem is to find "simple and uniform way". Something
> distro-independent. Rules should be prepared once, right after config file
> is finished. Fire and forget:)
> We need to handle hot/cold-plug events for action=spare, and hot-plug
> events for actions=spare-same-slot. Considering we use temporary udev rules
> directory they need to be regenerated each reboot, putting attention at the
> moment when we have to do it so we handle all cases. What options then?
> 
> As a last resort, maybe just a note in man pages with possibilities,
> leaving implementation to user/admin?
> 
> >
> > ???
> >
> > NeilBrown
> >
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: 0001-Dynamic-hot-plug-udev-rules-for-policies.patch --]
[-- Type: application/octet-stream, Size: 8176 bytes --]

From c0aecd4dd96691e8bfa6f2dc187261ec8bb2c5a2 Mon Sep 17 00:00:00 2001
From: Przemyslaw Czarnowski <przemyslaw.hawrylewicz.czarnowski@intel.com>
Date: Thu, 23 Dec 2010 16:35:01 +0100
Subject: [PATCH] Dynamic hot-plug udev rules for policies
Cc: linux-raid@vger.kernel.org, Williams, Dan J <dan.j.williams@intel.com>, Ciechanowski, Ed <ed.ciechanowski@intel.com>

When introducing policies, new hot-plug rules were added to support
bare disks. Mdadm was started for each hot plugged block device
to determine if it could be used as spare or as a replacement member for
degraded array.
This patch introduces limitation of range of devices that are handled
by mdadm.
It limits them to the ones specified in domains associated with
the actions: spare-same-port, spare and spare-force.
In order to enable hot-plug for bare disks one must update udev rules
with command

	mdadm --activate-domains[=filename]

Above command writes udev rule configuration to stdout. If 'filename'
is given output is written to the file provided as parameter. It is up
to system administrator what should be done later. To make such rule
permanent (i.e. remain after reboot) rule should be writen to
/lib/udev/rules.d directory. Other cases will just need to write it to
/dev/.udev/rules.d directory where temporary rules lies. One should be
aware of the meaning of names/priorities of the udev rules.

After mdadm.conf is changed one is obliged to re-run
"mdadm --activate-domains" command in order to bring the system
configuration up to date.
All hot-plugged disks containing metadata are still handled by existing
rules.

Note: this patch is just a proposition to minimize overhead of using mdadm
for each plugged block device.

Signed-off-by: Przemyslaw Czarnowski <przemyslaw.hawrylewicz.czarnowski@intel.com>
---
 ReadMe.c |    1 +
 mdadm.c  |   18 ++++++++
 mdadm.h  |    2 +
 policy.c |  141 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 162 insertions(+), 0 deletions(-)

diff --git a/ReadMe.c b/ReadMe.c
index 5714849..cf41fa5 100644
--- a/ReadMe.c
+++ b/ReadMe.c
@@ -110,6 +110,7 @@ struct option long_options[] = {
     {"detail-platform", 0, 0, DetailPlatform},
     {"kill-subarray", 1, 0, KillSubarray},
     {"update-subarray", 1, 0, UpdateSubarray},
+    {"activate-domains", 2, 0, ActivateDomains},
 
     /* synonyms */
     {"monitor",   0, 0, 'F'},
diff --git a/mdadm.c b/mdadm.c
index 2ffe94f..f3021cc 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -106,6 +106,7 @@ int main(int argc, char *argv[])
 	int auto_update_home = 0;
 	char *subarray = NULL;
 	char *remove_path = NULL;
+	char *udev_filename = NULL;
 
 	int print_help = 0;
 	FILE *outf;
@@ -234,6 +235,7 @@ int main(int argc, char *argv[])
 				}
 				subarray = optarg;
 			}
+		case ActivateDomains:
 		case 'K': if (!mode) newmode = MISC; break;
 		case NoSharing: newmode = MONITOR; break;
 		}
@@ -929,6 +931,20 @@ int main(int argc, char *argv[])
 			}
 			devmode = opt;
 			continue;
+		case O(MISC, ActivateDomains):
+			if (devmode && devmode != opt) {
+				fprintf(stderr, Name ": --ActivateDomains must"
+						     " be the only option.\n");
+			} else {
+				if (udev_filename)
+				fprintf(stderr, Name ": only specify one udev "
+						     "rule filename. %s ignored.\n",
+					optarg);
+				else
+					udev_filename = optarg;
+			}
+			devmode = opt;
+			continue;
 		case O(MISC,'t'):
 			test = 1;
 			continue;
@@ -1493,6 +1509,8 @@ int main(int argc, char *argv[])
 						free_mdstat(ms);
 					} while (!last && err);
 					if (err) rv |= 1;
+				} else if (devmode == ActivateDomains) {
+					rv = Activate_Domains(udev_filename);
 				} else {
 					fprintf(stderr, Name ": No devices given.\n");
 					exit(2);
diff --git a/mdadm.h b/mdadm.h
index 36124de..1242015 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -311,6 +311,7 @@ enum special_options {
 	Bitmap,
 	RebuildMapOpt,
 	InvalidBackup,
+	ActivateDomains
 };
 
 /* structures read from config file */
@@ -1031,6 +1032,7 @@ extern int CreateBitmap(char *filename, int force, char uuid[16],
 			unsigned long long array_size,
 			int major);
 extern int ExamineBitmap(char *filename, int brief, struct supertype *st);
+extern int Activate_Domains(char *rule_name);
 extern int bitmap_update_uuid(int fd, int *uuid, int swap);
 extern unsigned long bitmap_sectors(struct bitmap_super_s *bsb);
 
diff --git a/policy.c b/policy.c
index ba976db..81d3e70 100644
--- a/policy.c
+++ b/policy.c
@@ -764,3 +764,144 @@ int policy_check_path(struct mdinfo *disk, struct map_ent *array)
 	fclose(f);
 	return rv == 5;
 }
+
+/* invocation of udev rule file */
+char udev_template_start[] =
+"# do not edit this file, it will be overwritten on update\n"
+"\n"
+"SUBSYSTEM!=\"block\", GOTO=\"md_autorebuild_end\"\n"
+"\n"
+"ENV{ID_FS_TYPE}==\"linux_raid_member\", GOTO=\"md_autorebuild_end\"\n"
+"ENV{ID_FS_TYPE}==\"isw_raid_member\", GOTO=\"md_autorebuild_end\"\n"
+"\n";
+
+/* ending of udev rule file */
+char udev_template_end[] =
+"\n"
+"LABEL=\"md_autorebuild_end\"\n"
+"\n";
+
+/* find rule named rule_type and return its value */
+char *find_rule(struct rule *rule, char *rule_type)
+{
+	while (rule) {
+		if (rule->name == rule_type)
+			return rule->value;
+
+		rule = rule->next;
+	}
+	return NULL;
+}
+
+#define UDEV_RULE_FORMAT \
+"ACTION==\"add\", KERNEL!=\"md*\" ENV{ID_PATH}==\"%s\" " \
+"RUN+=\"/sbin/mdadm --incremental $env{DEVNAME}\", " \
+"GOTO=\"md_autorebuild_end\"\n" \
+
+/* Write rule in the rule file. Use format from UDEV_RULE_FORMAT */
+int write_rule(struct rule *rule, int fd)
+{
+	char line[1024];
+	char *r = find_rule(rule, rule_path);
+	if (!r)
+		return -1;
+
+	snprintf(line, sizeof(line) - 1, UDEV_RULE_FORMAT, r);
+	return write(fd, line, strlen(line));
+}
+
+/* Generate single entry in udev rule basing on POLICY line found in config
+ * file. Take only those with paths, only first occurrence if paths are equal
+ * and if actions supports handling of spares (>=act_spare_same_slot)
+ */
+int generate_entries(int fd)
+{
+	struct pol_rule *loop, *dup;
+	char *loop_value, *dup_value;
+	int duplicate;
+	int written = 0;
+
+	for (loop = config_rules; loop; loop = loop->next) {
+		if (loop->type != rule_policy && loop->type != rule_part)
+			continue;
+		duplicate = 0;
+
+		/* only policies with paths and with actions supporting
+		 * bare disks are considered */
+		loop_value = find_rule(loop->rule, pol_act);
+		if (!loop_value || map_act(loop_value) < act_spare_same_slot)
+			continue;
+		loop_value = find_rule(loop->rule, rule_path);
+		if (!loop_value)
+			continue;
+		for (dup = config_rules; dup != loop; dup = dup->next) {
+			if (dup->type != rule_policy && loop->type != rule_part)
+				continue;
+			dup_value = find_rule(dup->rule, pol_act);
+			if (!dup_value || map_act(dup_value) < act_spare_same_slot)
+				continue;
+			dup_value = find_rule(dup->rule, rule_path);
+			if (!dup_value)
+				continue;
+			if (strcmp(loop_value, dup_value) == 0) {
+				duplicate = 1;
+				break;
+			}
+		}
+
+		/* not a dup or first occurrence */
+		if (!duplicate) {
+			if (write_rule(loop->rule, fd) == -1)
+				return 0;
+			written++;
+		}
+	}
+	return written;
+}
+
+/* Activate_Domains routine creates dynamic udev rules used to handle
+ * hot-plug events for bare devices (and making them spares)
+ */
+int Activate_Domains(char *rule_name)
+{
+	int fd = 0;
+	int rv;
+	char udev_rule_file[PATH_MAX];
+
+	if (rule_name)
+		fd = creat(udev_rule_file,
+			   S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+	else
+		fd = dup(fileno(stdout));
+	if (fd == -1)
+		return 1;
+
+	/* write static invocation */
+	if (write(fd, udev_template_start,
+		sizeof(udev_template_start) - 1) == -1) {
+		close(fd);
+		if (rule_name)
+			unlink(udev_rule_file);
+		return 1;
+	}
+
+	/* iterate, if none created or error occurred, remove file */
+	rv = generate_entries(fd);
+	if (rv <= 0) {
+		close(fd);
+		if (rule_name)
+			unlink(udev_rule_file);
+		return rv == -1 ? -1 : 0;
+	}
+
+	/* write ending */
+	if (write(fd, udev_template_end, sizeof(udev_template_end) - 1) == -1) {
+		close(fd);
+		if (rule_name)
+			unlink(udev_rule_file);
+		return 1;
+	}
+
+	close(fd);
+	return 0;
+}
-- 
1.7.1

Re: [Patch 00/17] Autorebuild

[Neil Brown]

On Thu, 18 Nov 2010 23:14:49 +0000
"Czarnowska, Anna" <anna.czarnowska@intel.com> wrote:

> Hi Neil,
> I started testing new code today. Just the Incremental part.
> 
> There are few problems:
> 1. Cookie file is cleared before it is read so spare-same-slot can't work. It should be just open for reading. (probably a typo)

Yes, just a typo.  Fixed.


> 2. Container uuid instead of subarray uuid is written in cookie file, so for ddf it may not be clear which subarray used the slot.

This is deliberate.  It is really up to the ddf spare-assignment handler (in
super-ddf ... though it isn't written yet) to decide which sub array gets
which part of the new disk.  If an admin wants more control they need to do
it at a different level - probably having separate ddf containers in separate
domains.

> 3. Incremental fail does not work for external metadata. Przemek's original patch did fail the disk in subarrays. Now Manage_subdevs tries to fail a disk in container while subarray is expected. Do you intend to change Manage_subdevs to take a container?

Yes... I didn't notice that change in the patch.  This is one of the reasons
I like each patch to just make one change.
I have added a new patch which fails all the contained arrays before removing
from the container (though I haven't tested it yet).


> 4. With spare-same-slot when there is a cookie and disk has no metadata then we probably shouldn't look at domains. Just add.
> 

I disagree.  We must always check domains.
Why do you think we should ignore domains in that case?

Thanks for the testing.  I'll push a new devel-3.2 out later today.

NeilBrown

RE: [Patch 00/17] Autorebuild

[Czarnowska, Anna]

> > 4. With spare-same-slot when there is a cookie and disk has no
> metadata then we probably shouldn't look at domains. Just add.
> >
> 
> I disagree.  We must always check domains.
> Why do you think we should ignore domains in that case?
 
Easy example is: we have an array made on two disks sda and sdb.
Sda has domain d1. Sdb has domain d2. This is perfectly ok for Create.
Now sdb fails and we put a new disk in that place. It gets domain d2 because it is in the same slot as old disk.
When the array went degraded and sdb was removed, the domain for the array was reduced to d1 only.
New disk does not match any more so it is not added.

I think we should still add it because we have a file saying that this slot belongs to that array.
There is also controller domain that has the special meaning but I don't think it is a problem.
If the user originally created the array spanning different controllers why wouldn't we take a replacement occupying the same slot as original member?

My conclusion is: we should ignore domains when there is a cookie.

Note that we don't look at domains at all when we add a disk with spare metadata. Here it is indeed very much needed.
When someone creates some arrays and adds some spares, additionally defines domains to keep all related disks together, then he may be disappointed seeing that after reboot all spares end up in one container anyway. This happens for imsm and because of this issue some spare that was meant for a different array may be used in the first array from config instead (all spares will be added there regardless of domains).

Anna

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
z siedziba w Gdansku
ul. Slowackiego 173
80-298 Gdansk

Sad Rejonowy Gdansk Polnoc w Gdansku, 
VII Wydzial Gospodarczy Krajowego Rejestru Sadowego, 
numer KRS 101882

NIP 957-07-52-316
Kapital zakladowy 200.000 zl

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [Patch 00/17] Autorebuild

[Neil Brown]

On Mon, 22 Nov 2010 15:08:27 +0000
"Czarnowska, Anna" <anna.czarnowska@intel.com> wrote:

> > > 4. With spare-same-slot when there is a cookie and disk has no
> > metadata then we probably shouldn't look at domains. Just add.
> > >
> > 
> > I disagree.  We must always check domains.
> > Why do you think we should ignore domains in that case?
>  
> Easy example is: we have an array made on two disks sda and sdb.
> Sda has domain d1. Sdb has domain d2. This is perfectly ok for Create.
> Now sdb fails and we put a new disk in that place. It gets domain d2 because it is in the same slot as old disk.
> When the array went degraded and sdb was removed, the domain for the array was reduced to d1 only.
> New disk does not match any more so it is not added.
> 
> I think we should still add it because we have a file saying that this slot belongs to that array.
> There is also controller domain that has the special meaning but I don't think it is a problem.
> If the user originally created the array spanning different controllers why wouldn't we take a replacement occupying the same slot as original member?
> 
> My conclusion is: we should ignore domains when there is a cookie.

Yes, that make sense.  Thanks for the explanation.

So when we find a device with a cookie file that identifies a particular
array, we allow that device to be added to that array without further
reference to domain.
Sounds good.  It should go in the man-page somewhere of course.

> 
> Note that we don't look at domains at all when we add a disk with spare metadata. Here it is indeed very much needed.
> When someone creates some arrays and adds some spares, additionally defines domains to keep all related disks together, then he may be disappointed seeing that after reboot all spares end up in one container anyway. This happens for imsm and because of this issue some spare that was meant for a different array may be used in the first array from config instead (all spares will be added there regardless of domains).
> 

Presumably is required for both -I and -A.
Normally when assembling an array we ignore domains because if two devices
claim to be in an array, then they need to be assembled together no matter
what domains say.
But for truly global spares, the metadata doesn't tell us much, so we only
add such a spare to an array for which the domain says it is OK.

This is a little awkward for -I as if we get a spare first we have no idea
what to do with it.
I think we had an idea once of having a container for global spares.  We
could proceed with that, putting spares in that container as they are found.
and maybe have Monitor() move these spares to an active container if one is
found with a domain match.  Maybe?

NeilBrown



> Anna
> 
> ---------------------------------------------------------------------
> Intel Technology Poland sp. z o.o.
> z siedziba w Gdansku
> ul. Slowackiego 173
> 80-298 Gdansk
> 
> Sad Rejonowy Gdansk Polnoc w Gdansku, 
> VII Wydzial Gospodarczy Krajowego Rejestru Sadowego, 
> numer KRS 101882
> 
> NIP 957-07-52-316
> Kapital zakladowy 200.000 zl
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.

RE: [Patch 00/17] Autorebuild

[Labun, Marcin]

> -----Original Message-----
> From: Neil Brown [mailto:neilb@suse.de]
> Sent: Tuesday, November 23, 2010 2:34 AM
> To: Czarnowska, Anna
> Cc: linux-raid@vger.kernel.org; Neubauer, Wojciech; Williams, Dan J;
> Ciechanowski, Ed; Labun, Marcin; Hawrylewicz Czarnowski, Przemyslaw
> Subject: Re: [Patch 00/17] Autorebuild
> 
> On Mon, 22 Nov 2010 15:08:27 +0000
> "Czarnowska, Anna" <anna.czarnowska@intel.com> wrote:
> 
> > > > 4. With spare-same-slot when there is a cookie and disk has no
> > > metadata then we probably shouldn't look at domains. Just add.
> > > >
> > >
> > > I disagree.  We must always check domains.
> > > Why do you think we should ignore domains in that case?
> >
> > Easy example is: we have an array made on two disks sda and sdb.
> > Sda has domain d1. Sdb has domain d2. This is perfectly ok for
> Create.
> > Now sdb fails and we put a new disk in that place. It gets domain d2
> because it is in the same slot as old disk.
> > When the array went degraded and sdb was removed, the domain for the
> array was reduced to d1 only.
> > New disk does not match any more so it is not added.
> >
> > I think we should still add it because we have a file saying that
> this slot belongs to that array.
> > There is also controller domain that has the special meaning but I
> don't think it is a problem.
> > If the user originally created the array spanning different
> controllers why wouldn't we take a replacement occupying the same slot
> as original member?
> >
> > My conclusion is: we should ignore domains when there is a cookie.
> 
> Yes, that make sense.  Thanks for the explanation.
> 
> So when we find a device with a cookie file that identifies a
> particular
> array, we allow that device to be added to that array without further
> reference to domain.
> Sounds good.  It should go in the man-page somewhere of course.
> 
> >
> > Note that we don't look at domains at all when we add a disk with
> spare metadata. Here it is indeed very much needed.
> > When someone creates some arrays and adds some spares, additionally
> defines domains to keep all related disks together, then he may be
> disappointed seeing that after reboot all spares end up in one
> container anyway. This happens for imsm and because of this issue some
> spare that was meant for a different array may be used in the first
> array from config instead (all spares will be added there regardless of
> domains).
> >
> 
> Presumably is required for both -I and -A.
> Normally when assembling an array we ignore domains because if two
> devices
> claim to be in an array, then they need to be assembled together no
> matter
> what domains say.
> But for truly global spares, the metadata doesn't tell us much, so we
> only
> add such a spare to an array for which the domain says it is OK.
> 
> This is a little awkward for -I as if we get a spare first we have no
> idea
> what to do with it.
> I think we had an idea once of having a container for global spares.
> We
> could proceed with that, putting spares in that container as they are
> found.
> and maybe have Monitor() move these spares to an active container if
> one is
> found with a domain match.  Maybe?

Sounds good. So after Monitor initial run all spares fall in the right container, and the ones left will have no match.
Maybe it is easier then changing Incremental and Assembly to look for right container. 
Once we have presented two phase assembly that solved that problem (adding spares after all disks are placed in containers).
Marcin


> 
> NeilBrown
> 
> 
> 
> > Anna
> >
> > ---------------------------------------------------------------------
> > Intel Technology Poland sp. z o.o.
> > z siedziba w Gdansku
> > ul. Slowackiego 173
> > 80-298 Gdansk
> >
> > Sad Rejonowy Gdansk Polnoc w Gdansku,
> > VII Wydzial Gospodarczy Krajowego Rejestru Sadowego,
> > numer KRS 101882
> >
> > NIP 957-07-52-316
> > Kapital zakladowy 200.000 zl
> >
> > This e-mail and any attachments may contain confidential material for
> > the sole use of the intended recipient(s). Any review or distribution
> > by others is strictly prohibited. If you are not the intended
> > recipient, please contact the sender and delete all copies.

RE: [Patch 00/17] Autorebuild

[Czarnowska, Anna]

Back to spares and domains...

> > > Note that we don't look at domains at all when we add a disk with
> > spare metadata. Here it is indeed very much needed.
> > > When someone creates some arrays and adds some spares, additionally
> > defines domains to keep all related disks together, then he may be
> > disappointed seeing that after reboot all spares end up in one
> > container anyway. This happens for imsm and because of this issue
> some
> > spare that was meant for a different array may be used in the first
> > array from config instead (all spares will be added there regardless
> of
> > domains).
> > >
> >
> > Presumably is required for both -I and -A.
> > Normally when assembling an array we ignore domains because if two
> > devices
> > claim to be in an array, then they need to be assembled together no
> > matter
> > what domains say.
> > But for truly global spares, the metadata doesn't tell us much, so we
> > only
> > add such a spare to an array for which the domain says it is OK.

The problem is: we don't know the domain of an array until it is fully assembled
(we know all devices in it).

So in Assembly we can either
	- mark the spares and try them later (when choosing devices for an array we
	skip spares in the first run and add a second run choosing
	spares for an array with domain check against all members found in first run)
	this could be done with uuid_match_any for spares.
or
	- put spares in separate container and let Monitor take them out when they are needed.
	Here spares can't match any array. 

In both cases we must stop giving all imsm spares the uuid of the first array from config.

> >
> > This is a little awkward for -I as if we get a spare first we have no
> > idea
> > what to do with it.
> > I think we had an idea once of having a container for global spares.
> > We
> > could proceed with that, putting spares in that container as they are
> > found.

This is easily achieved for Incremental by just giving one uuid to all imsm spares.
This uuid cannot be used by any array. Probably just 0:0:0:0 would do.
This solution requires adding a special case to
Assemble or else no spares will be assembled.

If uuid_match_any is used for spares in Incremental then we don't know where to add them and exit.
We could take the list of all arrays from mdstat (not config) and try matching domains 
but then the final result will depend on the order of devices as some arrays may appear later
and some may not have complete set of devices when we look at the spare.

Probably the simplest solution is to put all imsm spares in separate container.
We don't risk then that any of them will be used in an array that is in other domain
and Monitor can always move them to their domain when needed. 
Monitor could also clean out this container when starting, even if there were no degraded arrays.

> > and maybe have Monitor() move these spares to an active container if
> > one is
> > found with a domain match.  Maybe?
> 
> Sounds good. So after Monitor initial run all spares fall in the right
> container, and the ones left will have no match.
> Maybe it is easier then changing Incremental and Assembly to look for
> right container.

Assembly would still require modification.

> Once we have presented two phase assembly that solved that problem
> (adding spares after all disks are placed in containers).

This added spares where they should be. But would require dealing with spares properly also in Incremental.
Or should we try to do the right thing in Assemble but throw all imsm spares into one container in Incremental?
What do you think?

Anna

Re: [Patch 00/17] Autorebuild

[Neil Brown]

On Thu, 9 Dec 2010 11:40:43 +0000 "Czarnowska, Anna"
<anna.czarnowska@intel.com> wrote:

> Back to spares and domains...
> 
> > > > Note that we don't look at domains at all when we add a disk with
> > > spare metadata. Here it is indeed very much needed.
> > > > When someone creates some arrays and adds some spares, additionally
> > > defines domains to keep all related disks together, then he may be
> > > disappointed seeing that after reboot all spares end up in one
> > > container anyway. This happens for imsm and because of this issue
> > some
> > > spare that was meant for a different array may be used in the first
> > > array from config instead (all spares will be added there regardless
> > of
> > > domains).
> > > >
> > >
> > > Presumably is required for both -I and -A.
> > > Normally when assembling an array we ignore domains because if two
> > > devices
> > > claim to be in an array, then they need to be assembled together no
> > > matter
> > > what domains say.
> > > But for truly global spares, the metadata doesn't tell us much, so we
> > > only
> > > add such a spare to an array for which the domain says it is OK.
> 
> The problem is: we don't know the domain of an array until it is fully assembled
> (we know all devices in it).
> 
> So in Assembly we can either
> 	- mark the spares and try them later (when choosing devices for an array we
> 	skip spares in the first run and add a second run choosing
> 	spares for an array with domain check against all members found in first run)
> 	this could be done with uuid_match_any for spares.
> or
> 	- put spares in separate container and let Monitor take them out when they are needed.
> 	Here spares can't match any array. 
> 
> In both cases we must stop giving all imsm spares the uuid of the first array from config.
> 
> > >
> > > This is a little awkward for -I as if we get a spare first we have no
> > > idea
> > > what to do with it.
> > > I think we had an idea once of having a container for global spares.
> > > We
> > > could proceed with that, putting spares in that container as they are
> > > found.
> 
> This is easily achieved for Incremental by just giving one uuid to all imsm spares.
> This uuid cannot be used by any array. Probably just 0:0:0:0 would do.
> This solution requires adding a special case to
> Assemble or else no spares will be assembled.
> 
> If uuid_match_any is used for spares in Incremental then we don't know where to add them and exit.
> We could take the list of all arrays from mdstat (not config) and try matching domains 
> but then the final result will depend on the order of devices as some arrays may appear later
> and some may not have complete set of devices when we look at the spare.
> 
> Probably the simplest solution is to put all imsm spares in separate container.
> We don't risk then that any of them will be used in an array that is in other domain
> and Monitor can always move them to their domain when needed. 
> Monitor could also clean out this container when starting, even if there were no degraded arrays.
> 
> > > and maybe have Monitor() move these spares to an active container if
> > > one is
> > > found with a domain match.  Maybe?
> > 
> > Sounds good. So after Monitor initial run all spares fall in the right
> > container, and the ones left will have no match.
> > Maybe it is easier then changing Incremental and Assembly to look for
> > right container.
> 
> Assembly would still require modification.
> 
> > Once we have presented two phase assembly that solved that problem
> > (adding spares after all disks are placed in containers).
> 
> This added spares where they should be. But would require dealing with spares properly also in Incremental.
> Or should we try to do the right thing in Assemble but throw all imsm spares into one container in Incremental?
> What do you think?
> 
> Anna
> 

I like the idea of using a uuid of 0:0:0:0 for the container of spares.
There might need to be some subtleties in there to ensure spares with
different metadata stay separate, but I doubt that would be much of a
complication.

I don't know what you mean by trying to "do the right thing in Assemble".
It isn't clear that there is always one correct place to put a spare, though
there could be several suitable places.  So you could do "a right thing", but
not necessarily "the right thing" ??


While I like the idea of always having a separate container for all spares, I
see one problem with it.
It should work fine when using --incremental or auto assembly (-As), but if
you explicitly identify an array to start, it would be started without any
spares which might not be what you would expect.

Assemble already has a mechanism to collect all devices that could possibly
be part of an array, and then to reject those that don't fit - typically
because there is another device which claims the same slot but has a newer
event count.

We could use the same mechanism to include any global spare found into an
array being assembled.  Then once we are nearly ready to go, we determine the
domain of the array based on the domains of the non-spares and reject any
spares that don't match that domain.  I think this is similar two the
two-pass approach that was suggested previously, but fits more cleanly into
the existing infrastructure.
This would me that any spare would be associated with the first array to be
assembled for which it was compatible.  This is a little non-deterministic,
but I don't think that is a problem.

For --incremental we would still need global spare to go into a separate
container, though when it comes time to start an array, would could at that
point migrate spares from the spare-container to the new array...

I suggest we start out by just having a single container for imsm spares and
make sure that --monitor can move devices out of there as required.  Once
that works reliably we can worry about making the unusual cases work better,
and possibly migrating spares more proactively.

NeilBrown

[PATCH] fix: Monitor doesn't return after starting daemon

[Czarnowska, Anna]

From 8b2465b0d314cc93bba5797dbad9fd2813f0a79e Mon Sep 17 00:00:00 2001
From: Anna Czarnowska <anna.czarnowska@intel.com>
Date: Tue, 14 Dec 2010 12:26:33 +0100
Subject: [PATCH] fix: Monitor doesn't return after starting daemon
Cc: linux-raid@vger.kernel.org, Williams, Dan J <dan.j.williams@intel.com>, Ciechanowski, Ed <ed.ciechanowski@intel.com>

Because both parent and child process continue after make_daemon succeeds.

Signed-off-by: Anna Czarnowska <anna.czarnowska@intel.com>
---
 Monitor.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/Monitor.c b/Monitor.c
index e7f6d03..4ae1d2b 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -152,9 +152,11 @@ int Monitor(struct mddev_dev *devlist,
 	info.mailfrom = mailfrom;
 	info.dosyslog = dosyslog;
 
-	if (daemonise)
-		if (make_daemon(pidfile))
-			return 1;
+	if (daemonise) {
+		int rv = make_daemon(pidfile);
+		if (rv >= 0)
+			return rv;
+	}
 
 	if (share) 
 		if (check_one_sharer(scan))
@@ -272,7 +274,7 @@ static int make_daemon(char *pidfile)
 	dup2(0,1);
 	dup2(0,2);
 	setsid();
-	return 0;
+	return -1;
 }
 
 static int check_one_sharer(int scan)
-- 
1.7.1

Re: [PATCH] fix: Monitor doesn't return after starting daemon

[Neil Brown]

On Tue, 14 Dec 2010 14:47:27 +0000 "Czarnowska, Anna"
<anna.czarnowska@intel.com> wrote:

> >From 8b2465b0d314cc93bba5797dbad9fd2813f0a79e Mon Sep 17 00:00:00 2001
> From: Anna Czarnowska <anna.czarnowska@intel.com>
> Date: Tue, 14 Dec 2010 12:26:33 +0100
> Subject: [PATCH] fix: Monitor doesn't return after starting daemon
> Cc: linux-raid@vger.kernel.org, Williams, Dan J <dan.j.williams@intel.com>, Ciechanowski, Ed <ed.ciechanowski@intel.com>
> 
> Because both parent and child process continue after make_daemon succeeds.

Applied, thanks.

NeilBrown


> 
> Signed-off-by: Anna Czarnowska <anna.czarnowska@intel.com>
> ---
>  Monitor.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/Monitor.c b/Monitor.c
> index e7f6d03..4ae1d2b 100644
> --- a/Monitor.c
> +++ b/Monitor.c
> @@ -152,9 +152,11 @@ int Monitor(struct mddev_dev *devlist,
>  	info.mailfrom = mailfrom;
>  	info.dosyslog = dosyslog;
>  
> -	if (daemonise)
> -		if (make_daemon(pidfile))
> -			return 1;
> +	if (daemonise) {
> +		int rv = make_daemon(pidfile);
> +		if (rv >= 0)
> +			return rv;
> +	}
>  
>  	if (share) 
>  		if (check_one_sharer(scan))
> @@ -272,7 +274,7 @@ static int make_daemon(char *pidfile)
>  	dup2(0,1);
>  	dup2(0,2);
>  	setsid();
> -	return 0;
> +	return -1;
>  }
>  
>  static int check_one_sharer(int scan)

RE: Devel 3.2 branch issues

[Czarnowska, Anna]

> by the way, some of the changes in you of the patches you sent have not
> been
> included in any form.  They include:
> 
> - the getinfo_super_disks method.  I couldn't see why you need this.
> All the
>   info about the state of the arrays should already be available.
>   If there is something that you need that we don't have, please
> explain and
>   we can see how best to add it back in.

Marcin has already answered this but here is my explanation.
Current test devstate[i]==0 is always true for container so any device seems a good candidate to move.
To be able to identify members, failed devices and real spares we updated devstate for containers.
To find members we can just check which disks are used in subarrays, but a failed disk is removed from subarray after a short while and as soon as it happens we are not able to see a difference between the failed disk and a spare unless we look at metadata. 

> - min_active_disk_size_in_array.  I don't think the minimum current
> size is
>   really a good guide.  I've kept the code for letting the metadata
> handler
>   check the size, but anything beyond that should be done with domains
> I
>   think.
>   E.g have a domain '2G-or-greater' which is assigned to all 2G or
> greater
>   devices.  Then anything smaller will automatically be excluded from
> arrays
>   with those devices.
 
So if someone doesn't base domains on size they may have a small spare added to an array where it cannot be used. 
Min_active_disk_size was more than required for an array that didn't occupy the whole disk but at least it ensured that we are not throwing in something that wouldn't help. If we do this the array will remain degraded but will have spare - so Monitor may think it does not need more.
For this reason we also checked the case when there was a spare in "to" container. If the spare was not suitable (size check here too) we would still look for a good one. 

And now back to assembly. There is still a segmentation fault when we try to assemble a subarray. Occurs when there is any config file and we run "mdadm -As" or "mdadm -Asc /etc/mdadm.conf". content is NULL when we try to compare uuid in line 413 in Assemble.c.
We are going to prepare some tests to add to current suite so it will be easier to verify new patches. 

Regards
Anna
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
z siedziba w Gdansku
ul. Slowackiego 173
80-298 Gdansk

Sad Rejonowy Gdansk Polnoc w Gdansku, 
VII Wydzial Gospodarczy Krajowego Rejestru Sadowego, 
numer KRS 101882

NIP 957-07-52-316
Kapital zakladowy 200.000 zl

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: Devel 3.2 branch issues

[Neil Brown]

On Mon, 22 Nov 2010 22:39:00 +0000
"Czarnowska, Anna" <anna.czarnowska@intel.com> wrote:

> 
> > by the way, some of the changes in you of the patches you sent have not
> > been
> > included in any form.  They include:
> > 
> > - the getinfo_super_disks method.  I couldn't see why you need this.
> > All the
> >   info about the state of the arrays should already be available.
> >   If there is something that you need that we don't have, please
> > explain and
> >   we can see how best to add it back in.
> 
> Marcin has already answered this but here is my explanation.
> Current test devstate[i]==0 is always true for container so any device seems a good candidate to move.
> To be able to identify members, failed devices and real spares we updated devstate for containers.
> To find members we can just check which disks are used in subarrays, but a failed disk is removed from subarray after a short while and as soon as it happens we are not able to see a difference between the failed disk and a spare unless we look at metadata. 

Thanks.  That makes sense.  I'll look at the code and see about applying it.

> 
> > - min_active_disk_size_in_array.  I don't think the minimum current
> > size is
> >   really a good guide.  I've kept the code for letting the metadata
> > handler
> >   check the size, but anything beyond that should be done with domains
> > I
> >   think.
> >   E.g have a domain '2G-or-greater' which is assigned to all 2G or
> > greater
> >   devices.  Then anything smaller will automatically be excluded from
> > arrays
> >   with those devices.
>  
> So if someone doesn't base domains on size they may have a small spare added to an array where it cannot be used. 
> Min_active_disk_size was more than required for an array that didn't occupy the whole disk but at least it ensured that we are not throwing in something that wouldn't help. If we do this the array will remain degraded but will have spare - so Monitor may think it does not need more.
> For this reason we also checked the case when there was a spare in "to" container. If the spare was not suitable (size check here too) we would still look for a good one. 

I don't think it is possible to come up with an automatic way to determine if
a given spare suits a given array that is always correct.  There are too many
subtleties.
So I would like to allow the sysadmin to exercise complete control, and have
defaults that make reasonable sense in common cases.

The 'complete control' can be exercised through domain - though I will
probably add some size based rule mechanism to the policy code so devices can
be categorised by size if wanted.
The 'safe default' is probably best left to the metadata handler.  So
ultimately all metadata types *should* specify min_acceptable_spare_size,
and we will just make do with that.

Does that sound OK?


> 
> And now back to assembly. There is still a segmentation fault when we try to assemble a subarray. Occurs when there is any config file and we run "mdadm -As" or "mdadm -Asc /etc/mdadm.conf". content is NULL when we try to compare uuid in line 413 in Assemble.c.

Yes - patch below should fix this.

> We are going to prepare some tests to add to current suite so it will be easier to verify new patches. 

That would be greatly appreciated!!

Thanks,
NeilBrown

commit 87477e6d5e4201bf2bd812f34f8321983310bd99
Author: NeilBrown <neilb@suse.de>
Date:   Tue Nov 23 11:34:36 2010 +1100

    Assemble: get content before testing it.
    
    When checking that a container matches the required uuid,
    we need to call 'getinfo_super' before we have a 'content'
    to test.
    
    Reported-by: "Czarnowska, Anna" <anna.czarnowska@intel.com>
    Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/Assemble.c b/Assemble.c
index 1a1e128..607f2af 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -409,6 +409,11 @@ int Assemble(struct supertype *st, char *mddev,
                                if (ident->container[0] != '/') {
                                        /* we have a uuid */
                                        int uuid[4];
+
+                                       content = &info;
+                                       memset(content, 0, sizeof(*content));
+                                       tst->ss->getinfo_super(tst, content, NULL);
+
                                        if (!parse_uuid(ident->container, uuid) ||
                                            !same_uuid(content->uuid, uuid, tst->ss->swapuuid)) {
                                                if (report_missmatch)

RE: Devel 3.2 branch issues

[Czarnowska, Anna]

> I don't think it is possible to come up with an automatic way to
> determine if
> a given spare suits a given array that is always correct.  There are
> too many
> subtleties.
> So I would like to allow the sysadmin to exercise complete control, and
> have
> defaults that make reasonable sense in common cases.
> 
> The 'complete control' can be exercised through domain - though I will
> probably add some size based rule mechanism to the policy code so
> devices can
> be categorised by size if wanted.
> The 'safe default' is probably best left to the metadata handler.  So
> ultimately all metadata types *should* specify
> min_acceptable_spare_size,
> and we will just make do with that.
> 
> Does that sound OK?

Yes. 
When all metadata types have min_acceptable_spare_size there will be no need for min_active_disk_size at all.
One more thought: Manage_subdevs checks component_size so for native metadata it will not allow to add a spare that is too small.
But checking size in Monitor will prevent unnecessary removal and re-adding. 
It would make sense to get Manage_subdevs to check the size properly for external metadata too.
Anna
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
z siedziba w Gdansku
ul. Slowackiego 173
80-298 Gdansk

Sad Rejonowy Gdansk Polnoc w Gdansku, 
VII Wydzial Gospodarczy Krajowego Rejestru Sadowego, 
numer KRS 101882

NIP 957-07-52-316
Kapital zakladowy 200.000 zl

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: Devel 3.2 branch issues

[Neil Brown]

On Tue, 23 Nov 2010 11:52:13 +1100 Neil Brown <neilb@suse.de> wrote:

> On Mon, 22 Nov 2010 22:39:00 +0000
> "Czarnowska, Anna" <anna.czarnowska@intel.com> wrote:
> 
> > 
> > > by the way, some of the changes in you of the patches you sent have not
> > > been
> > > included in any form.  They include:
> > > 
> > > - the getinfo_super_disks method.  I couldn't see why you need this.
> > > All the
> > >   info about the state of the arrays should already be available.
> > >   If there is something that you need that we don't have, please
> > > explain and
> > >   we can see how best to add it back in.
> > 
> > Marcin has already answered this but here is my explanation.
> > Current test devstate[i]==0 is always true for container so any device seems a good candidate to move.
> > To be able to identify members, failed devices and real spares we updated devstate for containers.
> > To find members we can just check which disks are used in subarrays, but a failed disk is removed from subarray after a short while and as soon as it happens we are not able to see a difference between the failed disk and a spare unless we look at metadata. 
> 
> Thanks.  That makes sense.  I'll look at the code and see about applying it.
> 

OK, I have something, though I haven't tested it.

It uses your getinfo_super_disks and does the following to choose a spare
from an external array.  There are a couple of rearrangement patches before
this so it won't apply as-it, but should appear in my devel-3.2 within a few
hours.

NeilBrown

commit 5739e0d007a3eea80f5108d73d444751dbbde1ef
Author: NeilBrown <neilb@suse.de>
Date:   Thu Nov 25 18:58:27 2010 +1100

    Monitor: choose spare correctly for external metadata.
    
    When metadata is managed externally - probably as a container - we
    need to examine that metadata to see which devices are spares.
    
    So use the getinfo_super_disk message and use the info returned.
    
    Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/Monitor.c b/Monitor.c
index 5fc18d1..9ba49f2 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -798,6 +798,63 @@ static int choose_spare(struct state *from, struct state *to,
 	return dev;
 }
 
+static int container_choose_spare(struct state *from, struct state *to,
+				  struct domainlist *domlist)
+{
+	/* This is similar to choose_spare, but we cannot trust devstate,
+	 * so we need to read the metadata instead
+	 */
+
+	struct supertype *st = from->metadata;
+	int fd = open(st->devname, O_RDONLY);
+	int err;
+	struct mdinfo *disks, *d;
+	unsigned long long min_size
+		= min_spare_size_required(to);
+	int dev;
+
+	if (fd < 0)
+		return 0;
+	if (!st->ss->getinfo_super_disks)
+		return 0;
+	
+	err = st->ss->load_container(st, fd, NULL);
+	close(fd);
+	if (err)
+		return 0;
+
+	disks = st->ss->getinfo_super_disks(st);
+	st->ss->free_super(st);
+
+	if (!disks)
+		return 0;
+	
+	for (d = disks->devs ; d && !dev ; d = d->next) {
+		if (d->disk.state == 0) {
+			struct dev_policy *pol;
+			unsigned long long dev_size;
+			dev = makedev(d->disk.major,d->disk.minor);
+			
+			if (min_size &&
+			    dev_size_from_id(dev,  &dev_size) &&
+			    dev_size < min_size)
+				continue;
+
+			pol = devnum_policy(dev);
+			if (from->spare_group)
+				pol_add(&pol, pol_domain,
+					from->spare_group, NULL);
+			if (!domain_test(domlist, pol, to->metadata->ss->name))
+				dev = 0;
+			
+			dev_policy_free(pol);
+		}
+	}
+	sysfs_free(disks);
+	return dev;
+}
+
+
 static void try_spare_migration(struct state *statelist, struct alert_info *info)
 {
 	struct state *from;
@@ -827,7 +884,11 @@ static void try_spare_migration(struct state *statelist, struct alert_info *info
 				int devid;
 				if (!check_donor(from, to, domlist))
 					continue;
-				devid = choose_spare(from, to, domlist);
+				if (from->metadata->ss->external)
+					devid = container_choose_spare(
+						from, to, domlist);
+				else
+					devid = choose_spare(from, to, domlist);
 				if (devid > 0
 				    && move_spare(from, to, devid, info))
 						break;

RE: Devel 3.2 branch issues

[Czarnowska, Anna]

> -----Original Message-----
> From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
> owner@vger.kernel.org] On Behalf Of Neil Brown
> Sent: Thursday, November 25, 2010 9:02 AM
> To: Czarnowska, Anna
> Cc: linux-raid@vger.kernel.org; Neubauer, Wojciech; Williams, Dan J;
> Ciechanowski, Ed; Labun, Marcin; Hawrylewicz Czarnowski, Przemyslaw
> Subject: Re: Devel 3.2 branch issues
> 
> On Tue, 23 Nov 2010 11:52:13 +1100 Neil Brown <neilb@suse.de> wrote:
> 
> > On Mon, 22 Nov 2010 22:39:00 +0000
> > "Czarnowska, Anna" <anna.czarnowska@intel.com> wrote:
> >
> > >
> > > > by the way, some of the changes in you of the patches you sent
> have not
> > > > been
> > > > included in any form.  They include:
> > > >
> > > > - the getinfo_super_disks method.  I couldn't see why you need
> this.
> > > > All the
> > > >   info about the state of the arrays should already be available.
> > > >   If there is something that you need that we don't have, please
> > > > explain and
> > > >   we can see how best to add it back in.
> > >
> > > Marcin has already answered this but here is my explanation.
> > > Current test devstate[i]==0 is always true for container so any
> device seems a good candidate to move.
> > > To be able to identify members, failed devices and real spares we
> updated devstate for containers.
> > > To find members we can just check which disks are used in
> subarrays, but a failed disk is removed from subarray after a short
> while and as soon as it happens we are not able to see a difference
> between the failed disk and a spare unless we look at metadata.
> >
> > Thanks.  That makes sense.  I'll look at the code and see about
> applying it.
> >
> 
> OK, I have something, though I haven't tested it.
> 
> It uses your getinfo_super_disks and does the following to choose a
> spare
> from an external array.  There are a couple of rearrangement patches
> before
> this so it won't apply as-it, but should appear in my devel-3.2 within
> a few
> hours.
> 
> NeilBrown
> 

Well, this didn't help. 
In the set of tests I have just posted even the basic ones fail for imsm.
For native there are still some problems with tests:
 5c - spare not moved to degraded array in the same domain. This is really basic test with 4 arrays instead of 2.
 9 - spare moved between different metadata arrays
13 - spare moved despite action=include which doesn't allow migration

Test9 run in scan mode generates a segmentation fault.

I will have a look at this in debugger and give you more info on the reasons later on.

Anna

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
z siedziba w Gdansku
ul. Slowackiego 173
80-298 Gdansk

Sad Rejonowy Gdansk Polnoc w Gdansku, 
VII Wydzial Gospodarczy Krajowego Rejestru Sadowego, 
numer KRS 101882

NIP 957-07-52-316
Kapital zakladowy 200.000 zl

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

RE: Devel 3.2 branch issues

[Czarnowska, Anna]

> Well, this didn't help.
> In the set of tests I have just posted even the basic ones fail for
> imsm.
> For native there are still some problems with tests:
>  5c - spare not moved to degraded array in the same domain. This is
> really basic test with 4 arrays instead of 2.
>  9 - spare moved between different metadata arrays
> 13 - spare moved despite action=include which doesn't allow migration
> 
> Test9 run in scan mode generates a segmentation fault.
> 
> I will have a look at this in debugger and give you more info on the
> reasons later on.
> 
> Anna

After applying yesterday's fixes test5 and test9 don't fail any more for native.

Test6 often fails because Monitor keeps removing and re-adding spare
that is too small to add to degraded array. Test fails when we see it removed.

I have prepared few further fixes to address possible problems with Monitor
as described in each patch.

Spare migration works now also for imsm. (test12 and 13 still fail).
Test12 - two spares are taken when just one needed.
Test13 - action=include so spare should not be moved

Scan mode still needs to be investigated.

Anna

Re: Devel 3.2 branch issues

[Neil Brown]

On Fri, 26 Nov 2010 18:23:34 +0000 "Czarnowska, Anna"
<anna.czarnowska@intel.com> wrote:

> > Well, this didn't help.
> > In the set of tests I have just posted even the basic ones fail for
> > imsm.
> > For native there are still some problems with tests:
> >  5c - spare not moved to degraded array in the same domain. This is
> > really basic test with 4 arrays instead of 2.
> >  9 - spare moved between different metadata arrays
> > 13 - spare moved despite action=include which doesn't allow migration
> > 
> > Test9 run in scan mode generates a segmentation fault.
> > 
> > I will have a look at this in debugger and give you more info on the
> > reasons later on.
> > 
> > Anna
> 
> After applying yesterday's fixes test5 and test9 don't fail any more for native.
> 
> Test6 often fails because Monitor keeps removing and re-adding spare
> that is too small to add to degraded array. Test fails when we see it removed.
> 
> I have prepared few further fixes to address possible problems with Monitor
> as described in each patch.

Thanks for these patches and more particularly for all the testing effort,
finding and fixing my bugs!

I have applied them all.

NeilBrown


> 
> Spare migration works now also for imsm. (test12 and 13 still fail).
> Test12 - two spares are taken when just one needed.
> Test13 - action=include so spare should not be moved
> 
> Scan mode still needs to be investigated.
> 
> Anna