[lkml]   [2008]   [Sep]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 2/4] libata: Implement disk shock protection support
Elias Oltmanns wrote:
> Tejun Heo <> wrote:
>> extern struct device_attribute **libata_sdev_attrs;
>> #define ATA_BASE_SHT(name) \
>> ....
>> .sdev_attrs = libata_sdev_attrs; \
>> ....
>> Which will give unload_heads to all libata drivers. As ahci needs its
>> own node it would need to define its own sdev_attrs tho.
> Dear me, I totally forgot about that, didn't I. Anyway, I meant to ask
> you about that when you mentioned it the last time round, so thanks for
> explaining in more detail. I'll do it this way then.


>> Isn't seconds a bit too crude? Or it just doesn't matter as it's
>> usually adjusted before expiring? For most time interval values
>> (except for transfer timings of course) in ATA land, millisecs seem to
>> be good enough and I've been trying to unify things that direction.
> Well, I can see your point. Technically, we are talking about magnitudes
> in the order of seconds rather than milliseconds here because the specs
> only guarantee command completion for head unload in 300 or even 500
> msecs. This means that the daemon should always schedule timeouts well
> above this limit. That's the reason why we have only accepted timeouts
> in seconds rather than milliseconds at the user's request. When reading
> from sysfs, we have returned seconds for consistency. I'm a bit torn
> between the options now:
> 1. Switch the interface completely to msecs: consistent with the rest of
> libata but slightly misleading because it may promise more accuracy
> than we can actually provide for;
> 2. keep it the way it was (i.e. seconds on read and write): we don't
> promise too much as far as accuracy is concerned, but it is
> inconsistent with the rest of libata. Besides, user space can still
> issue a 0 and another nonzero timeout within a very short time and we
> don't protect against that anyway;
> 3. only switch to msecs on read: probably the worst of all options.
> What do you think?

My favorite is #1. Millisecond is small amount of time but it's also
not hard to imagine some future cases where, say, 0.5 sec of
granuality makes some difference.

>> Hmmm... Sorry to bring another issue with it but I think the interface
>> is a bit convoluted. The unpark node is per-dev but the action is
>> per-port but devices can opt out by writing -2. Also, although the
>> sysfs nodes are per-dev, writing to a node changes the value of park
>> node in the device sharing the port except when the value is -1 or -2.
>> That's strange, right?
> Well, it is strange, but it pretty much reflects reality as close as it
> can get. Devices can only opt in / out of actually issuing the unload
> command but they will always stop I/O and thus be affected by the
> timeout (intentionally).
>> How about something like the following?
>> * In park_store: set dev->unpark_timeout, kick and wake up EH.
>> * In park EH action: until the latest of all unpark_timeout are
>> passed, park all drives whose unpark_timeout is in future. When
>> none of the drives needs to be parked (all timers expired), the
>> action completes.
>> * There probably needs to be a flag to indicate that the timeout is
>> valid; otherwise, we could get spurious head unparking after jiffies
>> wraps (or maybe just use jiffies_64?).
>> With something like the above, the interface is cleanly per-dev and we
>> wouldn't need -1/-2 special cases. The implementation is still
>> per-port but we can change that later without modifying userland
>> interface.
> First of all, we cannot do a proper per-dev implementation internally.

Not yet but I think we should move toward per-queue EH which will
enable fine-grained exception handling like this. Such approach would
also help things like ATAPI CHECK_SENSE behind PMP. I think it's
better to define the interface which suits the problem best rather
than reflects the current implementation.

> Admittedly, we could do it per-link rather than per-port, but the point
> I'm making is this: there really is just *one* grobal timeout (per-port
> now or perhaps per-link in the long run). The confusing thing right now
> is that you can read the current timeout on any device, but you can only
> set a timeout on a device that actually supports head unloading. Perhaps
> we should return something like "n/a" when reading the sysfs attribute
> for a device that doesn't support head unloads, even though a timer on
> that port may be running because the other device has just received an
> unload request. This way, both devices will be affected by the timeout,
> but you can only read it on the device where you can change it as well.
> Would that suit you?

If the timeout is global, it's best to have one knob. If the timeout
is per-port, it's best to have one knob per-port, and so on. I can't
think of a good reason to implement per-port timeout with per-device
opt out instead of doing per-device timeout from the beginning. It
just doesn't make much sense interface-wise to me. As this is an
interface which is gonna stick around for a long time, I really think
it should be done as straight forward as possible even though the
current implementation of the feature has to do it in more crude



 \ /
  Last update: 2008-09-05 10:55    [W:0.067 / U:10.388 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site