[PATCH] blk-mq: fix corruption with direct issue

[PATCH] blk-mq: fix corruption with direct issue

[Jens Axboe]

If we attempt a direct issue to a SCSI device, and it returns BUSY, then
we queue the request up normally. However, the SCSI layer may have
already setup SG tables etc for this particular command. If we later
merge with this request, then the old tables are no longer valid. Once
we issue the IO, we only read/write the original part of the request,
not the new state of it.

This causes data corruption, and is most often noticed with the file
system complaining about the just read data being invalid:

[  235.934465] EXT4-fs error (device sda1): ext4_iget:4831: inode #7142: comm dpkg-query: bad extra_isize 24937 (inode size 256)

because most of it is garbage...

This doesn't happen from the normal issue path, as we will simply defer
the request to the hardware queue dispatch list if we fail. Once it's on
the dispatch list, we never merge with it.

Fix this from the direct issue path by flagging the request as
REQ_NOMERGE so we don't change the size of it before issue.

See also:
  https://bugzilla.kernel.org/show_bug.cgi?id=201685

Fixes: 6ce3dd6eec1 ("blk-mq: issue directly if hw queue isn't busy in case of 'none'")
Signed-off-by: Jens Axboe <axboe@kernel.dk>

---

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 3f91c6e5b17a..d8f518c6ea38 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1715,6 +1715,15 @@ static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx,
 		break;
 	case BLK_STS_RESOURCE:
 	case BLK_STS_DEV_RESOURCE:
+		/*
+		 * If direct dispatch fails, we cannot allow any merging on
+		 * this IO. Drivers (like SCSI) may have set up permanent state
+		 * for this request, like SG tables and mappings, and if we
+		 * merge to it later on then we'll still only do IO to the
+		 * original part.
+		 */
+		rq->cmd_flags |= REQ_NOMERGE;
+
 		blk_mq_update_dispatch_busy(hctx, true);
 		__blk_mq_requeue_request(rq);
 		break;

-- 
Jens Axboe

Re: [PATCH] blk-mq: fix corruption with direct issue

[Ming Lei]

On Tue, Dec 04, 2018 at 03:47:46PM -0700, Jens Axboe wrote:
> If we attempt a direct issue to a SCSI device, and it returns BUSY, then
> we queue the request up normally. However, the SCSI layer may have
> already setup SG tables etc for this particular command. If we later
> merge with this request, then the old tables are no longer valid. Once
> we issue the IO, we only read/write the original part of the request,
> not the new state of it.
> 
> This causes data corruption, and is most often noticed with the file
> system complaining about the just read data being invalid:
> 
> [  235.934465] EXT4-fs error (device sda1): ext4_iget:4831: inode #7142: comm dpkg-query: bad extra_isize 24937 (inode size 256)
> 
> because most of it is garbage...
> 
> This doesn't happen from the normal issue path, as we will simply defer
> the request to the hardware queue dispatch list if we fail. Once it's on
> the dispatch list, we never merge with it.
> 
> Fix this from the direct issue path by flagging the request as
> REQ_NOMERGE so we don't change the size of it before issue.
> 
> See also:
>   https://bugzilla.kernel.org/show_bug.cgi?id=201685
> 
> Fixes: 6ce3dd6eec1 ("blk-mq: issue directly if hw queue isn't busy in case of 'none'")
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> ---
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 3f91c6e5b17a..d8f518c6ea38 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1715,6 +1715,15 @@ static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx,
>  		break;
>  	case BLK_STS_RESOURCE:
>  	case BLK_STS_DEV_RESOURCE:
> +		/*
> +		 * If direct dispatch fails, we cannot allow any merging on
> +		 * this IO. Drivers (like SCSI) may have set up permanent state
> +		 * for this request, like SG tables and mappings, and if we
> +		 * merge to it later on then we'll still only do IO to the
> +		 * original part.
> +		 */
> +		rq->cmd_flags |= REQ_NOMERGE;
> +
>  		blk_mq_update_dispatch_busy(hctx, true);
>  		__blk_mq_requeue_request(rq);
>  		break;
> 

Not sure it is enough to just mark it as NOMERGE, for example, driver
may have setup the .special_vec for discard, and NOMERGE may not prevent
request from entering elevator queue completely. Cause 'rq.rb_node' and
'rq.special_vec' share same space.

So how about inserting this request via blk_mq_request_bypass_insert()
in case that direct issue returns BUSY? Then it is invariant that
any request queued via .queue_rq() won't enter scheduler queue.

--

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 3f91c6e5b17a..4b2db0b2909e 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1764,7 +1764,7 @@ static blk_status_t __blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,
 	if (bypass_insert)
 		return BLK_STS_RESOURCE;
 
-	blk_mq_sched_insert_request(rq, false, run_queue, false);
+	blk_mq_request_bypass_insert(rq, run_queue);
 	return BLK_STS_OK;
 }
 
@@ -1780,7 +1780,7 @@ static void blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,
 
 	ret = __blk_mq_try_issue_directly(hctx, rq, cookie, false);
 	if (ret == BLK_STS_RESOURCE || ret == BLK_STS_DEV_RESOURCE)
-		blk_mq_sched_insert_request(rq, false, true, false);
+		blk_mq_request_bypass_insert(rq, true);
 	else if (ret != BLK_STS_OK)
 		blk_mq_end_request(rq, ret);
 


Thanks,
Ming

Re: [PATCH] blk-mq: fix corruption with direct issue

[Jens Axboe]

On 12/4/18 6:37 PM, Ming Lei wrote:
> On Tue, Dec 04, 2018 at 03:47:46PM -0700, Jens Axboe wrote:
>> If we attempt a direct issue to a SCSI device, and it returns BUSY, then
>> we queue the request up normally. However, the SCSI layer may have
>> already setup SG tables etc for this particular command. If we later
>> merge with this request, then the old tables are no longer valid. Once
>> we issue the IO, we only read/write the original part of the request,
>> not the new state of it.
>>
>> This causes data corruption, and is most often noticed with the file
>> system complaining about the just read data being invalid:
>>
>> [  235.934465] EXT4-fs error (device sda1): ext4_iget:4831: inode #7142: comm dpkg-query: bad extra_isize 24937 (inode size 256)
>>
>> because most of it is garbage...
>>
>> This doesn't happen from the normal issue path, as we will simply defer
>> the request to the hardware queue dispatch list if we fail. Once it's on
>> the dispatch list, we never merge with it.
>>
>> Fix this from the direct issue path by flagging the request as
>> REQ_NOMERGE so we don't change the size of it before issue.
>>
>> See also:
>>   https://bugzilla.kernel.org/show_bug.cgi?id=201685
>>
>> Fixes: 6ce3dd6eec1 ("blk-mq: issue directly if hw queue isn't busy in case of 'none'")
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>
>> ---
>>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 3f91c6e5b17a..d8f518c6ea38 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -1715,6 +1715,15 @@ static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx,
>>  		break;
>>  	case BLK_STS_RESOURCE:
>>  	case BLK_STS_DEV_RESOURCE:
>> +		/*
>> +		 * If direct dispatch fails, we cannot allow any merging on
>> +		 * this IO. Drivers (like SCSI) may have set up permanent state
>> +		 * for this request, like SG tables and mappings, and if we
>> +		 * merge to it later on then we'll still only do IO to the
>> +		 * original part.
>> +		 */
>> +		rq->cmd_flags |= REQ_NOMERGE;
>> +
>>  		blk_mq_update_dispatch_busy(hctx, true);
>>  		__blk_mq_requeue_request(rq);
>>  		break;
>>
> 
> Not sure it is enough to just mark it as NOMERGE, for example, driver
> may have setup the .special_vec for discard, and NOMERGE may not prevent
> request from entering elevator queue completely. Cause 'rq.rb_node' and
> 'rq.special_vec' share same space.

We should rather limit the scope of the direct dispatch instead. It
doesn't make sense to do for anything but read/write anyway.

> So how about inserting this request via blk_mq_request_bypass_insert()
> in case that direct issue returns BUSY? Then it is invariant that
> any request queued via .queue_rq() won't enter scheduler queue.

I did consider this, but I didn't want to experiment with exercising
a new path for an important bug fix. You do realize that your original
patch has been corrupting data for months? I think a little caution
is in order here.

-- 
Jens Axboe

Re: [PATCH] blk-mq: fix corruption with direct issue

[Jens Axboe]

