All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sumit Saxena <sumit.saxena@broadcom.com>
To: "Martin K. Petersen" <martin.petersen@oracle.com>,
	Tomas Henzl <thenzl@redhat.com>
Cc: linux-scsi@vger.kernel.org, Kashyap Desai <kashyap.desai@broadcom.com>
Subject: RE: [PATCH] megaraid_sas: move command counter to correct place
Date: Tue, 8 Aug 2017 13:07:02 +0530	[thread overview]
Message-ID: <730abe70a796241670b9443bd72e395b@mail.gmail.com> (raw)
In-Reply-To: <yq1lgmv9t51.fsf@oracle.com>

>-----Original Message-----
>From: Martin K. Petersen [mailto:martin.petersen@oracle.com]
>Sent: Monday, August 07, 2017 11:07 PM
>To: Tomas Henzl
>Cc: linux-scsi@vger.kernel.org; sumit.saxena@broadcom.com;
>kashyap.desai@broadcom.com
>Subject: Re: [PATCH] megaraid_sas: move command counter to correct place
>
>
>Tomas,
>
>> the eh reset function returns success when fw_outstanding equals zero,
>> that means that the counter shouldn't be decremented when the driver
>> still owns the command
>
>Applied to 4.13/scsi-fixes. Thank you!

Just realized that this patch may cause performance regression.
With this patch below scenario may occur-

-Consider outstanding IOs reaches to controller's Queue depth.
-Driver frees up command and complete it back to SML.
-Since host_busy is decremented, SML can issue one new  IO to driver.
-By the time, if "fw_outstanding" is not decremented by driver, then
driver will return newly submitted IO back to SML with return status
SCSI_MLQUEUE_HOST_BUSY because of below code
  in megaraid_sas driver's IO submission path-

    if (atomic_inc_return(&instance->fw_outstanding) >
            instance->host->can_queue) {
        atomic_dec(&instance->fw_outstanding);
        return SCSI_MLQUEUE_HOST_BUSY;
    }

This situation will be more evident when RAID1 fastpath IOs are running as
in that case driver will be issuing two IOs to firmware for single IO
issued from SML.
Above situation can be avoided, if this patch is removed.

Regarding Tomas' concern, there should not be any problem as driver calls
"synchronize_irq" before checking "fw_outstanding". Once fw_outstanding is
decremented and
driver frees up command, then only driver will be checking
"fw_outstanding" equals to zero or not so all this will always fall in a
sequence and will not
cause the problem stated by Tomas.

I am sorry for confusion and would request to revert this patch.

Thanks,
Sumit

>
>--
>Martin K. Petersen	Oracle Linux Engineering

  reply	other threads:[~2017-08-08  7:37 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-07-28 14:03 [PATCH] megaraid_sas: move command counter to correct place Tomas Henzl
2017-08-07 17:20 ` Martin K. Petersen
2017-08-07 17:31 ` Sumit Saxena
2017-08-07 17:36 ` Martin K. Petersen
2017-08-08  7:37   ` Sumit Saxena [this message]
2017-08-08 15:39     ` James Bottomley
2017-08-08 15:50       ` Martin K. Petersen
2017-08-09 14:09     ` Tomas Henzl

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=730abe70a796241670b9443bd72e395b@mail.gmail.com \
    --to=sumit.saxena@broadcom.com \
    --cc=kashyap.desai@broadcom.com \
    --cc=linux-scsi@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=thenzl@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.