From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 19 Oct 2016 15:41:31 +0200 From: Christoph Hellwig To: Bart Van Assche Subject: Re: [PATCH v3 11/11] nvme: Fix a race condition Message-ID: <20161019134131.GD6809@lst.de> References: <2a29b7ec-0113-3450-9a36-b925b47b1fb0@sandisk.com> MIME-Version: 1.0 In-Reply-To: <2a29b7ec-0113-3450-9a36-b925b47b1fb0@sandisk.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Keith Busch , Ming Lin , James Bottomley , "Martin K. Petersen" , Mike Snitzer , "linux-rdma@vger.kernel.org" , "linux-nvme@lists.infradead.org" , Jens Axboe , Doug Ledford , "linux-block@vger.kernel.org" , "linux-scsi@vger.kernel.org" , Laurence Oberman , Christoph Hellwig Content-Type: text/plain; charset="us-ascii" Sender: "Linux-nvme" Errors-To: linux-nvme-bounces+axboe=kernel.dk@lists.infradead.org List-ID: Hi Bart, this looks great! Reviewed-by: Christoph Hellwig Some minor nitpicks below: > void nvme_requeue_req(struct request *req) > { > + blk_mq_requeue_request(req, true); > } > EXPORT_SYMBOL_GPL(nvme_requeue_req); Please just remove the nvme_requeue_req wrapper. > > @@ -2074,11 +2068,14 @@ EXPORT_SYMBOL_GPL(nvme_kill_queues); > void nvme_stop_queues(struct nvme_ctrl *ctrl) > { > struct nvme_ns *ns; > + struct request_queue *q; > > mutex_lock(&ctrl->namespaces_mutex); > list_for_each_entry(ns, &ctrl->namespaces, list) { > + q = ns->queue; > + blk_mq_cancel_requeue_work(q); > + blk_mq_stop_hw_queues(q); > + blk_mq_quiesce_queue(q); > } I'd keep the q declaration in the minimal scope, e.g. list_for_each_entry(ns, &ctrl->namespaces, list) { struct request_queue *q = ns->queue; blk_mq_cancel_requeue_work(q); blk_mq_stop_hw_queues(q); blk_mq_quiesce_queue(q); } _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme From mboxrd@z Thu Jan 1 00:00:00 1970 From: hch@lst.de (Christoph Hellwig) Date: Wed, 19 Oct 2016 15:41:31 +0200 Subject: [PATCH v3 11/11] nvme: Fix a race condition In-Reply-To: <2a29b7ec-0113-3450-9a36-b925b47b1fb0@sandisk.com> References: <2a29b7ec-0113-3450-9a36-b925b47b1fb0@sandisk.com> Message-ID: <20161019134131.GD6809@lst.de> Hi Bart, this looks great! Reviewed-by: Christoph Hellwig Some minor nitpicks below: > void nvme_requeue_req(struct request *req) > { > + blk_mq_requeue_request(req, true); > } > EXPORT_SYMBOL_GPL(nvme_requeue_req); Please just remove the nvme_requeue_req wrapper. > > @@ -2074,11 +2068,14 @@ EXPORT_SYMBOL_GPL(nvme_kill_queues); > void nvme_stop_queues(struct nvme_ctrl *ctrl) > { > struct nvme_ns *ns; > + struct request_queue *q; > > mutex_lock(&ctrl->namespaces_mutex); > list_for_each_entry(ns, &ctrl->namespaces, list) { > + q = ns->queue; > + blk_mq_cancel_requeue_work(q); > + blk_mq_stop_hw_queues(q); > + blk_mq_quiesce_queue(q); > } I'd keep the q declaration in the minimal scope, e.g. list_for_each_entry(ns, &ctrl->namespaces, list) { struct request_queue *q = ns->queue; blk_mq_cancel_requeue_work(q); blk_mq_stop_hw_queues(q); blk_mq_quiesce_queue(q); } From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoph Hellwig Subject: Re: [PATCH v3 11/11] nvme: Fix a race condition Date: Wed, 19 Oct 2016 15:41:31 +0200 Message-ID: <20161019134131.GD6809@lst.de> References: <2a29b7ec-0113-3450-9a36-b925b47b1fb0@sandisk.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <2a29b7ec-0113-3450-9a36-b925b47b1fb0@sandisk.com> Sender: linux-block-owner@vger.kernel.org To: Bart Van Assche Cc: Jens Axboe , Christoph Hellwig , James Bottomley , "Martin K. Petersen" , Mike Snitzer , Doug Ledford , Keith Busch , Ming Lin , Laurence Oberman , "linux-block@vger.kernel.org" , "linux-scsi@vger.kernel.org" , "linux-rdma@vger.kernel.org" , "linux-nvme@lists.infradead.org" List-Id: linux-rdma@vger.kernel.org Hi Bart, this looks great! Reviewed-by: Christoph Hellwig Some minor nitpicks below: > void nvme_requeue_req(struct request *req) > { > + blk_mq_requeue_request(req, true); > } > EXPORT_SYMBOL_GPL(nvme_requeue_req); Please just remove the nvme_requeue_req wrapper. > > @@ -2074,11 +2068,14 @@ EXPORT_SYMBOL_GPL(nvme_kill_queues); > void nvme_stop_queues(struct nvme_ctrl *ctrl) > { > struct nvme_ns *ns; > + struct request_queue *q; > > mutex_lock(&ctrl->namespaces_mutex); > list_for_each_entry(ns, &ctrl->namespaces, list) { > + q = ns->queue; > + blk_mq_cancel_requeue_work(q); > + blk_mq_stop_hw_queues(q); > + blk_mq_quiesce_queue(q); > } I'd keep the q declaration in the minimal scope, e.g. list_for_each_entry(ns, &ctrl->namespaces, list) { struct request_queue *q = ns->queue; blk_mq_cancel_requeue_work(q); blk_mq_stop_hw_queues(q); blk_mq_quiesce_queue(q); }