lkml.org 
[lkml]   [2008]   [Sep]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/4] libata: Implement disk shock protection support
    Elias Oltmanns wrote:
    >> The correct way to do this is ata_eh_about_to_do(). After that, you
    >> can just look at ehc->i.dev_action[]. Also, you'll need to call
    >> ata_eh_done() later.
    >
    > We have a problem here, I'm afraid, because we may keep looping in EH
    > context and still want to pick up ATA_EH_PARK requests. Imagine that
    > ATA_EH_PARK has been scheduled for device A and the EH thread has
    > reached the call to schedule_timeout_uninterruptible(). Now, ATA_EH_PARK
    > is scheduled for device B on the same port. This will wake up the EH
    > thread, but ATA_EH_PARK is only recorded in link->eh_info, not in
    > link->eh_context.i. ata_eh_about_to_do() will unconditionally clear the
    > flag in eh_info, but checking ehc->i.dev_action afterwards will only
    > tell us whether this flag was set when we entered EH, not whether it had
    > been set since.
    >
    > Should I change ata_eh_about_to_do() so that it will record the action
    > in link->eh_context before clearing it in link->eh_info?

    That's what ata_eh_about_to_do() currently does, exactly. Actually,
    that's the whole reason it's there as the described problem exists for
    all other actions too. :-)

    >> And it's probably better to have ehc->unloaded_mask instead of
    >> ehc->did_unload_mask and clear it here so that if unload is scheduled
    >> after this point but before EH completes, it does unloading again.
    >> ie. Something like the following.
    >>
    >> ata_eh_done(ATA_EH_UNLOAD);
    >> ehc->i.unloaded_mask &= ~(1 << dev->devno);
    >
    > No need for that because link->eh_context is cleared in
    > ata_scsi_error().

    No, for example, later steps of EH could fail in which case eh_recover
    will be retried without going out to ata_scsi_error().

    >> Can't we just drop ATA_DFLAG_NO_UNLOAD? It doesn't provide any real
    >> functionality anymore.
    >
    > I was afraid you'd say something like that in the end ;-). Well, we
    > can't. We really should only issue the unload command if we know that
    > it's safe, i.e., the device supports that feature. We assume it to be
    > safe if ata_id_has_unload() returns true or if the user told us that the
    > device does support the command. ATA_DFLAG_NO_UNLOAD is initialised
    > during device setup by ata_id_has_unload(). For pre-ATA-7 devices (like
    > mine), the user can manually clear that flag afterwards.

    Oh I see, so it's initialized during dev_configure (I missed that) and
    the user needs to be able to override it. Alright, no objection then.

    Thanks.

    --
    tejun


    \
     
     \ /
      Last update: 2008-09-10 22:27    [W:4.129 / U:0.064 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site