From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jens Axboe Subject: Re: [PATCH block#for-2.6.31] block: fix an oops on BLKPREP_KILL Date: Sun, 31 May 2009 08:11:39 +0200 Message-ID: <20090531061139.GS11363@kernel.dk> References: <1243635503.2919.67.camel@localhost.localdomain> <20090530044137.GM11363@kernel.dk> <1243693384.5223.7.camel@mulgrave.int.hansenpartnership.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from brick.kernel.dk ([93.163.65.50]:50855 "EHLO kernel.dk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751190AbZEaGLi (ORCPT ); Sun, 31 May 2009 02:11:38 -0400 Content-Disposition: inline In-Reply-To: <1243693384.5223.7.camel@mulgrave.int.hansenpartnership.com> Sender: linux-ide-owner@vger.kernel.org List-Id: linux-ide@vger.kernel.org To: James Bottomley Cc: Tejun Heo , linux-scsi , linux-ide On Sat, May 30 2009, James Bottomley wrote: > On Sat, 2009-05-30 at 06:41 +0200, Jens Axboe wrote: > > On Fri, May 29 2009, James Bottomley wrote: > > > Doing a bit of torture testing, I ran across a BUG in the block > > > subsystem (at blk-core.c:2048): the test for if the request is queued. > > > > > > It turns out the trigger was a BLKPREP_KILL coming out of the SCSI prep > > > function. Currently for BLKPREP_KILL requests, we send them straight > > > into __blk_end_request_all() with an error, but they've never been > > > dequeued, so they trip the bug. Fix this by starting requests before > > > killing them. > > > > > > Signed-off-by: James Bottomley > > > > > > --- > > > > > > diff --git a/block/blk-core.c b/block/blk-core.c > > > index 8b3b74e..9a0568c 100644 > > > --- a/block/blk-core.c > > > +++ b/block/blk-core.c > > > @@ -1789,6 +1789,7 @@ struct request *blk_peek_request(struct request_queue *q) > > > break; > > > } else if (ret == BLKPREP_KILL) { > > > rq->cmd_flags |= REQ_QUIET; > > > + blk_start_request(rq); > > > __blk_end_request_all(rq, -EIO); > > > } else { > > > printk(KERN_ERR "%s: bad return=%d\n", __func__, ret); > > > > Given how illogical that now looks, I think it could do with a comment. > > I'll add that while applying. > > Um, if it looks illogical, then so is the new everything has to be > dequeued before completion requirement ... I agree a comment reminding > people why it has to work this way doesn't hurt, but if it looks > illogical then there might be something wrong with the rules requiring > this to happen ... It's not a bad idea to be a little extra paranoid with such an API change, we can always relax this restriction later. -- Jens Axboe