From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Woodhouse Subject: Re: [PATCH 1/5] [BLOCK] Add 'discard' request handling Date: Fri, 08 Aug 2008 10:33:15 +0100 Message-ID: <1218187995.12232.95.camel@pmac.infradead.org> References: <488B7281.4020007@gmail.com> <20080726130200.f541e604.akpm@linux-foundation.org> <1217900716.3454.667.camel@pmac.infradead.org> <20080805114210.GW20055@kernel.dk> <1217953741.3454.784.camel@pmac.infradead.org> <1217957140.3454.800.camel@pmac.infradead.org> <1218014703.5111.35.camel@pmac.infradead.org> <20080807184155.GE20055@kernel.dk> Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit Cc: Andrew Morton , Ric Wheeler , linux-fsdevel@vger.kernel.org, gilad@codefidence.com To: Jens Axboe Return-path: Received: from bombadil.infradead.org ([18.85.46.34]:51648 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751327AbYHHJd3 (ORCPT ); Fri, 8 Aug 2008 05:33:29 -0400 In-Reply-To: <20080807184155.GE20055@kernel.dk> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, 2008-08-07 at 20:41 +0200, Jens Axboe wrote: > I've queued this up for some testing, I'll add the merge support as > well. Thanks. Did you pull from my tree? If so I'll provide an incremental patch. Otherwise I'll go back and recommit it with your suggested changes. > > diff --git a/block/blk-barrier.c b/block/blk-barrier.c > > index a09ead1..29e60ff 100644 > > --- a/block/blk-barrier.c > > +++ b/block/blk-barrier.c > > Not sure why you are placing it here, it should probably just go into > blk-core.c Yeah, I vacillated about that for a while -- and even got as far as moving it there, and back again. It's not really _core_ code either. Since it started off almost identical to blkdev_issue_flush() I figured it might as well sit next to it. But I'm happy to move it too. > > + /* Many callers won't care at all about the outcome. After all, > > + this is just a hint to the underlying device. They'll just > > + ignore errors completely. So the end_io function can be just > > + a call to bio_put() */ > > + if (!end_io) > > + end_io = (void *)&bio_put; > > Please don't do that, that's pretty ugly. OK. > > @@ -1488,10 +1502,9 @@ void submit_bio(int rw, struct bio *bio) > > * If it's a regular read/write or a barrier with data attached, > > * go through the normal accounting stuff before submission. > > */ > > - if (!bio_empty_barrier(bio)) { > > + if (!bio_dataless(bio)) { > > > > BIO_BUG_ON(!bio->bi_size); > > - BIO_BUG_ON(!bio->bi_io_vec); > > > > if (rw & WRITE) { > > count_vm_events(PGPGOUT, count); > > Zach suggested bio_has_data() instead, which I agree is a lot more > readable. OK. > > diff --git a/block/elevator.c b/block/elevator.c > > index ed6f8f3..bb26424 100644 > > --- a/block/elevator.c > > +++ b/block/elevator.c > > @@ -675,7 +675,7 @@ void __elv_add_request(struct request_queue *q, struct request *rq, int where, > > if (q->ordcolor) > > rq->cmd_flags |= REQ_ORDERED_COLOR; > > > > - if (rq->cmd_flags & (REQ_SOFTBARRIER | REQ_HARDBARRIER)) { > > + if (rq->cmd_flags & (REQ_SOFTBARRIER | REQ_HARDBARRIER | REQ_DISCARD)) { > > /* > > * toggle ordered color > > */ > > Just make REQ_DISCARD set REQ_SOFTBARRIER? Do you care if this acts as a > barrier or not? Er, didn't you object to me setting REQ_SOFTBARRIER for discard requests, a few lines up? Admittedly, I shouldn't need to do _both_, but I think I need one or the other. In the long term no, I don't care if it acts as a barrier. I just did that until we implement the merge support for discard requests. > > static inline unsigned int bio_cur_sectors(struct bio *bio) > > { > > + if (unlikely(bio_discard(bio))) > > + return bio->bi_size >> 9; > > + > > if (bio->bi_vcnt) > > return bio_iovec(bio)->bv_len >> 9; > > Just make that > > if (bio->bi_vcnt) > return bio_iovec(bio)->bv_len >> 9; > else > return bio->bi_size >> 9; > > and add a comment about it. OK. -- David Woodhouse Open Source Technology Centre David.Woodhouse@intel.com Intel Corporation