On 12/4/18 7:16 PM, Jens Axboe wrote:
> On 12/4/18 6:37 PM, Ming Lei wrote:
>> On Tue, Dec 04, 2018 at 03:47:46PM -0700, Jens Axboe wrote:
>>> If we attempt a direct issue to a SCSI device, and it returns BUSY, then
>>> we queue the request up normally. However, the SCSI layer may have
>>> already setup SG tables etc for this particular command. If we later
>>> merge with this request, then the old tables are no longer valid. Once
>>> we issue the IO, we only read/write the original part of the request,
>>> not the new state of it.
>>>
>>> This causes data corruption, and is most often noticed with the file
>>> system complaining about the just read data being invalid:
>>>
>>> [  235.934465] EXT4-fs error (device sda1): ext4_iget:4831: inode #7142: comm dpkg-query: bad extra_isize 24937 (inode size 256)
>>>
>>> because most of it is garbage...
>>>
>>> This doesn't happen from the normal issue path, as we will simply defer
>>> the request to the hardware queue dispatch list if we fail. Once it's on
>>> the dispatch list, we never merge with it.
>>>
>>> Fix this from the direct issue path by flagging the request as
>>> REQ_NOMERGE so we don't change the size of it before issue.
>>>
>>> See also:
>>>   https://bugzilla.kernel.org/show_bug.cgi?id=201685
>>>
>>> Fixes: 6ce3dd6eec1 ("blk-mq: issue directly if hw queue isn't busy in case of 'none'")
>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>
>>> ---
>>>
>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>> index 3f91c6e5b17a..d8f518c6ea38 100644
>>> --- a/block/blk-mq.c
>>> +++ b/block/blk-mq.c
>>> @@ -1715,6 +1715,15 @@ static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx,
>>>  		break;
>>>  	case BLK_STS_RESOURCE:
>>>  	case BLK_STS_DEV_RESOURCE:
>>> +		/*
>>> +		 * If direct dispatch fails, we cannot allow any merging on
>>> +		 * this IO. Drivers (like SCSI) may have set up permanent state
>>> +		 * for this request, like SG tables and mappings, and if we
>>> +		 * merge to it later on then we'll still only do IO to the
>>> +		 * original part.
>>> +		 */
>>> +		rq->cmd_flags |= REQ_NOMERGE;
>>> +
>>>  		blk_mq_update_dispatch_busy(hctx, true);
>>>  		__blk_mq_requeue_request(rq);
>>>  		break;
>>>
>>
>> Not sure it is enough to just mark it as NOMERGE, for example, driver
>> may have setup the .special_vec for discard, and NOMERGE may not prevent
>> request from entering elevator queue completely. Cause 'rq.rb_node' and
>> 'rq.special_vec' share same space.
> 
> We should rather limit the scope of the direct dispatch instead. It
> doesn't make sense to do for anything but read/write anyway.
> 
>> So how about inserting this request via blk_mq_request_bypass_insert()
>> in case that direct issue returns BUSY? Then it is invariant that
>> any request queued via .queue_rq() won't enter scheduler queue.
> 
> I did consider this, but I didn't want to experiment with exercising
> a new path for an important bug fix. You do realize that your original
> patch has been corrupting data for months? I think a little caution
> is in order here.

Here's a further limiting version. And we seriously need to clean up the
direct issue paths, it's ridiculous.


diff --git a/block/blk-mq.c b/block/blk-mq.c
index 3f91c6e5b17a..3262d83b9e07 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1715,6 +1715,15 @@ static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx,
 		break;
 	case BLK_STS_RESOURCE:
 	case BLK_STS_DEV_RESOURCE:
+		/*
+		 * If direct dispatch fails, we cannot allow any merging on
+		 * this IO. Drivers (like SCSI) may have set up permanent state
+		 * for this request, like SG tables and mappings, and if we
+		 * merge to it later on then we'll still only do IO to the
+		 * original part.
+		 */
+		rq->cmd_flags |= REQ_NOMERGE;
+
 		blk_mq_update_dispatch_busy(hctx, true);
 		__blk_mq_requeue_request(rq);
 		break;
@@ -1727,6 +1736,18 @@ static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx,
 	return ret;
 }
 
+/*
+ * Don't allow direct dispatch of anything but regular reads/writes,
+ * as some of the other commands can potentially share request space
+ * with data we need for the IO scheduler. If we attempt a direct dispatch
+ * on those and fail, we can't safely add it to the scheduler afterwards
+ * without potentially overwriting data that the driver has already written.
+ */
+static bool blk_rq_can_direct_dispatch(struct request *rq)
+{
+	return req_op(rq) == REQ_OP_READ || req_op(rq) == REQ_OP_WRITE;
+}
+
 static blk_status_t __blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,
 						struct request *rq,
 						blk_qc_t *cookie,
@@ -1748,7 +1769,7 @@ static blk_status_t __blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,
 		goto insert;
 	}
 
-	if (q->elevator && !bypass_insert)
+	if (!blk_rq_can_direct_dispatch(rq) || (q->elevator && !bypass_insert))
 		goto insert;
 
 	if (!blk_mq_get_dispatch_budget(hctx))
