[mdadm git pull] support for detecting platform raid capabilities and some fixes

[mdadm git pull] support for detecting platform raid capabilities and some fixes

[Dan Williams]

Hi Neil,

This is hopefully the tail of the feature additions from me for
mdadm-3.0-final.  It adds the capability for mdadm to detect platform
raid capabilities, and honor them when creating new arrays.  For example
here is the output of the new --detail-platform option on an imsm
enabled platform:

# mdadm --detail-platform -e imsm
       Platform : Intel(R) Matrix Storage Manager
        Version : 7.6.0.1011
    RAID Levels : raid0 raid1 raid10 raid5
      Max Disks : 6
    Max Volumes : 2
 I/O Controller : /sys/devices/pci0000:00/0000:00:1f.2
          Port0 : /dev/sda (5RA4GKSS)
          Port1 : /dev/sdb (5RA4GKNC)
          Port2 : /dev/sdc (5RA4GKT8)
          Port3 : /dev/sdd (5RA4GQWR)
          Port5 : /dev/sde (5RA4GQYG)

This implementation crawls through sysfs to put this information
together, I believe it is crawling in a future proof fashion, but here
are my assumptions:
1/ /sys/bus/pci/drivers/ahci/<x>/device will identify a pci ahci device
with a bus id of 'x'.  This allows mdadm to detect which disks are
attached to which controller.
2/ The 'scsi_host' objects in /sys/bus/pci/drivers/ahci/<x> are named
'host%d' and there is one host per physical ahci port.  This is not
critical but allows the 'Port' information to be displayed.

It relies on /sys/dev/block so requires at least 2.6.27.

Other notables:
1/ An attempt to cover the delay between mdadm creating an array and the
friendly-named device node showing up in /dev/md/ by calling 'udevadm
settle' before starting starting Incremental assembly.  This
specifically fixes scripts that do:
mdadm -A /dev/md/<container>
mdadm -I /dev/md/<container>
There is a good chance there is a better place to put this call, but
putting it in create_mddev didn't work, and moving it up in main()
resulted in a hang.  I didn't want to hold up the other patches for this
debug.
2/ --wait-clean now honors the --scan option to allow shutdown scripts
to generically wait for any external metadata devices to finish
lingering writes.
3/ Now that we can do checking against the platform there are cases
where ->add_to_super should fail.

Please have a look.

Thanks,
Dan

PS: consuming large amounts of turkey may slow my responses until
Monday :-)

The following changes since commit a30b2ecd4a4215b0c0017edc6df538c264a97e85:
  NeilBrown (1):
        Assemble: don't auto-assemble if any arrays are listed in mdadm.conf

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/djbw/mdadm.git devel

Dan Williams (10):
      some warn_unused_result fixups
      udevadm settle barrier before invoking incremental assembly
      Support --wait-clean --scan
      allow add_to_super to return errors
      imsm: fix metadata reservation
      imsm: detect option-rom capabilities
      imsm: sysfs support routines for determining device connectivity
      imsm: validate arrays being created against firmware capabilities
      introduce --detail-platform to display platform raid capabilities
      imsm: provide a detail_platform method

 Create.c         |    8 +-
 Detail.c         |   41 ++++++++
 Makefile         |    6 +-
 Manage.c         |    9 ++-
 ReadMe.c         |    2 +
 mdadm.8          |   44 ++++++++--
 mdadm.c          |   28 +++++-
 mdadm.h          |    8 ++-
 mdopen.c         |   14 ++-
 platform-intel.c |  268 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 platform-intel.h |  129 ++++++++++++++++++++++++++
 super-ddf.c      |    9 +-
 super-intel.c    |  236 ++++++++++++++++++++++++++++++++++++++++++++----
 super0.c         |    5 +-
 super1.c         |    5 +-
 15 files changed, 765 insertions(+), 47 deletions(-)
 create mode 100644 platform-intel.c
 create mode 100644 platform-intel.h

