Re: [PATCH] block: cleanup and fix batch completion adding conditions

[alan.adamson@oracle.com]

On 2/19/25 11:17 AM, alan.adamson@oracle.com wrote:
>
> On 2/18/25 11:26 PM, Shinichiro Kawasaki wrote:
>> CC+: Alan,
>>
>> On Feb 13, 2025 / 08:18, Jens Axboe wrote:
>>> The conditions for whether or not a request is allowed adding to a
>>> completion batch are a bit hard to read, and they also have a few
>>> issues. One is that ioerror may indeed be a random value on 
>>> passthrough,
>>> and it's being checked unconditionally of whether or not the given
>>> request is a passthrough request or not.
>>>
>>> Rewrite the conditions to be separate for easier reading, and only 
>>> check
>>> ioerror for non-passthrough requests. This fixes an issue with bio
>>> unmapping on passthrough, where it fails getting added to a batch. This
>>> both leads to suboptimal performance, and may trigger a potential
>>> schedule-under-atomic condition for polled passthrough IO.
>>>
>>> Fixes: f794f3351f26 ("block: add support for 
>>> blk_mq_end_request_batch()")
>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> I observed the blktests test case nvme/039 failure with v6.14-rc3 
>> kernel. I
>> bisected and found that this patch in the v6.14-rc3 is the trigger.
>>
>> The test run output is as follows:
>>
>> nvme/039 => nvme0n1 (test error logging) [failed]
>>      runtime  5.378s  ...  5.354s
>>      --- tests/nvme/039.out      2024-09-20 11:20:26.405380875 +0900
>>      +++ 
>> /home/shin/Blktests/blktests/results/nvme0n1/nvme/039.out.bad 
>> 2025-02-19 16:13:05.061387179 +0900
>>      @@ -1,7 +1,3 @@
>>       Running nvme/039
>>      - Read(0x2) @ LBA 0, 1 blocks, Unrecovered Read Error (sct 0x2 / 
>> sc 0x81) DNR
>>      - Read(0x2) @ LBA 0, 1 blocks, Unknown (sct 0x3 / sc 0x75) DNR
>>      - Write(0x1) @ LBA 0, 1 blocks, Write Fault (sct 0x2 / sc 0x80) DNR
>>        Identify(0x6), Access Denied (sct 0x2 / sc 0x86) DNR cdw10=0x1 
>> cdw11=0x0 cdw12=0x0 cdw13=0x0 cdw14=0x0 cdw15=0x0
>>      - Read(0x2), Invalid Command Opcode (sct 0x0 / sc 0x1) DNR 
>> cdw10=0x0 cdw11=0x0 cdw12=0x1 cdw13=0x0 cdw14=0x0 cdw15=0x0
>>       Test complete
>>
>>
>> The test case does error injection. Test method requires 
>> reconsideration to
>> adjust to this kernel change, probably. Help for fix will be 
>> appreciated.
>
> Not really an issue with the test, rather the error injector is 
> broken. I'll investigate.

The following change allows the test to pass.

- The check of (ioerror < 0) should be (ioerror != 0)

- passthroughs can also have ioerror set, so false needs to be returned. 
This needs to be resolved.


diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index fa2a76cc2f73..b2bd2ac40441 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -865,17 +865,17 @@ static inline bool blk_mq_add_to_batch(struct 
request *req,
          * 1) No batch container
          * 2) Has scheduler data attached
          * 3) Not a passthrough request and end_io set
-        * 4) Not a passthrough request and an ioerror
+        * 4) An ioerror
          */
         if (!iob)
                 return false;
         if (req->rq_flags & RQF_SCHED_TAGS)
                 return false;
+       if (ioerror)
+               return false;
         if (!blk_rq_is_passthrough(req)) {
                 if (req->end_io)
                         return false;
-               if (ioerror < 0)
-                       return false;
         }

Alan