Re: [RFC] 'discard sectors' block request.

[David Woodhouse <dwmw2@infradead.org>]

On Tue, 2008-08-05 at 13:42 +0200, Jens Axboe wrote:
> I was going to suggest that if you want this sync, then you should cut
> this code out of blkdev_issue_flush() and reuse that helper for
> blkdev_issue_flush and blkdev_issue_discard(). But I don't think the
> sync interface makes a lot of sense. 

Yeah, I agree. That was just for the proof of concept, really.

> Also, get rid of the error_sector stuff. First of all it isn't really
> used consistently in the flush part since most block drivers don't
> have this information (and/or fill it in), and secondly you can just
> store that in bi_sector or whatever instead for this request.

OK.

> int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
> 			 unsigned nr_sects, bio_end_io_t *end_io)
> {
  ...

> That would be the simpler async variant. Let the caller pass in the
> end_io function as well, as the fs would definitely want to check for
> EOPNOTSUPP or other errors itself on completion.

Most of the time the fs doesn't care at all. The whole point is that it
no longer gives a monkey's about the contents of these sectors. It's
just giving the underlying storage an _opportunity_ to reclaim them, if
it wants to do so. Most of the time, a file system isn't going to do
_anything_ with an error other than ignore it.

Even -EOPNOTSUPP isn't massively interesting, because it'll want to
check whether the operation is supported _before_ it bothers to queue
the request, rather than queueing the request unconditionally.

>  Remember to do the bio_put() in there as well at the end. 

Are we allowed to just set the end_io function to &bio_put?

> Perhaps you want a sync interface as well? 

I think we can live without a sync interface. As long as the discard
request is guaranteed to finish before any subsequent write, which is
something for the elevator to care about rather than the caller, we
really don't care when it finishes. If the occasional strange caller
wants to do it synchronously, they can arrange that for themselves.

> Otherwise that akpm said, he covered basically all my points. Lets do a
> 
> static inline int bio_data_less(struct bio *bio)
> {
>         return !bio->bi_io_vec;
> }
> 
> like you suggested for checking whether the bio carries data or not,
> since we need that for the empty barrier as well (and other future bio
> hints we want to pass down).

OK. And for blk_dataless_rq() it would be checking !req->buffer? 

> > @@ -1886,7 +1899,7 @@ static int blk_end_io(struct request *rq, int error, unsigned int nr_bytes,
> >  	struct request_queue *q = rq->q;
> >  	unsigned long flags = 0UL;
> >  
> > -	if (blk_fs_request(rq) || blk_pc_request(rq)) {
> > +	if (blk_fs_request(rq) || blk_pc_request(rq) || blk_discard_rq(rq)) {
> >  		if (__end_that_request_first(rq, error, nr_bytes))
> >  			return 1;
> >  
> > @@ -1944,7 +1957,7 @@ EXPORT_SYMBOL_GPL(blk_end_request);
> >   **/
> >  int __blk_end_request(struct request *rq, int error, unsigned int nr_bytes)
> >  {
> > -	if (blk_fs_request(rq) || blk_pc_request(rq)) {
> > +	if (blk_fs_request(rq) || blk_pc_request(rq) || blk_discard_rq(rq)) {
> >  		if (__end_that_request_first(rq, error, nr_bytes))
> >  			return 1;
> >  	}
> 
> Do you need to call into __end_request_request_first()? You don't need
> to fill error sector now, and there's no bio data to complete.

That's what actually calls the ->end_io function, isn't it? The need to
add that was determined empirically... but at about 2am this morning, so
I'm not going to sulk if you tell me I'm wrong.

We'll probably also want the elevator to merge discard requests. But
that can come later.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation