Re: [PATCH 2/6] nvme-pci: fix the freeze and quiesce for shutdown and reset case
On Wed, Feb 07, 2018 at 10:13:51AM +0800, jianchao.wang wrote: > What's the difference ? Can you please point out. > I have shared my understanding below. > But actually, I don't get the point what's the difference you said. It sounds like you have all the pieces. Just keep this in mind: we don't want to fail IO if we can prevent it. A request is allocated from an hctx pool of tags. Once the request is allocated, it is permently tied to that hctx because that's where its tag came from. If that hctx becomes invalid, the request has to be ended with an error, and we can't do anything about that[*]. Prior to a reset, we currently halt new requests from being allocated by freezing the request queues. We unfreeze the queues after the new state of the hctx's is established. This way all IO requests that were gating on the unfreeze are guaranteed to enter into a valid context. You are proposing to skip freeze on a reset. New requests will then be allocated before we've established the hctx map. Any request allocated will have to be terminated in failure if the hctx is no longer valid once the reset completes. Yes, it's entirely possible today a request allocated prior to the reset may need to be terminated after the reset. There's nothing we can do about those except end them in failure, but we can prevent new ones from sharing the same fate. You are removing that prevention, and that's what I am complaining about. * Future consideration: we recently obtained a way to "steal" bios that looks like it may be used to back out certain types of requests and let the bio create a new one.
Re: [PATCH 2/6] nvme-pci: fix the freeze and quiesce for shutdown and reset case
Hi Keith Really thanks for your your precious time and kindly directive. That's really appreciated. :) On 02/08/2018 12:13 AM, Keith Busch wrote: > On Wed, Feb 07, 2018 at 10:13:51AM +0800, jianchao.wang wrote: >> What's the difference ? Can you please point out. >> I have shared my understanding below. >> But actually, I don't get the point what's the difference you said. > > It sounds like you have all the pieces. Just keep this in mind: we don't > want to fail IO if we can prevent it. > Yes, absolutely. > A request is allocated from an hctx pool of tags. Once the request is > allocated, it is permently tied to that hctx because that's where its > tag came from. If that hctx becomes invalid, the request has to be ended > with an error, and we can't do anything about that[*]. > > Prior to a reset, we currently halt new requests from being allocated by > freezing the request queues. We unfreeze the queues after the new state > of the hctx's is established. This way all IO requests that were gating > on the unfreeze are guaranteed to enter into a valid context. > > You are proposing to skip freeze on a reset. New requests will then be > allocated before we've established the hctx map. Any request allocated > will have to be terminated in failure if the hctx is no longer valid > once the reset completes. Yes, if any previous hctx doesn't come back, the requests on that hctx will be drained with BLK_STS_IOERR. __blk_mq_update_nr_hw_queues -> blk_mq_freeze_queue -> blk_freeze_queue -> blk_mq_freeze_queue_wait But the nvmeq's cq_vector is -1. > Yes, it's entirely possible today a request allocated prior to the reset > may need to be terminated after the reset. There's nothing we can do > about those except end them in failure, but we can prevent new ones from > sharing the same fate. You are removing that prevention, and that's what > I am complaining about. Thanks again for your precious time to detail this. So I got what you concern about is that this patch doesn't freeze the queue for reset case any more. And there maybe new requests enter, which will be failed when the associated hctx doesn't come back during reset procedure. And this should be avoided. I will change this in next V3 version. > * Future consideration: we recently obtained a way to "steal" bios that > looks like it may be used to back out certain types of requests and let > the bio create a new one. > Yeah, that will be a great idea to reduce the loss when hctx is gone. Sincerely Jianchao
Re: [PATCH 2/6] nvme-pci: fix the freeze and quiesce for shutdown and reset case
Hi Keith Sorry for bothering you again. ;) There is a dangerous scenario which caused by nvme_wait_freeze in nvme_reset_work. please consider it. nvme_reset_work -> nvme_start_queues -> nvme_wait_freeze if the controller no response, we have to rely on the timeout path. there are issues below: nvme_dev_disable need to be invoked. nvme_dev_disable will quiesce queues, cancel and requeue and outstanding requests. nvme_reset_work will hang at nvme_wait_freeze if we set NVME_REQ_CANCELLED and return BLK_EH_HANDLED as the RESETTING case, nvme_reset_work will hang forever, because no one could complete the entered requests. if we invoke nvme_reset_ctrl after modify the state machine to be able to change to RESETTING to RECONNECTING and queue reset_work, we still cannot move things forward, because the reset_work is being executed. if we use nvme_wait_freeze_timeout in nvme_reset_work, unfreeze and return if expires. But the timeout value is tricky.. Maybe we could use blk_set_preempt_only which is also gate on blk_queue_enter. We needn't to drain the queue for it. It is lightweight. And nvme needn't worry about the full queue prevent admin requests from being submitted. Looking forward your precious advice. Sincerely Jianchao On 02/08/2018 09:40 AM, jianchao.wang wrote: > Hi Keith > > > Really thanks for your your precious time and kindly directive. > That's really appreciated. :) > > On 02/08/2018 12:13 AM, Keith Busch wrote: >> On Wed, Feb 07, 2018 at 10:13:51AM +0800, jianchao.wang wrote: >>> What's the difference ? Can you please point out. >>> I have shared my understanding below. >>> But actually, I don't get the point what's the difference you said. >> >> It sounds like you have all the pieces. Just keep this in mind: we don't >> want to fail IO if we can prevent it. >> > Yes, absolutely. > >> A request is allocated from an hctx pool of tags. Once the request is >> allocated, it is permently tied to that hctx because that's where its >> tag came from. If that hctx becomes invalid, the request has to be ended >> with an error, and we can't do anything about that[*]. >> >> Prior to a reset, we currently halt new requests from being allocated by >> freezing the request queues. We unfreeze the queues after the new state >> of the hctx's is established. This way all IO requests that were gating >> on the unfreeze are guaranteed to enter into a valid context. >> >> You are proposing to skip freeze on a reset. New requests will then be >> allocated before we've established the hctx map. Any request allocated >> will have to be terminated in failure if the hctx is no longer valid >> once the reset completes. > Yes, if any previous hctx doesn't come back, the requests on that hctx > will be drained with BLK_STS_IOERR. > __blk_mq_update_nr_hw_queues > -> blk_mq_freeze_queue > -> blk_freeze_queue > -> blk_mq_freeze_queue_wait > But the nvmeq's cq_vector is -1. > >> Yes, it's entirely possible today a request allocated prior to the reset >> may need to be terminated after the reset. There's nothing we can do >> about those except end them in failure, but we can prevent new ones from >> sharing the same fate. You are removing that prevention, and that's what >> I am complaining about. > > Thanks again for your precious time to detail this. > So I got what you concern about is that this patch doesn't freeze the queue > for reset case > any more. And there maybe new requests enter, which will be failed when the > associated > hctx doesn't come back during reset procedure. And this should be avoided. > > I will change this in next V3 version. > > >> * Future consideration: we recently obtained a way to "steal" bios that >> looks like it may be used to back out certain types of requests and let >> the bio create a new one. >> > Yeah, that will be a great idea to reduce the loss when hctx is gone. > > Sincerely > Jianchao > > ___ > Linux-nvme mailing list > linux-n...@lists.infradead.org > https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Dnvme&d=DwICAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=7WdAxUBeiTUTCy8v-7zXyr4qk7sx26ATvfo6QSTvZyQ&m=qgIjamg94NFS7dkacDOfpPEcPa_5ImJwfZzC4Bj7wy8&s=nMzhDp4fnFjiHfbVcjI0pmiX42xK5vRWwOhFpaDV8To&e= >
Re: [PATCH 2/6] nvme-pci: fix the freeze and quiesce for shutdown and reset case
On Thu, Feb 08, 2018 at 10:17:00PM +0800, jianchao.wang wrote: > There is a dangerous scenario which caused by nvme_wait_freeze in > nvme_reset_work. > please consider it. > > nvme_reset_work > -> nvme_start_queues > -> nvme_wait_freeze > > if the controller no response, we have to rely on the timeout path. > there are issues below: > nvme_dev_disable need to be invoked. > nvme_dev_disable will quiesce queues, cancel and requeue and outstanding > requests. > nvme_reset_work will hang at nvme_wait_freeze We used to not requeue timed out commands, so that wasn't a problem before. Oh well, I'll take a look.
Re: [PATCH 2/6] nvme-pci: fix the freeze and quiesce for shutdown and reset case
Hi Keith Thanks for your precious time and kindly response. On 02/08/2018 11:15 PM, Keith Busch wrote: > On Thu, Feb 08, 2018 at 10:17:00PM +0800, jianchao.wang wrote: >> There is a dangerous scenario which caused by nvme_wait_freeze in >> nvme_reset_work. >> please consider it. >> >> nvme_reset_work >> -> nvme_start_queues >> -> nvme_wait_freeze >> >> if the controller no response, we have to rely on the timeout path. >> there are issues below: >> nvme_dev_disable need to be invoked. >> nvme_dev_disable will quiesce queues, cancel and requeue and outstanding >> requests. >> nvme_reset_work will hang at nvme_wait_freeze > > We used to not requeue timed out commands, so that wasn't a problem > before. Oh well, I'll take a look. > Yes, we indeed don't requeue the timed out commands, but nvme_dev_disable will requeue the other outstanding requests and quiesce the request queues, this will block the nvme_reset_work->nvme_wati_freeze to move forward. As I shared in last email, can we use(or abuse?) blk_set_preempt_only to gate the new bios on generic_make_request ? Freezing queues is good, but wait_freeze in reset_work is a devil. Many thanks Jianchao
Re: [PATCH 2/6] nvme-pci: fix the freeze and quiesce for shutdown and reset case
On Fri, Feb 02, 2018 at 03:00:45PM +0800, Jianchao Wang wrote: > Currently, request queue will be frozen and quiesced for both reset > and shutdown case. This will trigger ioq requests in RECONNECTING > state which should be avoided to prepare for following patch. > Just freeze request queue for shutdown case and drain all the resudual > entered requests after controller has been shutdown. Freezing is not just for shutdown. It's also used so blk_mq_update_nr_hw_queues will work if the queue count changes across resets.
Re: [PATCH 2/6] nvme-pci: fix the freeze and quiesce for shutdown and reset case
Hi Keith
Thanks for your kindly response.
On 02/03/2018 02:24 AM, Keith Busch wrote:
> On Fri, Feb 02, 2018 at 03:00:45PM +0800, Jianchao Wang wrote:
>> Currently, request queue will be frozen and quiesced for both reset
>> and shutdown case. This will trigger ioq requests in RECONNECTING
>> state which should be avoided to prepare for following patch.
>> Just freeze request queue for shutdown case and drain all the resudual
>> entered requests after controller has been shutdown.
> Freezing is not just for shutdown. It's also used so
> blk_mq_update_nr_hw_queues will work if the queue count changes across
> resets.
blk_mq_update_nr_hw_queues will freeze the queue itself. Please refer to.
static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
int nr_hw_queues)
{
struct request_queue *q;
lockdep_assert_held(&set->tag_list_lock);
if (nr_hw_queues > nr_cpu_ids)
nr_hw_queues = nr_cpu_ids;
if (nr_hw_queues < 1 || nr_hw_queues == set->nr_hw_queues)
return;
list_for_each_entry(q, &set->tag_list, tag_set_list)
blk_mq_freeze_queue(q);
set->nr_hw_queues = nr_hw_queues;
blk_mq_update_queue_map(set);
list_for_each_entry(q, &set->tag_list, tag_set_list) {
blk_mq_realloc_hw_ctxs(set, q);
blk_mq_queue_reinit(q);
}
list_for_each_entry(q, &set->tag_list, tag_set_list)
blk_mq_unfreeze_queue(q);
}
Thanks
Jianchao
Re: [PATCH 2/6] nvme-pci: fix the freeze and quiesce for shutdown and reset case
On Mon, Feb 05, 2018 at 10:26:03AM +0800, jianchao.wang wrote: > > Freezing is not just for shutdown. It's also used so > > blk_mq_update_nr_hw_queues will work if the queue count changes across > > resets. > blk_mq_update_nr_hw_queues will freeze the queue itself. Please refer to. > static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, Yes, but how many requests are you letting enter to their demise by freezing on the wrong side of the reset? We don't use the nuclear option out of convenience.
Re: [PATCH 2/6] nvme-pci: fix the freeze and quiesce for shutdown and reset case
Hi Keith Thanks for your kindly response. On 02/05/2018 11:13 PM, Keith Busch wrote: > but how many requests are you letting enter to their demise by > freezing on the wrong side of the reset? There are only two difference with this patch from the original one. 1. Don't freeze the queue for the reset case. At the moment, the outstanding requests will be requeued back to blk-mq queues. The new entered requests during reset will also stay in blk-mq queues. All this requests will not enter into nvme driver layer due to quiescent request_queues. And they will be issued after the reset is completed successfully. 2. Drain the request queue before nvme_dev_disable. This is nearly same with the previous rule which will also unquiesce the queue and let the requests be able to be drained. The only difference is this patch will invoke wait_freeze in nvme_dev_disable instead of nvme_reset_work. We don't sacrifice any request. This patch do the same thing with the previous one and make things clearer. :) Thanks Jianchao
Re: [PATCH 2/6] nvme-pci: fix the freeze and quiesce for shutdown and reset case
On Tue, Feb 06, 2018 at 09:46:36AM +0800, jianchao.wang wrote: > Hi Keith > > Thanks for your kindly response. > > On 02/05/2018 11:13 PM, Keith Busch wrote: > > but how many requests are you letting enter to their demise by > > freezing on the wrong side of the reset? > > There are only two difference with this patch from the original one. > 1. Don't freeze the queue for the reset case. At the moment, the outstanding > requests will be requeued back to blk-mq queues. >The new entered requests during reset will also stay in blk-mq queues. All > this requests will not enter into nvme driver layer >due to quiescent request_queues. And they will be issued after the reset > is completed successfully. > 2. Drain the request queue before nvme_dev_disable. This is nearly same with > the previous rule which will also unquiesce the queue >and let the requests be able to be drained. The only difference is this > patch will invoke wait_freeze in nvme_dev_disable instead >of nvme_reset_work. > > We don't sacrifice any request. This patch do the same thing with the > previous one and make things clearer. No, what you're proposing is quite different. By "enter", I'm referring to blk_queue_enter. Once a request enters into an hctx, it can not be backed out to re-enter a new hctx if the original one is invalidated. Prior to a reset, all requests that have entered the queue are committed to that hctx, and we can't do anything about that. The only thing we can do is prevent new requests from entering until we're sure that hctx is valid on the other side of the reset.
Re: [PATCH 2/6] nvme-pci: fix the freeze and quiesce for shutdown and reset case
Hi Keith Thanks for your time and kindly response on this. On 02/06/2018 11:13 PM, Keith Busch wrote: > On Tue, Feb 06, 2018 at 09:46:36AM +0800, jianchao.wang wrote: >> Hi Keith >> >> Thanks for your kindly response. >> >> On 02/05/2018 11:13 PM, Keith Busch wrote: >>> but how many requests are you letting enter to their demise by >>> freezing on the wrong side of the reset? >> >> There are only two difference with this patch from the original one. >> 1. Don't freeze the queue for the reset case. At the moment, the outstanding >> requests will be requeued back to blk-mq queues. >>The new entered requests during reset will also stay in blk-mq queues. >> All this requests will not enter into nvme driver layer >>due to quiescent request_queues. And they will be issued after the reset >> is completed successfully. >> 2. Drain the request queue before nvme_dev_disable. This is nearly same with >> the previous rule which will also unquiesce the queue >>and let the requests be able to be drained. The only difference is this >> patch will invoke wait_freeze in nvme_dev_disable instead >>of nvme_reset_work. >> >> We don't sacrifice any request. This patch do the same thing with the >> previous one and make things clearer. > > No, what you're proposing is quite different. > > By "enter", I'm referring to blk_queue_enter. When a request is allocated, it will hold a request_queue->q_usage_counter until it is freed. Please refer to blk_mq_get_request -> blk_queue_enter_live blk_mq_free_request -> blk_exit_queue Regarding to 'request enters into an hctx', I cannot get the point. I think you should mean it enter into nvme driver layer. > Once a request enters > into an hctx, it can not be backed out to re-enter a new hctx if the > original one is invalidated. I also cannot get the point here. We certainly will not issue a request which has been issued to other hctx. What this patch and also the original one does is that disable/shutdown controller, cancel and requeue or fail the outstanding requests. The requeue mechanism will ensure the requests to be inserted to the ctx where req->mq_ctx->cpu points to. > > Prior to a reset, all requests that have entered the queue are committed > to that hctx, A request could be on - blk-mq per-cpu ctx->rq_list, IO scheduler list\ - hctx->dispatch list or - request_queue->requeue_list (will be inserted to 1st case again) When requests are issued, they will be dequeued from 1st or 2nd case and submitted to nvme driver layer. These requests are _outstanding_ ones. When the request queue is quiesced, the request will be stayed in blk-mq per-cpu ctx->rq_list, IO scheduler list or hctx->dispatch list, and cannot be issued to driver layer any more. When the request queue is frozen, it will gate the bio out of generic_make_request, so new request cannot enter blk-mq layer any more, and certainly the nvme driver layer. For the reset case, the nvme controller will be back soon, we needn't freeze the queue, just quiescing is enough. The outstanding ones will be canceled and _requeued_ to request_queue->requeue_list, then they will be inserted into blk-mq layer again by requeue_work. When reset_work completes and start queues again, all the requests will be issued again. :) For the shutdown case, freezing and quiescing is safer. Also we will wait them to be completed if the controller is still alive. If dead, we need to fail them directly instead of requeue them, otherwise, IO hung will come up, because controller will be offline for some time. and we can't do anything about that. The only thing we can > do is prevent new requests from entering until we're sure that hctx is > valid on the other side of the reset. > yes, that's is what this patch does. Add some explaining about the 4th patch nvme-pci: break up nvme_timeout and nvme_dev_disable here. Also thanks for your time to look into this. That's really appreciated! The key point is blk_abort_request. It will force the request to be expired and then handle the request in timeout work context. It is safe to race with the irq completion path. This is the most important reason to use blk_abort_request. We don't _complete_ the request or _rearm_ the time, but just set a CANCELED flag. So request will not be freed. Then these requests cannot be taken away by irq completion path and time out path is also be avoid (no outstanding requests any more). So we say 'all the outstanding requests are grabbed'. When we close the controller totally, we could complete these requests safely. This is the core idea of the 4th patch. Many thanks Jianchao
Re: [PATCH 2/6] nvme-pci: fix the freeze and quiesce for shutdown and reset case
Hi Keith Sorry for bothering you again. On 02/07/2018 10:03 AM, jianchao.wang wrote: > Hi Keith > > Thanks for your time and kindly response on this. > > On 02/06/2018 11:13 PM, Keith Busch wrote: >> On Tue, Feb 06, 2018 at 09:46:36AM +0800, jianchao.wang wrote: >>> Hi Keith >>> >>> Thanks for your kindly response. >>> >>> On 02/05/2018 11:13 PM, Keith Busch wrote: but how many requests are you letting enter to their demise by freezing on the wrong side of the reset? >>> >>> There are only two difference with this patch from the original one. >>> 1. Don't freeze the queue for the reset case. At the moment, the >>> outstanding requests will be requeued back to blk-mq queues. >>>The new entered requests during reset will also stay in blk-mq queues. >>> All this requests will not enter into nvme driver layer >>>due to quiescent request_queues. And they will be issued after the reset >>> is completed successfully. >>> 2. Drain the request queue before nvme_dev_disable. This is nearly same >>> with the previous rule which will also unquiesce the queue >>>and let the requests be able to be drained. The only difference is this >>> patch will invoke wait_freeze in nvme_dev_disable instead >>>of nvme_reset_work. >>> >>> We don't sacrifice any request. This patch do the same thing with the >>> previous one and make things clearer. >> >> No, what you're proposing is quite different. What's the difference ? Can you please point out. I have shared my understanding below. But actually, I don't get the point what's the difference you said. Or what you refer to is about the 4th patch ? If yes, I also explain this below. Really appreciate your precious time to explain this. :) Many thanks Jianchao >> >> By "enter", I'm referring to blk_queue_enter. > When a request is allocated, it will hold a request_queue->q_usage_counter > until it is freed. > Please refer to > blk_mq_get_request -> blk_queue_enter_live > blk_mq_free_request -> blk_exit_queue > > Regarding to 'request enters into an hctx', I cannot get the point. > I think you should mean it enter into nvme driver layer. > >> Once a request enters >> into an hctx, it can not be backed out to re-enter a new hctx if the >> original one is invalidated. > > I also cannot get the point here. We certainly will not issue a request which > has been issued to other hctx. > What this patch and also the original one does is that disable/shutdown > controller, > cancel and requeue or fail the outstanding requests. > > The requeue mechanism will ensure the requests to be inserted to the ctx > where req->mq_ctx->cpu > points to. > >> >> Prior to a reset, all requests that have entered the queue are committed >> to that hctx, > > A request could be on > - blk-mq per-cpu ctx->rq_list, IO scheduler list\ > - hctx->dispatch list or > - request_queue->requeue_list (will be inserted to 1st case again) > > When requests are issued, they will be dequeued from 1st or 2nd case and > submitted to nvme driver layer. > These requests are _outstanding_ ones. > > When the request queue is quiesced, the request will be stayed in blk-mq > per-cpu ctx->rq_list, IO scheduler list > or hctx->dispatch list, and cannot be issued to driver layer any more. > When the request queue is frozen, it will gate the bio out of > generic_make_request, so new request cannot enter > blk-mq layer any more, and certainly the nvme driver layer. > > For the reset case, the nvme controller will be back soon, we needn't freeze > the queue, just quiescing is enough. > The outstanding ones will be canceled and _requeued_ to > request_queue->requeue_list, then they will be inserted into > blk-mq layer again by requeue_work. When reset_work completes and start > queues again, all the requests will be > issued again. :) > > For the shutdown case, freezing and quiescing is safer. Also we will wait > them to be completed if the controller is still > alive. If dead, we need to fail them directly instead of requeue them, > otherwise, IO hung will come up, because controller > will be offline for some time. > > > and we can't do anything about that. The only thing we can >> do is prevent new requests from entering until we're sure that hctx is >> valid on the other side of the reset. >> > yes, that's is what this patch does. > > Add some explaining about the 4th patch nvme-pci: break up nvme_timeout and > nvme_dev_disable here. > Also thanks for your time to look into this. That's really appreciated! > > The key point is blk_abort_request. It will force the request to be expired > and then handle the request > in timeout work context. It is safe to race with the irq completion path. > This is the most important reason > to use blk_abort_request. > We don't _complete_ the request or _rearm_ the time, but just set a CANCELED > flag. So request will not be freed. > Then these requests cannot be taken away by irq completion path and time out > path is also b
