S.M.A.R.T. Monitoring Tools

Marcus Sorensen wrote:
> Hi all,
>     I wrote a patch back in 5.40 that allows smartctl to report the
> drives advanced power management level, and whether it's disabled or
> enabled. I did this because I wanted to leverage the existing custom
> passthrough capabilities for megaraid and other cards (as opposed to
> just using hdparm), and be able to use a single utility to make all of
> my disk settings changes. I consider myself more of a hacker than an
> actual developer (which my additions may show) but the changes are
> minor enough that I thought I'd offer to contribute it back, and if
> anyone wants to add it in and support it then great! It has worked for
> us for over a year, with no ill effects.
>
> One thing that would likely need to change is that I simply added a -z
> flag to disable APM, and a -Z<digit>  flag to enable it with a
> particular level. This probably needs to be changed to -l apm,<digit>
> or something like that to fit existing features. The only change that
> I consider invasive is that every instance of words088_255 (e.g.
> words088_255[100-88]) has been changed to e.g. words092_255[100-92].
> Everything else is relatively self-contained and limited to adding a
> few variables, cases for things like printing and argument parsing,
> and functions.
>
> If there is any interest at all let me know and I'll post a patch.

Yes, please.

It IMO makes sense to include some of the popular use cases of hdparm 
(e.g. APM, AAM, standby timeout) into smartctl, even if unrelated to 
SMART. This would allow usage of these settings also in conjunction with 
RAID controllers, non-SAT USB-bridges and other platforms.


> Just today I rebased it on 5.42 and ran some basic tests to make sure
> it still worked.

If possible, please change option '-Z N' to '-l apm,N' as suggested and 
provide a brief addition to smartctl man page.

Thanks,
Christian

On 01/26/2012 07:35 PM, Christian Franke wrote:
> Marcus Sorensen wrote:
>> Hi all,
>>      I wrote a patch back in 5.40 that allows smartctl to report the
>> drives advanced power management level, and whether it's disabled or
>> enabled. I did this because I wanted to leverage the existing custom
>> passthrough capabilities for megaraid and other cards (as opposed to
>> just using hdparm), and be able to use a single utility to make all of
>> my disk settings changes. I consider myself more of a hacker than an
>> actual developer (which my additions may show) but the changes are
>> minor enough that I thought I'd offer to contribute it back, and if
>> anyone wants to add it in and support it then great! It has worked for
>> us for over a year, with no ill effects.
>>
>> One thing that would likely need to change is that I simply added a -z
>> flag to disable APM, and a -Z<digit>   flag to enable it with a
>> particular level. This probably needs to be changed to -l apm,<digit>
>> or something like that to fit existing features. The only change that
>> I consider invasive is that every instance of words088_255 (e.g.
>> words088_255[100-88]) has been changed to e.g. words092_255[100-92].
>> Everything else is relatively self-contained and limited to adding a
>> few variables, cases for things like printing and argument parsing,
>> and functions.
>>
>> If there is any interest at all let me know and I'll post a patch.
> Yes, please.
+1
> It IMO makes sense to include some of the popular use cases of hdparm
> (e.g. APM, AAM, standby timeout) into smartctl, even if unrelated to
> SMART. This would allow usage of these settings also in conjunction with
> RAID controllers, non-SAT USB-bridges and other platforms.
I am absolutely agreed. Another reason for this is that this will 
provide us a universal tool on different platforms. I am administrating 
a lot of different *nix-es and its always pain to remind if i need to 
use hdparm/sdparm/camcontrol/cldevice/... and their keys (sometime bad 
documented and non-intuitive).

Ok, I'll see what I can do regarding the -l flag. It seems horribly
overloaded with things not related to the log, but I can't think of
anything better, and I don't think it makes sense for it to have its
own flags.

