[PATCH] block: prevent merges of discard and write requests

[PATCH] block: prevent merges of discard and write requests

[Adrian Hunter]

Add logic to prevent two I/O requests being merged if
only one of them is a discard.  Ditto secure discard.

Without this fix, it is possible for write requests
to transform into discard requests.  For example:

  Submit bio 1 to discard 8 sectors from sector n
  Submit bio 2 to write 8 sectors from sector n + 16
  Submit bio 3 to write 8 sectors from sector n + 8

Bio 1 becomes request 1.  Bio 2 becomes request 2.
Bio 3 is merged with request 2, and then subsequently
request 2 is merged with request 1 resulting in just
one I/O request which discards all 24 sectors.

Signed-off-by: Adrian Hunter <adrian.hunter@nokia.com>
---
 block/blk-merge.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 6a72546..3d88c1f 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -375,6 +375,18 @@ static int attempt_merge(struct request_queue *q, struct request *req,
 	if (blk_rq_pos(req) + blk_rq_sectors(req) != blk_rq_pos(next))
 		return 0;
 
+	/*
+	 * Don't merge file system requests and discard requests
+	 */
+	if ((req->cmd_flags & REQ_DISCARD) != (next->cmd_flags & REQ_DISCARD))
+		return 0;
+
+	/*
+	 * Don't merge discard requests and secure discard requests
+	 */
+	if ((req->cmd_flags & REQ_SECURE) != (next->cmd_flags & REQ_SECURE))
+		return 0;
+
 	if (rq_data_dir(req) != rq_data_dir(next)
 	    || req->rq_disk != next->rq_disk
 	    || next->special)
-- 
1.7.0.4

Re: [PATCH] block: prevent merges of discard and write requests

[Jens Axboe]

On 2010-09-25 12:36, Adrian Hunter wrote:
> Add logic to prevent two I/O requests being merged if
> only one of them is a discard.  Ditto secure discard.
> 
> Without this fix, it is possible for write requests
> to transform into discard requests.  For example:
> 
>   Submit bio 1 to discard 8 sectors from sector n
>   Submit bio 2 to write 8 sectors from sector n + 16
>   Submit bio 3 to write 8 sectors from sector n + 8
> 
> Bio 1 becomes request 1.  Bio 2 becomes request 2.
> Bio 3 is merged with request 2, and then subsequently
> request 2 is merged with request 1 resulting in just
> one I/O request which discards all 24 sectors.

Wow, that's a disaster. We can now have requests in the
same direction and of the same type (fs), but not mergeable.

I would move the check up above the position calculations.
I will apply this and upstream it right away. Thanks a lot!

-- 
Jens Axboe

Re: [PATCH] block: prevent merges of discard and write requests

[Kyle McMartin]

On Sat, Sep 25, 2010 at 12:40:48PM +0200, Jens Axboe wrote:
> On 2010-09-25 12:36, Adrian Hunter wrote:
> > Add logic to prevent two I/O requests being merged if
> > only one of them is a discard.  Ditto secure discard.
> > 
> > Without this fix, it is possible for write requests
> > to transform into discard requests.  For example:
> > 
> >   Submit bio 1 to discard 8 sectors from sector n
> >   Submit bio 2 to write 8 sectors from sector n + 16
> >   Submit bio 3 to write 8 sectors from sector n + 8
> > 
> > Bio 1 becomes request 1.  Bio 2 becomes request 2.
> > Bio 3 is merged with request 2, and then subsequently
> > request 2 is merged with request 1 resulting in just
> > one I/O request which discards all 24 sectors.
> 
> Wow, that's a disaster. We can now have requests in the
> same direction and of the same type (fs), but not mergeable.
> 
> I would move the check up above the position calculations.
> I will apply this and upstream it right away. Thanks a lot!
> 

Jens, is this (the REQ_DISCARD hunk) required for stable as well? It
appears there's not much change relating to merging requests between
HEAD and v2.6.35, so I assume it is?

--Kyle

Re: [PATCH] block: prevent merges of discard and write requests

[Jens Axboe]

On 2010-09-27 12:30, Kyle McMartin wrote:
> On Sat, Sep 25, 2010 at 12:40:48PM +0200, Jens Axboe wrote:
>> On 2010-09-25 12:36, Adrian Hunter wrote:
>>> Add logic to prevent two I/O requests being merged if
>>> only one of them is a discard.  Ditto secure discard.
>>>
>>> Without this fix, it is possible for write requests
>>> to transform into discard requests.  For example:
>>>
>>>   Submit bio 1 to discard 8 sectors from sector n
>>>   Submit bio 2 to write 8 sectors from sector n + 16
>>>   Submit bio 3 to write 8 sectors from sector n + 8
>>>
>>> Bio 1 becomes request 1.  Bio 2 becomes request 2.
>>> Bio 3 is merged with request 2, and then subsequently
>>> request 2 is merged with request 1 resulting in just
>>> one I/O request which discards all 24 sectors.
>>
>> Wow, that's a disaster. We can now have requests in the
>> same direction and of the same type (fs), but not mergeable.
>>
>> I would move the check up above the position calculations.
>> I will apply this and upstream it right away. Thanks a lot!
>>
> 
> Jens, is this (the REQ_DISCARD hunk) required for stable as well? It
> appears there's not much change relating to merging requests between
> HEAD and v2.6.35, so I assume it is?

No, 2.6.35 and earlier is safe, it's only 2.6.36-rc that is
affected by this bug.

-- 
Jens Axboe

Re: [PATCH] block: prevent merges of discard and write requests

[Kyle McMartin]

On Mon, Sep 27, 2010 at 01:59:52PM +0900, Jens Axboe wrote:
> > Jens, is this (the REQ_DISCARD hunk) required for stable as well? It
> > appears there's not much change relating to merging requests between
> > HEAD and v2.6.35, so I assume it is?
> 
> No, 2.6.35 and earlier is safe, it's only 2.6.36-rc that is
> affected by this bug.
> 

Groovy! Thanks for the quick reply.

--Kyle

Re: [PATCH] block: prevent merges of discard and write requests

[Mike Snitzer]

On Mon, Sep 27, 2010 at 12:59 AM, Jens Axboe <axboe@kernel.dk> wrote:
> On 2010-09-27 12:30, Kyle McMartin wrote:
>> On Sat, Sep 25, 2010 at 12:40:48PM +0200, Jens Axboe wrote:
>>> On 2010-09-25 12:36, Adrian Hunter wrote:
>>>> Add logic to prevent two I/O requests being merged if
>>>> only one of them is a discard.  Ditto secure discard.
>>>>
>>>> Without this fix, it is possible for write requests
>>>> to transform into discard requests.  For example:
>>>>
>>>>   Submit bio 1 to discard 8 sectors from sector n
>>>>   Submit bio 2 to write 8 sectors from sector n + 16
>>>>   Submit bio 3 to write 8 sectors from sector n + 8
>>>>
>>>> Bio 1 becomes request 1.  Bio 2 becomes request 2.
>>>> Bio 3 is merged with request 2, and then subsequently
>>>> request 2 is merged with request 1 resulting in just
>>>> one I/O request which discards all 24 sectors.
>>>
>>> Wow, that's a disaster. We can now have requests in the
>>> same direction and of the same type (fs), but not mergeable.
>>>
>>> I would move the check up above the position calculations.
>>> I will apply this and upstream it right away. Thanks a lot!
>>>
>>
>> Jens, is this (the REQ_DISCARD hunk) required for stable as well? It
>> appears there's not much change relating to merging requests between
>> HEAD and v2.6.35, so I assume it is?
>
> No, 2.6.35 and earlier is safe, it's only 2.6.36-rc that is
> affected by this bug.

I'm not so sure... I think 2.6.35 is affected.  Jens, what do you hold
to be the regression point?

The initial discard merge implementation (commit e17fc0a1ccf88) always
set REQ_SOFTBARRIER for discards.  As such such non-ioctl
blkdev_issue_discard() discards were always !rq_mergeable().

As commit e17fc0a1ccf88 shared: it is always possible to send a
discard without the use of a barrier (but such callers were assumed to
be safe, e.g. mkfs.* discarding entire device via ioctl?).

But things really broke once we started playing games with barrier
flags associated with discards.  The regression point (relative to
discard merging) seems to have occurred when we got away from using
REQ_SOFTBARRIER with commit: fbbf055692aeb "block: fix DISCARD_BARRIER
requests".  Which was still committed to v2.6.35...

So long story short: it seems stable's 2.6.35.y needs this fix too.

Of course I could easily be missing something.. if I got any of this
wrong please correct me ;)

