block: fix merge of requests with different failfast settings

bio and request use the same set of failfast bits. This patch makes the following changes to simplify things.

* enumify BIO_RW* bits and reorder bits such that BIOS_RW_FAILFAST_* bits coincide with __REQ_FAILFAST_* bits.

* The above pushes BIO_RW_AHEAD out of sync with __REQ_FAILFAST_DEV but the matching is useless anyway. init_request_from_bio() is responsible for setting FAILFAST bits on FS requests and non-FS requests never use BIO_RW_AHEAD. Drop the code and comment from blk_rq_bio_prep().

* Define REQ_FAILFAST_MASK which is OR of all FAILFAST bits and simplify FAILFAST flags handling in init_request_from_bio().

Signed-off-by Tejun Heo <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-core.c       |   19 +++++++------------
 include/linux/bio.h    |   43 +++++++++++++++++++++++--------------------
 include/linux/blkdev.h |    4 ++++
 3 files changed, 34 insertions(+), 32 deletions(-)

block/blk-core.c +7 -12

index 8f4b9e0..cd3b265 100644
@@ -1119,17 +1119,13 @@ void init_request_from_bio(struct request *req, struct bio *bio)
 	req->cmd_type = REQ_TYPE_FS;
 
 	/*
-	 * inherit FAILFAST from bio (for read-ahead, and explicit FAILFAST)
+	 * Inherit FAILFAST from bio (for read-ahead, and explicit
+	 * FAILFAST).  FAILFAST flags are identical for req and bio.
 	 */
 	if (bio_rw_ahead(bio))