Re: [mdadm git pull] support for detecting platform raid capabilities and some fixes

[Kay Sievers]

On Thu, Nov 27, 2008 at 00:45, Dan Williams <dan.j.williams@intel.com> wrote:
> This is hopefully the tail of the feature additions from me for
> mdadm-3.0-final.  It adds the capability for mdadm to detect platform
> raid capabilities, and honor them when creating new arrays.

> This implementation crawls through sysfs to put this information
> together, I believe it is crawling in a future proof fashion, but here
> are my assumptions:
> 1/ /sys/bus/pci/drivers/ahci/<x>/device will identify a pci ahci device

Why "device"? You just check for the "device" attribute file of the PCI device?

> 1/ An attempt to cover the delay between mdadm creating an array and the
> friendly-named device node showing up in /dev/md/ by calling 'udevadm
> settle' before starting starting Incremental assembly.  This
> specifically fixes scripts that do:
> mdadm -A /dev/md/<container>
> mdadm -I /dev/md/<container>
> There is a good chance there is a better place to put this call, but
> putting it in create_mddev didn't work, and moving it up in main()
> resulted in a hang.  I didn't want to hold up the other patches for this
> debug.

If mdadm calls "udevadm settle", we can not call mdadm (which we do
already) from udev rules because it will just deadlock.

We need to make "udevadm settle" smarter and not to wait for its own
event, or make it wait for something specific and not just "all"
events, or find a different way for mdadm to sync with udev.

Thanks,
Kay

Re: [mdadm git pull] support for detecting platform raid capabilities and some fixes

[Dan Williams]

On Wed, Nov 26, 2008 at 5:13 PM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Thu, Nov 27, 2008 at 00:45, Dan Williams <dan.j.williams@intel.com> wrote:
>> This implementation crawls through sysfs to put this information
>> together, I believe it is crawling in a future proof fashion, but here
>> are my assumptions:
>> 1/ /sys/bus/pci/drivers/ahci/<x>/device will identify a pci ahci device
>
> Why "device"? You just check for the "device" attribute file of the PCI device?
>

$ ls /sys/bus/pci/drivers/ahci/
0000:00:1f.2/	module/	uevent
bind			new_id	unbind

I look for the 'device' file to disambiguate '0000:00:1f.2' from
'module' or some other object that appears in the future.  The other
attributes are discarded because they are not directories.

>> 1/ An attempt to cover the delay between mdadm creating an array and the
>> friendly-named device node showing up in /dev/md/ by calling 'udevadm
>> settle' before starting starting Incremental assembly.  This
>> specifically fixes scripts that do:
>> mdadm -A /dev/md/<container>
>> mdadm -I /dev/md/<container>
>> There is a good chance there is a better place to put this call, but
>> putting it in create_mddev didn't work, and moving it up in main()
>> resulted in a hang.  I didn't want to hold up the other patches for this
>> debug.
>
> If mdadm calls "udevadm settle", we can not call mdadm (which we do
> already) from udev rules because it will just deadlock.
>
> We need to make "udevadm settle" smarter and not to wait for its own
> event, or make it wait for something specific and not just "all"
> events, or find a different way for mdadm to sync with udev.
>

It just sounds like we need to be careful that the path that udev
triggers when it calls mdadm does not result in another call to
"udevadm settle" which should be ok because udev should never be
creating md devices, right?

Thanks,
Dan

Re: [mdadm git pull] support for detecting platform raid capabilities and some fixes

[Kay Sievers]

