elevator private data for REQ_FLUSH

Hello,

Commit
9d5a4e946ce5352f19400b6370f4cd8e72806278
block: skip elevator data initialization for flush requests

Skip elevator initialization for flush requests by passing priv=0 to blk_alloc_request() in get_request(). As such elv_set_request() is never called for flush requests.

introduced priv flag, to skip elevator_private data init for FLUSH requests. This, I guess, lead to NULL pointer deref on my machine in cfq_insert_request, which requires elevator_private to be set:

  1 [   78.982169] Call Trace:                                                                                                                                                                                                     
  2 [   78.982178]  [<ffffffff8122d1fe>] cfq_insert_request+0x4e/0x47d
  3 [   78.982184]  [<ffffffff8123e139>] ? do_raw_spin_lock+0x6b/0x122

Should we in that case use ELEVATOR_INSERT_FLUSH for REQ_FLUSH | REQ_FUA requests (like below)?

---

 block/elevator.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

block/elevator.c +0 -0

index c387d31..b17e577 100644

Quoted 26 lines · Sergey Senozhatsky

Hello,

Commit
9d5a4e946ce5352f19400b6370f4cd8e72806278
block: skip elevator data initialization for flush requests

Skip elevator initialization for flush requests by passing priv=0 to blk_alloc_request() in get_request(). As such elv_set_request() is never called for flush requests.

introduced priv flag, to skip elevator_private data init for FLUSH requests. This, I guess, lead to NULL pointer deref on my machine in cfq_insert_request, which requires elevator_private to be set:

  1 [   78.982169] Call Trace:                                                                                                                                                                                                     
  2 [   78.982178]  [<ffffffff8122d1fe>] cfq_insert_request+0x4e/0x47d
  3 [   78.982184]  [<ffffffff8123e139>] ? do_raw_spin_lock+0x6b/0x122

Should we in that case use ELEVATOR_INSERT_FLUSH for REQ_FLUSH | REQ_FUA requests (like below)?

---

 block/elevator.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

block/elevator.c void __elv_add_request(struct request_queue *q, struct request *rq, int where) · line 741

Quoted 13 lines

block/elevator.c +2 -0

index c387d31..b17e577 100644
@@ -734,6 +734,8 @@ void __elv_add_request(struct request_queue *q, struct request *rq, int where)
 			q->end_sector = rq_end_sector(rq);
 			q->boundary_rq = rq;
 		}
+	} else if (rq->cmd_flags & (REQ_FLUSH | REQ_FUA)) {
+		where = ELEVATOR_INSERT_FLUSH;
 	} else if (!(rq->cmd_flags & REQ_ELVPRIV) &&
 		    where == ELEVATOR_INSERT_SORT)
 		where = ELEVATOR_INSERT_BACK;

Thanks. That solves all (corruption-) problems that I reported earlier in an other thread.

Quoted 27 lines · Markus Trippelsdorf

Quoted 26 lines · Sergey Senozhatsky

Hello,

Commit
9d5a4e946ce5352f19400b6370f4cd8e72806278
block: skip elevator data initialization for flush requests

Skip elevator initialization for flush requests by passing priv=0 to blk_alloc_request() in get_request(). As such elv_set_request() is never called for flush requests.

introduced priv flag, to skip elevator_private data init for FLUSH requests. This, I guess, lead to NULL pointer deref on my machine in cfq_insert_request, which requires elevator_private to be set:

  1 [   78.982169] Call Trace:                                                                                                                                                                                                     
  2 [   78.982178]  [<ffffffff8122d1fe>] cfq_insert_request+0x4e/0x47d
  3 [   78.982184]  [<ffffffff8123e139>] ? do_raw_spin_lock+0x6b/0x122

Should we in that case use ELEVATOR_INSERT_FLUSH for REQ_FLUSH | REQ_FUA requests (like below)?

---

 block/elevator.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Quoted 16 lines

block/elevator.c void __elv_add_request(struct request_queue *q, struct request *rq, int where) · line 741

Quoted 13 lines

block/elevator.c +2 -0

index c387d31..b17e577 100644
@@ -734,6 +734,8 @@ void __elv_add_request(struct request_queue *q, struct request *rq, int where)
 			q->end_sector = rq_end_sector(rq);
 			q->boundary_rq = rq;
 		}