Thanks,
Mike

Re: [PATCH] block: prevent merges of discard and write requests

[Mike Snitzer]

On Mon, Sep 27, 2010 at 1:12 AM, Mike Snitzer <snitzer@redhat.com> wrote:
> On Mon, Sep 27, 2010 at 12:59 AM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 2010-09-27 12:30, Kyle McMartin wrote:
>>> On Sat, Sep 25, 2010 at 12:40:48PM +0200, Jens Axboe wrote:
>>>> On 2010-09-25 12:36, Adrian Hunter wrote:
>>>>> Add logic to prevent two I/O requests being merged if
>>>>> only one of them is a discard.  Ditto secure discard.
>>>>>
>>>>> Without this fix, it is possible for write requests
>>>>> to transform into discard requests.  For example:
>>>>>
>>>>>   Submit bio 1 to discard 8 sectors from sector n
>>>>>   Submit bio 2 to write 8 sectors from sector n + 16
>>>>>   Submit bio 3 to write 8 sectors from sector n + 8
>>>>>
>>>>> Bio 1 becomes request 1.  Bio 2 becomes request 2.
>>>>> Bio 3 is merged with request 2, and then subsequently
>>>>> request 2 is merged with request 1 resulting in just
>>>>> one I/O request which discards all 24 sectors.
>>>>
>>>> Wow, that's a disaster. We can now have requests in the
>>>> same direction and of the same type (fs), but not mergeable.
>>>>
>>>> I would move the check up above the position calculations.
>>>> I will apply this and upstream it right away. Thanks a lot!
>>>>
>>>
>>> Jens, is this (the REQ_DISCARD hunk) required for stable as well? It
>>> appears there's not much change relating to merging requests between
>>> HEAD and v2.6.35, so I assume it is?
>>
>> No, 2.6.35 and earlier is safe, it's only 2.6.36-rc that is
>> affected by this bug.
>
> I'm not so sure... I think 2.6.35 is affected.  Jens, what do you hold
> to be the regression point?
...
> But things really broke once we started playing games with barrier
> flags associated with discards.  The regression point (relative to
> discard merging) seems to have occurred when we got away from using
> REQ_SOFTBARRIER with commit: fbbf055692aeb "block: fix DISCARD_BARRIER
> requests".  Which was still committed to v2.6.35...