-		req->cmd_flags |= (REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
-				   REQ_FAILFAST_DRIVER);
-	if (bio_failfast_dev(bio))
-		req->cmd_flags |= REQ_FAILFAST_DEV;
-	if (bio_failfast_transport(bio))
-		req->cmd_flags |= REQ_FAILFAST_TRANSPORT;
-	if (bio_failfast_driver(bio))
-		req->cmd_flags |= REQ_FAILFAST_DRIVER;
+		req->cmd_flags |= REQ_FAILFAST_MASK;
+	else
+		req->cmd_flags |= bio->bi_rw & REQ_FAILFAST_MASK;
 
 	if (unlikely(bio_discard(bio))) {
 		req->cmd_flags |= REQ_DISCARD;

@@ -2247,9 +2243,8 @@ EXPORT_SYMBOL_GPL(__blk_end_request_cur);
 void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
 		     struct bio *bio)
 {
-	/* Bit 0 (R/W) is identical in rq->cmd_flags and bio->bi_rw, and
-	   we want BIO_RW_AHEAD (bit 1) to imply REQ_FAILFAST (bit 1). */
-	rq->cmd_flags |= (bio->bi_rw & 3);
+	/* Bit 0 (R/W) is identical in rq->cmd_flags and bio->bi_rw */
+	rq->cmd_flags |= bio->bi_rw & REQ_RW;
 
 	if (bio_has_data(bio)) {
 		rq->nr_phys_segments = bio_phys_segments(q, bio);

include/linux/bio.h +0 -0

index 2892b71..a299ed3 100644

include/linux/bio.h +11 -18

  *	Insert a serialization point in the IO queue, forcing previously
  *	submitted IO to be completed before this one is issued.
- * bit 3 -- synchronous I/O hint.
- * bit 4 -- Unplug the device immediately after submitting this bio.
- * bit 5 -- metadata request
+ * bit 6 -- synchronous I/O hint.
+ * bit 7 -- Unplug the device immediately after submitting this bio.
+ * bit 8 -- metadata request
  *	Used for tracing to differentiate metadata and data IO. May also
  *	get some preferential treatment in the IO scheduler
- * bit 6 -- discard sectors
+ * bit 9 -- discard sectors
  *	Informs the lower level device that this range of sectors is no longer
  *	used by the file system and may thus be freed by the device. Used
  *	for flash based storage.
- * bit 7 -- fail fast device errors
- * bit 8 -- fail fast transport errors
- * bit 9 -- fail fast driver errors
  *	Don't want driver retries for any fast fail whatever the reason.
  * bit 10 -- Tell the IO scheduler not to wait for more requests after this
 	one has been submitted, even if it is a SYNC request.
  */
-#define BIO_RW		0	/* Must match RW in req flags (blkdev.h) */
-#define BIO_RW_AHEAD	1	/* Must match FAILFAST in req flags */
-#define BIO_RW_BARRIER	2
-#define BIO_RW_SYNCIO	3
-#define BIO_RW_UNPLUG	4
-#define BIO_RW_META	5
-#define BIO_RW_DISCARD	6
-#define BIO_RW_FAILFAST_DEV		7
-#define BIO_RW_FAILFAST_TRANSPORT	8
-#define BIO_RW_FAILFAST_DRIVER		9
-#define BIO_RW_NOIDLE	10
+enum bio_rw_flags {
+	BIO_RW,
+	BIO_RW_FAILFAST_DEV,
+	BIO_RW_FAILFAST_TRANSPORT,
+	BIO_RW_FAILFAST_DRIVER,
+	/* above flags must match REQ_* */
+	BIO_RW_AHEAD,

include/linux/blkdev.h +1 -0

index 49ae079..a0e5ce1 100644
@@ -98,6 +98,7 @@ enum rq_flag_bits {
 	__REQ_FAILFAST_DEV,	/* no driver retries of device errors */
 	__REQ_FAILFAST_TRANSPORT, /* no driver retries of transport errors */
 	__REQ_FAILFAST_DRIVER,	/* no driver retries of driver errors */
+	/* above flags must match BIO_RW_* */
 	__REQ_DISCARD,		/* request to discard sectors */
 	__REQ_SORTED,		/* elevator knows about this request */
 	__REQ_SOFTBARRIER,	/* may not be passed by ioscheduler */

Thanks for doing this, it has been neglected for a long time. However, it will happen again, I don't like these implicit matches which are not enforced, They get to drift away. There are several ways to make sure two sets of enums stay in sync. (I'll have a try at it tomorrow. if you want).

include/linux/bio.h struct bio { · line 149

Quoted 11 lines

include/linux/bio.h +5 -2

@@ -142,37 +142,40 @@ struct bio {
  *
  * bit 0 -- data direction
  *	If not set, bio is a read from device. If set, it's a write to device.
- * bit 1 -- rw-ahead when set
- * bit 2 -- barrier
+ * bit 1 -- fail fast device errors
+ * bit 2 -- fail fast transport errors
+ * bit 3 -- fail fast driver errors
+ * bit 4 -- rw-ahead when set
+ * bit 5 -- barrier

Please kill all these evil bit 1, bit 2 ,bit n comments. The ways we invent to torture ourselfs...

Just move all the comments to the enums declarations below. And be done with it, also for the next time.

include/linux/bio.h struct bio { · line 181

Quoted 2 lines

include/linux/bio.h +0 -0

 #define bio_rw_flagged(bio, flag)	((bio)->bi_rw & (1 << (flag)))
 

I wish there was also an helper to set these bits. it gives me an heart attack every time I need to:

	bio->bi_rw &= ~(1 << BIO_RW);

Hello, Boaz.

Quoted 5 lines · Boaz Harrosh

Thanks for doing this, it has been neglected for a long time. However, it will happen again, I don't like these implicit matches which are not enforced, They get to drift away. There are several ways to make sure two sets of enums stay in sync. (I'll have a try at it tomorrow. if you want).

They don't share the exact same set of bits, so it's a bit blurry but yeah it would be better if the bits are defined in more systematic way.

Quoted 14 lines

include/linux/bio.h struct bio { · line 149

Quoted 11 lines

include/linux/bio.h +5 -2

@@ -142,37 +142,40 @@ struct bio {
  *
  * bit 0 -- data direction
  *	If not set, bio is a read from device. If set, it's a write to device.
- * bit 1 -- rw-ahead when set
- * bit 2 -- barrier
+ * bit 1 -- fail fast device errors
+ * bit 2 -- fail fast transport errors
+ * bit 3 -- fail fast driver errors
+ * bit 4 -- rw-ahead when set
+ * bit 5 -- barrier

Please kill all these evil bit 1, bit 2 ,bit n comments. The ways we invent to torture ourselfs...

Quoted 2 lines

Just move all the comments to the enums declarations below. And be done with it, also for the next time.

Heh... I agree too. Unless ABI is fixed, this type of comments are often painful. Care to submit a patch. This series is already in block#for-next.

Quoted 5 lines

include/linux/bio.h struct bio { · line 181

Quoted 2 lines

include/linux/bio.h +0 -0

 #define bio_rw_flagged(bio, flag)	((bio)->bi_rw & (1 << (flag)))
 

I wish there was also an helper to set these bits. it gives me an heart attack every time I need to:

	bio->bi_rw &= ~(1 << BIO_RW);

What's more disturbing to me is the different between RQ and BIO flags. __REQ_* are bit positions, REQ_* are masks while BIO_* are bit positions. Sadly it seems it's already too late to change that. I personally an not a big fan of simple accessors or flags defined as bit positions. They seem to obscure things without much benefit.

Thanks.

What's more disturbing to me is the different between RQ and BIO flags. __REQ_* are bit positions, REQ_* are masks while BIO_* are bit positions. Sadly it seems it's already too late to change that. I personally an not a big fan of simple accessors or flags defined as bit positions. They seem to obscure things without much benefit.

Quoted 5 lines · Tejun Heo

What's more disturbing to me is the different between RQ and BIO flags. __REQ_* are bit positions, REQ_* are masks while BIO_* are bit positions. Sadly it seems it's already too late to change that. I personally an not a big fan of simple accessors or flags defined as bit positions. They seem to obscure things without much benefit.

flags as bit positions generally only make sense if you use test/set/clear_bit, otherwise they just confuse things. And the accessors are pretty annoying, especially in the block layer. Trying to find the places where a BIO flag has an actual effect is pretty painful due to the mix of the different flags and the accessors.

Quoted 7 lines · Christoph Hellwig

Quoted 5 lines · Tejun Heo

What's more disturbing to me is the different between RQ and BIO flags. __REQ_* are bit positions, REQ_* are masks while BIO_* are bit positions. Sadly it seems it's already too late to change that. I personally an not a big fan of simple accessors or flags defined as bit positions. They seem to obscure things without much benefit.

Quoted 5 lines

flags as bit positions generally only make sense if you use test/set/clear_bit, otherwise they just confuse things. And the accessors are pretty annoying, especially in the block layer. Trying to find the places where a BIO flag has an actual effect is pretty painful due to the mix of the different flags and the accessors.

Indeed -- the accessors mean in practice that you always have at least _two_ things to grep for, just to catch all accesses. Block layer is pretty bad about that style of usage :/

Quoted 5 lines · Tejun Heo

What's more disturbing to me is the different between RQ and BIO flags. __REQ_* are bit positions, REQ_* are masks while BIO_* are bit positions. Sadly it seems it's already too late to change that. I personally an not a big fan of simple accessors or flags defined as bit positions. They seem to obscure things without much benefit.

flags as bit positions generally only make sense if you use test/set/clear_bit, otherwise they just confuse things.

And the accessors are pretty annoying, especially in the block layer. Trying to find the places where a BIO flag has an actual effect is pretty painful due to the mix of the different flags and the accessors.

Hello, Christoph.

Quoted 8 lines · Christoph Hellwig

Quoted 5 lines · Tejun Heo

What's more disturbing to me is the different between RQ and BIO flags. __REQ_* are bit positions, REQ_* are masks while BIO_* are bit positions. Sadly it seems it's already too late to change that. I personally an not a big fan of simple accessors or flags defined as bit positions. They seem to obscure things without much benefit.

flags as bit positions generally only make sense if you use test/set/clear_bit, otherwise they just confuse things.

Another shortcoming of bit position flags is masking / multi flag operations. It's just awful. I think it's always better to define flags as masks even when it's used with test/set/clear_bit(). If such usages are common enough, we can easily add test/set/clear_bit_mask(). The conversion from mask to bit would be constant most of the time and it's not like fls/ffs() are expensive.

Quoted 4 lines

And the accessors are pretty annoying, especially in the block layer. Trying to find the places where a BIO flag has an actual effect is pretty painful due to the mix of the different flags and the accessors.

Yeap, fully agreed.

Thanks.

Quoted 2 lines

Quoted 2 lines

flags as bit positions generally only make sense if you use test/set/clear_bit, otherwise they just confuse things.

first please make a distinction between test/set/clear_bit and test/__set/__clear_bit the former is not an option since it's not what we need.

What can be more clear then rq_set_flags(req, QUEUE_FLAG_QUEUED) then rq_clear_flags(req, QUEUE_FLAG_QUEUED) later.

That's why I suggested the set/clear_flags() variable size macro which can set/clear multiple bit-flags at same cost of masks, only that the compiler calculates the mask in compile time.

This can also be good for the greps above. .eg:
test_flags(&rq->cmd_flags, REQ_FAILFAST_DEV, REQ_FAILFAST_TRANSPORT, REQ_FAILFAST_DRIVER);

As said, yes, the the lower-case accessors for upper-case bits does nothing, but use __set/__clear/test is a different matter that can also replace the sugary need of these.

Failfast has characteristics from other attributes. When issuing, executing and successuflly completing requests, failfast doesn't make any difference. It only affects how a request is handled on failure. Allowing requests with different failfast settings to be merged cause normal IOs to fail prematurely while not allowing has performance penalties as failfast is used for read aheads which are likely to be located near in-flight or to-be-issued normal IOs.

This patch introduces the concept of 'mixed merge'. A request is a mixed merge if it is merge of segments which require different handling on failure. Currently the only mixable attributes are failfast ones (or lack thereof).

When a bio with different failfast settings is added to an existing request or requests of different failfast settings are merged, the merged request is marked mixed. Each bio carries failfast settings and the request always tracks failfast state of the first bio. When the request fails, blk_rq_err_bytes() can be used to determine how many bytes can be safely failed without crossing into an area which requires further retrials.

This allows request merging regardless of failfast settings while keeping the failure handling correct.

This patch only implements mixed merge but doesn't enable it. The next one will update SCSI to make use of mixed merge.

Signed-off-by Tejun Heo <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Niel Lambrechts <[email protected]>

---
 block/blk-core.c       |   99 ++++++++++++++++++++++++++++++++++++++++++++++++
 block/blk-merge.c      |   43 +++++++++++++++++++++
 block/blk.h            |    1 +
 include/linux/blkdev.h |   23 +++++++++--
 4 files changed, 161 insertions(+), 5 deletions(-)

block/blk-core.c +0 -0

index cd3b265..0214837 100644

block/blk-core.c +98 -0

 	int rw_flags;
 
 	if (bio_barrier(bio) && bio_has_data(bio) &&

@@ -1194,6 +1195,9 @@ static int __make_request(struct request_queue *q, struct bio *bio)
 
 		trace_block_bio_backmerge(q, bio);
 
+		if ((req->cmd_flags & REQ_FAILFAST_MASK) != ff)
+			blk_rq_set_mixed_merge(req);
+
 		req->biotail->bi_next = bio;
 		req->biotail = bio;
 		req->__data_len += bytes;

@@ -1213,6 +1217,12 @@ static int __make_request(struct request_queue *q, struct bio *bio)
 
 		trace_block_bio_frontmerge(q, bio);
 
+		if ((req->cmd_flags & REQ_FAILFAST_MASK) != ff) {
+			blk_rq_set_mixed_merge(req);
+			req->cmd_flags &= ~REQ_FAILFAST_MASK;
+			req->cmd_flags |= ff;
+		}
+
 		bio->bi_next = req->bio;
 		req->bio = bio;
 

@@ -1657,6 +1667,50 @@ int blk_insert_cloned_request(struct request_queue *q, struct request *rq)
 }
 EXPORT_SYMBOL_GPL(blk_insert_cloned_request);
 
+/**
+ * blk_rq_err_bytes - determine number of bytes till the next failure boundary
+ * @rq: request to examine
+ *
+ * Description:
+ *     A request could be merge of IOs which require different failure
+ *     handling.  This function determines the number of bytes which
+ *     can be failed from the beginning of the request without
+ *     crossing into area which need to be retried further.
+ *
+ * Return:
+ *     The number of bytes to fail.
+ *
+ * Context:
+ *     queue_lock must be held.
+ */
+unsigned int blk_rq_err_bytes(const struct request *rq)
+{
+	unsigned int ff = rq->cmd_flags & REQ_FAILFAST_MASK;
+	unsigned int bytes = 0;
+	struct bio *bio;
+
+	if (!(rq->cmd_flags & REQ_MIXED_MERGE))
+		return blk_rq_bytes(rq);
+
+	/*
+	 * Currently the only 'mixing' which can happen is between
+	 * different fastfail types.  We can safely fail portions
+	 * which have all the failfast bits that the first one has -
+	 * the ones which are at least as eager to fail as the first
+	 * one.
+	 */
+	for (bio = rq->bio; bio; bio = bio->bi_next) {
+		if ((bio->bi_rw & ff) != ff)
+			break;
+		bytes += bio->bi_size;
+	}
+
+	/* this could lead to infinite loop */
+	BUG_ON(blk_rq_bytes(rq) && !bytes);
+	return bytes;
+}
+EXPORT_SYMBOL_GPL(blk_rq_err_bytes);
+
 static void blk_account_io_completion(struct request *req, unsigned int bytes)
 {
 	if (blk_do_io_stat(req)) {

@@ -2003,6 +2057,12 @@ bool blk_update_request(struct request *req, int error, unsigned int nr_bytes)
 	if (blk_fs_request(req) || blk_discard_rq(req))
 		req->__sector += total_bytes >> 9;
 
+	/* mixed attributes always follow the first bio */
+	if (req->cmd_flags & REQ_MIXED_MERGE) {
+		req->cmd_flags &= ~REQ_FAILFAST_MASK;
+		req->cmd_flags |= req->bio->bi_rw & REQ_FAILFAST_MASK;
+	}
+
 	/*
 	 * If total number of sectors is less than the first segment
 	 * size, something has gone terribly wrong.

@@ -2182,6 +2242,25 @@ bool blk_end_request_cur(struct request *rq, int error)
 EXPORT_SYMBOL_GPL(blk_end_request_cur);
 
 /**
+ * blk_end_request_err - Finish a request till the next failure boundary.
+ * @rq: the request to finish till the next failure boundary for
+ * @error: must be negative errno
+ *
+ * Description:
+ *     Complete @rq till the next failure boundary.
+ *
+ * Return:
+ *     %false - we are done with this request
+ *     %true  - still buffers pending for this request
+ */
+bool blk_end_request_err(struct request *rq, int error)
+{
+	WARN_ON(error >= 0);
+	return blk_end_request(rq, error, blk_rq_err_bytes(rq));
+}
+EXPORT_SYMBOL_GPL(blk_end_request_err);
+
+/**
  * __blk_end_request - Helper function for drivers to complete the request.
  * @rq:       the request being processed
  * @error:    %0 for success, < %0 for error

@@ -2240,6 +2319,26 @@ bool __blk_end_request_cur(struct request *rq, int error)
 }
 EXPORT_SYMBOL_GPL(__blk_end_request_cur);
 
+/**
+ * __blk_end_request_err - Finish a request till the next failure boundary.
+ * @rq: the request to finish till the next failure boundary for
+ * @error: must be negative errno
+ *
+ * Description:
+ *     Complete @rq till the next failure boundary.  Must be called
+ *     with queue lock held.
+ *
+ * Return:
+ *     %false - we are done with this request
+ *     %true  - still buffers pending for this request
+ */
+bool __blk_end_request_err(struct request *rq, int error)
+{
+	WARN_ON(error >= 0);
+	return __blk_end_request(rq, error, blk_rq_err_bytes(rq));
+}
+EXPORT_SYMBOL_GPL(__blk_end_request_err);
+
 void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
 		     struct bio *bio)
 {

block/blk-merge.c +43 -0

index e199967..7c9ca01 100644
@@ -311,6 +311,36 @@ static int ll_merge_requests_fn(struct request_queue *q, struct request *req,
 	return 1;
 }
 
+/**
+ * blk_rq_set_mixed_merge - mark a request as mixed merge
+ * @rq: request to mark as mixed merge
+ *
+ * Description:
+ *     @rq is about to be mixed merged.  Make sure the attributes
+ *     which can be mixed are set in each bio and mark @rq as mixed
+ *     merged.
+ */
+void blk_rq_set_mixed_merge(struct request *rq)
+{
+	unsigned int ff = rq->cmd_flags & REQ_FAILFAST_MASK;
+	struct bio *bio;
+
+	if (rq->cmd_flags & REQ_MIXED_MERGE)
+		return;
+
+	/*
+	 * @rq will no longer represent mixable attributes for all the
+	 * contained bios.  It will just track those of the first one.
+	 * Distributes the attributs to each bio.
+	 */
+	for (bio = rq->bio; bio; bio = bio->bi_next) {
+		WARN_ON_ONCE((bio->bi_rw & REQ_FAILFAST_MASK) &&
+			     (bio->bi_rw & REQ_FAILFAST_MASK) != ff);
+		bio->bi_rw |= ff;
+	}
+	rq->cmd_flags |= REQ_MIXED_MERGE;
+}
+
 static void blk_account_io_merge(struct request *req)
 {
 	if (blk_do_io_stat(req)) {

@@ -366,6 +396,19 @@ static int attempt_merge(struct request_queue *q, struct request *req,
 		return 0;
 
 	/*
+	 * If failfast settings disagree or any of the two is already
+	 * a mixed merge, mark both as mixed before proceeding.  This
+	 * makes sure that all involved bios have mixable attributes
+	 * set properly.
+	 */
+	if ((req->cmd_flags | next->cmd_flags) & REQ_MIXED_MERGE ||
+	    (req->cmd_flags & REQ_FAILFAST_MASK) !=
+	    (next->cmd_flags & REQ_FAILFAST_MASK)) {
+		blk_rq_set_mixed_merge(req);
+		blk_rq_set_mixed_merge(next);
+	}
+
+	/*
 	 * At this point we have either done a back merge
 	 * or front merge. We need the smaller start_time of
 	 * the merged requests to be the current request

block/blk.h +1 -0

index 3fae6ad..5ee3d7e 100644
@@ -104,6 +104,7 @@ int ll_front_merge_fn(struct request_queue *q, struct request *req,
 int attempt_back_merge(struct request_queue *q, struct request *rq);
 int attempt_front_merge(struct request_queue *q, struct request *rq);
 void blk_recalc_rq_segments(struct request *rq);
+void blk_rq_set_mixed_merge(struct request *rq);
 
 void blk_queue_congestion_threshold(struct request_queue *q);
 

include/linux/blkdev.h +16 -5

index a0e5ce1..e58079f 100644
@@ -120,6 +120,7 @@ enum rq_flag_bits {
 	__REQ_INTEGRITY,	/* integrity metadata has been remapped */
 	__REQ_NOIDLE,		/* Don't anticipate more IO after this one */
 	__REQ_IO_STAT,		/* account I/O stat */
+	__REQ_MIXED_MERGE,	/* merge of different types, fail separately */
 	__REQ_NR_BITS,		/* stops here */
 };
 

@@ -148,6 +149,7 @@ enum rq_flag_bits {
 #define REQ_INTEGRITY	(1 << __REQ_INTEGRITY)
 #define REQ_NOIDLE	(1 << __REQ_NOIDLE)
 #define REQ_IO_STAT	(1 << __REQ_IO_STAT)
+#define REQ_MIXED_MERGE	(1 << __REQ_MIXED_MERGE)
 
 #define REQ_FAILFAST_MASK	(REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT | \
 				 REQ_FAILFAST_DRIVER)

@@ -836,11 +838,13 @@ static inline void blk_run_address_space(struct address_space *mapping)
 }
 
 /*
- * blk_rq_pos()		: the current sector
- * blk_rq_bytes()	: bytes left in the entire request
- * blk_rq_cur_bytes()	: bytes left in the current segment
- * blk_rq_sectors()	: sectors left in the entire request
- * blk_rq_cur_sectors()	: sectors left in the current segment
+ * blk_rq_pos()			: the current sector
+ * blk_rq_bytes()		: bytes left in the entire request
+ * blk_rq_cur_bytes()		: bytes left in the current segment
+ * blk_rq_err_bytes()		: bytes left till the next error boundary
+ * blk_rq_sectors()		: sectors left in the entire request
+ * blk_rq_cur_sectors()		: sectors left in the current segment
+ * blk_rq_err_sectors()		: sectors left till the next error boundary
  */
 static inline sector_t blk_rq_pos(const struct request *rq)
 {

@@ -857,6 +861,8 @@ static inline int blk_rq_cur_bytes(const struct request *rq)
 	return rq->bio ? bio_cur_bytes(rq->bio) : 0;
 }
 
+extern unsigned int blk_rq_err_bytes(const struct request *rq);
+
 static inline unsigned int blk_rq_sectors(const struct request *rq)
 {
 	return blk_rq_bytes(rq) >> 9;

@@ -867,6 +873,11 @@ static inline unsigned int blk_rq_cur_sectors(const struct request *rq)
 	return blk_rq_cur_bytes(rq) >> 9;
 }
 
+static inline unsigned int blk_rq_err_sectors(const struct request *rq)
+{
+	return blk_rq_err_bytes(rq) >> 9;
+}
+
 /*
  * Request issue related functions.
  */

@@ -893,10 +904,12 @@ extern bool blk_end_request(struct request *rq, int error,
 			    unsigned int nr_bytes);
 extern void blk_end_request_all(struct request *rq, int error);

block/blk-core.c static int __make_request(struct request_queue *q, struct bio *bio) · line 1168

Quoted 5 lines

block/blk-core.c +1 -0

@@ -1165,6 +1165,7 @@ static int __make_request(struct request_queue *q, struct bio *bio)
 	const unsigned short prio = bio_prio(bio);
 	const int sync = bio_sync(bio);
 	const int unplug = bio_unplug(bio);
+	const unsigned int ff = bio->bi_rw & REQ_FAILFAST_MASK;

Perhaps a bio_fail_fast(bio)
and also an blk_failfast(rq).

Hello,

Quoted 7 lines · Boaz Harrosh

block/blk-core.c static int __make_request(struct request_queue *q, struct bio *bio) · line 1168

Quoted 5 lines

block/blk-core.c +1 -0

@@ -1165,6 +1165,7 @@ static int __make_request(struct request_queue *q, struct bio *bio)
 	const unsigned short prio = bio_prio(bio);
 	const int sync = bio_sync(bio);
 	const int unplug = bio_unplug(bio);
+	const unsigned int ff = bio->bi_rw & REQ_FAILFAST_MASK;

Perhaps a bio_fail_fast(bio)
and also an blk_failfast(rq).

Me not being a big fan of those simple accessors, I want to avoid adding those especially the use of bio ones are mostly confined to block layer proper.

Thanks.

Quoted 10 lines · Tejun Heo

Hello,

Quoted 7 lines · Boaz Harrosh

block/blk-core.c static int __make_request(struct request_queue *q, struct bio *bio) · line 1168

Quoted 5 lines

block/blk-core.c +1 -0

@@ -1165,6 +1165,7 @@ static int __make_request(struct request_queue *q, struct bio *bio)
 	const unsigned short prio = bio_prio(bio);
 	const int sync = bio_sync(bio);
 	const int unplug = bio_unplug(bio);
+	const unsigned int ff = bio->bi_rw & REQ_FAILFAST_MASK;

Perhaps a bio_fail_fast(bio)
and also an blk_failfast(rq).

Quoted 4 lines

Me not being a big fan of those simple accessors, I want to avoid adding those especially the use of bio ones are mostly confined to block layer proper.

OK but at least take care of blk_noretry_request(), at the minimum kill it, and use req->cmd_flags & REQ_FAILFAST_MASK everywhere.

Hello,

Block layer didn't consider failfast status while merging requests and it led to premature failure of normal (non-failfast) IOs. Niel Lambrechts could trigger the problem semi-reliably on ext4 when resuming from STR. ext4 uses readahead when reading inodes and combined with the deterministic extra SATA PHY exception cycle during resume on the specific configuration, non-readahead inode read would fail causing ext4 errors. Please read the following thread for details.

http://lkml.org/lkml/2009/5/23/21

This patchset contains the following four patches to fix the problem.

0001-block-don-t-merge-requests-of-different-failfast-se.patch 0002-block-use-the-same-failfast-bits-for-bio-and-reques.patch 0003-block-implement-mixed-merge-of-different-failfast-r.patch 0004-scsi-block-update-SCSI-to-handle-mixed-merge-failur.patch

0001 disallows merge between requests with different failfast settings. This one is the quick fix and should go into 2.6.31 and later to -stable as the bug is pretty serious and may lead to data loss.

0002 preps for later changes.

0003-0004 implements and applies mixed merge. Requests of different failfast settings are merged as before but failure handling is updated such that parts which shouldn't fail without retrial are properly retried.

I spent quite some time thinking about and testing it but I'd really like more pairs of eyes on this patchset as dangerous bugs can go unnoticed for quite a while in this area (anyone knows when the failfast bug was introduced?).

Jens, I think the best way to merge this is to first push 0001 to Linus's tree and then pull it into for-next and then apply the rest on top of them.