All of lore.kernel.org
 help / color / mirror / Atom feed
From: "jianchao.wang" <jianchao.w.wang@oracle.com>
To: Ming Lei <ming.lei@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>,
	Keith Busch <keith.busch@intel.com>,
	stable@vger.kernel.org, Sagi Grimberg <sagi@grimberg.me>,
	linux-nvme@lists.infradead.org, Xiao Liang <xiliang@redhat.com>
Subject: Re: [PATCH] nvme: don't retry request marked as NVME_REQ_CANCELLED
Date: Sat, 27 Jan 2018 22:29:30 +0800	[thread overview]
Message-ID: <e9456baf-a112-b340-378a-71eeab28f97b@oracle.com> (raw)
In-Reply-To: <20180127133127.GA19560@ming.t460p>

Hi ming 

Thanks for your detailed response.
That's really appreciated.

On 01/27/2018 09:31 PM, Ming Lei wrote:
>> But nvme_dev_disable may run with nvme_timeout in parallel or race with it.
> But that doesn't mean it is a race, blk_mq_complete_request() can avoid race
> between timeout and other completions, such as cancel.
> Yes, I know blk_mq_complete_request could avoid the a request is accessed by timeout
path and other completion path concurrently. :)
What's I worry about is the timeout path could hold the expired request, so when
nvme_dev_disable return, we cannot ensure all the previous outstanding requests has been
handled. That's really bad. 

>> The best way to fix this is to ensure the timeout path has been completed before cancel the
>> previously outstanding requests. (Just ignore the case where the nvme_timeout will invoke nvme_dev_disable,
>> it has to be fixed by other way.)
> Maybe your approach looks a bit clean and simplify the implementation, but seems
> it isn't necessary.
> 
> So could you explain a bit what the exact issue you are worrying about? deadlock?
> or others?
There is indeed potential issue. But it is in very narrow window.
Please refer to https://lkml.org/lkml/2018/1/19/68

As you said, the approach looks a bit clean and simplify the implementation.
That's what I really want, break the complicated relationship between 
nvme_timeout and nvme_dev_diable. 

Thanks
Jianchao

WARNING: multiple messages have this Message-ID (diff)
From: jianchao.w.wang@oracle.com (jianchao.wang)
Subject: [PATCH] nvme: don't retry request marked as NVME_REQ_CANCELLED
Date: Sat, 27 Jan 2018 22:29:30 +0800	[thread overview]
Message-ID: <e9456baf-a112-b340-378a-71eeab28f97b@oracle.com> (raw)
In-Reply-To: <20180127133127.GA19560@ming.t460p>

Hi ming 

Thanks for your detailed response.
That's really appreciated.

On 01/27/2018 09:31 PM, Ming Lei wrote:
>> But nvme_dev_disable may run with nvme_timeout in parallel or race with it.
> But that doesn't mean it is a race, blk_mq_complete_request() can avoid race
> between timeout and other completions, such as cancel.
> Yes, I know blk_mq_complete_request could avoid the a request is accessed by timeout
path and other completion path concurrently. :)
What's I worry about is the timeout path could hold the expired request, so when
nvme_dev_disable return, we cannot ensure all the previous outstanding requests has been
handled. That's really bad. 

>> The best way to fix this is to ensure the timeout path has been completed before cancel the
>> previously outstanding requests. (Just ignore the case where the nvme_timeout will invoke nvme_dev_disable,
>> it has to be fixed by other way.)
> Maybe your approach looks a bit clean and simplify the implementation, but seems
> it isn't necessary.
> 
> So could you explain a bit what the exact issue you are worrying about? deadlock?
> or others?
There is indeed potential issue. But it is in very narrow window.
Please refer to https://lkml.org/lkml/2018/1/19/68

As you said, the approach looks a bit clean and simplify the implementation.
That's what I really want, break the complicated relationship between 
nvme_timeout and nvme_dev_diable. 

Thanks
Jianchao

  reply	other threads:[~2018-01-27 14:30 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-25  8:10 [PATCH] nvme: don't retry request marked as NVME_REQ_CANCELLED Ming Lei
2018-01-25  8:10 ` Ming Lei
2018-01-25  8:52 ` jianchao.wang
2018-01-25  8:52   ` jianchao.wang
2018-01-25 10:15   ` Ming Lei
2018-01-25 10:15     ` Ming Lei
2018-01-27 12:33     ` jianchao.wang
2018-01-27 12:33       ` jianchao.wang
2018-01-27 13:31       ` Ming Lei
2018-01-27 13:31         ` Ming Lei
2018-01-27 14:29         ` jianchao.wang [this message]
2018-01-27 14:29           ` jianchao.wang
2018-01-27 15:44           ` Ming Lei
2018-01-27 15:44             ` Ming Lei
2018-01-28  9:01             ` jianchao.wang
2018-01-28  9:01               ` jianchao.wang

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=e9456baf-a112-b340-378a-71eeab28f97b@oracle.com \
    --to=jianchao.w.wang@oracle.com \
    --cc=hch@lst.de \
    --cc=keith.busch@intel.com \
    --cc=linux-nvme@lists.infradead.org \
    --cc=ming.lei@redhat.com \
    --cc=sagi@grimberg.me \
    --cc=stable@vger.kernel.org \
    --cc=xiliang@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.