On Thu, Nov 27, 2008 at 04:25, Dan Williams <dan.j.williams@intel.com> wrote:
> On Wed, Nov 26, 2008 at 5:13 PM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>> On Thu, Nov 27, 2008 at 00:45, Dan Williams <dan.j.williams@intel.com> wrote:
>>> This implementation crawls through sysfs to put this information
>>> together, I believe it is crawling in a future proof fashion, but here
>>> are my assumptions:
>>> 1/ /sys/bus/pci/drivers/ahci/<x>/device will identify a pci ahci device
>>
>> Why "device"? You just check for the "device" attribute file of the PCI device?
>>
>
> $ ls /sys/bus/pci/drivers/ahci/
> 0000:00:1f.2/   module/ uevent
> bind                    new_id  unbind
>
> I look for the 'device' file to disambiguate '0000:00:1f.2' from
> 'module' or some other object that appears in the future.  The other
> attributes are discarded because they are not directories.

But only devices of the bus "pci" have this attribute. The code looks
like it it should be able to handle some other buses to, at least the
bus values is passed around, but the pci-device attribute "device" is
hard coded. That's why I asked.
It's fine if you want to do it that way. If you just look for a
"struct device" of that <bus>, resolving "subsystem" and checking if
the tail of the string matches "/<bus>" should be fine.

>>> 1/ An attempt to cover the delay between mdadm creating an array and the
>>> friendly-named device node showing up in /dev/md/ by calling 'udevadm
>>> settle' before starting starting Incremental assembly.  This
>>> specifically fixes scripts that do:
>>> mdadm -A /dev/md/<container>
>>> mdadm -I /dev/md/<container>
>>> There is a good chance there is a better place to put this call, but
>>> putting it in create_mddev didn't work, and moving it up in main()
>>> resulted in a hang.  I didn't want to hold up the other patches for this
>>> debug.
>>
>> If mdadm calls "udevadm settle", we can not call mdadm (which we do
>> already) from udev rules because it will just deadlock.
>>
>> We need to make "udevadm settle" smarter and not to wait for its own
>> event, or make it wait for something specific and not just "all"
>> events, or find a different way for mdadm to sync with udev.
>>
>
> It just sounds like we need to be careful that the path that udev
> triggers when it calls mdadm does not result in another call to
> "udevadm settle" which should be ok because udev should never be
> creating md devices, right?

Udev calls mdadm also for the md devices, but with different options I
guess. In the future, udev will also create the device nodes instead
of mdadm.
This needs to be handled very careful, because _any_ call to udevadm
from mdadm will deadlock if called from udev. Something we need to
make sure, it can't happen, on udev's or mdadm's side.

Thanks,
Kay

Re: [mdadm git pull] support for detecting platform raid capabilities and some fixes

[Neil Brown]

On Wednesday November 26, dan.j.williams@intel.com wrote:
> Hi Neil,
> 
> This is hopefully the tail of the feature additions from me for
> mdadm-3.0-final.  It adds the capability for mdadm to detect platform
> raid capabilities, and honor them when creating new arrays.  For example
> here is the output of the new --detail-platform option on an imsm
> enabled platform:
> 
> # mdadm --detail-platform -e imsm
>        Platform : Intel(R) Matrix Storage Manager
>         Version : 7.6.0.1011
>     RAID Levels : raid0 raid1 raid10 raid5
>       Max Disks : 6
>     Max Volumes : 2
>  I/O Controller : /sys/devices/pci0000:00/0000:00:1f.2
>           Port0 : /dev/sda (5RA4GKSS)
>           Port1 : /dev/sdb (5RA4GKNC)
>           Port2 : /dev/sdc (5RA4GKT8)
>           Port3 : /dev/sdd (5RA4GQWR)
>           Port5 : /dev/sde (5RA4GQYG)

No "Port4" - seems odd.

So what happens when you try to create an array on devices that aren't
attached to a detected platform?  Or create an array that crosses two
separate controllers?
Just a warning?  Require --force?  Do nothing ??

Sounds like a useful thing!

