From: Bart Van Assche <firstname.lastname@example.org> To: Can Guo <email@example.com> Cc: "Martin K . Petersen" <firstname.lastname@example.org>, "James E . J . Bottomley" <email@example.com>, firstname.lastname@example.org, Bean Huo <email@example.com>, Avri Altman <firstname.lastname@example.org>, Stanley Chu <email@example.com>, Tomas Winkler <firstname.lastname@example.org> Subject: Re: [PATCH 6/6] ufs: Remove the SCSI timeout handler Date: Thu, 28 May 2020 09:12:05 -0700 [thread overview] Message-ID: <email@example.com> (raw) In-Reply-To: <firstname.lastname@example.org> 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 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.
next prev parent reply other threads:[~2020-05-28 16:12 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 [this message] 2020-05-29 1:39 ` Can Guo 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 \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --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