From patchwork Tue Aug 9 17:43:47 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mike Snitzer X-Patchwork-Id: 1050552 Received: from mx4-phx2.redhat.com (mx4-phx2.redhat.com [209.132.183.25]) by demeter2.kernel.org (8.14.4/8.14.4) with ESMTP id p79Hlnlv013122 for ; Tue, 9 Aug 2011 17:48:10 GMT Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by mx4-phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p79Hht88005020; Tue, 9 Aug 2011 13:43:58 -0400 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p79HhrxR025192 for ; Tue, 9 Aug 2011 13:43:53 -0400 Received: from localhost (dhcp-100-19-150.bos.redhat.com [10.16.19.150]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p79HhlRY008142 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Tue, 9 Aug 2011 13:43:48 -0400 Date: Tue, 9 Aug 2011 13:43:47 -0400 From: Mike Snitzer To: Tejun Heo Message-ID: <20110809174347.GA13293@redhat.com> References: <20110809153826.GB23842@htj.dyndns.org> <20110809161334.GC23842@htj.dyndns.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20110809161334.GC23842@htj.dyndns.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-Scanned-By: MIMEDefang 2.68 on 10.5.11.25 X-loop: dm-devel@redhat.com Cc: Jens Axboe , Jeff Moyer , dm-devel@redhat.com, linux-kernel@vger.kernel.org, Vivek Goyal Subject: Re: [dm-devel] block: properly handle flush/fua requests in blk_insert_cloned_request X-BeenThere: dm-devel@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk Reply-To: device-mapper development List-Id: device-mapper development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter2.kernel.org [140.211.167.43]); Tue, 09 Aug 2011 17:48:10 +0000 (UTC) On Tue, Aug 09 2011 at 12:13pm -0400, Tejun Heo wrote: > Hello, > > On Tue, Aug 09, 2011 at 11:53:51AM -0400, Jeff Moyer wrote: > > Tejun Heo writes: > > > I'm a bit confused. We still need ELEVATOR_INSERT_FLUSH fix for > > > insertion paths, right? Or is blk_insert_cloned_request() supposed to > > > used only by request based dm which lives under the elevator? If so, > > > it would be great to make that explicit in the comment. Maybe just > > > renaming it to blk_insert_dm_cloned_request() would be better as it > > > wouldn't be safe for other cases anyway. > > > > request-based dm is the only caller at present. I'm not a fan of > > renaming the function, but I'm more than willing to comment it. > > I'm still confused and don't think the patch is correct (you can't > turn off REQ_FUA without decomposing it to data + post flush). > > Going through flush machinery twice is okay and I think is the right > thing to do. At the upper queue, the request is decomposed to member > requests. After decomposition, it's either REQ_FLUSH w/o data or data > request w/ or w/o REQ_FUA. When the decomposed request reaches lower > queue, the lower queue will then either short-circuit it, execute > as-is or decompose data w/ REQ_FUA into data + REQ_FLUSH sequence. > > AFAICS, the breakages are... > > * ELEVATOR_INSERT_FLUSH not used properly from insert paths. > > * Short circuit not kicking in for the dm requests. (the above and the > policy patch should solve this, right?) > > * BUG(!rq->bio || ...) in blk_insert_flush(). I think we can lift > this restriction for empty REQ_FLUSH but also dm can just send down > requests with empty bio. [cc'ing dm-devel] All of these issues have come to light because DM was not setting flush_flags based on the underlying device(s). Now fixed in v3.1-rc1: ed8b752 dm table: set flush capability based on underlying devices Given that commit, and that request-based DM is beneath the elevator, it seems any additional effort to have DM flushes re-enter the flush machinary is unnecessary. We expect: 1) flushes to have gone through the flush machinary 2) no FLUSH/FUA should be entering underlying queues if not supported I think it best to just document the expectation that any FLUSH/FUA request that enters blk_insert_cloned_request() will already match the queue that the request is being sent to. One way to document it is to change Jeff's flag striping in to pure BUG_ON()s, e.g.: --- block/blk-core.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel diff --git a/block/blk-core.c b/block/blk-core.c index b627558..201bb27 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -1710,6 +1710,14 @@ int blk_insert_cloned_request(struct request_queue *q, struct request *rq) should_fail_request(&rq->rq_disk->part0, blk_rq_bytes(rq))) return -EIO; + /* + * All FLUSH/FUA requests are expected to have gone through the + * flush machinary. If a request's cmd_flags doesn't match the + * flush_flags of the underlying request_queue it is a bug. + */ + BUG_ON((rq->cmd_flags & REQ_FLUSH) && !(q->flush_flags & REQ_FLUSH)); + BUG_ON((rq->cmd_flags & REQ_FUA) && !(q->flush_flags & REQ_FUA)); + spin_lock_irqsave(q->queue_lock, flags); /*