linux-scsi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Can Guo <cang@codeaurora.org>
To: Bart Van Assche <bvanassche@acm.org>
Cc: "Martin K . Petersen" <martin.petersen@oracle.com>,
	"James E . J . Bottomley" <jejb@linux.vnet.ibm.com>,
	linux-scsi@vger.kernel.org, Bean Huo <beanhuo@micron.com>,
	Avri Altman <avri.altman@wdc.com>,
	Stanley Chu <stanley.chu@mediatek.com>,
	Tomas Winkler <tomas.winkler@intel.com>
Subject: Re: [PATCH 6/6] ufs: Remove the SCSI timeout handler
Date: Fri, 29 May 2020 09:39:12 +0800	[thread overview]
Message-ID: <8bd12e9fe32b0f996209ac2d4e8aa484@codeaurora.org> (raw)
In-Reply-To: <1728e2d6-5b00-e71f-5476-b082f4201aa1@acm.org>

On 2020-05-29 00:12, Bart Van Assche wrote:
> On 2020-05-28 02:47, Can Guo wrote:
>> Hi Bart,
>> 
>> On 2019-12-25 06:02, Bart Van Assche wrote:
>>> The UFS SCSI timeout handler was needed to compensate that
>>> ufshcd_queuecommand() could return SCSI_MLQUEUE_HOST_BUSY for a long
>>> time. Commit a276c19e3e98 ("scsi: ufs: Avoid busy-waiting by 
>>> eliminating
>>> tag conflicts") fixed this so the timeout handler is no longer 
>>> necessary.
>>> 
>>> See also commit f550c65b543b ("scsi: ufs: implement scsi host timeout
>>> handler").
>>> 
>> 
>> Sorry for bugging you on this old change. I am afraid we may need to 
>> add
>> this timeout handler back. Because there is till chances that a 
>> request
>> gets stuck somewhere in ufshcd_queuecommand() path before
>> ufshcd_send_command() gets called. e.g.
>> 
>> ufshcd_queuecommand()
>> ->ufshcd_map_sg()
>> -->scsi_dma_map()
>> --->dma_map_sg()
>> ---->dev->ops->map_sg()
>> 
>> map_sg() ops may get stuck. map_sg() method can vary on different 
>> platforms
>> based on actual IOMMU engines. We cannot gaurantee map_sg() ops must 
>> return
>> immediately as we don't know what is actually inside map_sg() ops.
>> 
>> And if it gets stuck there for a long time till the request times out,
>> without
>> the UFS timeout handler, scsi layer will try to abort this request 
>> from UFS
>> driver by calling ufshcd_abort() eventually. ufshcd_abort() will think 
>> this
>> request has been completed due to its tag is not in 
>> hba->outstanding_reqs
>> or UFS host's door bell reg. However, actually, this request is still 
>> in
>> ufshcd_queuecommand() path. I don't need to continue on the subsequent
>> impact
>> to UFS driver if ufshcd_abort() happens in this case. This is a corner
>> case,
>> but it is still possible (I did see map_sg() ops hangs on real 
>> devices).
>> 
>> Having the UFS timeout handler back will prevent this situation as UFS
>> timeout
>> handler checks if the tag is in hba->outstanding_reqs (for our case, 
>> it
>> is not
>> in there), if no, it returns BLK_EH_RESET_TIMER so that scsi/block 
>> layer
>> will
>> keep waiting.
>> 
>> What do you think? Please let me know your ideas on this, thanks!

Hi Bart,

> 
> Hi Can,
> 
> I see the following issues with the above proposal:
> - Although I haven't been able to find explicit documentation of this, 
> I
>   think that dma_map_sg() must not sleep. If it would sleep that would
>   break most block and SCSI drivers because many of these drivers call
>   dma_map_sg() from their .queue_rq() or .queuecommand() implementation
>   and if BLK_MQ_F_BLOCKING has not been set these functions must not
>   sleep.
> - A timeout handler must not be invoked while .queuecommand() is still
>   in progress. The SCSI core calls blk_mq_start_request() before it
>   calls ufshcd_queuecommand(). The blk_mq_start_request() activates the
>   block layer timeout mechanism. ufshcd_queuecommand() must have
>   finished before the block layer timeout handler is activated.
> 
> Please fix the root cause, namely the map_sg implementation that may 
> get
> stuck.
> 
> Thanks,
> 
> Bart.

queuecommand path should not sleep - that is right, due to queuecommand
can be invoked from contexts where preempt is disabled, e.g. softirq.

I don't know why map_sg() ops can take that long, but apparently it does
not sleep, otherwise we should have seen schedule while atomic error 
long
time ago.

> ufshcd_queuecommand() must have
> finished before the block layer timeout handler is activated.
This is the ideal/expected situation, but we are seeing the corner case.

Fixing the root cause of that is one thing, but having the timeout 
handler
back can prevent UFS driver from messing up the subsequent requests 
further
in such case, causing possible data corruption. Is there any drawbacks 
if
we have it back?

Thanks,
Can Guo.

  reply	other threads:[~2020-05-29  1:39 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-24 22:02 [PATCH 0/6] Six UFS patches Bart Van Assche
2019-12-24 22:02 ` [PATCH 1/6] ufs: Fix indentation in ufshcd_query_attr_retry() Bart Van Assche
2019-12-25  7:16   ` Stanley Chu
2019-12-25  8:17   ` Can Guo
2019-12-27  1:13   ` Alim Akhtar
2019-12-24 22:02 ` [PATCH 2/6] ufs: Make ufshcd_add_command_trace() easier to read Bart Van Assche
2019-12-25  7:16   ` Stanley Chu
2019-12-25  8:17   ` Can Guo
2019-12-27  1:17   ` Alim Akhtar
2019-12-24 22:02 ` [PATCH 3/6] ufs: Make ufshcd_prepare_utp_scsi_cmd_upiu() " Bart Van Assche
2019-12-25  7:17   ` Stanley Chu
2019-12-25  8:18   ` Can Guo
2019-12-24 22:02 ` [PATCH 4/6] ufs: Fix a race condition in the tracing code Bart Van Assche
2019-12-25 10:59   ` Can Guo
2019-12-26 17:35     ` Bart Van Assche
2019-12-27  5:50       ` Can Guo
2019-12-27  1:21   ` Alim Akhtar
2019-12-24 22:02 ` [PATCH 5/6] ufs: Remove superfluous memory barriers Bart Van Assche
2019-12-25 10:31   ` Can Guo
2019-12-26 17:36     ` Bart Van Assche
2019-12-24 22:02 ` [PATCH 6/6] ufs: Remove the SCSI timeout handler Bart Van Assche
2019-12-25 11:02   ` Can Guo
2019-12-27  1:24   ` Alim Akhtar
2020-05-28  9:47   ` Can Guo
2020-05-28 16:12     ` Bart Van Assche
2020-05-29  1:39       ` Can Guo [this message]
2020-05-29  3:56         ` Bart Van Assche
2020-01-03  2:58 ` [PATCH 0/6] Six UFS patches Martin K. Petersen

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=8bd12e9fe32b0f996209ac2d4e8aa484@codeaurora.org \
    --to=cang@codeaurora.org \
    --cc=avri.altman@wdc.com \
    --cc=beanhuo@micron.com \
    --cc=bvanassche@acm.org \
    --cc=jejb@linux.vnet.ibm.com \
    --cc=linux-scsi@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=stanley.chu@mediatek.com \
    --cc=tomas.winkler@intel.com \
    --subject='Re: [PATCH 6/6] ufs: Remove the SCSI timeout handler' \
    /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

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox