Opened 12 months ago

Closed 7 months ago

#730 closed enhancement (fixed)

Added an option for IntelliProp RAID controllers for choosing the drive to send some ATA commands to.

Reported by: cbiemiller Owned by: chrfranke
Priority: minor Milestone: Release 6.6
Component: all Version: 6.5
Keywords: intelliprop Cc:

Description

Added an option that would allow the choice of selecting which drive to send certain commands to behind an IntelliProp? RAID controller. The commands this mainly works for are SMART, GPL Read Log, and GPL Write Log commands. Since smartctl uses these commands, this routing functionality would be necessary to be able to send smartctl commands to each individual drive behind an IntelliProp? RAID controller.

Attachments (10)

atacmds.cpp.diff (1.3 KB) - added by cbiemiller 12 months ago.
A diff of the atacmds.cpp
atacmds.h.diff (902 bytes) - added by cbiemiller 12 months ago.
A diff of atacmds.h
ataprint.h.diff (680 bytes) - added by cbiemiller 12 months ago.
A diff of ataprint.h
Makefile.am.diff (361 bytes) - added by cbiemiller 12 months ago.
A diff of Makefile.am
smartctl.8.in.diff (551 bytes) - added by cbiemiller 12 months ago.
A diff of smartctl.8.in
smartctl.cpp.diff (4.1 KB) - added by cbiemiller 12 months ago.
A diff of smartctl.cpp
intelliprop.cpp (8.8 KB) - added by cbiemiller 12 months ago.
The source of an IntelliProp? specific file.
intelliprop.h (5.1 KB) - added by cbiemiller 12 months ago.
The header of an IntelliProp? specific file.
ChangeLog.diff (860 bytes) - added by cbiemiller 12 months ago.
A diff of ChangeLog? that can act as another summary of changes.
iprop_patch.diff (19.1 KB) - added by cbiemiller 7 months ago.
Single file diff of every change

Download all attachments as: .zip

Change History (23)

Changed 12 months ago by cbiemiller

A diff of the atacmds.cpp

Changed 12 months ago by cbiemiller

A diff of atacmds.h

Changed 12 months ago by cbiemiller

A diff of ataprint.h

Changed 12 months ago by cbiemiller

A diff of Makefile.am

Changed 12 months ago by cbiemiller

A diff of smartctl.8.in

Changed 12 months ago by cbiemiller

A diff of smartctl.cpp

Changed 12 months ago by cbiemiller

The source of an IntelliProp? specific file.

Changed 12 months ago by cbiemiller

The header of an IntelliProp? specific file.

Changed 12 months ago by cbiemiller

A diff of ChangeLog? that can act as another summary of changes.

comment:1 Changed 12 months ago by chrfranke

  • Keywords intelliprop added; IntelliProp ATA smartctl removed
  • Milestone set to undecided
  • Priority changed from major to minor

Thanks for this patch.

Some notes from a quick review:

  • Please provide the patch again as one large patch file which cleanly applies to current SVN HEAD (with svn patch ... or patch < FILE.patch). The patch file should also include the new files (e.g. use svn add NEWFILES and then svn diff > FILE.patch).
  • The file intelliprop.cpp contains a license info ("IntelliProp Software Products License") which is likely not compatible with GPLv2(+).
  • Please explain which controller products and OS platforms benefit from this patch.

comment:2 Changed 12 months ago by cbiemiller

I added the patch in one file as requested and I also removed the IntelliProp? Software Products License from the intelliprop.cpp/h files. As for the controllers this code benefits, they are as follows: IPA-SA117-BR, IPA-SA149-BR, and IPA-SA145-BR. This change was only tested in Linux as that is the only OS it is currently desired to work with, but it may work in other OS platforms as well(there was no Linux specific code added).

comment:3 Changed 12 months ago by chrfranke

Thanks, but a closer look shows that the patch breaks the overall design decision that all controller specific issues should be handled behind the smart_device and smart_interface scenes. It should not be needed to add any code to smartctl.cpp or smartd.cpp to support new controllers. Controller and physical drives should be selected by some -d TYPE[,PARAMETERS] option.

Please provide a complete usage example (smartctl commands and resulting outputs) for your patch.

Which platforms supported by smartmontools do actually support these controllers (e.g. by existing device drivers) ?

comment:4 Changed 12 months ago by cbiemiller

Usage example:
smartctl -H /dev/sd[a-z]
smartctl --iprop-routecmd=1 /dev/sd[a-z]
smartctl -H /dev/sd[a-z]
smartctl --iprop-routecmd=2 /dev/sd[a-z]
smartctl -H /dev/sd[a-z]
smartctl --iprop-routecmd=3 /dev/sd[a-z]
smartctl -H /dev/sd[a-z]
smartctl --iprop-routecmd=0 /dev/sd[a-z]
This can be looped and other smarctl commands can either replace the -H option or multiple smartctl can be used in between each routecmd to get information from each individual drive.

