On Thu, 2008-08-07 at 20:41 +0200, Jens Axboe wrote:
quoted text
> 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.

quoted text
> > 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.

quoted text
> > +	/* 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.

quoted text
> > @@ -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.

quoted text
> > 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.

quoted text
> >  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



--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html