On Thu, Jan 26, 2012 at 11:45 AM, Alex Samorukov <ml@...> wrote:
> On 01/26/2012 07:35 PM, Christian Franke wrote:
>>
>> Marcus Sorensen wrote:
>>>
>>> Hi all,
>>>     I wrote a patch back in 5.40 that allows smartctl to report the
>>> drives advanced power management level, and whether it's disabled or
>>> enabled. I did this because I wanted to leverage the existing custom
>>> passthrough capabilities for megaraid and other cards (as opposed to
>>> just using hdparm), and be able to use a single utility to make all of
>>> my disk settings changes. I consider myself more of a hacker than an
>>> actual developer (which my additions may show) but the changes are
>>> minor enough that I thought I'd offer to contribute it back, and if
>>> anyone wants to add it in and support it then great! It has worked for
>>> us for over a year, with no ill effects.
>>>
>>> One thing that would likely need to change is that I simply added a -z
>>> flag to disable APM, and a -Z<digit>   flag to enable it with a
>>> particular level. This probably needs to be changed to -l apm,<digit>
>>> or something like that to fit existing features. The only change that
>>> I consider invasive is that every instance of words088_255 (e.g.
>>> words088_255[100-88]) has been changed to e.g. words092_255[100-92].
>>> Everything else is relatively self-contained and limited to adding a
>>> few variables, cases for things like printing and argument parsing,
>>> and functions.
>>>
>>> If there is any interest at all let me know and I'll post a patch.
>>
>> Yes, please.
>
> +1
>
>> It IMO makes sense to include some of the popular use cases of hdparm
>> (e.g. APM, AAM, standby timeout) into smartctl, even if unrelated to
>> SMART. This would allow usage of these settings also in conjunction with
>> RAID controllers, non-SAT USB-bridges and other platforms.
>
> I am absolutely agreed. Another reason for this is that this will provide us
> a universal tool on different platforms. I am administrating a lot of
> different *nix-es and its always pain to remind if i need to use
> hdparm/sdparm/camcontrol/cldevice/... and their keys (sometime bad
> documented and non-intuitive).
>