I most likely can use the -d TYPE[,PARAMETERS] option for an interface(I haven't looked at the code for that yet). Is the idea of intelliprop.cpp/.h addition against the overall design decision or is that fine to work with when working on using the above option?

So far, I have only used the controller with smartctl on the latest version of Ubuntu, but I can test it on other platforms. The controller itself has been tested on Windows 7 as well as a variety of Linux versions. No special drivers are required for the controller.

comment:5 Changed 12 months ago by chrfranke

  • Component changed from smartctl to all
  • Milestone changed from undecided to unscheduled

Please rework the code as follows:

Discard any changes in smartctl.cpp and ataprint.*

Create new file dev_intelliprop.cpp which implements a wrapper class intelliprop_device. Example:

namespace intelliprop {

class intelliprop_device
: public tunnelled_device<
    /*implements*/ ata_device,
    /*by using an*/ ata_device
  >
{
public:
  intelliprop_device(smart_interface * intf, unsigned phydrive, ata_device * atadev);

  virtual bool open();

  virtual bool ata_pass_through(const ata_cmd_in & in, ata_cmd_out & out);

private:
  unsigned m_phydrive;
};

intelliprop_device::intelliprop_device(smart_interface * intf, unsigned phydrive, ata_device * atadev)
: smart_device(intf, atadev->get_dev_name(), "intelliprop", "intelliprop"),
  tunnelled_device<ata_device, ata_device>(atadev),
  m_phydrive(phydrive)
{
  set_info().info_name = strprintf("%s [intelliprop_disk_%u]", atadev->get_info_name(), phydrive);
}

bool intelliprop_device::open()
{
  ata_device * atadev = get_tunnel_dev();
  if (!atadev->open())
    return false;

  // >>>>> TODO: Add command(s) to select phydrive here <<<<<
  return true;
}

bool intelliprop_device::ata_pass_through(const ata_cmd_in & in, ata_cmd_out & out)
{
  return get_tunnel_dev()->ata_pass_through(in, out);
}

} // namespace intelliprop

ata_device * get_intelliprop_device(smart_interface * intf, unsigned phydrive, ata_device * atadev)
{
  return new intelliprop::intelliprop_device(intf, phydrive, atadev);
}

Copy required code from intelliprop.* to this file. Discard intelliprop.*.

Create new file dev_intelliprop.h which contains the prototype of get_intelliprop_device().

Include this file to dev_interface.cppand add -d intelliprop,N support to smart_interface::get_smart_device(). Example:

  • dev_interface.cpp

    $ svn diff
     
    422422    return get_sat_device(sattype.c_str(), basedev.release()->to_scsi());
    423423  }
    424424
     425  else if (str_starts_with(type, "intelliprop")) {
     426    int n = -1, len = strlen(type); unsigned phydrive = ~0;
     427    sscanf(type, "intelliprop,%u%n", &phydrive, &n);
     428    if (!(n == len && phydrive <= 3)) {
     429      set_err(EINVAL, "Option '-d intelliprop,N' requires N between 0 and 3");
     430      return 0;
     431    }
     432    // Recurse to get SAT base device
     433    smart_device * basedev = get_smart_device(name, "sat");
     434    if (!basedev)
     435       return 0;
     436    return get_intelliprop_device(this, phydrive, basedev->to_ata());
     437  }
     438
    425439  else {
    426440    set_err(EINVAL, "Unknown device type '%s'", type);
    427441    return 0;

(This is currently limited to SATA controllers behind SAT layers which should be sufficient for Linux. We could later add -d intelliprop,N[+TYPE] to select another base device).

Adjust Makefile.am accordingly.

Test smartctl -d intelliprop,N -x /dev/sdX and report test results and platform here.

Finally note that this all adds -d intelliprop,N support for smartd daemon for free :-)

comment:6 Changed 8 months ago by chrfranke

  • Milestone changed from unscheduled to undecided

The patch is not suitable for inclusion (see above). Set milestone back to undecided due to missing feedback from patch author.

comment:7 Changed 8 months ago by cbiemiller

Sorry for the long delay in responding. The above code works if the Intelliprop controller is plugged in via SATA. If the controller is in the following required situation, it doesn't work. SAS HBA -> SAS/SATA Convertor -> Intelliprop Controller. Using -d sat sends command correctly, but the above code causes errors, even if I don't use any of the routing code. -d sat does show that the basedev is SCSI, but -d intelliprop shows the basedev as ATA. If I set smartctl to ignore errors when doing -a, while using -d intelliprop, I am able to get some SMART information, but many of the commands fail. Any thoughts on this? I'm still working on a solution.

comment:8 Changed 8 months ago by chrfranke

Due to lack of access to this specific hardware, I only could provide limited help. Please attach the full patch (as a single .patch file) here. You could also send it via private mail if desired.

comment:9 Changed 7 months ago by chrfranke

  • Milestone changed from undecided to Release 6.6

comment:10 Changed 7 months ago by cbiemiller

I have been able to confirm the code works for Linux via ATA directly and for Linux through a SCSI HBA <---> SCSI/ATA converter interface. The man page for smartctl has been updated as well.

comment:11 Changed 7 months ago by chrfranke

  • Owner set to chrfranke
  • Status changed from new to accepted

Changed 7 months ago by cbiemiller

Single file diff of every change

comment:12 Changed 7 months ago by cbiemiller

I included more documentation of the added -d option as well as a few other changes to the code that have been tested.

comment:13 Changed 7 months ago by chrfranke

  • Resolution set to fixed
  • Status changed from accepted to closed

Patch applied in r4370. Thanks.

Note: See TracTickets for help on using tickets.