lkml.org 
[lkml]   [2010]   [Nov]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH][resend][SCSI] Reduce number of sequential pointer derefs in scsi_error.c and reduce size as well
    On Thu, 18 Nov 2010, Jesper Juhl wrote:

    > On Thu, 18 Nov 2010, Linus Torvalds wrote:
    >
    > > On Thu, Nov 18, 2010 at 11:30 AM, Jesper Juhl <jj@chaosbits.net> wrote:
    > > >
    > > > This is the fourth time I send this patch. For some reason it seems unable
    > > > to get any feedback at all. I'd really appreciate a clear ACK or NACK on
    > > > it and I'll keep resending it until it's either merged or I get a NACK
    > > > with a reason.
    > >
    > > The patch looks ok to me, but you've basically selected the least
    > > interesting file possible. No wonder people can't seem to care.
    > >
    >
    > Thank you very much for reviewing the patch and replying with your
    > feedback!
    >
    > I agree that this is not the most sexy file in the tree to be modifying,
    > but I'm not seeking fame and fortune by doing this, just trying to
    > optimize the few corners I find - every little bit helps; right?
    >
    >
    > > Also, this is just ugly as hell, and doesn't help anything:
    > >
    > > + int (*eh_abort_handler)(struct scsi_cmnd *) =
    > > + scmd->device->host->hostt->eh_abort_handler;
    > >
    > > since the compiler will have optimized that double access away anyway
    > > (no writes in between). So you could have made it about a thousand
    > > times more readable with no downside by doing
    > >
    > > struct scsi_host_template *hostt = scmd->device->host->hostt;
    > >
    > > if (!hostt->eh_abort_handler)
    > > return FAILED;
    > > return hostt->eh_abort_handler(scmd);
    > >
    > > instead. Look ma, no long lines.
    > >
    > > Rule of thumb: if you need more than one line for an expression or
    > > variable definition, you're doing something wrong.
    > >
    >
    > Fair enough. Seeing your version of this and looking a second time at mine
    > I have to agree completely.
    > You are absolutely right in stating that the compiler will handle this
    > just fine, so it's only a readabillity issue and your version is a *lot*
    > more readable than what I came up with.
    >
    > I've updated the patch below - the original changelog bit still applies,
    > only change is what you pointed out above.
    >
    Aww crap, emacs just screwed me over on this one - that'll teach me to
    keep my .emacs files in sync across machines. Sorry about that, the
    previous mail used spaces for indentation not tabs - this should be
    better:

    Signed-off-by: Jesper Juhl <jj@chaosbits.net>
    ---
    scsi_error.c | 85 +++++++++++++++++++++++++++++++----------------------------
    1 file changed, 45 insertions(+), 40 deletions(-)

    diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
    index 824b8fc..ef89f3f 100644
    --- a/drivers/scsi/scsi_error.c
    +++ b/drivers/scsi/scsi_error.c
    @@ -3,14 +3,14 @@
    *
    * SCSI error/timeout handling
    * Initial versions: Eric Youngdale. Based upon conversations with
    - * Leonard Zubkoff and David Miller at Linux Expo,
    + * Leonard Zubkoff and David Miller at Linux Expo,
    * ideas originating from all over the place.
    *
    * Restructured scsi_unjam_host and associated functions.
    * September 04, 2002 Mike Anderson (andmike@us.ibm.com)
    *
    * Forward port of Russell King's (rmk@arm.linux.org.uk) changes and
    - * minor cleanups.
    + * minor cleanups.
    * September 30, 2002 Mike Anderson (andmike@us.ibm.com)
    */

    @@ -129,14 +129,15 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
    {
    struct scsi_cmnd *scmd = req->special;
    enum blk_eh_timer_return rtn = BLK_EH_NOT_HANDLED;
    + struct Scsi_Host *host = scmd->device->host;

    trace_scsi_dispatch_cmd_timeout(scmd);
    scsi_log_completion(scmd, TIMEOUT_ERROR);

    - if (scmd->device->host->transportt->eh_timed_out)
    - rtn = scmd->device->host->transportt->eh_timed_out(scmd);
    - else if (scmd->device->host->hostt->eh_timed_out)
    - rtn = scmd->device->host->hostt->eh_timed_out(scmd);
    + if (host->transportt->eh_timed_out)
    + rtn = host->transportt->eh_timed_out(scmd);
    + else if (host->hostt->eh_timed_out)
    + rtn = host->hostt->eh_timed_out(scmd);

    if (unlikely(rtn == BLK_EH_NOT_HANDLED &&
    !scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD))) {
    @@ -195,7 +196,7 @@ static inline void scsi_eh_prt_fail_stats(struct Scsi_Host *shost,
    ++total_failures;
    if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD)
    ++cmd_cancel;
    - else
    + else
    ++cmd_failed;
    }
    }
    @@ -294,7 +295,7 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
    return NEEDS_RETRY;
    }
    /*
    - * if the device is in the process of becoming ready, we
    + * if the device is in the process of becoming ready, we
    * should retry.
    */
    if ((sshdr.asc == 0x04) && (sshdr.ascq == 0x01))
    @@ -507,22 +508,23 @@ static int scsi_try_host_reset(struct scsi_cmnd *scmd)
    {
    unsigned long flags;
    int rtn;
    + struct Scsi_Host *host = scmd->device->host;
    + struct scsi_host_template *hostt = host->hostt;

    SCSI_LOG_ERROR_RECOVERY(3, printk("%s: Snd Host RST\n",
    __func__));

    - if (!scmd->device->host->hostt->eh_host_reset_handler)
    + if (!hostt->eh_host_reset_handler)
    return FAILED;

    - rtn = scmd->device->host->hostt->eh_host_reset_handler(scmd);
    + rtn = hostt->eh_host_reset_handler(scmd);

    if (rtn == SUCCESS) {
    - if (!scmd->device->host->hostt->skip_settle_delay)
    + if (!hostt->skip_settle_delay)
    ssleep(HOST_RESET_SETTLE_TIME);
    - spin_lock_irqsave(scmd->device->host->host_lock, flags);
    - scsi_report_bus_reset(scmd->device->host,
    - scmd_channel(scmd));
    - spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
    + spin_lock_irqsave(host->host_lock, flags);
    + scsi_report_bus_reset(host, scmd_channel(scmd));
    + spin_unlock_irqrestore(host->host_lock, flags);
    }

    return rtn;
    @@ -536,22 +538,23 @@ static int scsi_try_bus_reset(struct scsi_cmnd *scmd)
    {
    unsigned long flags;
    int rtn;
    + struct Scsi_Host *host = scmd->device->host;
    + struct scsi_host_template *hostt = host->hostt;

    SCSI_LOG_ERROR_RECOVERY(3, printk("%s: Snd Bus RST\n",
    __func__));

    - if (!scmd->device->host->hostt->eh_bus_reset_handler)
    + if (!hostt->eh_bus_reset_handler)
    return FAILED;

    - rtn = scmd->device->host->hostt->eh_bus_reset_handler(scmd);
    + rtn = hostt->eh_bus_reset_handler(scmd);

    if (rtn == SUCCESS) {
    - if (!scmd->device->host->hostt->skip_settle_delay)
    + if (!hostt->skip_settle_delay)
    ssleep(BUS_RESET_SETTLE_TIME);
    - spin_lock_irqsave(scmd->device->host->host_lock, flags);
    - scsi_report_bus_reset(scmd->device->host,
    - scmd_channel(scmd));
    - spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
    + spin_lock_irqsave(host->host_lock, flags);
    + scsi_report_bus_reset(host, scmd_channel(scmd));
    + spin_unlock_irqrestore(host->host_lock, flags);
    }

    return rtn;
    @@ -577,16 +580,18 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
    {
    unsigned long flags;
    int rtn;
    + struct Scsi_Host *host = scmd->device->host;
    + struct scsi_host_template *hostt = host->hostt;

    - if (!scmd->device->host->hostt->eh_target_reset_handler)
    + if (!hostt->eh_target_reset_handler)
    return FAILED;

    - rtn = scmd->device->host->hostt->eh_target_reset_handler(scmd);
    + rtn = hostt->eh_target_reset_handler(scmd);
    if (rtn == SUCCESS) {
    - spin_lock_irqsave(scmd->device->host->host_lock, flags);
    + spin_lock_irqsave(host->host_lock, flags);
    __starget_for_each_device(scsi_target(scmd->device), NULL,
    __scsi_report_device_reset);
    - spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
    + spin_unlock_irqrestore(host->host_lock, flags);
    }

    return rtn;
    @@ -605,11 +610,12 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
    static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
    {
    int rtn;
    + struct scsi_host_template *hostt = scmd->device->host->hostt;

    - if (!scmd->device->host->hostt->eh_device_reset_handler)
    + if (!hostt->eh_device_reset_handler)
    return FAILED;

    - rtn = scmd->device->host->hostt->eh_device_reset_handler(scmd);
    + rtn = hostt->eh_device_reset_handler(scmd);
    if (rtn == SUCCESS)
    __scsi_report_device_reset(scmd->device, NULL);
    return rtn;
    @@ -617,10 +623,10 @@ static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)

    static int __scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
    {
    - if (!scmd->device->host->hostt->eh_abort_handler)
    + struct scsi_host_template *hostt = scmd->device->host->hostt;
    + if (!hostt->eh_abort_handler)
    return FAILED;
    -
    - return scmd->device->host->hostt->eh_abort_handler(scmd);
    + return hostt->eh_abort_handler(scmd);
    }

    /**
    @@ -867,7 +873,7 @@ EXPORT_SYMBOL(scsi_eh_finish_cmd);
    *
    * Description:
    * See if we need to request sense information. if so, then get it
    - * now, so we have a better idea of what to do.
    + * now, so we have a better idea of what to do.
    *
    * Notes:
    * This has the unfortunate side effect that if a shost adapter does
    @@ -987,7 +993,6 @@ static int scsi_eh_abort_cmds(struct list_head *work_q,
    !scsi_eh_tur(scmd)) {
    scsi_eh_finish_cmd(scmd, done_q);
    }
    -
    } else
    SCSI_LOG_ERROR_RECOVERY(3, printk("%s: aborting"
    " cmd failed:"
    @@ -1031,7 +1036,7 @@ static int scsi_eh_try_stu(struct scsi_cmnd *scmd)
    *
    * Notes:
    * If commands are failing due to not ready, initializing command required,
    - * try revalidating the device, which will end up sending a start unit.
    + * try revalidating the device, which will end up sending a start unit.
    */
    static int scsi_eh_stu(struct Scsi_Host *shost,
    struct list_head *work_q,
    @@ -1085,7 +1090,7 @@ static int scsi_eh_stu(struct Scsi_Host *shost,
    * Try a bus device reset. Still, look to see whether we have multiple
    * devices that are jammed or not - if we have multiple devices, it
    * makes no sense to try bus_device_reset - we really would need to try
    - * a bus_reset instead.
    + * a bus_reset instead.
    */
    static int scsi_eh_bus_device_reset(struct Scsi_Host *shost,
    struct list_head *work_q,
    @@ -1196,7 +1201,7 @@ static int scsi_eh_target_reset(struct Scsi_Host *shost,
    }

    /**
    - * scsi_eh_bus_reset - send a bus reset
    + * scsi_eh_bus_reset - send a bus reset
    * @shost: &scsi host being recovered.
    * @work_q: &list_head for pending commands.
    * @done_q: &list_head for processed commands.
    @@ -1213,7 +1218,7 @@ static int scsi_eh_bus_reset(struct Scsi_Host *shost,
    * we really want to loop over the various channels, and do this on
    * a channel by channel basis. we should also check to see if any
    * of the failed commands are on soft_reset devices, and if so, skip
    - * the reset.
    + * the reset.
    */

    for (channel = 0; channel <= shost->max_channel; channel++) {
    @@ -1255,7 +1260,7 @@ static int scsi_eh_bus_reset(struct Scsi_Host *shost,
    }

    /**
    - * scsi_eh_host_reset - send a host reset
    + * scsi_eh_host_reset - send a host reset
    * @work_q: list_head for processed commands.
    * @done_q: list_head for processed commands.
    */
    @@ -1408,7 +1413,7 @@ int scsi_decide_disposition(struct scsi_cmnd *scmd)
    return SUCCESS;
    /*
    * when the low level driver returns did_soft_error,
    - * it is responsible for keeping an internal retry counter
    + * it is responsible for keeping an internal retry counter
    * in order to avoid endless loops (db)
    *
    * actually this is a bug in this function here. we should
    @@ -2005,7 +2010,7 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
    if (sb_len > 7)
    sshdr->additional_length = sense_buffer[7];
    } else {
    - /*
    + /*
    * fixed format
    */
    if (sb_len > 2)


    --
    Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
    Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
    Plain text mails only, please.



    \
     
     \ /
      Last update: 2010-11-18 22:13    [W:0.061 / U:1.016 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site