> 
> This implementation crawls through sysfs to put this information
> together, I believe it is crawling in a future proof fashion, but here
> are my assumptions:
> 1/ /sys/bus/pci/drivers/ahci/<x>/device will identify a pci ahci device
> with a bus id of 'x'.  This allows mdadm to detect which disks are
> attached to which controller.
> 2/ The 'scsi_host' objects in /sys/bus/pci/drivers/ahci/<x> are named
> 'host%d' and there is one host per physical ahci port.  This is not
> critical but allows the 'Port' information to be displayed.
> 

IMSM is only ever ahci?  Never SCSI etc?

And I notice that you hunt through all of the option-rom memory to
find the option from for the IMSM to read some details.
Once you have the I/O Controller, can you just look in the "resource"
file to get start/length info and read just that area ???

I wonder if libpci or libdiscover can do some of this for us.  It isn't
a lot of code, but I'm wondering if it is really a generic and
future-proof as it should be?

(reads code)

It looks like libpci can read 'resource' files etc, but it simply
reads everything - presumably to support 'lspci'.  It isn't clear that
you can ask it to just read the 'resource' for one device which is all
we really want.  That is a bit sad.

And libdiscover seems more complicated than we really want.

What would you think of using the 'resource' info, either via libpci
or more directly, possibly lifting the parser code from libpci?
I think I'd feel more comfortable about that.


> It relies on /sys/dev/block so requires at least 2.6.27.

Lots of the 'container' related stuff requires at least 2.6.27, so
that isn't a big cost.

> 
> Other notables:
> 1/ An attempt to cover the delay between mdadm creating an array and the
> friendly-named device node showing up in /dev/md/ by calling 'udevadm
> settle' before starting starting Incremental assembly.  This
> specifically fixes scripts that do:
> mdadm -A /dev/md/<container>
> mdadm -I /dev/md/<container>
> There is a good chance there is a better place to put this call, but
> putting it in create_mddev didn't work, and moving it up in main()
> resulted in a hang.  I didn't want to hold up the other patches for this
> debug.

I recently added "wait_for" to wait a little while for a device to
appear in /dev.  I don't seem to be calling it at the end of
--assemble.
Maybe putting that in place will be enough?

> 2/ --wait-clean now honors the --scan option to allow shutdown scripts
> to generically wait for any external metadata devices to finish
> lingering writes.

That looks good.

> 3/ Now that we can do checking against the platform there are cases
> where ->add_to_super should fail.

Makes sense.
I think in the case where it does fail we are getting an error message
from the ->add_to_super method, and then a generic "failed to add".

Maybe we should ensure that every error path does report the error,
then get rid of the generic error?

> 
> Please have a look.

I'll cherry-pick out the bits I definitely like and apply them.  Then
we can discuss the rest.

... In fact I have already done this.  I thought I had sent this Email
yesterday some time but it seems I didn't.

NeilBrown

Re: [mdadm git pull] support for detecting platform raid capabilities and some fixes

[Dan Williams]

On Wed, Nov 26, 2008 at 11:24 PM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Thu, Nov 27, 2008 at 04:25, Dan Williams <dan.j.williams@intel.com> wrote:
>> On Wed, Nov 26, 2008 at 5:13 PM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>>> On Thu, Nov 27, 2008 at 00:45, Dan Williams <dan.j.williams@intel.com> wrote:
>>>> This implementation crawls through sysfs to put this information
>>>> together, I believe it is crawling in a future proof fashion, but here
>>>> are my assumptions:
>>>> 1/ /sys/bus/pci/drivers/ahci/<x>/device will identify a pci ahci device
>>>
>>> Why "device"? You just check for the "device" attribute file of the PCI device?
>>>
>>
>> $ ls /sys/bus/pci/drivers/ahci/
>> 0000:00:1f.2/   module/ uevent
>> bind                    new_id  unbind
>>
>> I look for the 'device' file to disambiguate '0000:00:1f.2' from
>> 'module' or some other object that appears in the future.  The other
>> attributes are discarded because they are not directories.
>
> But only devices of the bus "pci" have this attribute. The code looks
> like it it should be able to handle some other buses to, at least the
> bus values is passed around, but the pci-device attribute "device" is
> hard coded. That's why I asked.
> It's fine if you want to do it that way. If you just look for a
> "struct device" of that <bus>, resolving "subsystem" and checking if
> the tail of the string matches "/<bus>" should be fine.