OK I take that back, with commit fbbf055692aeb REQ_HARDBARRIER is used
for discards.. which is equally applicable to !rq_mergeable().

Anyway, I'd still like to understand what you feel is the regression point.

Mike

Re: [PATCH] block: prevent merges of discard and write requests

[Jens Axboe]

On 2010-09-27 14:26, Mike Snitzer wrote:
> On Mon, Sep 27, 2010 at 1:12 AM, Mike Snitzer <snitzer@redhat.com> wrote:
>> On Mon, Sep 27, 2010 at 12:59 AM, Jens Axboe <axboe@kernel.dk> wrote:
>>> On 2010-09-27 12:30, Kyle McMartin wrote:
>>>> On Sat, Sep 25, 2010 at 12:40:48PM +0200, Jens Axboe wrote:
>>>>> On 2010-09-25 12:36, Adrian Hunter wrote:
>>>>>> Add logic to prevent two I/O requests being merged if
>>>>>> only one of them is a discard.  Ditto secure discard.
>>>>>>
>>>>>> Without this fix, it is possible for write requests
>>>>>> to transform into discard requests.  For example:
>>>>>>
>>>>>>   Submit bio 1 to discard 8 sectors from sector n
>>>>>>   Submit bio 2 to write 8 sectors from sector n + 16
>>>>>>   Submit bio 3 to write 8 sectors from sector n + 8
>>>>>>
>>>>>> Bio 1 becomes request 1.  Bio 2 becomes request 2.
>>>>>> Bio 3 is merged with request 2, and then subsequently
>>>>>> request 2 is merged with request 1 resulting in just
>>>>>> one I/O request which discards all 24 sectors.
>>>>>
>>>>> Wow, that's a disaster. We can now have requests in the
>>>>> same direction and of the same type (fs), but not mergeable.
>>>>>
>>>>> I would move the check up above the position calculations.
>>>>> I will apply this and upstream it right away. Thanks a lot!
>>>>>
>>>>
>>>> Jens, is this (the REQ_DISCARD hunk) required for stable as well? It
>>>> appears there's not much change relating to merging requests between
>>>> HEAD and v2.6.35, so I assume it is?
>>>
>>> No, 2.6.35 and earlier is safe, it's only 2.6.36-rc that is
>>> affected by this bug.
>>
>> I'm not so sure... I think 2.6.35 is affected.  Jens, what do you hold
>> to be the regression point?
> ...
>> But things really broke once we started playing games with barrier
>> flags associated with discards.  The regression point (relative to
>> discard merging) seems to have occurred when we got away from using
>> REQ_SOFTBARRIER with commit: fbbf055692aeb "block: fix DISCARD_BARRIER
>> requests".  Which was still committed to v2.6.35...
> 
> OK I take that back, with commit fbbf055692aeb REQ_HARDBARRIER is used
> for discards.. which is equally applicable to !rq_mergeable().
> 
> Anyway, I'd still like to understand what you feel is the regression point.

Looking at the end result, 2.6.35 does look like it's affected for
request-to-request merges, if those were submitted outside of
blkdev_issue_discard() (all in-kernel users have BARRIER set). But we
can't rule out of-of-tree drivers, so I suppose we should submit
the patch to stable just to be on the safe side.

Thankfully it is a very rare condition. Request-to-request merges
are fairly uncommon, and the chance of an unrelated merge (most users do
waiting issues) is small. So it's affected in the same sense that .36-rc
is, I initially thought the problem was worse there.

-- 
Jens Axboe