From: Stephen M. C. <sca...@be...> - 2010-03-15 14:47:04
|
Hi Smartmontools people, I'm looking at trying to improve smartmontools support for HP Smart Arrays a little bit and would like some feed back on what I'm doing, though at this time, I am sure you don't want all of these patches (as they aren't finished.) I'm not so hot with C++, nor with autotools. One of these patches adds 4 files, and I couldn't figure out how to tweak the autotools configuration files to get it to notice this fact. The current design of smartmontools does not match up very well with how smart arrays work, so I'm trying to tweak it a little bit to make it a better fit, without warping it too much so that it remains a good fit with other devices. The main thing I did was add the concept of a "host bus adapter" (hba) to which devices could be associated, and things which could be done on a per-host-bus-adapter basis (eg. CISS_REPORT_PHYS) I did there. For non-Smart-Array devices, there is a kind of "no-op" hba which can be shared by all devices. For smart array, I derived a subclass of this host_bus_adapter which does the smart array specific stuff. It may be the case that other kinds of controllers can benefit from this hba concept as well, if they have similar things which can be done on a per-hba basis rather than a per-disk basis. The transition is not complete... I've got smartctl working in this new way, but not smartd. I haven't quite grokked smartd enough to know how to fit it in. I think I need a list of hp_smart_array_hbas, one per controller, with which I associate each device (and all non-smart-array devices can share the same no-op hba), but the details of how to do it, I'm not exactly clear yet. Anyway, have a look, and if I'm way off in the weeds, tell me that, or if I'm more or less on the right track, tell me that, or tell me whatever you think. I just wanted to get some feed back before I put too much time into this, in case you guys think I'm doing it all wrong. -- steve |
From: Stephen M. C. <sca...@be...> - 2010-03-15 14:43:41
|
From: root <ro...@si...> Factor out cciss report lun Signed-off-by: Stephen M. Cameron <sca...@be...> --- cciss.cpp | 27 ++++++++++++++++----------- cciss.h | 1 + 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/cciss.cpp b/cciss.cpp index d6f8bcd..3f47020 100644 --- a/cciss.cpp +++ b/cciss.cpp @@ -1,6 +1,7 @@ #include <stdio.h> #include <string.h> #include <sys/types.h> + #include <arpa/inet.h> /* for htonl, etc. */ #include "config.h" @@ -29,6 +30,7 @@ #include "int64.h" #include "scsicmds.h" #include "utility.h" +#include "cciss.h" const char *cciss_c_cvsid="$Id: cciss.cpp,v 1.9 2008/07/30 20:42:53 chrfranke Exp $" CONFIG_H_CVSID INT64_H_CVSID SCSICMDS_H_CVSID UTILITY_H_CVSID; @@ -171,25 +173,28 @@ static int cciss_sendpassthru(unsigned int cmdtype, unsigned char *CDB, return err; } -static int cciss_getlun(int device, int target, unsigned char *physlun, int report) +int cciss_report_physical_luns(int fd, ReportLunData_struct *report_lun_data) { unsigned char CDB[16]= {0}; + uint32_t buffer_size = sizeof(*report_lun_data); + + memset(report_lun_data, 0, sizeof(*report_lun_data)); + buffer_size = htonl(buffer_size); + CDB[0] = CISS_REPORT_PHYS; /* Get Physical LUN Info (for physical device) */ + memcpy(&CDB[6], &buffer_size, sizeof(buffer_size)); /* unaligned, so memcpy */ + return cciss_sendpassthru(0, CDB, 12, (char *) report_lun_data, sizeof(*report_lun_data), 0, NULL, fd); +} + +static int cciss_getlun(int device, int target, unsigned char *physlun, int report) +{ ReportLunData_struct *luns; int reportlunsize = sizeof(*luns) + CISS_MAX_PHYS_LUN * 8; int ret; luns = (ReportLunData_struct *)malloc(reportlunsize); - memset(luns, 0, reportlunsize); - - /* Get Physical LUN Info (for physical device) */ - CDB[0] = CISS_REPORT_PHYS; - CDB[6] = (reportlunsize >> 24) & 0xFF; /* MSB */ - CDB[7] = (reportlunsize >> 16) & 0xFF; - CDB[8] = (reportlunsize >> 8) & 0xFF; - CDB[9] = reportlunsize & 0xFF; - - if ((ret = cciss_sendpassthru(0, CDB, 12, (char *)luns, reportlunsize, 0, NULL, device))) + ret = cciss_report_physical_luns(device, luns); + if (ret) { free(luns); return ret; diff --git a/cciss.h b/cciss.h index 2e8648d..71e9e8b 100644 --- a/cciss.h +++ b/cciss.h @@ -13,5 +13,6 @@ typedef struct _ReportLUNdata_struct int cciss_io_interface(int device, int target, struct scsi_cmnd_io * iop, int report); +int cciss_report_physical_luns(int fd, ReportLunData_struct *report_lun_data); #endif /* CCISS_H_ */ |
From: Stephen M. C. <sca...@be...> - 2010-03-15 14:44:25
|
From: root <ro...@si...> Fix the report lun structure. We are a doing a CISS_REPORT_PHYS, not CISS_REPORT_LOG, so the size of the array needs to be big enough to hold the maximum number of physical luns, not logical. Also, move it into cciss.h, so it may be used elsewhere. Signed-off-by: Stephen M. Cameron <sca...@be...> --- cciss.cpp | 8 -------- cciss.h | 8 ++++++++ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/cciss.cpp b/cciss.cpp index fca75ac..d6f8bcd 100644 --- a/cciss.cpp +++ b/cciss.cpp @@ -33,18 +33,10 @@ const char *cciss_c_cvsid="$Id: cciss.cpp,v 1.9 2008/07/30 20:42:53 chrfranke Exp $" CONFIG_H_CVSID INT64_H_CVSID SCSICMDS_H_CVSID UTILITY_H_CVSID; -typedef struct _ReportLUNdata_struct -{ - uint32_t LUNListLength; /* always big-endian */ - uint32_t reserved; - uint8_t LUN[CISS_MAX_LUN][8]; -} ReportLunData_struct; - /* Structure/defines of Report Physical LUNS of drive */ #ifndef CISS_MAX_LUN #define CISS_MAX_LUN 16 #endif -#define CISS_MAX_PHYS_LUN 1024 #define CISS_REPORT_PHYS 0xc3 #define LSCSI_DRIVER_SENSE 0x8 /* alternate CHECK CONDITION indication */ diff --git a/cciss.h b/cciss.h index 99452be..2e8648d 100644 --- a/cciss.h +++ b/cciss.h @@ -3,6 +3,14 @@ #define CCISS_H_CVSID "$Id: cciss.h,v 1.1 2007/04/01 16:49:46 shattered Exp $\n" +#define CISS_MAX_PHYS_LUN 1024 +typedef struct _ReportLUNdata_struct +{ + uint32_t LUNListLength; /* always big-endian */ + uint32_t reserved; + uint8_t LUN[CISS_MAX_PHYS_LUN][8]; +} ReportLunData_struct; + int cciss_io_interface(int device, int target, struct scsi_cmnd_io * iop, int report); |
From: Stephen M. C. <sca...@be...> - 2010-03-15 14:44:29
|
From: root <ro...@si...> Introduce concept of an hba (mainly for HP Smart Array) Hi Smartmontools people. I'm looking at trying to improve smartmontools for use with HP Smart arrays. I'm not asking that this patch be integrated at this time, (as it's not complete) but I would like some feedback, (especially if you think I've gone off into the weeds) before I spend too much more time on this. This patch introduces the concept of a "host bus adapter". The "host bus adapter" is introduced for the benefit of HP Smart Arrays. HP Smart Arrays present only logical disks and a controller device to the OS. Physical disks are not presented to the OS. This to some extent violates the default assumption which the design of Smartmontools appears to make, though it has been tweaked to get around this assumption somewhat already. The current design of Smartmontools does not fit very well with how Smart Arrays work. For example, with the current design, each command which is to be sent to a phyiscal device must first be preceded by a query to the controller to determine the 8-byte LUNID of the device. This because the current design has no place to cache such information. The more natural way to do it would be to do this query once per controller per polling interval, and use that data for all the subsequent polling of all the physical drives for that polling interval. This patch is (a so far incomplete) attempt to do that. it works for smartctl, but I haven't grokked smartd quite enough to attempt to get that one to work with this hba concept yet. This patch adds 4 files, host_bus_adapter.h, host_bus_adapter.cpp, hp_smart_array_hba.h, hp_smart_array_hba.cpp. host_bus_adapter.* implements a default "no-op" hba class for use with non-smart-array devices -- it makes things behave as they have always done. hp_smart_array_hba is a subclass of host_bus_adapter, and implements smart array specific stuff, which amounts to 2 things. A way to get a list of physical luns from the hba, and update this list, and a way to translate a device number into an 8-byte lun. The idea would be ane hp_smart_array_hba instance would be instantiated once for each smart array controller (in the case of smartd) or once for whatever smart array device was specified on the command line (for smartctl) and this would do the REPORT PHYSICAL LUNS once (smartctl) or once per polling cycle (smartd), and communication with the physical disks would rely on the LUN information from the hba instead of fetching prior to every single command. So... have a look at the patch, and let me know if I've gone off the deep end, or if I'm more or less on the right track, and if you have advice about how to fit this into smartd, I'd like to hear that too. (I think I need to have a list of host_bus_adapters... and have each device associated with the right one -- for non-smart-array, that means a no-op hba (they can all share the same one), and for smart arrays, I'd need one hp_smart_array_hba instance per controller.) The details of how to do that for smartd I haven't quite worked out yet. -- steve Signed-off-by: Stephen M. Cameron <sca...@be...> --- cciss.cpp | 53 ++++++--------------------------------- cciss.h | 4 ++- dev_interface.cpp | 39 +++++++++++++++++++++++++--- dev_interface.h | 21 +++++++++++++++ host_bus_adapter.cpp | 28 ++++++++++++++++++++ host_bus_adapter.h | 33 ++++++++++++++++++++++++ hp_smart_array_hba.cpp | 66 ++++++++++++++++++++++++++++++++++++++++++++++++ hp_smart_array_hba.h | 34 +++++++++++++++++++++++++ os_linux.cpp | 17 ++++++++---- smartctl.cpp | 7 ++++- 10 files changed, 242 insertions(+), 60 deletions(-) create mode 100644 host_bus_adapter.cpp create mode 100644 host_bus_adapter.h create mode 100644 hp_smart_array_hba.cpp create mode 100644 hp_smart_array_hba.h diff --git a/cciss.cpp b/cciss.cpp index 3f47020..232d9fe 100644 --- a/cciss.cpp +++ b/cciss.cpp @@ -32,6 +32,9 @@ #include "utility.h" #include "cciss.h" +#include "host_bus_adapter.h" +#include "cciss.h" + const char *cciss_c_cvsid="$Id: cciss.cpp,v 1.9 2008/07/30 20:42:53 chrfranke Exp $" CONFIG_H_CVSID INT64_H_CVSID SCSICMDS_H_CVSID UTILITY_H_CVSID; @@ -44,7 +47,6 @@ CONFIG_H_CVSID INT64_H_CVSID SCSICMDS_H_CVSID UTILITY_H_CVSID; #define LSCSI_DRIVER_SENSE 0x8 /* alternate CHECK CONDITION indication */ #define SEND_IOCTL_RESP_SENSE_LEN 16 /* ioctl limitation */ -static int cciss_getlun(int device, int target, unsigned char *physlun, int report); static int cciss_sendpassthru(unsigned int cmdtype, unsigned char *CDB, unsigned int CDBlen, char *buff, unsigned int size, unsigned int LunID, @@ -54,17 +56,18 @@ static int cciss_sendpassthru(unsigned int cmdtype, unsigned char *CDB, This is an interface that uses the cciss passthrough to talk to the SMART controller on the HP system. The cciss driver provides a way to send SCSI cmds through the CCISS passthrough. */ -int cciss_io_interface(int device, int target, struct scsi_cmnd_io * iop, int report) +int cciss_io_interface(int device, int target, host_bus_adapter *hba, + struct scsi_cmnd_io * iop, int report) { unsigned char pBuf[512] = {0}; unsigned char phylun[8] = {0}; int iBufLen = 512; int status = -1; int len = 0; // used later in the code. - - status = cciss_getlun(device, target, phylun, report); + + status = hba->device_get_info(target, phylun); if (report > 0) - printf(" cciss_getlun(%d, %d) = 0x%x; scsi3addr: %02x %02x %02x %02x %02x %02x %02x %02x\n", + printf(" hba->device_get_info(%d, %d) = 0x%x; scsi3addr: %02x %02x %02x %02x %02x %02x %02x %02x\n", device, target, status, phylun[0], phylun[1], phylun[2], phylun[3], phylun[4], phylun[5], phylun[6], phylun[7]); if (status) { @@ -185,44 +188,4 @@ int cciss_report_physical_luns(int fd, ReportLunData_struct *report_lun_data) return cciss_sendpassthru(0, CDB, 12, (char *) report_lun_data, sizeof(*report_lun_data), 0, NULL, fd); } -static int cciss_getlun(int device, int target, unsigned char *physlun, int report) -{ - ReportLunData_struct *luns; - int reportlunsize = sizeof(*luns) + CISS_MAX_PHYS_LUN * 8; - int ret; - - luns = (ReportLunData_struct *)malloc(reportlunsize); - - ret = cciss_report_physical_luns(device, luns); - if (ret) - { - free(luns); - return ret; - } - - if (report > 1) - { - unsigned int i,j; - unsigned char *stuff = (unsigned char *)luns; - - pout("\n===== [%s] DATA START (BASE-16) =====\n", "LUN DATA"); - for (i=0; i<(sizeof(_ReportLUNdata_struct)+15)/16; i++){ - pout("%03d-%03d: ", 16*i, 16*(i+1)-1); - for (j=0; j<15; j++) - pout("%02x ",*stuff++); - pout("%02x\n",*stuff++); - } - pout("===== [%s] DATA END (%u Bytes) =====\n\n", "LUN DATA", (unsigned)sizeof(_ReportLUNdata_struct)); - } - - if (target >= 0 && target < (int) be32toh(luns->LUNListLength) / 8) - { - memcpy(physlun, luns->LUN[target], 8); - free(luns); - return 0; - } - - free(luns); - return 1; -} #endif diff --git a/cciss.h b/cciss.h index 71e9e8b..1cdf871 100644 --- a/cciss.h +++ b/cciss.h @@ -3,6 +3,8 @@ #define CCISS_H_CVSID "$Id: cciss.h,v 1.1 2007/04/01 16:49:46 shattered Exp $\n" +#include "host_bus_adapter.h" + #define CISS_MAX_PHYS_LUN 1024 typedef struct _ReportLUNdata_struct { @@ -11,7 +13,7 @@ typedef struct _ReportLUNdata_struct uint8_t LUN[CISS_MAX_PHYS_LUN][8]; } ReportLunData_struct; -int cciss_io_interface(int device, int target, +int cciss_io_interface(int device, int target, host_bus_adapter *hba, struct scsi_cmnd_io * iop, int report); int cciss_report_physical_luns(int fd, ReportLunData_struct *report_lun_data); diff --git a/dev_interface.cpp b/dev_interface.cpp index a14f47c..0dc8d41 100644 --- a/dev_interface.cpp +++ b/dev_interface.cpp @@ -25,16 +25,25 @@ #include <stdexcept> +static host_bus_adapter no_op_hba; // default "no-op" host bus adapter. + const char * dev_interface_cpp_cvsid = "$Id: dev_interface.cpp 2971 2009-10-26 22:05:54Z chrfranke $" DEV_INTERFACE_H_CVSID; ///////////////////////////////////////////////////////////////////////////// // smart_device - smart_device::smart_device(smart_interface * intf, const char * dev_name, const char * dev_type, const char * req_type) : m_intf(intf), m_info(dev_name, dev_type, req_type), - m_ata_ptr(0), m_scsi_ptr(0) + m_ata_ptr(0), m_scsi_ptr(0), m_hba_ptr(&no_op_hba) +{ +} + +smart_device::smart_device(smart_interface * intf, host_bus_adapter * hba, + const char * dev_name, + const char * dev_type, const char * req_type) +: m_intf(intf), m_info(dev_name, dev_type, req_type), + m_ata_ptr(0), m_scsi_ptr(0), m_hba_ptr(hba) { } @@ -269,11 +278,20 @@ const char * smart_interface::get_msg_for_errno(int no) return strerror(no); } +///////////////////////////////////////////////////////////////////////////// +// Default host bus adapter factory +host_bus_adapter * smart_interface::get_smart_hba(const char *name, const char * type) +{ + if (strncmp(type, "cciss", 5)) + return &no_op_hba; + return new hp_smart_array_hba(name); +} ///////////////////////////////////////////////////////////////////////////// // Default device factory -smart_device * smart_interface::get_smart_device(const char * name, const char * type) +smart_device * smart_interface::get_smart_device(const char * name, + host_bus_adapter * hba, const char * type) { clear_err(); if (!type || !*type) { @@ -283,7 +301,7 @@ smart_device * smart_interface::get_smart_device(const char * name, const char * return dev; } - smart_device * dev = get_custom_smart_device(name, type); + smart_device * dev = get_custom_smart_device(name, hba, type); if (dev || get_errno()) return dev; @@ -328,11 +346,22 @@ smart_device * smart_interface::get_smart_device(const char * name, const char * return dev; } -smart_device * smart_interface::get_custom_smart_device(const char * /*name*/, const char * /*type*/) +smart_device * smart_interface::get_smart_device(const char * name, const char * type) +{ + return get_smart_device(name, &no_op_hba, type); +} + +smart_device * smart_interface::get_custom_smart_device(const char * /*name*/, + host_bus_adapter * /* hba */, const char * /*type*/) { return 0; } +smart_device * smart_interface::get_custom_smart_device(const char * name, const char * type) +{ + return get_custom_smart_device(name, &no_op_hba, type); +} + std::string smart_interface::get_valid_custom_dev_types_str() { return ""; diff --git a/dev_interface.h b/dev_interface.h index 5f54fd8..5456061 100644 --- a/dev_interface.h +++ b/dev_interface.h @@ -25,6 +25,9 @@ #include <string> #include <vector> +#include "host_bus_adapter.h" +#include "hp_smart_array_hba.h" + #if !defined(__GNUC__) && !defined(__attribute__) #define __attribute__(x) /**/ #endif @@ -80,6 +83,8 @@ protected: /// Must be called in implementation classes. smart_device(smart_interface * intf, const char * dev_name, const char * dev_type, const char * req_type); + smart_device(smart_interface * intf, host_bus_adapter * hba, const char * dev_name, + const char * dev_type, const char * req_type); /// Dummy enum for dummy constructor. enum do_not_use_in_implementation_classes { never_called }; @@ -227,6 +232,7 @@ private: ata_device * m_ata_ptr; scsi_device * m_scsi_ptr; error_info m_err; + host_bus_adapter *m_hba_ptr; // Prevent copy/assigment smart_device(const smart_device &); @@ -740,11 +746,22 @@ public: /////////////////////////////////////////////////////////////////////////// // Device factory: + /// Return host bus adatper object for device 'name' with some 'type'. + /// 'type' is 0 if not specified by user. + /// Return 0 on error. + /// Default implementation selects no_op_hba + virtual host_bus_adapter * get_smart_hba(const char * name, const char * type); + + /////////////////////////////////////////////////////////////////////////// + // Device factory: + /// Return device object for device 'name' with some 'type'. /// 'type' is 0 if not specified by user. /// Return 0 on error. /// Default implementation selects between ata, scsi and custom device. - virtual smart_device * get_smart_device(const char * name, const char * type); + virtual smart_device * get_smart_device(const char * name, const char * type); + virtual smart_device * get_smart_device(const char * name, + host_bus_adapter * hba, const char * type); /// Fill 'devlist' with devices of some 'type' with devices names. /// specified by some optional 'pattern'. @@ -765,6 +782,8 @@ protected: /// Return device for platform specific 'type'. /// Default implementation returns 0. virtual smart_device * get_custom_smart_device(const char * name, const char * type); + virtual smart_device * get_custom_smart_device(const char * name, host_bus_adapter *, + const char * type); /// Return valid 'type' args accepted by above. /// This is called in get_valid_dev_types_str(). diff --git a/host_bus_adapter.cpp b/host_bus_adapter.cpp new file mode 100644 index 0000000..821b409 --- /dev/null +++ b/host_bus_adapter.cpp @@ -0,0 +1,28 @@ +/* + * host_bus_adapter.cpp + * + * Home page of code is: http://smartmontools.sourceforge.net + * + * Copyright (C) 2008-9 Christian Franke <sma...@li...> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2, or (at your option) + * any later version. + * + * You should have received a copy of the GNU General Public License + * (for example COPYING); If not, see <http://www.gnu.org/licenses/>. + * + */ + +#include "host_bus_adapter.h" + +int host_bus_adapter::device_get_info(int device, void *buffer) +{ + return 0; +} + +void host_bus_adapter::update_hba_info() +{ + return; +} diff --git a/host_bus_adapter.h b/host_bus_adapter.h new file mode 100644 index 0000000..a9f1b1a --- /dev/null +++ b/host_bus_adapter.h @@ -0,0 +1,33 @@ +#ifndef __HOST_BUS_ADAPTER_H__ +#define __HOST_BUS_ADAPTER_H__ +/* + * host_bus_adapter.h + * + * Home page of code is: http://smartmontools.sourceforge.net + * + * Copyright (C) 2008-9 Christian Franke <sma...@li...> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2, or (at your option) + * any later version. + * + * You should have received a copy of the GNU General Public License + * (for example COPYING); If not, see <http://www.gnu.org/licenses/>. + * + */ + +#include <string> + +// Abstract base class in case there is anything which needs to be done per +// host bus adapter, as opposed to per target device, +// or things which the various targets can cache on a per-hba +// basis, this is where that would go. This was introduced for HP +// Smart Arrays initially. + +class host_bus_adapter { +public: + virtual int device_get_info(int device, void *buffer); + virtual void update_hba_info(); +}; +#endif diff --git a/hp_smart_array_hba.cpp b/hp_smart_array_hba.cpp new file mode 100644 index 0000000..2363e73 --- /dev/null +++ b/hp_smart_array_hba.cpp @@ -0,0 +1,66 @@ +/* + * hp_smart_array_hba.h + * + * Home page of code is: http://smartmontools.sourceforge.net + * + * Copyright (C) 2008-9 Christian Franke <sma...@li...> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2, or (at your option) + * any later version. + * + * You should have received a copy of the GNU General Public License + * (for example COPYING); If not, see <http://www.gnu.org/licenses/>. + * + */ + +#include <stdint.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <arpa/inet.h> /* for ntohl, etc. */ +#include <string.h> +#include <errno.h> + +#include "hp_smart_array_hba.h" +#include "cciss.h" + +hp_smart_array_hba::hp_smart_array_hba(std::string device_name) : host_bus_adapter() +{ + dev_name = device_name; + (void) update_hba_info(); +} + +// In the case of smart array, the "device info" is the 8-byte LUNID +int hp_smart_array_hba::device_get_info(int device, void *buffer) +{ + uint32_t num_luns; + + if (!data_valid) { + (void) update_hba_info(); + if (!data_valid) + return -1; + } + + num_luns = ntohl(report_lun_data.LUNListLength) / 8; + + if (device < 0 || (uint32_t) device >= num_luns) + return -ENXIO; + + memcpy(buffer, &report_lun_data.LUN[device][0], 8); + return 0; +} + +void hp_smart_array_hba::update_hba_info() +{ + int fd; + + data_valid = 0; + fd = open(dev_name.c_str(), O_RDWR); + if (fd < 0) + return; + data_valid = !cciss_report_physical_luns(fd, &report_lun_data); + close(fd); +} + diff --git a/hp_smart_array_hba.h b/hp_smart_array_hba.h new file mode 100644 index 0000000..98753ab --- /dev/null +++ b/hp_smart_array_hba.h @@ -0,0 +1,34 @@ +#ifndef __HP_SMART_ARRAY_HBA_H__ +#define __HP_SMART_ARRAY_HBA_H__ +/* + * hp_smart_array_hba.h + * + * Home page of code is: http://smartmontools.sourceforge.net + * + * Copyright (C) 2008-9 Christian Franke <sma...@li...> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2, or (at your option) + * any later version. + * + * You should have received a copy of the GNU General Public License + * (for example COPYING); If not, see <http://www.gnu.org/licenses/>. + * + */ +#include "host_bus_adapter.h" +#include "cciss.h" + +#define CISS_MAX_PHY_LUN 1024 // this is probably too big, but no real harm comes of it. + +class hp_smart_array_hba : public host_bus_adapter { +public: + hp_smart_array_hba(std::string device_name); + virtual int device_get_info(int device, void *buffer); + virtual void update_hba_info(); +private: + ReportLunData_struct report_lun_data; + int data_valid; + std::string dev_name; +}; +#endif diff --git a/os_linux.cpp b/os_linux.cpp index cf6a728..66ee11a 100644 --- a/os_linux.cpp +++ b/os_linux.cpp @@ -1178,26 +1178,29 @@ class linux_cciss_device public /*extends*/ linux_smart_device { public: - linux_cciss_device(smart_interface * intf, const char * name, unsigned char disknum); + linux_cciss_device(smart_interface * intf, host_bus_adapter *hba, + const char * name, unsigned char disknum); virtual bool scsi_pass_through(scsi_cmnd_io * iop); private: unsigned char m_disknum; ///< Disk number. + host_bus_adapter *m_hba_ptr; }; linux_cciss_device::linux_cciss_device(smart_interface * intf, + host_bus_adapter *hba, const char * dev_name, unsigned char disknum) : smart_device(intf, dev_name, "cciss", "cciss"), linux_smart_device(O_RDWR | O_NONBLOCK), - m_disknum(disknum) + m_disknum(disknum), m_hba_ptr(hba) { set_info().info_name = strprintf("%s [cciss_disk_%02d]", dev_name, disknum); } bool linux_cciss_device::scsi_pass_through(scsi_cmnd_io * iop) { - int status = cciss_io_interface(get_fd(), m_disknum, iop, con->reportscsiioctl); + int status = cciss_io_interface(get_fd(), m_disknum, m_hba_ptr, iop, con->reportscsiioctl); if (status < 0) return set_err(-status); return true; @@ -2810,7 +2813,8 @@ protected: virtual smart_device * autodetect_smart_device(const char * name); - virtual smart_device * get_custom_smart_device(const char * name, const char * type); + virtual smart_device * get_custom_smart_device(const char * name, + host_bus_adapter *hba, const char * type); virtual std::string get_valid_custom_dev_types_str(); @@ -3100,7 +3104,8 @@ smart_device * linux_smart_interface::autodetect_smart_device(const char * name) return 0; } -smart_device * linux_smart_interface::get_custom_smart_device(const char * name, const char * type) +smart_device * linux_smart_interface::get_custom_smart_device(const char * name, + host_bus_adapter *hba, const char * type) { // Marvell ? if (!strcmp(type, "marvell")) @@ -3176,7 +3181,7 @@ smart_device * linux_smart_interface::get_custom_smart_device(const char * name, set_err(EINVAL, "Option -d cciss,N (N=%d) must have 0 <= N <= 15", disknum); return 0; } - return new linux_cciss_device(this, name, disknum); + return new linux_cciss_device(this, hba, name, disknum); } #endif // HAVE_LINUX_CCISS_IOCTL_H diff --git a/smartctl.cpp b/smartctl.cpp index a5f165a..f57d3b2 100644 --- a/smartctl.cpp +++ b/smartctl.cpp @@ -872,6 +872,7 @@ static const char * get_protocol_info(const smart_device * dev) // Main program without exception handling int main_worker(int argc, char **argv) { + host_bus_adapter *hba; // Initialize interface smart_interface::init(); if (!smi()) @@ -903,9 +904,11 @@ int main_worker(int argc, char **argv) } dev = get_parsed_ata_device(smi(), name); } - else + else { // get device of appropriate type - dev = smi()->get_smart_device(name, type); + hba = smi()->get_smart_hba(name, type); + dev = smi()->get_smart_device(name, hba, type); + } if (!dev) { pout("%s: %s\n", name, smi()->get_errmsg()); |
From: Stephen M. C. <sca...@be...> - 2010-03-15 14:45:37
|
From: root <ro...@si...> Remove limit of 15 devices for cciss Signed-of-by: Stephen M. Cameron <sca...@be...> --- dev_legacy.cpp | 5 +++-- os_freebsd.cpp | 5 +++-- os_linux.cpp | 5 +++-- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/dev_legacy.cpp b/dev_legacy.cpp index c2a8cfa..4189495 100644 --- a/dev_legacy.cpp +++ b/dev_legacy.cpp @@ -639,8 +639,9 @@ smart_device * legacy_smart_interface::get_custom_smart_device(const char * name set_err(EINVAL, "Option -d cciss,N requires N to be a non-negative integer"); return 0; } - if (!(0 <= disknum && disknum <= 15)) { - set_err(EINVAL, "Option -d cciss,N (N=%d) must have 0 <= N <= 15", disknum); + if (!(0 <= disknum && disknum <= CISS_MAX_PHYS_LUN - 1)) { + set_err(EINVAL, "Option -d cciss,N (N=%d) must have 0 <= N <= %d", disknum, + CISS_MAX_PHYS_LUN - 1); return 0; } return new legacy_cciss_device(this, name, disknum); diff --git a/os_freebsd.cpp b/os_freebsd.cpp index 0627af5..4097a2d 100644 --- a/os_freebsd.cpp +++ b/os_freebsd.cpp @@ -1897,8 +1897,9 @@ smart_device * freebsd_smart_interface::get_custom_smart_device(const char * nam set_err(EINVAL, "Option -d cciss,N requires N to be a non-negative integer"); return 0; } - if (!(0 <= disknum && disknum <= 15)) { - set_err(EINVAL, "Option -d cciss,N (N=%d) must have 0 <= N <= 15", disknum); + if (!(0 <= disknum && disknum <= CISS_MAX_PHYS_LUN - 1)) { + set_err(EINVAL, "Option -d cciss,N (N=%d) must have 0 <= N <= %d", disknum, + CISS_MAX_PHYS_LUN - 1); return 0; } return new freebsd_cciss_device(this, name, disknum); diff --git a/os_linux.cpp b/os_linux.cpp index 66ee11a..1365c72 100644 --- a/os_linux.cpp +++ b/os_linux.cpp @@ -3177,8 +3177,9 @@ smart_device * linux_smart_interface::get_custom_smart_device(const char * name, set_err(EINVAL, "Option -d cciss,N requires N to be a non-negative integer"); return 0; } - if (!(0 <= disknum && disknum <= 15)) { - set_err(EINVAL, "Option -d cciss,N (N=%d) must have 0 <= N <= 15", disknum); + if (!(0 <= disknum && disknum <= CISS_MAX_PHYS_LUN - 1)) { + set_err(EINVAL, "Option -d cciss,N (N=%d) must have 0 <= N <= %d", disknum, + CISS_MAX_PHYS_LUN - 1); return 0; } return new linux_cciss_device(this, hba, name, disknum); |
From: Alex K. <ad...@li...> - 2010-03-15 19:33:24
|
15.03.2010 17:31, Stephen M. Cameron пишет: > From: root<ro...@si...> > > Remove limit of 15 devices for cciss > > Signed-of-by: Stephen M. Cameron<sca...@be...> > --- > dev_legacy.cpp | 5 +++-- > os_freebsd.cpp | 5 +++-- > os_linux.cpp | 5 +++-- > 3 files changed, 9 insertions(+), 6 deletions(-) > > diff --git a/dev_legacy.cpp b/dev_legacy.cpp > index c2a8cfa..4189495 100644 > --- a/dev_legacy.cpp > +++ b/dev_legacy.cpp > @@ -639,8 +639,9 @@ smart_device * legacy_smart_interface::get_custom_smart_device(const char * name > set_err(EINVAL, "Option -d cciss,N requires N to be a non-negative integer"); > return 0; > } > - if (!(0<= disknum&& disknum<= 15)) { > - set_err(EINVAL, "Option -d cciss,N (N=%d) must have 0<= N<= 15", disknum); > + if (!(0<= disknum&& disknum<= CISS_MAX_PHYS_LUN - 1)) { > + set_err(EINVAL, "Option -d cciss,N (N=%d) must have 0<= N<= %d", disknum, > + CISS_MAX_PHYS_LUN - 1); > return 0; > } > return new legacy_cciss_device(this, name, disknum); > diff --git a/os_freebsd.cpp b/os_freebsd.cpp > index 0627af5..4097a2d 100644 > --- a/os_freebsd.cpp > +++ b/os_freebsd.cpp > @@ -1897,8 +1897,9 @@ smart_device * freebsd_smart_interface::get_custom_smart_device(const char * nam > set_err(EINVAL, "Option -d cciss,N requires N to be a non-negative integer"); > return 0; > } > - if (!(0<= disknum&& disknum<= 15)) { > - set_err(EINVAL, "Option -d cciss,N (N=%d) must have 0<= N<= 15", disknum); > + if (!(0<= disknum&& disknum<= CISS_MAX_PHYS_LUN - 1)) { > + set_err(EINVAL, "Option -d cciss,N (N=%d) must have 0<= N<= %d", disknum, > + CISS_MAX_PHYS_LUN - 1); > return 0; > } > return new freebsd_cciss_device(this, name, disknum); > diff --git a/os_linux.cpp b/os_linux.cpp > index 66ee11a..1365c72 100644 > --- a/os_linux.cpp > +++ b/os_linux.cpp > @@ -3177,8 +3177,9 @@ smart_device * linux_smart_interface::get_custom_smart_device(const char * name, > set_err(EINVAL, "Option -d cciss,N requires N to be a non-negative integer"); > return 0; > } > - if (!(0<= disknum&& disknum<= 15)) { > - set_err(EINVAL, "Option -d cciss,N (N=%d) must have 0<= N<= 15", disknum); > + if (!(0<= disknum&& disknum<= CISS_MAX_PHYS_LUN - 1)) { > + set_err(EINVAL, "Option -d cciss,N (N=%d) must have 0<= N<= %d", disknum, > + CISS_MAX_PHYS_LUN - 1); > return 0; > } > return new linux_cciss_device(this, hba, name, disknum); It work for FreeBSD? I have box with 24 disk on ciss - FreeBSD8 amd64 |
From: Christian F. <Chr...@t-...> - 2010-03-15 16:57:42
|
Hi, Stephen M. Cameron wrote: > Hi Smartmontools people, > > I'm looking at trying to improve smartmontools support for HP Smart Arrays > a little bit and would like some feed back on what I'm doing, though at > this time, I am sure you don't want all of these patches (as they aren't > finished.) > > Thanks for your contribution! > The current design of smartmontools does not match up very well with > how smart arrays work, so I'm trying to tweak it a little bit to make > it a better fit, without warping it too much so that it remains a good > fit with other devices. The main thing I did was add the concept of a > "host bus adapter" (hba) to which devices could be associated, and things > which could be done on a per-host-bus-adapter basis (eg. CISS_REPORT_PHYS) > I did there. For non-Smart-Array devices, there is a kind of "no-op" hba > which can be shared by all devices. For smart array, I derived a subclass > of this host_bus_adapter which does the smart array specific stuff. It may > be the case that other kinds of controllers can benefit from this hba concept > as well, if they have similar things which can be done on a per-hba > basis rather than a per-disk basis. > This makes sense but is IMO still a controller-specific implementation issue. Could you please explain the benefit of making the host_bus_adapter object visible in the dev_interface.h classes? The interface visible to smartctl+smartd should be kept as simple as possible. > The transition is not complete... I've got smartctl working in this > new way, but not smartd. I haven't quite grokked smartd enough to know > how to fit it in. Don't touch dev_interface.h => get smartd support for free :-) smartctl is a simpler use case: smartctl allocates only one device object and opens it only once. smartd allocates several device objects and performs open/close within each check cycle. > I think I need a list of hp_smart_array_hbas, one > per controller, with which I associate each device (and all non-smart-array > devices can share the same no-op hba), but the details of how to do it, > I'm not exactly clear yet. > If such a list is needed, it can and should be maintained behind the os_*.cpp scenes. > Anyway, have a look, and if I'm way off in the weeds, tell me that, > or if I'm more or less on the right track, tell me that, or tell > me whatever you think. > > See further notes below: > diff --git a/dev_interface.cpp b/dev_interface.cpp > ... > smart_device::smart_device(smart_interface * intf, const char * dev_name, > const char * dev_type, const char * req_type) > : m_intf(intf), m_info(dev_name, dev_type, req_type), > - m_ata_ptr(0), m_scsi_ptr(0) > + m_ata_ptr(0), m_scsi_ptr(0), m_hba_ptr(&no_op_hba) > The m_hba_ptr is an implementation issue. It should be added to the related implementation class. > +// Default host bus adapter factory > +host_bus_adapter * smart_interface::get_smart_hba(const char *name, const char * type) > Not needed. > +{ > + if (strncmp(type, "cciss", 5)) > + return&no_op_hba; > + return new hp_smart_array_hba(name); > BTW: new without delete. > diff --git a/host_bus_adapter.h b/host_bus_adapter.h > ... > +class host_bus_adapter { > +public: > + virtual int device_get_info(int device, void *buffer); > + virtual void update_hba_info(); > +}; > This interface does not provide useful functionality to smartctl+smartd and apparently only returns controller-specific (???void *) data. > diff --git a/smartctl.cpp b/smartctl.cpp > index a5f165a..f57d3b2 100644 > --- a/smartctl.cpp > +++ b/smartctl.cpp > @@ -872,6 +872,7 @@ static const char * get_protocol_info(const smart_device * dev) > // Main program without exception handling > int main_worker(int argc, char **argv) > { > + host_bus_adapter *hba; > // Initialize interface > smart_interface::init(); > if (!smi()) > @@ -903,9 +904,11 @@ int main_worker(int argc, char **argv) > } > dev = get_parsed_ata_device(smi(), name); > } > - else > + else { > // get device of appropriate type > - dev = smi()->get_smart_device(name, type); > + hba = smi()->get_smart_hba(name, type); > + dev = smi()->get_smart_device(name, hba, type); > This and related changes should not be necessary. The hba object is not used elsewere in smartctl. I would suggest: - Move m_hba_ptr from smart_device to linux/freebsd_cciss_device. - Allocate hp_smart_array_hba object in the CCISS section of linux/freebsd_smart_interface::get_custom_smart_device(). - Attach hba object to device object in linux/freebsd_cciss_device constructor. - Make sure hba object is deleted somewhere. Maintain a reference count if several device objects share one hba object. - Move hp_smart_array_hba and other shared (Linux+FreeBSD) functions to cciss.cpp - Remove host_bus_adapter base class. - Discard all changes to dev_interface.*. This would make the patch much simpler without changing functionality. Thanks, Christian |
From: <sca...@be...> - 2010-03-15 18:42:11
|
On Mon, Mar 15, 2010 at 05:57:19PM +0100, Christian Franke wrote: > Hi, > > Stephen M. Cameron wrote: > >Hi Smartmontools people, > > > >I'm looking at trying to improve smartmontools support for HP Smart Arrays > >a little bit and would like some feed back on what I'm doing, though at > >this time, I am sure you don't want all of these patches (as they aren't > >finished.) > > > > > > Thanks for your contribution! Thanks for the feedback, just what I was looking for. It will take me awhile to digest it, and it'll probably be a week or two before I can get back to looking at this. The main problem I'm trying to fix is that before *any* command may be sent to smart array disks, it currently has to do a CISS_REPORT_PHYS, *every time* for *every command*, and this means it's talking to the hardware approximately 2x as much as it really needs to, which is really not cool. So, to avoid that there needs to be some way, at the time each disk object is made , to say, "by the way, here's your 8-byte LUNID so you don't have to look it up every single time." And you don't want each disk looking up its LUNID and caching it, because if you have, say 128 disks, then you're going to be doing the identical CISS_REPORT_PHYS 128 times, getting back identically the same data each time, once per disk. You want to do the CISS_REPORT_PHYS *once per hba* (smartctl) or *once per hba per polling cycle* (smartd), and re-use that data for all the disks. It's just not a per-disk thing. The current implementation abuses the smart array for the purpose of conforming to the existing smartmontools design. Or so it appears to me. That's my motivation for this change, and that's the problem I'm trying to solve. I was having a hard time figuring out how to change that behavior without changing the interface a bit to have a way to give the disk device objects their own addressing information *once*, but well, you saw what I came up with -- a general notion of an HBA, and a derived class particularized for smart array. The device_get_info was meant as a hardware specific method which gets whatever it is some disk device may need to get on a once-per-hba basis. Maybe Smart Array is the only thing out there with anything like that, though I would think any hardware raid controller which does not present physical disks to the OS might have something similar going on. I was not seeing any way to inform the disk devices of any information which might have been gathered previously in the current interfaces. Below, you say, about maintaining a list of hbas in smartd: > If such a list is needed, it can and should be maintained behind the > os_*.cpp scenes. If you have some more concrete suggestions it might help me to know what you have in mind on how to implement this. Of course, it's possible I am missing something obvious, as I'm not so hot at C++ (as you can probably tell.) -- steve > > >The current design of smartmontools does not match up very well with > >how smart arrays work, so I'm trying to tweak it a little bit to make > >it a better fit, without warping it too much so that it remains a good > >fit with other devices. The main thing I did was add the concept of a > >"host bus adapter" (hba) to which devices could be associated, and things > >which could be done on a per-host-bus-adapter basis (eg. CISS_REPORT_PHYS) > >I did there. For non-Smart-Array devices, there is a kind of "no-op" hba > >which can be shared by all devices. For smart array, I derived a subclass > >of this host_bus_adapter which does the smart array specific stuff. It may > >be the case that other kinds of controllers can benefit from this hba > >concept > >as well, if they have similar things which can be done on a per-hba > >basis rather than a per-disk basis. > > > > This makes sense but is IMO still a controller-specific implementation > issue. > > Could you please explain the benefit of making the host_bus_adapter > object visible in the dev_interface.h classes? The interface visible to > smartctl+smartd should be kept as simple as possible. > > > >The transition is not complete... I've got smartctl working in this > >new way, but not smartd. I haven't quite grokked smartd enough to know > >how to fit it in. > > Don't touch dev_interface.h => get smartd support for free :-) > > smartctl is a simpler use case: > smartctl allocates only one device object and opens it only once. > smartd allocates several device objects and performs open/close within > each check cycle. > > > >I think I need a list of hp_smart_array_hbas, one > >per controller, with which I associate each device (and all non-smart-array > >devices can share the same no-op hba), but the details of how to do it, > >I'm not exactly clear yet. > > > > If such a list is needed, it can and should be maintained behind the > os_*.cpp scenes. > > > >Anyway, have a look, and if I'm way off in the weeds, tell me that, > >or if I'm more or less on the right track, tell me that, or tell > >me whatever you think. > > > > > > See further notes below: > > > >diff --git a/dev_interface.cpp b/dev_interface.cpp > >... > > smart_device::smart_device(smart_interface * intf, const char * dev_name, > > const char * dev_type, const char * req_type) > > : m_intf(intf), m_info(dev_name, dev_type, req_type), > >- m_ata_ptr(0), m_scsi_ptr(0) > >+ m_ata_ptr(0), m_scsi_ptr(0), m_hba_ptr(&no_op_hba) > > > > The m_hba_ptr is an implementation issue. It should be added to the > related implementation class. > > > >+// Default host bus adapter factory > >+host_bus_adapter * smart_interface::get_smart_hba(const char *name, const > >char * type) > > > > Not needed. > > > >+{ > >+ if (strncmp(type, "cciss", 5)) > >+ return&no_op_hba; > >+ return new hp_smart_array_hba(name); > > > > BTW: new without delete. > > > >diff --git a/host_bus_adapter.h b/host_bus_adapter.h > >... > >+class host_bus_adapter { > >+public: > >+ virtual int device_get_info(int device, void *buffer); > >+ virtual void update_hba_info(); > >+}; > > > > This interface does not provide useful functionality to smartctl+smartd > and apparently only returns controller-specific (???void *) data. > > > >diff --git a/smartctl.cpp b/smartctl.cpp > >index a5f165a..f57d3b2 100644 > >--- a/smartctl.cpp > >+++ b/smartctl.cpp > >@@ -872,6 +872,7 @@ static const char * get_protocol_info(const > >smart_device * dev) > > // Main program without exception handling > > int main_worker(int argc, char **argv) > > { > >+ host_bus_adapter *hba; > > // Initialize interface > > smart_interface::init(); > > if (!smi()) > >@@ -903,9 +904,11 @@ int main_worker(int argc, char **argv) > > } > > dev = get_parsed_ata_device(smi(), name); > > } > >- else > >+ else { > > // get device of appropriate type > >- dev = smi()->get_smart_device(name, type); > >+ hba = smi()->get_smart_hba(name, type); > >+ dev = smi()->get_smart_device(name, hba, type); > > > > This and related changes should not be necessary. The hba object is not > used elsewere in smartctl. > > I would suggest: > - Move m_hba_ptr from smart_device to linux/freebsd_cciss_device. > - Allocate hp_smart_array_hba object in the CCISS section of > linux/freebsd_smart_interface::get_custom_smart_device(). > - Attach hba object to device object in linux/freebsd_cciss_device > constructor. > - Make sure hba object is deleted somewhere. Maintain a reference count > if several device objects share one hba object. > - Move hp_smart_array_hba and other shared (Linux+FreeBSD) functions to > cciss.cpp > - Remove host_bus_adapter base class. > - Discard all changes to dev_interface.*. > > This would make the patch much simpler without changing functionality. > > Thanks, > Christian |
From: Christian F. <Chr...@t-...> - 2010-03-16 13:14:22
|
Stephen M. Cameron wrote: > On Mon, Mar 15, 2010 at 05:57:19PM +0100, Christian Franke wrote: > >> Hi, >> >> Stephen M. Cameron wrote: >> >>> Hi Smartmontools people, >>> >>> I'm looking at trying to improve smartmontools support for HP Smart Arrays >>> a little bit and would like some feed back on what I'm doing, though at >>> this time, I am sure you don't want all of these patches (as they aren't >>> finished.) >>> >>> >>> >> Thanks for your contribution! >> > Thanks for the feedback, just what I was looking for. > It will take me awhile to digest it, and it'll probably > be a week or two before I can get back to looking at this. > > The main problem I'm trying to fix is that before *any* > command may be sent to smart array disks, it currently has > to do a CISS_REPORT_PHYS, *every time* for *every command*, > and this means it's talking to the hardware approximately > 2x as much as it really needs to, which is really not cool. > > Of course :-) > So, to avoid that there needs to be some way, at the time each > disk object is made , to say, "by the way, here's your 8-byte > LUNID so you don't have to look it up every single time." And > you don't want each disk looking up its LUNID and caching it, > because if you have, say 128 disks, then you're going to be > doing the identical CISS_REPORT_PHYS 128 times, getting back > identically the same data each time, once per disk. You > want to do the CISS_REPORT_PHYS *once per hba* (smartctl) or *once > per hba per polling cycle* (smartd), and re-use that data for > all the disks. It's just not a per-disk thing. A first simple step would be to add the LUNID info as a member to the linux_cciss_device class and update it in a new controller specific open() function. This would reduce CISS_REPORT_PHYS calls from "once per passthrough" to "once per device open" which is the same as "once per smartctl run". A second step would be to add a cache of HBA->LUNIDs info as a static member of linux_cciss_device and query/update it in the open() function. A cache would be easy to implement by std::map. If this cache should be cleared after a smartd polling cycle, we could add something like smart_interface::on_start/end_polling_cycle() and call these in smartd. This could reduce CISS_REPORT_PHYS calls to "once per hba polling cycle" and requires only trivial changes to smartd. > The current > implementation abuses the smart array for the purpose of conforming > to the existing smartmontools design. Or so it appears to me. > > This is true. The CCISS code in os_linux.cpp is essentially a thin wrapper around the old C code which used the old interface (see dev_legacy.cpp and os_*.cpp except freebsd, linux, win32). During migration of os_linux.cpp to the new interface, I did only the bare minimum, especially because I don't have access to all those RAID controllers for testing. http://sourceforge.net/apps/trac/smartmontools/wiki/DeveloperHowToMigrate Thanks, Christian |
From: <sca...@be...> - 2010-03-16 15:34:25
|
On Tue, Mar 16, 2010 at 02:14:01PM +0100, Christian Franke wrote: > Stephen M. Cameron wrote: > >On Mon, Mar 15, 2010 at 05:57:19PM +0100, Christian Franke wrote: > > > >>Hi, > >> > >>Stephen M. Cameron wrote: > >> > >>>Hi Smartmontools people, > >>> > >>>I'm looking at trying to improve smartmontools support for HP Smart > >>>Arrays > >>>a little bit and would like some feed back on what I'm doing, though at > >>>this time, I am sure you don't want all of these patches (as they aren't > >>>finished.) > >>> > >>> > >>> > >>Thanks for your contribution! > >> > >Thanks for the feedback, just what I was looking for. > >It will take me awhile to digest it, and it'll probably > >be a week or two before I can get back to looking at this. > > > >The main problem I'm trying to fix is that before *any* > >command may be sent to smart array disks, it currently has > >to do a CISS_REPORT_PHYS, *every time* for *every command*, > >and this means it's talking to the hardware approximately > >2x as much as it really needs to, which is really not cool. > > > > > > Of course :-) > > > >So, to avoid that there needs to be some way, at the time each > >disk object is made , to say, "by the way, here's your 8-byte > >LUNID so you don't have to look it up every single time." And > >you don't want each disk looking up its LUNID and caching it, > >because if you have, say 128 disks, then you're going to be > >doing the identical CISS_REPORT_PHYS 128 times, getting back > >identically the same data each time, once per disk. You > >want to do the CISS_REPORT_PHYS *once per hba* (smartctl) or *once > >per hba per polling cycle* (smartd), and re-use that data for > >all the disks. It's just not a per-disk thing. > > A first simple step would be to add the LUNID info as a member to the > linux_cciss_device class and update it in a new controller specific > open() function. > This would reduce CISS_REPORT_PHYS calls from "once per passthrough" to > "once per device open" which is the same as "once per smartctl run". > > A second step would be to add a cache of HBA->LUNIDs info as a static > member of linux_cciss_device and query/update it in the open() function. > A cache would be easy to implement by std::map. > > If this cache should be cleared after a smartd polling cycle, we could > add something like smart_interface::on_start/end_polling_cycle() and > call these in smartd. This could reduce CISS_REPORT_PHYS calls to "once > per hba polling cycle" and requires only trivial changes to smartd. > > > > >The current > >implementation abuses the smart array for the purpose of conforming > >to the existing smartmontools design. Or so it appears to me. > > > > > > This is true. The CCISS code in os_linux.cpp is essentially a thin > wrapper around the old C code which used the old interface (see > dev_legacy.cpp and os_*.cpp except freebsd, linux, win32). > > During migration of os_linux.cpp to the new interface, I did only the > bare minimum, especially because I don't have access to all those RAID > controllers for testing. > http://sourceforge.net/apps/trac/smartmontools/wiki/DeveloperHowToMigrate Ok, good to see that we're mostly in agreement. It will definitely be at least one week before I can look at this again, but when I do, I'll be trying to mash the code to fit your ideas of how it should be. Thanks for the guidance. -- steve > > > Thanks, > Christian |