Ok, here's my patch. It applies against the stable 5.42 from
sourceforge (as that's what we want to roll out).

One thing I noticed while changing it over to -l was that scterc is
using it's own function 'ataGetSetSCTErrorRecoveryControltime' to set
SCT values, whereas I just added the necessary commands to allow me to
use the existing 'smartcommandhandler'. I'm not sure if one method is
necessarily preferred over the other, but I imagine someone will want
to be consistent, and I'm not really excited to change what I've
already got.

I give no guarantee that this won't break someone else's features in
strange ways, I can only say that we've been running smart with this
patch (well the -z flag version) for awhile and haven't run into
anything unexpected.

On Thu, Jan 26, 2012 at 12:18 PM, Marcus Sorensen <shadowsor@...> wrote:
> Ok, I'll see what I can do regarding the -l flag. It seems horribly
> overloaded with things not related to the log, but I can't think of
> anything better, and I don't think it makes sense for it to have its
> own flags.
>
> On Thu, Jan 26, 2012 at 11:45 AM, Alex Samorukov <ml@...> wrote:
>> On 01/26/2012 07:35 PM, Christian Franke wrote:
>>>
>>> Marcus Sorensen wrote:
>>>>
>>>> Hi all,
>>>>     I wrote a patch back in 5.40 that allows smartctl to report the
>>>> drives advanced power management level, and whether it's disabled or
>>>> enabled. I did this because I wanted to leverage the existing custom
>>>> passthrough capabilities for megaraid and other cards (as opposed to
>>>> just using hdparm), and be able to use a single utility to make all of
>>>> my disk settings changes. I consider myself more of a hacker than an
>>>> actual developer (which my additions may show) but the changes are
>>>> minor enough that I thought I'd offer to contribute it back, and if
>>>> anyone wants to add it in and support it then great! It has worked for
>>>> us for over a year, with no ill effects.
>>>>
>>>> One thing that would likely need to change is that I simply added a -z
>>>> flag to disable APM, and a -Z<digit>   flag to enable it with a
>>>> particular level. This probably needs to be changed to -l apm,<digit>
>>>> or something like that to fit existing features. The only change that
>>>> I consider invasive is that every instance of words088_255 (e.g.
>>>> words088_255[100-88]) has been changed to e.g. words092_255[100-92].
>>>> Everything else is relatively self-contained and limited to adding a
>>>> few variables, cases for things like printing and argument parsing,
>>>> and functions.
>>>>
>>>> If there is any interest at all let me know and I'll post a patch.
>>>
>>> Yes, please.
>>
>> +1
>>
>>> It IMO makes sense to include some of the popular use cases of hdparm
>>> (e.g. APM, AAM, standby timeout) into smartctl, even if unrelated to
>>> SMART. This would allow usage of these settings also in conjunction with
>>> RAID controllers, non-SAT USB-bridges and other platforms.
>>
>> I am absolutely agreed. Another reason for this is that this will provide us
>> a universal tool on different platforms. I am administrating a lot of
>> different *nix-es and its always pain to remind if i need to use
>> hdparm/sdparm/camcontrol/cldevice/... and their keys (sometime bad
>> documented and non-intuitive).
>>

Marcus Sorensen wrote:
> Ok, here's my patch. It applies against the stable 5.42 from
> sourceforge (as that's what we want to roll out).

Thanks


> One thing I noticed while changing it over to -l was that scterc is
> using it's own function 'ataGetSetSCTErrorRecoveryControltime' to set
> SCT values, whereas I just added the necessary commands to allow me to
> use the existing 'smartcommandhandler'. I'm not sure if one method is
> necessarily preferred over the other, but I imagine someone will want
> to be consistent, and I'm not really excited to change what I've
> already got.

smartcommandhandler() is actually legacy code from the old interface 
which only supports ATA commands from smart_command_set. The enum is 
still used by the os_*.cpp parts which are not migrated to new 
interface. New code should directly call dev->ata_pass_through(.).

I will change this and commit your patch in a week or so.


>
> I give no guarantee that this won't break someone else's features in
> strange ways, I can only say that we've been running smart with this
> patch (well the -z flag version) for awhile and haven't run into
> anything unexpected.

Thanks,
Christian

Marcus Sorensen wrote:
> Ok, I'll see what I can do regarding the -l flag. It seems horribly
> overloaded with things not related to the log, but I can't think of
> anything better, and I don't think it makes sense for it to have its
> own flags.

Yes, the -l (--log) option has some uses not related to logs and e.g. 
'--log scterc' does not make much sense. But for the first prototype it 
is IMO OK to use -l apm.

For the long term I would suggest to add a new option --set NAME[,VALUE] 
(or: NAME[=VALUE]) for such settings. We could reuse '-s' as the 
corresponding short option provided that '-s on/off' (for '--smart 
on/off') is kept for backwards compatiblity.

Christian

cool, will do

On Wed, Feb 8, 2012 at 2:25 PM, Christian Franke
<Christian.Franke@...> wrote:
> Christian Franke wrote:
>>
>> Marcus Sorensen wrote:
>>>
>>> Ok, here's my patch. It applies against the stable 5.42 from
>>> sourceforge (as that's what we want to roll out).
>>
>>
>> Thanks
>>
>>
>>> One thing I noticed while changing it over to -l was that scterc is
>>> using it's own function 'ataGetSetSCTErrorRecoveryControltime' to set
>>> SCT values, whereas I just added the necessary commands to allow me to
>>> use the existing 'smartcommandhandler'. I'm not sure if one method is
>>> necessarily preferred over the other, but I imagine someone will want
>>> to be consistent, and I'm not really excited to change what I've
>>> already got.
>>
>>
>> smartcommandhandler() is actually legacy code from the old interface which
>> only supports ATA commands from smart_command_set. The enum is still used by
>> the os_*.cpp parts which are not migrated to new interface. New code should
>> directly call dev->ata_pass_through(.).
>>
>> I will change this and commit your patch in a week or so.
>>
>
> Done, r3503. Please test if possible.
>
> I did some changes and moved -l apm to new --get/set options. I will add
> some more settings soon.
>
> Thanks,
> Christian
>

This passes basic testing for me, does what I'd expect it to do.

On Wed, Feb 8, 2012 at 4:05 PM, Marcus Sorensen <shadowsor@...> wrote:
> cool, will do
>
> On Wed, Feb 8, 2012 at 2:25 PM, Christian Franke
> <Christian.Franke@...> wrote:
>> Christian Franke wrote:
>>>
>>> Marcus Sorensen wrote:
>>>>
>>>> Ok, here's my patch. It applies against the stable 5.42 from
>>>> sourceforge (as that's what we want to roll out).
>>>
>>>
>>> Thanks
>>>
>>>
>>>> One thing I noticed while changing it over to -l was that scterc is
>>>> using it's own function 'ataGetSetSCTErrorRecoveryControltime' to set
>>>> SCT values, whereas I just added the necessary commands to allow me to
>>>> use the existing 'smartcommandhandler'. I'm not sure if one method is
>>>> necessarily preferred over the other, but I imagine someone will want
>>>> to be consistent, and I'm not really excited to change what I've
>>>> already got.
>>>
>>>
>>> smartcommandhandler() is actually legacy code from the old interface which
>>> only supports ATA commands from smart_command_set. The enum is still used by
>>> the os_*.cpp parts which are not migrated to new interface. New code should
>>> directly call dev->ata_pass_through(.).
>>>
>>> I will change this and commit your patch in a week or so.
>>>
>>
>> Done, r3503. Please test if possible.
>>
>> I did some changes and moved -l apm to new --get/set options. I will add
>> some more settings soon.
>>
>> Thanks,
>> Christian
>>