gah, I was confusing the 'device' I find here "/sys/block/sda/device"
with the pci device id.  Yes, checking on 'subsystem' seems to be
exactly what I want.

Thanks,
Dan

Re: [mdadm git pull] support for detecting platform raid capabilities and some fixes

[Dan Williams]

On Thu, Nov 27, 2008 at 4:39 PM, Neil Brown <neilb@suse.de> wrote:
> On Wednesday November 26, dan.j.williams@intel.com wrote:
>> Hi Neil,
>>
>> This is hopefully the tail of the feature additions from me for
>> mdadm-3.0-final.  It adds the capability for mdadm to detect platform
>> raid capabilities, and honor them when creating new arrays.  For example
>> here is the output of the new --detail-platform option on an imsm
>> enabled platform:
>>
>> # mdadm --detail-platform -e imsm
>>        Platform : Intel(R) Matrix Storage Manager
>>         Version : 7.6.0.1011
>>     RAID Levels : raid0 raid1 raid10 raid5
>>       Max Disks : 6
>>     Max Volumes : 2
>>  I/O Controller : /sys/devices/pci0000:00/0000:00:1f.2
>>           Port0 : /dev/sda (5RA4GKSS)
>>           Port1 : /dev/sdb (5RA4GKNC)
>>           Port2 : /dev/sdc (5RA4GKT8)
>>           Port3 : /dev/sdd (5RA4GQWR)
>>           Port5 : /dev/sde (5RA4GQYG)
>
> No "Port4" - seems odd.
>

Port4 is attached to a sata-dvd in this case.  I'll expand this output
to show empty ports, and non-disk attached ports.

> So what happens when you try to create an array on devices that aren't
> attached to a detected platform?  Or create an array that crosses two
> separate controllers?
> Just a warning?  Require --force?  Do nothing ??
>
> Sounds like a useful thing!

Right now it just returns errors from ->validate_geometry and
->add_to_super.  The environment variable IMSM_NO_PLATFORM turns off
this checking.  The --assemble case could take advantage of this as
well to warn or fail to assemble when disks are found on "non-raid"
ports, currently 'platform' checking is silent at assembly.  Different
environments could have different policies...

Here is a lingering idea that may be post mdadm-3.0 material: What
about exposing these policy decisions via a new configuration file
variable: HBA?
HBA device=<'platform' | sysfs device path | some other identification
tuple> enforce_ports=<no | yes | warn> auto_hotplug=<no | yes>

Where enforce_ports checks for assembly or create events talking to
HBA-attached disks and 'auto_hotplug' handles re-adding disks on a
hotplug event where the administrator expects this to happen for the
"raid controller" but not for example usb-storage.

>>
>> This implementation crawls through sysfs to put this information
>> together, I believe it is crawling in a future proof fashion, but here
>> are my assumptions:
>> 1/ /sys/bus/pci/drivers/ahci/<x>/device will identify a pci ahci device
>> with a bus id of 'x'.  This allows mdadm to detect which disks are
>> attached to which controller.
>> 2/ The 'scsi_host' objects in /sys/bus/pci/drivers/ahci/<x> are named
>> 'host%d' and there is one host per physical ahci port.  This is not
>> critical but allows the 'Port' information to be displayed.
>>
>
> IMSM is only ever ahci?  Never SCSI etc?

Yes, only ahci-sata.

> And I notice that you hunt through all of the option-rom memory to
> find the option from for the IMSM to read some details.
> Once you have the I/O Controller, can you just look in the "resource"
> file to get start/length info and read just that area ???