+	} else if (rq->cmd_flags & (REQ_FLUSH | REQ_FUA)) {
+		where = ELEVATOR_INSERT_FLUSH;
 	} else if (!(rq->cmd_flags & REQ_ELVPRIV) &&
 		    where == ELEVATOR_INSERT_SORT)
 		where = ELEVATOR_INSERT_BACK;

Thanks. That solves all (corruption-) problems that I reported earlier in an other thread.

So the flush-merge changes introduced ELEVATOR_INSERT_FLUSH (via commit ae1b1539). And the flush bio will now get ELEVATOR_INSERT_FLUSH in __make_request().

So it is interesting that the flush is getting inserted in the elevator at all. AFAIK that shouldn't be (and historically hasn't been) the case.

Combination of onstack plug changes?

Quoted 51 lines · Mike Snitzer

Quoted 27 lines · Markus Trippelsdorf

Quoted 26 lines · Sergey Senozhatsky

Hello,

Commit
9d5a4e946ce5352f19400b6370f4cd8e72806278
block: skip elevator data initialization for flush requests

Skip elevator initialization for flush requests by passing priv=0 to blk_alloc_request() in get_request(). As such elv_set_request() is never called for flush requests.

introduced priv flag, to skip elevator_private data init for FLUSH requests. This, I guess, lead to NULL pointer deref on my machine in cfq_insert_request, which requires elevator_private to be set:

  1 [   78.982169] Call Trace:                                                                                                                                                                                                     
  2 [   78.982178]  [<ffffffff8122d1fe>] cfq_insert_request+0x4e/0x47d
  3 [   78.982184]  [<ffffffff8123e139>] ? do_raw_spin_lock+0x6b/0x122

Should we in that case use ELEVATOR_INSERT_FLUSH for REQ_FLUSH | REQ_FUA requests (like below)?

---

 block/elevator.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Quoted 16 lines

block/elevator.c void __elv_add_request(struct request_queue *q, struct request *rq, int where) · line 741

Quoted 13 lines

block/elevator.c +2 -0

index c387d31..b17e577 100644
@@ -734,6 +734,8 @@ void __elv_add_request(struct request_queue *q, struct request *rq, int where)
 			q->end_sector = rq_end_sector(rq);
 			q->boundary_rq = rq;
 		}
+	} else if (rq->cmd_flags & (REQ_FLUSH | REQ_FUA)) {
+		where = ELEVATOR_INSERT_FLUSH;
 	} else if (!(rq->cmd_flags & REQ_ELVPRIV) &&
 		    where == ELEVATOR_INSERT_SORT)
 		where = ELEVATOR_INSERT_BACK;

Thanks. That solves all (corruption-) problems that I reported earlier in an other thread.

So the flush-merge changes introduced ELEVATOR_INSERT_FLUSH (via commit ae1b1539). And the flush bio will now get ELEVATOR_INSERT_FLUSH in __make_request().

Quoted 5 lines

So it is interesting that the flush is getting inserted in the elevator at all. AFAIK that shouldn't be (and historically hasn't been) the case.

Combination of onstack plug changes?

It is, it forces a sort insert. I'll fix this up, I'm relieved we have a good handle on this issue now.

Quoted 56 lines · Jens Axboe

Quoted 55 lines · Mike Snitzer

Quoted 27 lines · Markus Trippelsdorf

Quoted 26 lines · Sergey Senozhatsky

Hello,

Commit
9d5a4e946ce5352f19400b6370f4cd8e72806278
block: skip elevator data initialization for flush requests

Skip elevator initialization for flush requests by passing priv=0 to blk_alloc_request() in get_request(). As such elv_set_request() is never called for flush requests.

introduced priv flag, to skip elevator_private data init for FLUSH requests. This, I guess, lead to NULL pointer deref on my machine in cfq_insert_request, which requires elevator_private to be set:

  1 [   78.982169] Call Trace:                                                                                                                                                                                                     
  2 [   78.982178]  [<ffffffff8122d1fe>] cfq_insert_request+0x4e/0x47d
  3 [   78.982184]  [<ffffffff8123e139>] ? do_raw_spin_lock+0x6b/0x122

Should we in that case use ELEVATOR_INSERT_FLUSH for REQ_FLUSH | REQ_FUA requests (like below)?

