Opened 8 years ago

Closed 8 years ago

#730 closed enhancement (fixed)

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

Reported by: Casey Biemiller Owned by: Christian Franke
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 Casey Biemiller 8 years ago.
A diff of the atacmds.cpp
atacmds.h.diff (902 bytes ) - added by Casey Biemiller 8 years ago.
A diff of atacmds.h
ataprint.h.diff (680 bytes ) - added by Casey Biemiller 8 years ago.
A diff of ataprint.h
Makefile.am.diff (361 bytes ) - added by Casey Biemiller 8 years ago.
A diff of Makefile.am
smartctl.8.in.diff (551 bytes ) - added by Casey Biemiller 8 years ago.
A diff of smartctl.8.in
smartctl.cpp.diff (4.1 KB ) - added by Casey Biemiller 8 years ago.
A diff of smartctl.cpp
intelliprop.cpp (8.8 KB ) - added by Casey Biemiller 8 years ago.
The source of an IntelliProp specific file.
intelliprop.h (5.1 KB ) - added by Casey Biemiller 8 years ago.
The header of an IntelliProp specific file.
ChangeLog.diff (860 bytes ) - added by Casey Biemiller 8 years ago.
A diff of ChangeLog that can act as another summary of changes.
iprop_patch.diff (19.1 KB ) - added by Casey Biemiller 8 years ago.
Single file diff of every change

Download all attachments as: .zip

Change History (23)

by Casey Biemiller, 8 years ago

Attachment: atacmds.cpp.diff added

A diff of the atacmds.cpp

by Casey Biemiller, 8 years ago

Attachment: atacmds.h.diff added

A diff of atacmds.h

by Casey Biemiller, 8 years ago

Attachment: ataprint.h.diff added

A diff of ataprint.h

by Casey Biemiller, 8 years ago

Attachment: Makefile.am.diff added

A diff of Makefile.am

by Casey Biemiller, 8 years ago

Attachment: smartctl.8.in.diff added

A diff of smartctl.8.in

by Casey Biemiller, 8 years ago

Attachment: smartctl.cpp.diff added

A diff of smartctl.cpp

by Casey Biemiller, 8 years ago

Attachment: intelliprop.cpp added

The source of an IntelliProp specific file.

by Casey Biemiller, 8 years ago

Attachment: intelliprop.h added

The header of an IntelliProp specific file.

by Casey Biemiller, 8 years ago

Attachment: ChangeLog.diff added

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

comment:1 by Christian Franke, 8 years ago

Keywords: intelliprop added; IntelliProp ATA smartctl removed
Milestone: undecided
Priority: majorminor

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 by Casey Biemiller, 8 years ago

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 by Christian Franke, 8 years ago

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 by Casey Biemiller, 8 years ago

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 by Christian Franke, 8 years ago

Component: smartctlall
Milestone: undecidedunscheduled

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 by Christian Franke, 8 years ago

Milestone: unscheduledundecided

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

comment:7 by Casey Biemiller, 8 years ago

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 by Christian Franke, 8 years ago

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 by Christian Franke, 8 years ago

Milestone: undecidedRelease 6.6

comment:10 by Casey Biemiller, 8 years ago

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 by Christian Franke, 8 years ago

Owner: set to Christian Franke
Status: newaccepted

by Casey Biemiller, 8 years ago

Attachment: iprop_patch.diff added

Single file diff of every change

comment:12 by Casey Biemiller, 8 years ago

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 by Christian Franke, 8 years ago

Resolution: fixed
Status: acceptedclosed

Patch applied in r4370. Thanks.

Note: See TracTickets for help on using tickets.