@@ -1810,6 +1831,9 @@ void blk_mq_try_issue_list_directly(struct blk_mq_hw_ctx *hctx,
 		struct request *rq = list_first_entry(list, struct request,
 				queuelist);
 
+		if (!blk_rq_can_direct_dispatch(rq))
+			break;
+
 		list_del_init(&rq->queuelist);
 		ret = blk_mq_request_issue_directly(rq);
 		if (ret != BLK_STS_OK) {

-- 
Jens Axboe

Re: [PATCH] blk-mq: fix corruption with direct issue

[Ming Lei]

On Tue, Dec 04, 2018 at 07:16:11PM -0700, Jens Axboe wrote:
> On 12/4/18 6:37 PM, Ming Lei wrote:
> > On Tue, Dec 04, 2018 at 03:47:46PM -0700, Jens Axboe wrote:
> >> If we attempt a direct issue to a SCSI device, and it returns BUSY, then
> >> we queue the request up normally. However, the SCSI layer may have
> >> already setup SG tables etc for this particular command. If we later
> >> merge with this request, then the old tables are no longer valid. Once
> >> we issue the IO, we only read/write the original part of the request,
> >> not the new state of it.
> >>
> >> This causes data corruption, and is most often noticed with the file
> >> system complaining about the just read data being invalid:
> >>
> >> [  235.934465] EXT4-fs error (device sda1): ext4_iget:4831: inode #7142: comm dpkg-query: bad extra_isize 24937 (inode size 256)
> >>
> >> because most of it is garbage...
> >>
> >> This doesn't happen from the normal issue path, as we will simply defer
> >> the request to the hardware queue dispatch list if we fail. Once it's on
> >> the dispatch list, we never merge with it.
> >>
> >> Fix this from the direct issue path by flagging the request as
> >> REQ_NOMERGE so we don't change the size of it before issue.
> >>
> >> See also:
> >>   https://bugzilla.kernel.org/show_bug.cgi?id=201685
> >>
> >> Fixes: 6ce3dd6eec1 ("blk-mq: issue directly if hw queue isn't busy in case of 'none'")
> >> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> >>
> >> ---
> >>
> >> diff --git a/block/blk-mq.c b/block/blk-mq.c
> >> index 3f91c6e5b17a..d8f518c6ea38 100644
> >> --- a/block/blk-mq.c
> >> +++ b/block/blk-mq.c
> >> @@ -1715,6 +1715,15 @@ static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx,
> >>  		break;
> >>  	case BLK_STS_RESOURCE:
> >>  	case BLK_STS_DEV_RESOURCE:
> >> +		/*
> >> +		 * If direct dispatch fails, we cannot allow any merging on
> >> +		 * this IO. Drivers (like SCSI) may have set up permanent state
> >> +		 * for this request, like SG tables and mappings, and if we
> >> +		 * merge to it later on then we'll still only do IO to the
> >> +		 * original part.
> >> +		 */
> >> +		rq->cmd_flags |= REQ_NOMERGE;
> >> +
> >>  		blk_mq_update_dispatch_busy(hctx, true);
> >>  		__blk_mq_requeue_request(rq);
> >>  		break;
> >>
> > 
> > Not sure it is enough to just mark it as NOMERGE, for example, driver
> > may have setup the .special_vec for discard, and NOMERGE may not prevent
> > request from entering elevator queue completely. Cause 'rq.rb_node' and
> > 'rq.special_vec' share same space.
> 
> We should rather limit the scope of the direct dispatch instead. It
> doesn't make sense to do for anything but read/write anyway.

discard is kind of write, and it isn't treated very specially in make
request path, except for multi-range discard.

> 
> > So how about inserting this request via blk_mq_request_bypass_insert()
> > in case that direct issue returns BUSY? Then it is invariant that
> > any request queued via .queue_rq() won't enter scheduler queue.
> 
> I did consider this, but I didn't want to experiment with exercising
> a new path for an important bug fix. You do realize that your original
> patch has been corrupting data for months? I think a little caution
> is in order here.

But marking NOMERGE still may have a hole on re-insert discard request as
mentioned above.

Given we never allow to re-insert queued request to scheduler queue
except for 6ce3dd6eec1, I think it is the correct thing to do, and the
fix is simple too.

Thanks,
Ming

Re: [PATCH] blk-mq: fix corruption with direct issue

[Jens Axboe]

On 12/4/18 7:27 PM, Ming Lei wrote:
> On Tue, Dec 04, 2018 at 07:16:11PM -0700, Jens Axboe wrote:
>> On 12/4/18 6:37 PM, Ming Lei wrote:
>>> On Tue, Dec 04, 2018 at 03:47:46PM -0700, Jens Axboe wrote:
>>>> If we attempt a direct issue to a SCSI device, and it returns BUSY, then
>>>> we queue the request up normally. However, the SCSI layer may have
>>>> already setup SG tables etc for this particular command. If we later
>>>> merge with this request, then the old tables are no longer valid. Once
>>>> we issue the IO, we only read/write the original part of the request,
>>>> not the new state of it.
>>>>
>>>> This causes data corruption, and is most often noticed with the file
>>>> system complaining about the just read data being invalid:
>>>>
>>>> [  235.934465] EXT4-fs error (device sda1): ext4_iget:4831: inode #7142: comm dpkg-query: bad extra_isize 24937 (inode size 256)
>>>>
>>>> because most of it is garbage...
>>>>
>>>> This doesn't happen from the normal issue path, as we will simply defer
>>>> the request to the hardware queue dispatch list if we fail. Once it's on
>>>> the dispatch list, we never merge with it.
>>>>
>>>> Fix this from the direct issue path by flagging the request as
>>>> REQ_NOMERGE so we don't change the size of it before issue.
>>>>
>>>> See also:
>>>>   https://bugzilla.kernel.org/show_bug.cgi?id=201685
>>>>
>>>> Fixes: 6ce3dd6eec1 ("blk-mq: issue directly if hw queue isn't busy in case of 'none'")
>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>>
>>>> ---
>>>>
>>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>>> index 3f91c6e5b17a..d8f518c6ea38 100644
>>>> --- a/block/blk-mq.c
>>>> +++ b/block/blk-mq.c
>>>> @@ -1715,6 +1715,15 @@ static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx,
>>>>  		break;
>>>>  	case BLK_STS_RESOURCE:
>>>>  	case BLK_STS_DEV_RESOURCE:
>>>> +		/*
>>>> +		 * If direct dispatch fails, we cannot allow any merging on
>>>> +		 * this IO. Drivers (like SCSI) may have set up permanent state
>>>> +		 * for this request, like SG tables and mappings, and if we
>>>> +		 * merge to it later on then we'll still only do IO to the
>>>> +		 * original part.
>>>> +		 */
>>>> +		rq->cmd_flags |= REQ_NOMERGE;
>>>> +
>>>>  		blk_mq_update_dispatch_busy(hctx, true);
>>>>  		__blk_mq_requeue_request(rq);
>>>>  		break;
>>>>
>>>
>>> Not sure it is enough to just mark it as NOMERGE, for example, driver
>>> may have setup the .special_vec for discard, and NOMERGE may not prevent
>>> request from entering elevator queue completely. Cause 'rq.rb_node' and
>>> 'rq.special_vec' share same space.
>>
>> We should rather limit the scope of the direct dispatch instead. It
>> doesn't make sense to do for anything but read/write anyway.
> 
> discard is kind of write, and it isn't treated very specially in make
> request path, except for multi-range discard.

The point of direct dispatch is to reduce latencies for requests,
discards are so damn slow on ALL devices anyway that it doesn't make any
sense to try direct dispatch to begin with, regardless of whether it
possible or not.

>>> So how about inserting this request via blk_mq_request_bypass_insert()
>>> in case that direct issue returns BUSY? Then it is invariant that
>>> any request queued via .queue_rq() won't enter scheduler queue.
>>
>> I did consider this, but I didn't want to experiment with exercising
>> a new path for an important bug fix. You do realize that your original
>> patch has been corrupting data for months? I think a little caution
>> is in order here.
> 
> But marking NOMERGE still may have a hole on re-insert discard request as
> mentioned above.

What I said was further limit the scope of direct dispatch, which means
not allowing anything that isn't a read/write.

> Given we never allow to re-insert queued request to scheduler queue
> except for 6ce3dd6eec1, I think it is the correct thing to do, and the
> fix is simple too.

As I said, it's not the time to experiment. This issue has been there
since 4.19-rc1. The alternative is yanking both those patches, and then
looking at it later when the direct issue path has been cleaned up
first.

-- 
Jens Axboe

Re: [PATCH] blk-mq: fix corruption with direct issue

[Ming Lei]

On Tue, Dec 04, 2018 at 07:30:24PM -0700, Jens Axboe wrote:
> On 12/4/18 7:27 PM, Ming Lei wrote:
> > On Tue, Dec 04, 2018 at 07:16:11PM -0700, Jens Axboe wrote:
> >> On 12/4/18 6:37 PM, Ming Lei wrote:
> >>> On Tue, Dec 04, 2018 at 03:47:46PM -0700, Jens Axboe wrote:
> >>>> If we attempt a direct issue to a SCSI device, and it returns BUSY, then
> >>>> we queue the request up normally. However, the SCSI layer may have
> >>>> already setup SG tables etc for this particular command. If we later
> >>>> merge with this request, then the old tables are no longer valid. Once
> >>>> we issue the IO, we only read/write the original part of the request,
> >>>> not the new state of it.
> >>>>
> >>>> This causes data corruption, and is most often noticed with the file
> >>>> system complaining about the just read data being invalid:
> >>>>
> >>>> [  235.934465] EXT4-fs error (device sda1): ext4_iget:4831: inode #7142: comm dpkg-query: bad extra_isize 24937 (inode size 256)
> >>>>
> >>>> because most of it is garbage...
> >>>>
> >>>> This doesn't happen from the normal issue path, as we will simply defer
> >>>> the request to the hardware queue dispatch list if we fail. Once it's on
> >>>> the dispatch list, we never merge with it.
> >>>>
> >>>> Fix this from the direct issue path by flagging the request as
> >>>> REQ_NOMERGE so we don't change the size of it before issue.
> >>>>
> >>>> See also:
> >>>>   https://bugzilla.kernel.org/show_bug.cgi?id=201685
> >>>>
> >>>> Fixes: 6ce3dd6eec1 ("blk-mq: issue directly if hw queue isn't busy in case of 'none'")
> >>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> >>>>
> >>>> ---
> >>>>
> >>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
> >>>> index 3f91c6e5b17a..d8f518c6ea38 100644
> >>>> --- a/block/blk-mq.c
> >>>> +++ b/block/blk-mq.c
> >>>> @@ -1715,6 +1715,15 @@ static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx,
> >>>>  		break;
> >>>>  	case BLK_STS_RESOURCE:
> >>>>  	case BLK_STS_DEV_RESOURCE:
> >>>> +		/*
> >>>> +		 * If direct dispatch fails, we cannot allow any merging on
> >>>> +		 * this IO. Drivers (like SCSI) may have set up permanent state
> >>>> +		 * for this request, like SG tables and mappings, and if we
> >>>> +		 * merge to it later on then we'll still only do IO to the
> >>>> +		 * original part.
> >>>> +		 */
> >>>> +		rq->cmd_flags |= REQ_NOMERGE;
> >>>> +
> >>>>  		blk_mq_update_dispatch_busy(hctx, true);
> >>>>  		__blk_mq_requeue_request(rq);
> >>>>  		break;
> >>>>
> >>>
> >>> Not sure it is enough to just mark it as NOMERGE, for example, driver
> >>> may have setup the .special_vec for discard, and NOMERGE may not prevent
> >>> request from entering elevator queue completely. Cause 'rq.rb_node' and
> >>> 'rq.special_vec' share same space.
> >>
> >> We should rather limit the scope of the direct dispatch instead. It
> >> doesn't make sense to do for anything but read/write anyway.
> > 
> > discard is kind of write, and it isn't treated very specially in make
> > request path, except for multi-range discard.
> 
> The point of direct dispatch is to reduce latencies for requests,
> discards are so damn slow on ALL devices anyway that it doesn't make any
> sense to try direct dispatch to begin with, regardless of whether it
> possible or not.

SCSI MQ device may benefit from direct dispatch from reduced lock contention.

> 
> >>> So how about inserting this request via blk_mq_request_bypass_insert()
> >>> in case that direct issue returns BUSY? Then it is invariant that
> >>> any request queued via .queue_rq() won't enter scheduler queue.
> >>
> >> I did consider this, but I didn't want to experiment with exercising
> >> a new path for an important bug fix. You do realize that your original
> >> patch has been corrupting data for months? I think a little caution
> >> is in order here.
> > 
> > But marking NOMERGE still may have a hole on re-insert discard request as
> > mentioned above.
> 
> What I said was further limit the scope of direct dispatch, which means
> not allowing anything that isn't a read/write.

IMO, the conservative approach is to take the one used in legacy io
path, in which it is never allowed to re-insert queued request to
scheduler queue except for requeue, however RQF_DONTPREP is cleared
before requeuing request to scheduler.

> 
> > Given we never allow to re-insert queued request to scheduler queue
> > except for 6ce3dd6eec1, I think it is the correct thing to do, and the
> > fix is simple too.
> 
> As I said, it's not the time to experiment. This issue has been there
> since 4.19-rc1. The alternative is yanking both those patches, and then
> looking at it later when the direct issue path has been cleaned up
> first.

The issue should have been there from v4.1, especially after commit
f984df1f0f7 ("blk-mq: do limited block plug for multiple queue case"),
which is the 1st one to re-insert the queued request into scheduler
queue.


Thanks,
Ming

Re: [PATCH] blk-mq: fix corruption with direct issue

[Ming Lei]

On Wed, Dec 05, 2018 at 10:58:02AM +0800, Ming Lei wrote:
> On Tue, Dec 04, 2018 at 07:30:24PM -0700, Jens Axboe wrote:
> > On 12/4/18 7:27 PM, Ming Lei wrote:
> > > On Tue, Dec 04, 2018 at 07:16:11PM -0700, Jens Axboe wrote:
> > >> On 12/4/18 6:37 PM, Ming Lei wrote:
> > >>> On Tue, Dec 04, 2018 at 03:47:46PM -0700, Jens Axboe wrote:
> > >>>> If we attempt a direct issue to a SCSI device, and it returns BUSY, then
> > >>>> we queue the request up normally. However, the SCSI layer may have
> > >>>> already setup SG tables etc for this particular command. If we later
> > >>>> merge with this request, then the old tables are no longer valid. Once
> > >>>> we issue the IO, we only read/write the original part of the request,
> > >>>> not the new state of it.
> > >>>>
> > >>>> This causes data corruption, and is most often noticed with the file
> > >>>> system complaining about the just read data being invalid:
> > >>>>
> > >>>> [  235.934465] EXT4-fs error (device sda1): ext4_iget:4831: inode #7142: comm dpkg-query: bad extra_isize 24937 (inode size 256)
> > >>>>
> > >>>> because most of it is garbage...
> > >>>>
> > >>>> This doesn't happen from the normal issue path, as we will simply defer
> > >>>> the request to the hardware queue dispatch list if we fail. Once it's on
> > >>>> the dispatch list, we never merge with it.
> > >>>>
> > >>>> Fix this from the direct issue path by flagging the request as
> > >>>> REQ_NOMERGE so we don't change the size of it before issue.
> > >>>>
> > >>>> See also:
> > >>>>   https://bugzilla.kernel.org/show_bug.cgi?id=201685
> > >>>>
> > >>>> Fixes: 6ce3dd6eec1 ("blk-mq: issue directly if hw queue isn't busy in case of 'none'")
> > >>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> > >>>>
> > >>>> ---
> > >>>>
> > >>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
> > >>>> index 3f91c6e5b17a..d8f518c6ea38 100644
> > >>>> --- a/block/blk-mq.c
> > >>>> +++ b/block/blk-mq.c
> > >>>> @@ -1715,6 +1715,15 @@ static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx,
> > >>>>  		break;
> > >>>>  	case BLK_STS_RESOURCE:
> > >>>>  	case BLK_STS_DEV_RESOURCE:
> > >>>> +		/*
> > >>>> +		 * If direct dispatch fails, we cannot allow any merging on
> > >>>> +		 * this IO. Drivers (like SCSI) may have set up permanent state
> > >>>> +		 * for this request, like SG tables and mappings, and if we
> > >>>> +		 * merge to it later on then we'll still only do IO to the
> > >>>> +		 * original part.
> > >>>> +		 */
> > >>>> +		rq->cmd_flags |= REQ_NOMERGE;
> > >>>> +
> > >>>>  		blk_mq_update_dispatch_busy(hctx, true);
> > >>>>  		__blk_mq_requeue_request(rq);
> > >>>>  		break;
> > >>>>
> > >>>
> > >>> Not sure it is enough to just mark it as NOMERGE, for example, driver
> > >>> may have setup the .special_vec for discard, and NOMERGE may not prevent
> > >>> request from entering elevator queue completely. Cause 'rq.rb_node' and
> > >>> 'rq.special_vec' share same space.
> > >>
> > >> We should rather limit the scope of the direct dispatch instead. It
> > >> doesn't make sense to do for anything but read/write anyway.
> > > 
> > > discard is kind of write, and it isn't treated very specially in make
> > > request path, except for multi-range discard.
> > 
> > The point of direct dispatch is to reduce latencies for requests,
> > discards are so damn slow on ALL devices anyway that it doesn't make any
> > sense to try direct dispatch to begin with, regardless of whether it
> > possible or not.
> 
> SCSI MQ device may benefit from direct dispatch from reduced lock contention.
> 
> > 
> > >>> So how about inserting this request via blk_mq_request_bypass_insert()
> > >>> in case that direct issue returns BUSY? Then it is invariant that
> > >>> any request queued via .queue_rq() won't enter scheduler queue.
> > >>
> > >> I did consider this, but I didn't want to experiment with exercising
> > >> a new path for an important bug fix. You do realize that your original
> > >> patch has been corrupting data for months? I think a little caution
> > >> is in order here.
> > > 
> > > But marking NOMERGE still may have a hole on re-insert discard request as
> > > mentioned above.
> > 
> > What I said was further limit the scope of direct dispatch, which means
> > not allowing anything that isn't a read/write.
> 
> IMO, the conservative approach is to take the one used in legacy io
> path, in which it is never allowed to re-insert queued request to
> scheduler queue except for requeue, however RQF_DONTPREP is cleared
> before requeuing request to scheduler.
> 
> > 
> > > Given we never allow to re-insert queued request to scheduler queue
> > > except for 6ce3dd6eec1, I think it is the correct thing to do, and the
> > > fix is simple too.
> > 
> > As I said, it's not the time to experiment. This issue has been there
> > since 4.19-rc1. The alternative is yanking both those patches, and then
> > looking at it later when the direct issue path has been cleaned up
> > first.
> 
> The issue should have been there from v4.1, especially after commit
> f984df1f0f7 ("blk-mq: do limited block plug for multiple queue case"),
> which is the 1st one to re-insert the queued request into scheduler
> queue.

But at that time, there isn't io scheduler for MQ, so in theory the
issue should be there since v4.11, especially 945ffb60c11d ("mq-deadline:
add blk-mq adaptation of the deadline IO scheduler").

Thanks,
Ming

Re: [PATCH] blk-mq: fix corruption with direct issue

[Jens Axboe]

On 12/4/18 8:03 PM, Ming Lei wrote:
> On Wed, Dec 05, 2018 at 10:58:02AM +0800, Ming Lei wrote:
>> On Tue, Dec 04, 2018 at 07:30:24PM -0700, Jens Axboe wrote:
>>> On 12/4/18 7:27 PM, Ming Lei wrote:
>>>> On Tue, Dec 04, 2018 at 07:16:11PM -0700, Jens Axboe wrote:
>>>>> On 12/4/18 6:37 PM, Ming Lei wrote:
>>>>>> On Tue, Dec 04, 2018 at 03:47:46PM -0700, Jens Axboe wrote:
>>>>>>> If we attempt a direct issue to a SCSI device, and it returns BUSY, then
>>>>>>> we queue the request up normally. However, the SCSI layer may have
>>>>>>> already setup SG tables etc for this particular command. If we later
>>>>>>> merge with this request, then the old tables are no longer valid. Once
>>>>>>> we issue the IO, we only read/write the original part of the request,
>>>>>>> not the new state of it.
>>>>>>>
>>>>>>> This causes data corruption, and is most often noticed with the file
>>>>>>> system complaining about the just read data being invalid:
>>>>>>>
>>>>>>> [  235.934465] EXT4-fs error (device sda1): ext4_iget:4831: inode #7142: comm dpkg-query: bad extra_isize 24937 (inode size 256)
>>>>>>>
>>>>>>> because most of it is garbage...
>>>>>>>
>>>>>>> This doesn't happen from the normal issue path, as we will simply defer
>>>>>>> the request to the hardware queue dispatch list if we fail. Once it's on
>>>>>>> the dispatch list, we never merge with it.
>>>>>>>
>>>>>>> Fix this from the direct issue path by flagging the request as
>>>>>>> REQ_NOMERGE so we don't change the size of it before issue.
>>>>>>>
>>>>>>> See also:
>>>>>>>   https://bugzilla.kernel.org/show_bug.cgi?id=201685
>>>>>>>
>>>>>>> Fixes: 6ce3dd6eec1 ("blk-mq: issue directly if hw queue isn't busy in case of 'none'")
>>>>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>>>>>> index 3f91c6e5b17a..d8f518c6ea38 100644
>>>>>>> --- a/block/blk-mq.c
>>>>>>> +++ b/block/blk-mq.c
>>>>>>> @@ -1715,6 +1715,15 @@ static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx,
>>>>>>>  		break;
>>>>>>>  	case BLK_STS_RESOURCE:
>>>>>>>  	case BLK_STS_DEV_RESOURCE:
>>>>>>> +		/*
>>>>>>> +		 * If direct dispatch fails, we cannot allow any merging on
>>>>>>> +		 * this IO. Drivers (like SCSI) may have set up permanent state
>>>>>>> +		 * for this request, like SG tables and mappings, and if we
>>>>>>> +		 * merge to it later on then we'll still only do IO to the
>>>>>>> +		 * original part.
>>>>>>> +		 */
>>>>>>> +		rq->cmd_flags |= REQ_NOMERGE;
>>>>>>> +
>>>>>>>  		blk_mq_update_dispatch_busy(hctx, true);
>>>>>>>  		__blk_mq_requeue_request(rq);
>>>>>>>  		break;
>>>>>>>
>>>>>>
>>>>>> Not sure it is enough to just mark it as NOMERGE, for example, driver
>>>>>> may have setup the .special_vec for discard, and NOMERGE may not prevent
>>>>>> request from entering elevator queue completely. Cause 'rq.rb_node' and
>>>>>> 'rq.special_vec' share same space.
>>>>>
>>>>> We should rather limit the scope of the direct dispatch instead. It
>>>>> doesn't make sense to do for anything but read/write anyway.
>>>>
>>>> discard is kind of write, and it isn't treated very specially in make
>>>> request path, except for multi-range discard.
>>>
>>> The point of direct dispatch is to reduce latencies for requests,
>>> discards are so damn slow on ALL devices anyway that it doesn't make any
>>> sense to try direct dispatch to begin with, regardless of whether it
>>> possible or not.
>>
>> SCSI MQ device may benefit from direct dispatch from reduced lock contention.
>>
>>>
>>>>>> So how about inserting this request via blk_mq_request_bypass_insert()
>>>>>> in case that direct issue returns BUSY? Then it is invariant that
>>>>>> any request queued via .queue_rq() won't enter scheduler queue.
>>>>>
>>>>> I did consider this, but I didn't want to experiment with exercising
>>>>> a new path for an important bug fix. You do realize that your original
>>>>> patch has been corrupting data for months? I think a little caution
>>>>> is in order here.
>>>>
>>>> But marking NOMERGE still may have a hole on re-insert discard request as
>>>> mentioned above.
>>>
>>> What I said was further limit the scope of direct dispatch, which means
>>> not allowing anything that isn't a read/write.
>>
>> IMO, the conservative approach is to take the one used in legacy io
>> path, in which it is never allowed to re-insert queued request to
>> scheduler queue except for requeue, however RQF_DONTPREP is cleared
>> before requeuing request to scheduler.
>>
>>>
>>>> Given we never allow to re-insert queued request to scheduler queue
>>>> except for 6ce3dd6eec1, I think it is the correct thing to do, and the
>>>> fix is simple too.
>>>
>>> As I said, it's not the time to experiment. This issue has been there
>>> since 4.19-rc1. The alternative is yanking both those patches, and then
>>> looking at it later when the direct issue path has been cleaned up
>>> first.
>>
>> The issue should have been there from v4.1, especially after commit
>> f984df1f0f7 ("blk-mq: do limited block plug for multiple queue case"),
>> which is the 1st one to re-insert the queued request into scheduler
>> queue.
> 
> But at that time, there isn't io scheduler for MQ, so in theory the
> issue should be there since v4.11, especially 945ffb60c11d ("mq-deadline:
> add blk-mq adaptation of the deadline IO scheduler").

Ming, I'm getting really tired of this. As mentioned in the other email,
we're not having a theoretical or hypothetical debate here. The facts
are on the table, there's no point in trying to shift blame. We need
to deal with the current situation.

-- 
Jens Axboe

Re: [PATCH] blk-mq: fix corruption with direct issue

[Theodore Y. Ts'o]

On Wed, Dec 05, 2018 at 11:03:01AM +0800, Ming Lei wrote:
> 
> But at that time, there isn't io scheduler for MQ, so in theory the
> issue should be there since v4.11, especially 945ffb60c11d ("mq-deadline:
> add blk-mq adaptation of the deadline IO scheduler").

Hi Ming,

How were serious you about this issue being there (theoretically) an
issue since 4.11?  Can you talk about how it might get triggered, and
how we can test for it?  The reason why I ask is because we're trying
to track down a mysterious file system corruption problem on a 4.14.x
stable kernel.  The symptoms are *very* eerily similar to kernel
bugzilla #201685.

The problem is that the problem is super-rare --- roughly once a week
out of a popuation of about 2500 systems.  The workload is NFS
serving.  Unfortunately, the problem is since 4.14.63, we can no
longer disable blk-mq for the virtio-scsi driver, thanks to the commit
b5b6e8c8d3b4 ("scsi: virtio_scsi: fix IO hang caused by automatic irq
vector affinity") getting backported into 4.14.63 as commit
70b522f163bbb32.

We're considering reverting this patch in our 4.14 LTS kernel, and
seeing whether it makes the problem go away.  Is there any thing else
you might suggest?

Thanks,

						- Ted

P.S.  Unlike the repro's that users were seeing in #201685, we *did*
have an I/O scheduler enabled --- it was mq-deadline.  But right now,
given your comments, and the corruptions that we're seeing, I'm not
feeling very warm and fuzzy about block-mq.  :-( :-( :-(

Re: [PATCH] blk-mq: fix corruption with direct issue

[Jens Axboe]

On 12/6/18 7:46 PM, Theodore Y. Ts'o wrote:
> On Wed, Dec 05, 2018 at 11:03:01AM +0800, Ming Lei wrote:
>>
>> But at that time, there isn't io scheduler for MQ, so in theory the
>> issue should be there since v4.11, especially 945ffb60c11d ("mq-deadline:
>> add blk-mq adaptation of the deadline IO scheduler").
> 
> Hi Ming,
> 
> How were serious you about this issue being there (theoretically) an
> issue since 4.11?  Can you talk about how it might get triggered, and
> how we can test for it?  The reason why I ask is because we're trying
> to track down a mysterious file system corruption problem on a 4.14.x
> stable kernel.  The symptoms are *very* eerily similar to kernel
> bugzilla #201685.
> 
> The problem is that the problem is super-rare --- roughly once a week
> out of a popuation of about 2500 systems.  The workload is NFS
> serving.  Unfortunately, the problem is since 4.14.63, we can no
> longer disable blk-mq for the virtio-scsi driver, thanks to the commit
> b5b6e8c8d3b4 ("scsi: virtio_scsi: fix IO hang caused by automatic irq
> vector affinity") getting backported into 4.14.63 as commit
> 70b522f163bbb32.
> 
> We're considering reverting this patch in our 4.14 LTS kernel, and
> seeing whether it makes the problem go away.  Is there any thing else
> you might suggest?

We should just make SCSI do the right thing, which is to unprep if
it sees BUSY and prep next time again. Otherwise I fear the direct
dispatch isn't going to be super useful, if a failed direct dispatch
prevents future merging.

This would be a lot less error prone as well for other cases.

-- 
Jens Axboe

Re: [PATCH] blk-mq: fix corruption with direct issue

[Ming Lei]

On Thu, Dec 06, 2018 at 09:46:42PM -0500, Theodore Y. Ts'o wrote:
> On Wed, Dec 05, 2018 at 11:03:01AM +0800, Ming Lei wrote:
> > 
> > But at that time, there isn't io scheduler for MQ, so in theory the
> > issue should be there since v4.11, especially 945ffb60c11d ("mq-deadline:
> > add blk-mq adaptation of the deadline IO scheduler").
> 
> Hi Ming,
> 
> How were serious you about this issue being there (theoretically) an
> issue since 4.11?  Can you talk about how it might get triggered, and
> how we can test for it?  The reason why I ask is because we're trying
> to track down a mysterious file system corruption problem on a 4.14.x
> stable kernel.  The symptoms are *very* eerily similar to kernel
> bugzilla #201685.

Hi Theodore,

It is just a theory analysis.

blk_mq_try_issue_directly() is called in two branches of blk_mq_make_request(),
both are on real MQ disks.

IO merge can be done on none or real io schedulers, so in theory there might
be the risk from v4.1, but IO merge on sw queue didn't work for a bit long,
especially it was fixed by ab42f35d9cb5ac49b5a2.

As Jens mentioned in bugzilla, there are several conditions required
for triggering the issue:

- MQ device

- queue busy can be triggered. It is hard to trigger in NVMe PCI,
  but may be possible on NVMe FC. However, it can be quite easy to
  trigger on SCSI devices. We know there are some MQ SCSI HBA,
  qlogic FC, megaraid_sas.

- IO merge is enabled. 

I have setup scsi_debug in the following way:

modprobe scsi_debug dev_size_mb=4096 clustering=1 \
		max_luns=1 submit_queues=2 max_queue=2

- submit_queues=2 may set this disk as MQ
- max_queue=4 may trigger the queue busy condition easily

and run some write IO on ext4 over the disk: fio, kernel building,... for
some time, but still can't trigger the data corruption once.

I should have created more LUN, so that queue may be easier to become
busy, will do that soon.

> 
> The problem is that the problem is super-rare --- roughly once a week
> out of a popuation of about 2500 systems.  The workload is NFS
> serving.  Unfortunately, the problem is since 4.14.63, we can no
> longer disable blk-mq for the virtio-scsi driver, thanks to the commit
> b5b6e8c8d3b4 ("scsi: virtio_scsi: fix IO hang caused by automatic irq
> vector affinity") getting backported into 4.14.63 as commit
> 70b522f163bbb32.

virtio_scsi supports multi-queue mode, if that is enabled in your
setting, you may try single queue and see if difference can be made.

If multi-queue mode isn't enabled, your problem should be different with
this one. I remember multi-queue mode isn't enabled on virtio-scsi in GCE.

> We're considering reverting this patch in our 4.14 LTS kernel, and
> seeing whether it makes the problem go away.  Is there any thing else
> you might suggest?

IO hang is only triggered on machine with special CPU topo, it should be
fine to revert b5b6e8c8d3b4 on normal VM.

No other suggestions yet.

Thanks,
Ming

Re: [PATCH] blk-mq: fix corruption with direct issue

[Ming Lei]

On Fri, Dec 07, 2018 at 11:44:39AM +0800, Ming Lei wrote:
> On Thu, Dec 06, 2018 at 09:46:42PM -0500, Theodore Y. Ts'o wrote:
> > On Wed, Dec 05, 2018 at 11:03:01AM +0800, Ming Lei wrote:
> > > 
> > > But at that time, there isn't io scheduler for MQ, so in theory the
> > > issue should be there since v4.11, especially 945ffb60c11d ("mq-deadline:
> > > add blk-mq adaptation of the deadline IO scheduler").
> > 
> > Hi Ming,
> > 
> > How were serious you about this issue being there (theoretically) an
> > issue since 4.11?  Can you talk about how it might get triggered, and
> > how we can test for it?  The reason why I ask is because we're trying
> > to track down a mysterious file system corruption problem on a 4.14.x
> > stable kernel.  The symptoms are *very* eerily similar to kernel
> > bugzilla #201685.
> 
> Hi Theodore,
> 
> It is just a theory analysis.
> 
> blk_mq_try_issue_directly() is called in two branches of blk_mq_make_request(),
> both are on real MQ disks.
> 
> IO merge can be done on none or real io schedulers, so in theory there might
> be the risk from v4.1, but IO merge on sw queue didn't work for a bit long,
> especially it was fixed by ab42f35d9cb5ac49b5a2.
> 
> As Jens mentioned in bugzilla, there are several conditions required
> for triggering the issue:
> 
> - MQ device
> 
> - queue busy can be triggered. It is hard to trigger in NVMe PCI,
>   but may be possible on NVMe FC. However, it can be quite easy to
>   trigger on SCSI devices. We know there are some MQ SCSI HBA,
>   qlogic FC, megaraid_sas.
> 
> - IO merge is enabled. 
> 
> I have setup scsi_debug in the following way:
> 
> modprobe scsi_debug dev_size_mb=4096 clustering=1 \
> 		max_luns=1 submit_queues=2 max_queue=2
> 
> - submit_queues=2 may set this disk as MQ
> - max_queue=4 may trigger the queue busy condition easily
> 
> and run some write IO on ext4 over the disk: fio, kernel building,... for
> some time, but still can't trigger the data corruption once.
> 
> I should have created more LUN, so that queue may be easier to become
> busy, will do that soon.

Actually I should have used SDEBUG_OPT_HOST_BUSY to simulate the queue busy.

Thanks,
Ming

Re: [PATCH] blk-mq: fix corruption with direct issue

[Jens Axboe]

On 12/4/18 7:58 PM, Ming Lei wrote:
> On Tue, Dec 04, 2018 at 07:30:24PM -0700, Jens Axboe wrote:
>> On 12/4/18 7:27 PM, Ming Lei wrote:
>>> On Tue, Dec 04, 2018 at 07:16:11PM -0700, Jens Axboe wrote:
>>>> On 12/4/18 6:37 PM, Ming Lei wrote:
>>>>> On Tue, Dec 04, 2018 at 03:47:46PM -0700, Jens Axboe wrote:
>>>>>> If we attempt a direct issue to a SCSI device, and it returns BUSY, then
>>>>>> we queue the request up normally. However, the SCSI layer may have
>>>>>> already setup SG tables etc for this particular command. If we later
>>>>>> merge with this request, then the old tables are no longer valid. Once
>>>>>> we issue the IO, we only read/write the original part of the request,
>>>>>> not the new state of it.
>>>>>>
>>>>>> This causes data corruption, and is most often noticed with the file
>>>>>> system complaining about the just read data being invalid:
>>>>>>
>>>>>> [  235.934465] EXT4-fs error (device sda1): ext4_iget:4831: inode #7142: comm dpkg-query: bad extra_isize 24937 (inode size 256)
>>>>>>
>>>>>> because most of it is garbage...
>>>>>>
>>>>>> This doesn't happen from the normal issue path, as we will simply defer
>>>>>> the request to the hardware queue dispatch list if we fail. Once it's on
>>>>>> the dispatch list, we never merge with it.
>>>>>>
>>>>>> Fix this from the direct issue path by flagging the request as
>>>>>> REQ_NOMERGE so we don't change the size of it before issue.
>>>>>>
>>>>>> See also:
>>>>>>   https://bugzilla.kernel.org/show_bug.cgi?id=201685
>>>>>>
>>>>>> Fixes: 6ce3dd6eec1 ("blk-mq: issue directly if hw queue isn't busy in case of 'none'")
>>>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>>>>> index 3f91c6e5b17a..d8f518c6ea38 100644
>>>>>> --- a/block/blk-mq.c
>>>>>> +++ b/block/blk-mq.c
>>>>>> @@ -1715,6 +1715,15 @@ static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx,
>>>>>>  		break;
>>>>>>  	case BLK_STS_RESOURCE:
>>>>>>  	case BLK_STS_DEV_RESOURCE:
>>>>>> +		/*
>>>>>> +		 * If direct dispatch fails, we cannot allow any merging on
>>>>>> +		 * this IO. Drivers (like SCSI) may have set up permanent state
>>>>>> +		 * for this request, like SG tables and mappings, and if we
>>>>>> +		 * merge to it later on then we'll still only do IO to the
>>>>>> +		 * original part.
>>>>>> +		 */
>>>>>> +		rq->cmd_flags |= REQ_NOMERGE;
>>>>>> +
>>>>>>  		blk_mq_update_dispatch_busy(hctx, true);
>>>>>>  		__blk_mq_requeue_request(rq);
>>>>>>  		break;
>>>>>>
>>>>>
>>>>> Not sure it is enough to just mark it as NOMERGE, for example, driver
>>>>> may have setup the .special_vec for discard, and NOMERGE may not prevent
>>>>> request from entering elevator queue completely. Cause 'rq.rb_node' and
>>>>> 'rq.special_vec' share same space.
>>>>
>>>> We should rather limit the scope of the direct dispatch instead. It
>>>> doesn't make sense to do for anything but read/write anyway.
>>>
>>> discard is kind of write, and it isn't treated very specially in make
>>> request path, except for multi-range discard.
>>
>> The point of direct dispatch is to reduce latencies for requests,
>> discards are so damn slow on ALL devices anyway that it doesn't make any
>> sense to try direct dispatch to begin with, regardless of whether it
>> possible or not.
> 
> SCSI MQ device may benefit from direct dispatch from reduced lock contention.
> 
>>
>>>>> So how about inserting this request via blk_mq_request_bypass_insert()
>>>>> in case that direct issue returns BUSY? Then it is invariant that
>>>>> any request queued via .queue_rq() won't enter scheduler queue.
>>>>
>>>> I did consider this, but I didn't want to experiment with exercising
>>>> a new path for an important bug fix. You do realize that your original
>>>> patch has been corrupting data for months? I think a little caution
>>>> is in order here.
>>>
>>> But marking NOMERGE still may have a hole on re-insert discard request as
>>> mentioned above.
>>
>> What I said was further limit the scope of direct dispatch, which means
>> not allowing anything that isn't a read/write.
> 
> IMO, the conservative approach is to take the one used in legacy io
> path, in which it is never allowed to re-insert queued request to
> scheduler queue except for requeue, however RQF_DONTPREP is cleared
> before requeuing request to scheduler.

I don't agree. Whether you agree or not, I'm doing the simple fix for
4.20 and queueing it up for 4.21 as well. Then I hope Jianchao can
rework his direct dispatch cleanup patches for 4.21 on top of that,
and then we can experiment on top of that with other solutions.

>>> Given we never allow to re-insert queued request to scheduler queue
>>> except for 6ce3dd6eec1, I think it is the correct thing to do, and the
>>> fix is simple too.
>>
>> As I said, it's not the time to experiment. This issue has been there
>> since 4.19-rc1. The alternative is yanking both those patches, and then
>> looking at it later when the direct issue path has been cleaned up
>> first.
> 
> The issue should have been there from v4.1, especially after commit
> f984df1f0f7 ("blk-mq: do limited block plug for multiple queue case"),
> which is the 1st one to re-insert the queued request into scheduler
> queue.

You can hand wave all you want, fact is that people have been hitting
this left and right since 4.19 was released, and it was even bisected
down to the specific two direct issue patches you did. This isn't
some theoretical debate. You've seen the bugzilla, I suggest you go
read the whole thing.

-- 
Jens Axboe

Re: [PATCH] blk-mq: fix corruption with direct issue

[Guenter Roeck]

On Tue, Dec 04, 2018 at 03:47:46PM -0700, Jens Axboe wrote:
> If we attempt a direct issue to a SCSI device, and it returns BUSY, then
> we queue the request up normally. However, the SCSI layer may have
> already setup SG tables etc for this particular command. If we later
> merge with this request, then the old tables are no longer valid. Once
> we issue the IO, we only read/write the original part of the request,
> not the new state of it.
> 
> This causes data corruption, and is most often noticed with the file
> system complaining about the just read data being invalid:
> 
> [  235.934465] EXT4-fs error (device sda1): ext4_iget:4831: inode #7142: comm dpkg-query: bad extra_isize 24937 (inode size 256)
> 
> because most of it is garbage...
> 
> This doesn't happen from the normal issue path, as we will simply defer
> the request to the hardware queue dispatch list if we fail. Once it's on
> the dispatch list, we never merge with it.
> 
> Fix this from the direct issue path by flagging the request as
> REQ_NOMERGE so we don't change the size of it before issue.
> 
> See also:
>   https://bugzilla.kernel.org/show_bug.cgi?id=201685
> 
> Fixes: 6ce3dd6eec1 ("blk-mq: issue directly if hw queue isn't busy in case of 'none'")
> Signed-off-by: Jens Axboe <axboe@kernel.dk>

Tested-by: Guenter Roeck <linux@roeck-us.net>

... on two systems affected by the problem.

> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 3f91c6e5b17a..d8f518c6ea38 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1715,6 +1715,15 @@ static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx,
>  		break;
>  	case BLK_STS_RESOURCE:
>  	case BLK_STS_DEV_RESOURCE:
> +		/*
> +		 * If direct dispatch fails, we cannot allow any merging on
> +		 * this IO. Drivers (like SCSI) may have set up permanent state
> +		 * for this request, like SG tables and mappings, and if we
> +		 * merge to it later on then we'll still only do IO to the
> +		 * original part.
> +		 */
> +		rq->cmd_flags |= REQ_NOMERGE;
> +
>  		blk_mq_update_dispatch_busy(hctx, true);
>  		__blk_mq_requeue_request(rq);
>  		break;

Re: [PATCH] blk-mq: fix corruption with direct issue

[Jens Axboe]

On 12/4/18 6:38 PM, Guenter Roeck wrote:
> On Tue, Dec 04, 2018 at 03:47:46PM -0700, Jens Axboe wrote:
>> If we attempt a direct issue to a SCSI device, and it returns BUSY, then
>> we queue the request up normally. However, the SCSI layer may have
>> already setup SG tables etc for this particular command. If we later
>> merge with this request, then the old tables are no longer valid. Once
>> we issue the IO, we only read/write the original part of the request,
>> not the new state of it.
>>
>> This causes data corruption, and is most often noticed with the file
>> system complaining about the just read data being invalid:
>>
>> [  235.934465] EXT4-fs error (device sda1): ext4_iget:4831: inode #7142: comm dpkg-query: bad extra_isize 24937 (inode size 256)
>>
>> because most of it is garbage...
>>
>> This doesn't happen from the normal issue path, as we will simply defer
>> the request to the hardware queue dispatch list if we fail. Once it's on
>> the dispatch list, we never merge with it.
>>
>> Fix this from the direct issue path by flagging the request as
>> REQ_NOMERGE so we don't change the size of it before issue.
>>
>> See also:
>>   https://bugzilla.kernel.org/show_bug.cgi?id=201685
>>
>> Fixes: 6ce3dd6eec1 ("blk-mq: issue directly if hw queue isn't busy in case of 'none'")
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> Tested-by: Guenter Roeck <linux@roeck-us.net>
> 
> ... on two systems affected by the problem.

Thanks for testing! And for being persistent in reproducing and
providing clues for getting this nailed.

-- 
Jens Axboe

Re: [PATCH] blk-mq: fix corruption with direct issue

[Guenter Roeck]

On Tue, Dec 04, 2018 at 07:25:05PM -0700, Jens Axboe wrote:
> On 12/4/18 6:38 PM, Guenter Roeck wrote:
> > On Tue, Dec 04, 2018 at 03:47:46PM -0700, Jens Axboe wrote:
> >> If we attempt a direct issue to a SCSI device, and it returns BUSY, then
> >> we queue the request up normally. However, the SCSI layer may have
> >> already setup SG tables etc for this particular command. If we later
> >> merge with this request, then the old tables are no longer valid. Once
> >> we issue the IO, we only read/write the original part of the request,
> >> not the new state of it.
> >>
> >> This causes data corruption, and is most often noticed with the file
> >> system complaining about the just read data being invalid:
> >>
> >> [  235.934465] EXT4-fs error (device sda1): ext4_iget:4831: inode #7142: comm dpkg-query: bad extra_isize 24937 (inode size 256)
> >>
> >> because most of it is garbage...
> >>
> >> This doesn't happen from the normal issue path, as we will simply defer
> >> the request to the hardware queue dispatch list if we fail. Once it's on
> >> the dispatch list, we never merge with it.
> >>
> >> Fix this from the direct issue path by flagging the request as
> >> REQ_NOMERGE so we don't change the size of it before issue.
> >>
> >> See also:
> >>   https://bugzilla.kernel.org/show_bug.cgi?id=201685
> >>
> >> Fixes: 6ce3dd6eec1 ("blk-mq: issue directly if hw queue isn't busy in case of 'none'")
> >> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> > 
> > Tested-by: Guenter Roeck <linux@roeck-us.net>
> > 
> > ... on two systems affected by the problem.
> 
> Thanks for testing! And for being persistent in reproducing and
> providing clues for getting this nailed.
> 

My pleasure.

I see that there is some discussion about this patch.

Unfortunately, everyone running a 4.19 or later kernel is at serious
risk of data corruption. Given that, if this patch doesn't make it
upstream for one reason or another, would it be possible to at least
revert the two patches introducing the problem until this is sorted
out for good ? If this is not acceptable either, maybe mark blk-mq
as broken ? After all, it _is_ broken. This is even more true if it
turns out that a problem may exist since 4.1, as suggested in the
discussion.

Also, it seems to me that even with this problem fixed, blk-mq may not
be ready for primetime after all. With that in mind, maybe commit
d5038a13eca72 ("scsi: core: switch to scsi-mq by default") was a
bit premature. Should that be reverted ?

Thanks,
Guenter

Re: [PATCH] blk-mq: fix corruption with direct issue

[Jens Axboe]

On 12/5/18 10:55 AM, Guenter Roeck wrote:
> On Tue, Dec 04, 2018 at 07:25:05PM -0700, Jens Axboe wrote:
>> On 12/4/18 6:38 PM, Guenter Roeck wrote:
>>> On Tue, Dec 04, 2018 at 03:47:46PM -0700, Jens Axboe wrote:
>>>> If we attempt a direct issue to a SCSI device, and it returns BUSY, then
>>>> we queue the request up normally. However, the SCSI layer may have
>>>> already setup SG tables etc for this particular command. If we later
>>>> merge with this request, then the old tables are no longer valid. Once
>>>> we issue the IO, we only read/write the original part of the request,
>>>> not the new state of it.
>>>>
>>>> This causes data corruption, and is most often noticed with the file
>>>> system complaining about the just read data being invalid:
>>>>
>>>> [  235.934465] EXT4-fs error (device sda1): ext4_iget:4831: inode #7142: comm dpkg-query: bad extra_isize 24937 (inode size 256)
>>>>
>>>> because most of it is garbage...
>>>>
>>>> This doesn't happen from the normal issue path, as we will simply defer
>>>> the request to the hardware queue dispatch list if we fail. Once it's on
>>>> the dispatch list, we never merge with it.
>>>>
>>>> Fix this from the direct issue path by flagging the request as
>>>> REQ_NOMERGE so we don't change the size of it before issue.
>>>>
>>>> See also:
>>>>   https://bugzilla.kernel.org/show_bug.cgi?id=201685
>>>>
>>>> Fixes: 6ce3dd6eec1 ("blk-mq: issue directly if hw queue isn't busy in case of 'none'")
>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>
>>> Tested-by: Guenter Roeck <linux@roeck-us.net>
>>>
>>> ... on two systems affected by the problem.
>>
>> Thanks for testing! And for being persistent in reproducing and
>> providing clues for getting this nailed.
>>
> 
> My pleasure.
> 
> I see that there is some discussion about this patch.
> 
> Unfortunately, everyone running a 4.19 or later kernel is at serious
> risk of data corruption. Given that, if this patch doesn't make it
> upstream for one reason or another, would it be possible to at least
> revert the two patches introducing the problem until this is sorted
> out for good ? If this is not acceptable either, maybe mark blk-mq
> as broken ? After all, it _is_ broken. This is even more true if it
> turns out that a problem may exist since 4.1, as suggested in the
> discussion.

It is queued up, it'll go upstream later today.

> Also, it seems to me that even with this problem fixed, blk-mq may not
> be ready for primetime after all. With that in mind, maybe commit
> d5038a13eca72 ("scsi: core: switch to scsi-mq by default") was a
> bit premature. Should that be reverted ?

I have to strongly disagree with that, the timing is just unfortunate.
There are literally millions of machines running blk-mq/scsi-mq, and
this is the only hickup we've had. So I want to put this one to rest
once and for all, there's absolutely no reason not to continue with
what we've planned.

-- 
Jens Axboe

Re: [PATCH] blk-mq: fix corruption with direct issue

[Guenter Roeck]

On Wed, Dec 05, 2018 at 10:59:21AM -0700, Jens Axboe wrote:
[ ... ]
> 
> > Also, it seems to me that even with this problem fixed, blk-mq may not
> > be ready for primetime after all. With that in mind, maybe commit
> > d5038a13eca72 ("scsi: core: switch to scsi-mq by default") was a
> > bit premature. Should that be reverted ?
> 
> I have to strongly disagree with that, the timing is just unfortunate.
> There are literally millions of machines running blk-mq/scsi-mq, and
> this is the only hickup we've had. So I want to put this one to rest
> once and for all, there's absolutely no reason not to continue with
> what we've planned.
> 

Guess we have to agree to disagree. In my opinion, for infrastructure as
critical as this, a single hickup is one hickup too many. Not that I would
describe this as hickup in the first place; I would describe it as major
disaster. I also don't think the timing is unfortunate. If anything,
it proves my point.

Thanks,
Guenter

Re: [PATCH] blk-mq: fix corruption with direct issue

[Jens Axboe]

On 12/5/18 12:09 PM, Guenter Roeck wrote:
> On Wed, Dec 05, 2018 at 10:59:21AM -0700, Jens Axboe wrote:
> [ ... ]
>>
>>> Also, it seems to me that even with this problem fixed, blk-mq may not
>>> be ready for primetime after all. With that in mind, maybe commit
>>> d5038a13eca72 ("scsi: core: switch to scsi-mq by default") was a
>>> bit premature. Should that be reverted ?
>>
>> I have to strongly disagree with that, the timing is just unfortunate.
>> There are literally millions of machines running blk-mq/scsi-mq, and
>> this is the only hickup we've had. So I want to put this one to rest
>> once and for all, there's absolutely no reason not to continue with
>> what we've planned.
>>
> 
> Guess we have to agree to disagree. In my opinion, for infrastructure
> as critical as this, a single hickup is one hickup too many. Not that
> I would describe this as hickup in the first place; I would describe
> it as major disaster.

Don't get me wrong, I don't mean to use hickup in a diminishing fashion,
this was by all means a disaster for the ones hit by it. But if you look
at the scope of how many folks are using blk-mq/scsi-mq and have been
for years, we're really talking about a tiny tiny percentage here. This
could just as easily have happened with the old IO stack. The bug was a
freak accident, and even with full knowledge of why it happened, I'm
still having an extraordinarily hard time triggering it at will on my
test boxes. As with any disaster, it's usually a combination of multiple
things that go wrong, and this one is no different. The folks that hit
this generally hit it pretty easily, and (by far) the majority would
never hit it.

Bugs happen, whether you like it or not. They happen in file systems,
memory management, and they happen in storage. Things are continually
developed, and that sometimes introduces bugs. We do our best to ensure
that doesn't happen, but sometimes freak accidents like this happen. I
think my track record of decades of work speaks for itself there, it's
not like this is a frequent occurence. And if this particular issue
wasn't well understood and instead just reverted the offending commits,
then I would agree with you. But that's not the case. I'm very confident
in the stability, among other things, of blk-mq and the drivers that
utilize it. Most of the storage drivers are using it today, and have
been for a long time.

-- 
Jens Axboe

Re: [PATCH] blk-mq: fix corruption with direct issue

[Christoph Hellwig]

On Tue, Dec 04, 2018 at 03:47:46PM -0700, Jens Axboe wrote:
> If we attempt a direct issue to a SCSI device, and it returns BUSY, then
> we queue the request up normally. However, the SCSI layer may have
> already setup SG tables etc for this particular command. If we later
> merge with this request, then the old tables are no longer valid. Once
> we issue the IO, we only read/write the original part of the request,
> not the new state of it.
> 
> This causes data corruption, and is most often noticed with the file
> system complaining about the just read data being invalid:

Looks good as the first quick fix:

Reviewed-by: Christoph Hellwig <hch@lst.de>

But as we discussed I think we need to be even more careful. including
the only read and write check below in the thread or some higher level
approach.

Re: [PATCH] blk-mq: fix corruption with direct issue

[Jens Axboe]

On 12/5/18 7:41 AM, Christoph Hellwig wrote:
> On Tue, Dec 04, 2018 at 03:47:46PM -0700, Jens Axboe wrote:
>> If we attempt a direct issue to a SCSI device, and it returns BUSY, then
>> we queue the request up normally. However, the SCSI layer may have
>> already setup SG tables etc for this particular command. If we later
>> merge with this request, then the old tables are no longer valid. Once
>> we issue the IO, we only read/write the original part of the request,
>> not the new state of it.
>>
>> This causes data corruption, and is most often noticed with the file
>> system complaining about the just read data being invalid:
> 
> Looks good as the first quick fix:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> But as we discussed I think we need to be even more careful. including
> the only read and write check below in the thread or some higher level
> approach.

I agree, I'm reviewing the patches from Jianchao now and will do
something cleaner on top of that.

-- 
Jens Axboe