---

 block/elevator.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Quoted 16 lines

block/elevator.c void __elv_add_request(struct request_queue *q, struct request *rq, int where) · line 741

Quoted 13 lines

block/elevator.c +2 -0

index c387d31..b17e577 100644
@@ -734,6 +734,8 @@ void __elv_add_request(struct request_queue *q, struct request *rq, int where)
 			q->end_sector = rq_end_sector(rq);
 			q->boundary_rq = rq;
 		}
+	} else if (rq->cmd_flags & (REQ_FLUSH | REQ_FUA)) {
+		where = ELEVATOR_INSERT_FLUSH;
 	} else if (!(rq->cmd_flags & REQ_ELVPRIV) &&
 		    where == ELEVATOR_INSERT_SORT)
 		where = ELEVATOR_INSERT_BACK;

Thanks. That solves all (corruption-) problems that I reported earlier in an other thread.

So the flush-merge changes introduced ELEVATOR_INSERT_FLUSH (via commit ae1b1539). And the flush bio will now get ELEVATOR_INSERT_FLUSH in __make_request().

So it is interesting that the flush is getting inserted in the elevator at all. AFAIK that shouldn't be (and historically hasn't been) the case.

Quoted 4 lines

Quoted 1 line

Combination of onstack plug changes?

It is, it forces a sort insert. I'll fix this up, I'm relieved we have a good handle on this issue now.

Should we also add a safety net to avoid the potential for future silent corruption, etc? E.g.:

Quoted 64 lines · Mike Snitzer

Quoted 60 lines · Jens Axboe

Quoted 51 lines · Mike Snitzer

Quoted 27 lines · Markus Trippelsdorf

Quoted 26 lines · Sergey Senozhatsky

Hello,

Commit
9d5a4e946ce5352f19400b6370f4cd8e72806278
block: skip elevator data initialization for flush requests

Skip elevator initialization for flush requests by passing priv=0 to blk_alloc_request() in get_request(). As such elv_set_request() is never called for flush requests.

introduced priv flag, to skip elevator_private data init for FLUSH requests. This, I guess, lead to NULL pointer deref on my machine in cfq_insert_request, which requires elevator_private to be set:

  1 [   78.982169] Call Trace:                                                                                                                                                                                                     
  2 [   78.982178]  [<ffffffff8122d1fe>] cfq_insert_request+0x4e/0x47d
  3 [   78.982184]  [<ffffffff8123e139>] ? do_raw_spin_lock+0x6b/0x122

Should we in that case use ELEVATOR_INSERT_FLUSH for REQ_FLUSH | REQ_FUA requests (like below)?

---

 block/elevator.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Quoted 16 lines

block/elevator.c void __elv_add_request(struct request_queue *q, struct request *rq, int where) · line 741

Quoted 13 lines

block/elevator.c +2 -0

index c387d31..b17e577 100644
@@ -734,6 +734,8 @@ void __elv_add_request(struct request_queue *q, struct request *rq, int where)
 			q->end_sector = rq_end_sector(rq);
 			q->boundary_rq = rq;
 		}
+	} else if (rq->cmd_flags & (REQ_FLUSH | REQ_FUA)) {
+		where = ELEVATOR_INSERT_FLUSH;
 	} else if (!(rq->cmd_flags & REQ_ELVPRIV) &&
 		    where == ELEVATOR_INSERT_SORT)
 		where = ELEVATOR_INSERT_BACK;

Thanks. That solves all (corruption-) problems that I reported earlier in an other thread.

So the flush-merge changes introduced ELEVATOR_INSERT_FLUSH (via commit ae1b1539). And the flush bio will now get ELEVATOR_INSERT_FLUSH in __make_request().

Quoted 5 lines

So it is interesting that the flush is getting inserted in the elevator at all. AFAIK that shouldn't be (and historically hasn't been) the case.

Combination of onstack plug changes?

It is, it forces a sort insert. I'll fix this up, I'm relieved we have a good handle on this issue now.

Quoted 2 lines

Should we also add a safety net to avoid the potential for future silent corruption, etc? E.g.:

Yes, I was thinking about something like that. I consider the patch merged an immediate stop gap, we need to improve this situation. It's not exactly pretty to have this sort of condition in both __make_request() and flush_plug_list(). Clearly it should be handled further down.

