Re: [PATCH 0/7] Discard requests, v2

[David Woodhouse <dwmw2@infradead.org>]

On Tue, 2008-08-12 at 14:53 +0200, Jens Axboe wrote:
> On Tue, Aug 12 2008, David Woodhouse wrote:
> > On Tue, 2008-08-12 at 12:16 +0100, David Woodhouse wrote:
> > > There's a check in __make_request() which will return -EOPNOTSUPP for
> > > bios with BIO_RW_BARRIER set, in some circumstances. Is that OK too?
> > 
> > A: No, it breaks submission of DISCARD|BARRIER requests. And I have to
> > change blk_empty_barrier() to explicitly omit discards too, to avoid the
> > check at line 1419 (in __generic_make_request()) returning -EOPNOTSUPP
> > before we even get there.
> > 
> > Using BIO_RW_BARRIER seems conceptually cleaner, but in practice it's
> > messier. I think this version will work, but I'm inclined to prefer the
> > previous one I posted, using the R/W bit. 
> 
> Or just match the check before -EOPNOTSUPP with bio_has_data(), since it
> only applies to a barrier that carries data.

Like this, you mean? Empty barriers don't get to there?

I still prefer the R/W version, but I'll commit it this way if you
want...

diff --git a/block/blk-core.c b/block/blk-core.c
index be2fe52..1b38994 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1075,19 +1075,13 @@ void init_request_from_bio(struct request *req, struct bio *bio)
 	/*
 	 * REQ_BARRIER implies no merging, but lets make it explicit
 	 */
-	if (unlikely(bio_barrier(bio)))
-		req->cmd_flags |= (REQ_HARDBARRIER | REQ_NOMERGE);
 	if (unlikely(bio_discard(bio))) {
 		req->cmd_flags |= REQ_DISCARD;
-		/*
-		 * READ discard requests are soft barriers; WRITE
-		 * indicates the file system will take care of that
-		 * for itself.
-		*/
-		if (!bio_data_dir(bio))
+		if (bio_barrier(bio))
 			req->cmd_flags |= REQ_SOFTBARRIER;
 		req->q->prepare_discard_fn(req->q, req);
-	}
+	} else if (unlikely(bio_barrier(bio)))
+		req->cmd_flags |= (REQ_HARDBARRIER | REQ_NOMERGE);
 
 	if (bio_sync(bio))
 		req->cmd_flags |= REQ_RW_SYNC;
@@ -1119,7 +1113,8 @@ static int __make_request(struct request_queue *q, struct bio *bio)
 	blk_queue_bounce(q, &bio);
 
 	barrier = bio_barrier(bio);
-	if (unlikely(barrier) && (q->next_ordered == QUEUE_ORDERED_NONE)) {
+	if (unlikely(barrier) && bio_has_data(bio) &&
+	    (q->next_ordered == QUEUE_ORDERED_NONE)) {
 		err = -EOPNOTSUPP;
 		goto end_io;
 	}
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 1fdfc56..33c3947 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -188,8 +188,8 @@ struct bio {
 #define bio_failfast(bio)	((bio)->bi_rw & (1 << BIO_RW_FAILFAST))
 #define bio_rw_ahead(bio)	((bio)->bi_rw & (1 << BIO_RW_AHEAD))
 #define bio_rw_meta(bio)	((bio)->bi_rw & (1 << BIO_RW_META))
-#define bio_empty_barrier(bio)	(bio_barrier(bio) && !bio_has_data(bio))
 #define bio_discard(bio)	((bio)->bi_rw & (1 << BIO_RW_DISCARD))
+#define bio_empty_barrier(bio)	(bio_barrier(bio) && !bio_has_data(bio) && !bio_discard(bio))
 
 static inline unsigned int bio_cur_sectors(struct bio *bio)
 {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 5cfacb3..860689f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -87,8 +87,8 @@ extern int dir_notify_enable;
 #define WRITE_SYNC	(WRITE | (1 << BIO_RW_SYNC))
 #define SWRITE_SYNC	(SWRITE | (1 << BIO_RW_SYNC))
 #define WRITE_BARRIER	(WRITE | (1 << BIO_RW_BARRIER))
-#define DISCARD_NOBARRIER (WRITE | (1 << BIO_RW_DISCARD))
-#define DISCARD_BARRIER (1 << BIO_RW_DISCARD)
+#define DISCARD_NOBARRIER (1 << BIO_RW_DISCARD)
+#define DISCARD_BARRIER ((1 << BIO_RW_DISCARD) | (1 << BIO_RW_BARRIER))
 
 #define SEL_IN		1
 #define SEL_OUT		2

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