Scanning through option-rom memory was a bit unpalatable to me as
well, and I expected to find this region mapped via an expansion-rom
bar.  However this is not the case as there does not appear to be an
associated 'resource' file with this range.  /proc/iomem reports:
000c0000-000dffff : pnp 00:01

/sys/bus/pnp/devices/00:01/ does not contain anything that would point
me to the eventual 0xce840 in this case.

> What would you think of using the 'resource' info, either via libpci
> or more directly, possibly lifting the parser code from libpci?
> I think I'd feel more comfortable about that.

Me too, but I don't think this capability lends itself to generic discovery.

[..]
>> Other notables:
>> 1/ An attempt to cover the delay between mdadm creating an array and the
>> friendly-named device node showing up in /dev/md/ by calling 'udevadm
>> settle' before starting starting Incremental assembly.  This
>> specifically fixes scripts that do:
>> mdadm -A /dev/md/<container>
>> mdadm -I /dev/md/<container>
>> There is a good chance there is a better place to put this call, but
>> putting it in create_mddev didn't work, and moving it up in main()
>> resulted in a hang.  I didn't want to hold up the other patches for this
>> debug.
>
> I recently added "wait_for" to wait a little while for a device to
> appear in /dev.  I don't seem to be calling it at the end of
> --assemble.
> Maybe putting that in place will be enough?

I'll take a look.

[..]
>> Please have a look.
>
> I'll cherry-pick out the bits I definitely like and apply them.  Then
> we can discuss the rest.
>

Sounds good.

Thanks,
Dan

Re: [mdadm git pull] support for detecting platform raid capabilities and some fixes

[Neil Brown]

On Friday November 28, dan.j.williams@intel.com wrote:
> On Thu, Nov 27, 2008 at 4:39 PM, Neil Brown <neilb@suse.de> wrote:
> > So what happens when you try to create an array on devices that aren't
> > attached to a detected platform?  Or create an array that crosses two
> > separate controllers?
> > Just a warning?  Require --force?  Do nothing ??
> >
> > Sounds like a useful thing!
> 
> Right now it just returns errors from ->validate_geometry and
> ->add_to_super.  The environment variable IMSM_NO_PLATFORM turns off
> this checking.  The --assemble case could take advantage of this as
> well to warn or fail to assemble when disks are found on "non-raid"
> ports, currently 'platform' checking is silent at assembly.  Different
> environments could have different policies...
> 
> Here is a lingering idea that may be post mdadm-3.0 material: What
> about exposing these policy decisions via a new configuration file
> variable: HBA?
> HBA device=<'platform' | sysfs device path | some other identification
> tuple> enforce_ports=<no | yes | warn> auto_hotplug=<no | yes>
> 
> Where enforce_ports checks for assembly or create events talking to
> HBA-attached disks and 'auto_hotplug' handles re-adding disks on a
> hotplug event where the administrator expects this to happen for the
> "raid controller" but not for example usb-storage.

I think were certainly want something like that.

> > And I notice that you hunt through all of the option-rom memory to
> > find the option from for the IMSM to read some details.
> > Once you have the I/O Controller, can you just look in the "resource"
> > file to get start/length info and read just that area ???
> 
> Scanning through option-rom memory was a bit unpalatable to me as
> well, and I expected to find this region mapped via an expansion-rom
> bar.  However this is not the case as there does not appear to be an
> associated 'resource' file with this range.  /proc/iomem reports:
> 000c0000-000dffff : pnp 00:01
> 
> /sys/bus/pnp/devices/00:01/ does not contain anything that would point
> me to the eventual 0xce840 in this case.

That's fairly boring.
Can we at least check that it is 'mem' space rather than 'io' space
that we are hunting through.
I can see in  /sys/bus/pnp/devices/*/resources  that some ranges are
'mem' and some are 'io'.
??


Thanks,
NeilBrown