Quoted 5 lines

Yes, I was thinking about something like that. I consider the patch merged an immediate stop gap, we need to improve this situation. It's not exactly pretty to have this sort of condition in both __make_request() and flush_plug_list(). Clearly it should be handled further down.

OK, and btw my patch was too restrictive. blk_kick_flush() elv_insert()s a flush request with ELEVATOR_INSERT_REQUEUE.

Should blk_kick_flush() process the flush request without calling elv_insert() -- like is done with open coded list_add() in blk_insert_flush()?

Or should blk_insert_flush() use elv_insert() with ELEVATOR_INSERT_REQUEUE too?

Quoted 4 lines · Mike Snitzer

Should blk_kick_flush() process the flush request without calling elv_insert() -- like is done with open coded list_add() in blk_insert_flush()?

Quoted 2 lines

Or should blk_insert_flush() use elv_insert() with ELEVATOR_INSERT_REQUEUE too?

Hmmm... I would prefer the latter. Given that INSERT_REQUEUE and FRONT are no longer different, it would probably be better to use FRONT tho. The only reason REQUEUE is used there is to avoid kicking the queue from elv_insert(), which is gone now.

Quoted 5 lines · Tejun Heo

Quoted 4 lines · Mike Snitzer

Should blk_kick_flush() process the flush request without calling elv_insert() -- like is done with open coded list_add() in blk_insert_flush()?

Quoted 7 lines

Quoted 2 lines

Or should blk_insert_flush() use elv_insert() with ELEVATOR_INSERT_REQUEUE too?

Hmmm... I would prefer the latter. Given that INSERT_REQUEUE and FRONT are no longer different, it would probably be better to use FRONT tho. The only reason REQUEUE is used there is to avoid kicking the queue from elv_insert(), which is gone now.

OK, I came up with the following patch.

Jens, this is just a natural cleanup given the code that resulted from the flush-merge and onstack plugging changes coming together.

Quoted 16 lines · Mike Snitzer

Quoted 12 lines · Tejun Heo

Quoted 6 lines · Mike Snitzer

Should blk_kick_flush() process the flush request without calling elv_insert() -- like is done with open coded list_add() in blk_insert_flush()?

Or should blk_insert_flush() use elv_insert() with ELEVATOR_INSERT_REQUEUE too?

Hmmm... I would prefer the latter. Given that INSERT_REQUEUE and FRONT are no longer different, it would probably be better to use FRONT tho. The only reason REQUEUE is used there is to avoid kicking the queue from elv_insert(), which is gone now.

Quoted 4 lines

OK, I came up with the following patch.

Jens, this is just a natural cleanup given the code that resulted from the flush-merge and onstack plugging changes coming together.

That looks nice and clean. What kind of testing has been done?

Btw, is there any need to actually keep the elv_insert interface?

Itw has two callers in current mainline, two of them hardcode ELEVATOR_INSERT_REQUEUE as the reason and could opencode it, and the other cases could be merged into __elv_add_request.

Btw, is there any need to actually keep the elv_insert interface?

Itw has two callers in current mainline, two of them hardcode ELEVATOR_INSERT_REQUEUE as the reason and could opencode it, and the other cases could be merged into __elv_add_request.

OK, I came up with the following patch.

Jens, this is just a natural cleanup given the code that resulted from the flush-merge and onstack plugging changes coming together.

From: Mike Snitzer <[email protected]>
Subject: block: eliminate ELEVATOR_INSERT_REQUEUE

elv_insert() no longer has a need to differentiate between ELEVATOR_INSERT_REQUEUE and ELEVATOR_INSERT_FRONT. The onstack plugging changes eliminated the need to avoid unplugging the queue (via ELEVATOR_INSERT_REQUEUE).

Also, in blk_insert_flush(), use elv_insert() with ELEVATOR_INSERT_FRONT rather than open-coding the equivalent.

Quoted 14 lines · Mike Snitzer

OK, I came up with the following patch.

Jens, this is just a natural cleanup given the code that resulted from the flush-merge and onstack plugging changes coming together.

From: Mike Snitzer <[email protected]>
Subject: block: eliminate ELEVATOR_INSERT_REQUEUE

