[GIT PULL] One important block fix for 2.6.36-rc

[GIT PULL] One important block fix for 2.6.36-rc

[Jens Axboe]

Hi Linus,

Adrian Hunter just discovered that we may inadvertently merge writes
with discards, since discards are now of the same type as file system
requests. This is very problematic, as it may turn a write into a
discard or vice versa.

Please pull asap, thanks.

are available in the git repository at:
  git://git.kernel.dk/linux-2.6-block.git for-linus

Adrian Hunter (1):
      block: prevent merges of discard and write requests

 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 3b0cd42..eafc94f 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -362,6 +362,18 @@ static int attempt_merge(struct request_queue *q, struct request *req,
 		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;
+
+	/*
 	 * not contiguous
 	 */
 	if (blk_rq_pos(req) + blk_rq_sectors(req) != blk_rq_pos(next))

-- 
Jens Axboe

Re: [GIT PULL] One important block fix for 2.6.36-rc

[Linus Torvalds]

On Sat, Sep 25, 2010 at 3:45 AM, Jens Axboe <jaxboe@fusionio.com> wrote:
>
> Please pull asap, thanks.

Pulled, but I _really_ wish you started looking at cleanliness issues,
even with bug-reports.

That's a singularly stupid and unreadable way of testing "do those
flags match". It is quite possible that the compiler fixes up the
stupidity, but that doesn't make it much better.

Here's how things like this _should_ be tested:

   #define REQ_FLAGS_MUST_MATCH (REQ_SECURE | REQ_DISCARD)

   ...
   /* Check that flags match in the required bits */
   if ((req->cmd_flags ^ next->cmd_flags) & REQ_FLAGS_MUST_MATCH)
      return 0;
   ...

which is (a) smaller (b) easier and clearer to add flags as needed and
(c) actually more readable due to not having duplicated logic (not
just one if-statement, but one mask too).

And yes, I think you'd want to add a few flags there. Looking at the
REQ_xyz flags, I suspect most o them should really really match for
requests to be mergeable. Wouldn't it be better to think of it in
terms of "do we really allow different cmd_flags requests to merge"
than adding one bit ad-hoc at a time?

                           Linus

Re: [GIT PULL] One important block fix for 2.6.36-rc

[Jens Axboe]

On 2010-09-26 01:59, Linus Torvalds wrote:
> On Sat, Sep 25, 2010 at 3:45 AM, Jens Axboe <jaxboe@fusionio.com> wrote:
>>
>> Please pull asap, thanks.
> 
> Pulled, but I _really_ wish you started looking at cleanliness issues,
> even with bug-reports.
> 
> That's a singularly stupid and unreadable way of testing "do those
> flags match". It is quite possible that the compiler fixes up the
> stupidity, but that doesn't make it much better.
> 
> Here's how things like this _should_ be tested:
> 
>    #define REQ_FLAGS_MUST_MATCH (REQ_SECURE | REQ_DISCARD)
> 
>    ...
>    /* Check that flags match in the required bits */
>    if ((req->cmd_flags ^ next->cmd_flags) & REQ_FLAGS_MUST_MATCH)
>       return 0;
>    ...
> 
> which is (a) smaller (b) easier and clearer to add flags as needed and
> (c) actually more readable due to not having duplicated logic (not
> just one if-statement, but one mask too).

Completely agree, I will clean this up for the .37. Since we are very
late in the rc cycle, I prefer obvious patches greatly. Especially
for something like this, which could cause data corruption.

> And yes, I think you'd want to add a few flags there. Looking at the
> REQ_xyz flags, I suspect most o them should really really match for
> requests to be mergeable. Wouldn't it be better to think of it in
> terms of "do we really allow different cmd_flags requests to merge"
> than adding one bit ad-hoc at a time?

The above bits and the failfast bits at least should disallow merging,
and the direction bit as well. So yes, I think we should add an
explicit mask for this.

-- 
Jens Axboe