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@xxxxxxxxxxxx> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/