elv_insert() no longer has a need to differentiate between ELEVATOR_INSERT_REQUEUE and ELEVATOR_INSERT_FRONT. The onstack plugging changes eliminated the need to avoid unplugging the queue (via ELEVATOR_INSERT_REQUEUE).

Quoted 2 lines

Also, in blk_insert_flush(), use elv_insert() with ELEVATOR_INSERT_FRONT rather than open-coding the equivalent.

What you change by doing the call to elv_insert is that now the request will have REQ_SOFTBARRIER set. I don't think that affects anything, though (I checked). The rest looks pretty straight-forward.

What is the significance of REQ_SOFTBARRIER? Why it should be set for all INSERT_FRONT and requeue requests.

With flush logic we got rid of all hardbarriers and hence there is no notion of ordering as such. One has to wait for request to finish to enforce ordering.

Should we get rid of softbarriers too?

IIRC, at least in the requeue path, some drivers depend on front queueing for forward progress guarantee. Forward progress for prepping is guaranteed by mempool (or something like that) and when the request is retried, it should stay at the front of the queue; otherwise, prepping can stall with prepped requests stuck behind unprepped ones.

Quoted 6 lines · Tejun Heo

IIRC, at least in the requeue path, some drivers depend on front queueing for forward progress guarantee. Forward progress for prepping is guaranteed by mempool (or something like that) and when the request is retried, it should stay at the front of the queue; otherwise, prepping can stall with prepped requests stuck behind unprepped ones.

After writing the above, I think the current flush implementation actually is violating the above. I think the problem is over-use of front insertion. Flush can just append to the dispatch queue. Front insertion should only be used by requeueing, really.

Pure front insert should be used for requeue and internal commands (like spin up this drive, or get error information). Flush should append to the dispatch list.

Hello, Jens.

Quoted 3 lines · Jens Axboe

Pure front insert should be used for requeue and internal commands (like spin up this drive, or get error information). Flush should append to the dispatch list.

Yeah, right. The reason I used REQUEUE/FRONT was because BACK insertion involves draining the elevator and then appending the request at the end of the dispatch queue, which is unnecessary and inefficient. So, front insertion was a quick work around that. If we're removing elv_insert(), we can just append directly to the dispatch queue from flush code.

How does this look? It's really two patches, but rolled up into one for easier posting here.

block/blk-flush.c +3 -3

index 93d5fd8..6a16fea 100644
@@ -261,7 +261,7 @@ static bool blk_kick_flush(struct request_queue *q)
 	q->flush_rq.end_io = flush_end_io;
 
 	q->flush_pending_idx ^= 1;
-	elv_insert(q, &q->flush_rq, ELEVATOR_INSERT_REQUEUE);
+	__elv_add_request(q, &q->flush_rq, ELEVATOR_INSERT_FLUSH);
 	return true;
 }
 

@@ -281,7 +281,7 @@ static void flush_data_end_io(struct request *rq, int error)
  * blk_insert_flush - insert a new FLUSH/FUA request
  * @rq: request to insert
  *
- * To be called from elv_insert() for %ELEVATOR_INSERT_FLUSH insertions.
+ * To be called from __elv_add_request() for %ELEVATOR_INSERT_FLUSH insertions.
  * @rq is being submitted.  Analyze what needs to be done and put it on the
  * right queue.
  *

@@ -312,7 +312,7 @@ void blk_insert_flush(struct request *rq)
 	 */
 	if ((policy & REQ_FSEQ_DATA) &&
 	    !(policy & (REQ_FSEQ_PREFLUSH | REQ_FSEQ_POSTFLUSH))) {
-		list_add(&rq->queuelist, &q->queue_head);
+		list_add_tail(&rq->queuelist, &q->queue_head);
 		return;
 	}
 

block/elevator.c +15 -20

index c387d31..0cdb4e7 100644
@@ -610,7 +610,7 @@ void elv_requeue_request(struct request_queue *q, struct request *rq)
 
 	rq->cmd_flags &= ~REQ_STARTED;
 
-	elv_insert(q, rq, ELEVATOR_INSERT_REQUEUE);
+	__elv_add_request(q, rq, ELEVATOR_INSERT_REQUEUE);
 }
 
 void elv_drain_elevator(struct request_queue *q)

