From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tejun Heo Subject: Re: [RFC PATCH 1/5] block: Introduce q->abort_queue_fn() Date: Tue, 22 May 2012 08:14:41 -0700 Message-ID: <20120522151441.GB14339@google.com> References: <1337591313-26333-1-git-send-email-asias@redhat.com> <20120521154213.GB6549@google.com> <4FBB409D.4070201@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: Jens Axboe , kvm@vger.kernel.org, "Michael S. Tsirkin" , virtualization@lists.linux-foundation.org, linux-fsdevel@vger.kernel.org To: Asias He Return-path: Content-Disposition: inline In-Reply-To: <4FBB409D.4070201@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org List-Id: kvm.vger.kernel.org Hello, On Tue, May 22, 2012 at 03:30:37PM +0800, Asias He wrote: > On 05/21/2012 11:42 PM, Tejun Heo wrote: > 1) if the queue is stopped, q->request_fn() will never call called. > we will be stuck in the loop forever. This can happen if the remove > method is called after the q->request_fn() calls blk_stop_queue() to > stop the queue when the device is full, and before the device > interrupt handler to start the queue. This can be fixed by calling > blk_start_queue() before __blk_run_queue(q). > > blk_drain_queue() { > while(true) { > ... > if (!list_empty(&q->queue_head)) > __blk_run_queue(q); > ... > } > } Wouldn't that be properly fixed by making queue cleanup override stopped state? > 2) Since the device is gonna be removed, is it safe to rely on the > device to finish the request before the DEAD marking? E.g, In > vritio-blk, We reset the device and thus disable the interrupt > before we call blk_cleanup_queue(). I also suspect that the real > hardware can finish the pending requests when being hot-unplugged. Yes, it should be safe (otherwise it's a driver bug). Device driver already knows the state of the device it is driving. If the device can't service requests for whatever reason, the device driver should abort any in-flight and future requests. That's how other block drivers behave and I don't see why virtio should be any different. Also, blk_drain_queue() is used for other purposes too - elevator switch and blkcg policy changes. You definitely don't want to be aborting requests across those events. So, NACK. Thanks. -- tejun