@@ -655,12 +655,25 @@ void elv_quiesce_end(struct request_queue *q)
 	queue_flag_clear(QUEUE_FLAG_ELVSWITCH, q);
 }
 
-void elv_insert(struct request_queue *q, struct request *rq, int where)
+void __elv_add_request(struct request_queue *q, struct request *rq, int where)
 {
 	trace_block_rq_insert(q, rq);
 
 	rq->q = q;
 
+	BUG_ON(rq->cmd_flags & REQ_ON_PLUG);
+
+	if (rq->cmd_flags & REQ_SOFTBARRIER) {
+		/* barriers are scheduling boundary, update end_sector */
+		if (rq->cmd_type == REQ_TYPE_FS ||
+		    (rq->cmd_flags & REQ_DISCARD)) {
+			q->end_sector = rq_end_sector(rq);
+			q->boundary_rq = rq;
+		}
+	} else if (!(rq->cmd_flags & REQ_ELVPRIV) &&
+		    where == ELEVATOR_INSERT_SORT)
+		where = ELEVATOR_INSERT_BACK;
+
 	switch (where) {
 	case ELEVATOR_INSERT_REQUEUE:
 	case ELEVATOR_INSERT_FRONT:

@@ -722,24 +735,6 @@ void elv_insert(struct request_queue *q, struct request *rq, int where)
 		BUG();
 	}
 }
-
-void __elv_add_request(struct request_queue *q, struct request *rq, int where)
-{
-	BUG_ON(rq->cmd_flags & REQ_ON_PLUG);
-
-	if (rq->cmd_flags & REQ_SOFTBARRIER) {
-		/* barriers are scheduling boundary, update end_sector */
-		if (rq->cmd_type == REQ_TYPE_FS ||
-		    (rq->cmd_flags & REQ_DISCARD)) {
-			q->end_sector = rq_end_sector(rq);
-			q->boundary_rq = rq;
-		}
-	} else if (!(rq->cmd_flags & REQ_ELVPRIV) &&
-		    where == ELEVATOR_INSERT_SORT)
-		where = ELEVATOR_INSERT_BACK;
-
-	elv_insert(q, rq, where);
-}
 EXPORT_SYMBOL(__elv_add_request);
 
 void elv_add_request(struct request_queue *q, struct request *rq, int where)

include/linux/elevator.h +0 -1

index d93efcc445..21a8ebf 100644
@@ -101,7 +101,6 @@ extern void elv_dispatch_sort(struct request_queue *, struct request *);
 extern void elv_dispatch_add_tail(struct request_queue *, struct request *);
 extern void elv_add_request(struct request_queue *, struct request *, int);
 extern void __elv_add_request(struct request_queue *, struct request *, int);
-extern void elv_insert(struct request_queue *, struct request *, int);
 extern int elv_merge(struct request_queue *, struct request **, struct bio *);
 extern int elv_try_merge(struct request *, struct bio *);
 extern void elv_merge_requests(struct request_queue *, struct request *,

Signature · 2 lines

-- 
Jens Axboe

Hello,

Quoted 3 lines · Jens Axboe

How does this look? It's really two patches, but rolled up into one for easier posting here.

Nope, doesn't work for me. fsck.ext4 crashed the system.

__elv_add_request
blk_flush_complete_seq
blk_insert_flush
__elv_add_request
__make_request
generic_make_request
?bio_alloc_bioset
blkdev_issue_flush
blkdev_fsync
vfs_fsync
[..]

Quoted 13 lines

Nope, doesn't work for me. fsck.ext4 crashed the system.

__elv_add_request
blk_flush_complete_seq
blk_insert_flush
__elv_add_request
__make_request
generic_make_request
?bio_alloc_bioset
blkdev_issue_flush
blkdev_fsync
vfs_fsync
[..]

Ah, this addon should behave better.

block/blk-flush.c +1 -1

index 6a16fea..eba4a27 100644
@@ -261,7 +261,7 @@ static bool blk_kick_flush(struct request_queue *q)
 	q->flush_rq.end_io = flush_end_io;
 
 	q->flush_pending_idx ^= 1;
-	__elv_add_request(q, &q->flush_rq, ELEVATOR_INSERT_FLUSH);
+	list_add_tail(&q->flush_rq.queuelist, &q->queue_head);
 	return true;
 }
 

Signature · 2 lines

-- 
Jens Axboe

Hello, Jens.

Quoted 3 lines · Jens Axboe

block/blk-flush.c +0 -0

@@ -261,7 +261,7 @@ static bool blk_kick_flush(struct request_queue *q)
 	q->flush_rq.end_io = flush_end_io;
 

block/blk-flush.c static bool blk_kick_flush(struct request_queue *q) · line 264

Quoted 3 lines

block/blk-flush.c +1 -1

 	q->flush_pending_idx ^= 1;
-	elv_insert(q, &q->flush_rq, ELEVATOR_INSERT_REQUEUE);
+	__elv_add_request(q, &q->flush_rq, ELEVATOR_INSERT_FLUSH);

Shouldn't this be list_add_tail(&rq->queuelist, &q->queue_head)? __elv_add_request(FLUSH) calls back into blk_insert_flush() which decomposes the flush request from FS into flush sequence. blk_kick_flush() is used to insert the decomposed sequence requests into the dispatch queue.

Hello, Jens.

Quoted 3 lines · Jens Axboe

Pure front insert should be used for requeue and internal commands (like spin up this drive, or get error information). Flush should append to the dispatch list.

Yeah, right. The reason I used REQUEUE/FRONT was because BACK insertion involves draining the elevator and then appending the request at the end of the dispatch queue, which is unnecessary and inefficient. So, front insertion was a quick work around that. If we're removing elv_insert(), we can just append directly to the dispatch queue from flush code.

With ordering semantics gone, do we still need to drain the elevator before queuing flush at the end of request queue.

Hello, Jens.

Quoted 3 lines · Jens Axboe

Pure front insert should be used for requeue and internal commands (like spin up this drive, or get error information). Flush should append to the dispatch list.

Yeah, right. The reason I used REQUEUE/FRONT was because BACK insertion involves draining the elevator and then appending the request at the end of the dispatch queue, which is unnecessary and inefficient. So, front insertion was a quick work around that. If we're removing elv_insert(), we can just append directly to the dispatch queue from flush code.

Why can we all of a sudden append the flush to the dispatch queue _but_ not have any concern about queue draining? Seems that avoiding use of BACK, by using list_add_tail, is enabling that. Couldn't we have always done that?

The folding of elv_insert() into __elv_add_request() seems irrelevant.

Can we take a step back and be more explicit about the implications of having inserted the flush with REQUEUE/FRONT? Seems to me that having _not_ inserted the flush at the end of the dispatch queue is cause for potential corruption (preceding data hasn't been issued to the device yet).

And just to be clear: none of this is a concern for stable right? It is just the flush-merge code introduced for 2.6.39 that needs fixing?

Quoted 2 lines

And just to be clear: none of this is a concern for stable right? It is just the flush-merge code introduced for 2.6.39 that needs fixing?

I think 2.6.38 needs a -stable fix. It has the previous version of the new flush implementation and is using front insertion.

So wee need this as stable fix because FLUSH request can get ahead of REQUEUED requests and it can break some drivers?

Quoted 2 lines · Vivek Goyal

So wee need this as stable fix because FLUSH request can get ahead of REQUEUED requests and it can break some drivers?

Yes, I think so. All we need is just replacing elv_insert() calls in blk-flush.c with list_add_tail(). Something like the following. I'll test it and send a proper patch later. Thanks.

Quoted 7 lines · Tejun Heo

Quoted 2 lines · Vivek Goyal

So wee need this as stable fix because FLUSH request can get ahead of REQUEUED requests and it can break some drivers?

Yes, I think so. All we need is just replacing elv_insert() calls in blk-flush.c with list_add_tail(). Something like the following. I'll test it and send a proper patch later. Thanks.

I'd suggest I just mark the queued patch for stable.

Quoted 7 lines · Tejun Heo

Quoted 2 lines · Vivek Goyal

So wee need this as stable fix because FLUSH request can get ahead of REQUEUED requests and it can break some drivers?

Yes, I think so. All we need is just replacing elv_insert() calls in blk-flush.c with list_add_tail(). Something like the following. I'll test it and send a proper patch later. Thanks.

I'd suggest I just mark the queued patch for stable.

Quoted 5 lines · Jens Axboe

Quoted 4 lines · Tejun Heo

Quoted 2 lines · Vivek Goyal

So wee need this as stable fix because FLUSH request can get ahead of REQUEUED requests and it can break some drivers?

Quoted 5 lines

Quoted 3 lines

Yes, I think so. All we need is just replacing elv_insert() calls in blk-flush.c with list_add_tail(). Something like the following. I'll test it and send a proper patch later. Thanks.

I'd suggest I just mark the queued patch for stable.

Could you document the nuance in that patch, please?

Quoted 26 lines · Sergey Senozhatsky

Hello,

Commit
9d5a4e946ce5352f19400b6370f4cd8e72806278
block: skip elevator data initialization for flush requests

Skip elevator initialization for flush requests by passing priv=0 to blk_alloc_request() in get_request(). As such elv_set_request() is never called for flush requests.

introduced priv flag, to skip elevator_private data init for FLUSH requests. This, I guess, lead to NULL pointer deref on my machine in cfq_insert_request, which requires elevator_private to be set:

  1 [   78.982169] Call Trace:                                                                                                                                                                                                     
  2 [   78.982178]  [<ffffffff8122d1fe>] cfq_insert_request+0x4e/0x47d
  3 [   78.982184]  [<ffffffff8123e139>] ? do_raw_spin_lock+0x6b/0x122

Should we in that case use ELEVATOR_INSERT_FLUSH for REQ_FLUSH | REQ_FUA requests (like below)?

---

 block/elevator.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

block/elevator.c void __elv_add_request(struct request_queue *q, struct request *rq, int where) · line 741

Quoted 13 lines

block/elevator.c +2 -0

index c387d31..b17e577 100644
@@ -734,6 +734,8 @@ void __elv_add_request(struct request_queue *q, struct request *rq, int where)
 			q->end_sector = rq_end_sector(rq);
 			q->boundary_rq = rq;
 		}
+	} else if (rq->cmd_flags & (REQ_FLUSH | REQ_FUA)) {
+		where = ELEVATOR_INSERT_FLUSH;
 	} else if (!(rq->cmd_flags & REQ_ELVPRIV) &&
 		    where == ELEVATOR_INSERT_SORT)
 		where = ELEVATOR_INSERT_BACK;

Thanks. That solves all (corruption-) problems that I reported earlier in an other thread.

Quoted 27 lines · Markus Trippelsdorf

Quoted 26 lines · Sergey Senozhatsky

Hello,

Commit
9d5a4e946ce5352f19400b6370f4cd8e72806278
block: skip elevator data initialization for flush requests

Skip elevator initialization for flush requests by passing priv=0 to blk_alloc_request() in get_request(). As such elv_set_request() is never called for flush requests.

introduced priv flag, to skip elevator_private data init for FLUSH requests. This, I guess, lead to NULL pointer deref on my machine in cfq_insert_request, which requires elevator_private to be set:

  1 [   78.982169] Call Trace:                                                                                                                                                                                                     
  2 [   78.982178]  [<ffffffff8122d1fe>] cfq_insert_request+0x4e/0x47d
  3 [   78.982184]  [<ffffffff8123e139>] ? do_raw_spin_lock+0x6b/0x122

Should we in that case use ELEVATOR_INSERT_FLUSH for REQ_FLUSH | REQ_FUA requests (like below)?

---

 block/elevator.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Quoted 16 lines

block/elevator.c void __elv_add_request(struct request_queue *q, struct request *rq, int where) · line 741

Quoted 13 lines

block/elevator.c +2 -0

index c387d31..b17e577 100644
@@ -734,6 +734,8 @@ void __elv_add_request(struct request_queue *q, struct request *rq, int where)
 			q->end_sector = rq_end_sector(rq);
 			q->boundary_rq = rq;
 		}
+	} else if (rq->cmd_flags & (REQ_FLUSH | REQ_FUA)) {
+		where = ELEVATOR_INSERT_FLUSH;
 	} else if (!(rq->cmd_flags & REQ_ELVPRIV) &&
 		    where == ELEVATOR_INSERT_SORT)
 		where = ELEVATOR_INSERT_BACK;

Thanks. That solves all (corruption-) problems that I reported earlier in an other thread.

That's great. I'm surprised that this would cause silent corruption for you, should have been accompanied by an oops. Or was that with noop only?