[PATCH] block: set rq->cmd_flags with bio->opf instead of data->cmd_flags when bio is not Null

[PATCH] block: set rq->cmd_flags with bio->opf instead of data->cmd_flags when bio is not Null

[chenxiang]

In function blk_mq_make_request(), though data->cmd_flags will be
initialized with bio->opf, later bio->opf may be set as REQ_INTEGRITY
if enabled DIX. So need to use bio->opf instead of data->cmd_flags in
function blk_mq_rq_ctx_init(), or  flags REQ_INTEGRITY will not be set
for rq->cmd_flags. It will cause dix=0 in function
sd_setup_read_write_cmnd() when enabled DIX, which will cause IO 
exception when enabled DIX.

For some IOs such as internal IO from SCSI layer, the parameter bio of
function blk_mq_get_request() is Null, so need to check bio to
decise rq->cmd_flags.

Fixes: f9afca4d367b ("blk-mq: pass in request/bio flags to queue mapping")

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Reviewed-by: John Garry <john.garry@huawei.com>
---
 block/blk-mq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 3ba37b9..c4a1c63 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -394,7 +394,7 @@ static struct request *blk_mq_get_request(struct request_queue *q,
 		return NULL;
 	}
 
-	rq = blk_mq_rq_ctx_init(data, tag, data->cmd_flags);
+	rq = blk_mq_rq_ctx_init(data, tag, bio ? bio->bi_opf : data->cmd_flags);
 	if (!op_is_flush(data->cmd_flags)) {
 		rq->elv.icq = NULL;
 		if (e && e->type->ops.prepare_request) {
-- 
2.8.1

Re: [PATCH] block: set rq->cmd_flags with bio->opf instead of data->cmd_flags when bio is not Null

[chenxiang (M)]

+cc Jens + linux-block

在 2019/1/24 21:43, chenxiang 写道:
> In function blk_mq_make_request(), though data->cmd_flags will be
> initialized with bio->opf, later bio->opf may be set as REQ_INTEGRITY
> if enabled DIX. So need to use bio->opf instead of data->cmd_flags in
> function blk_mq_rq_ctx_init(), or  flags REQ_INTEGRITY will not be set
> for rq->cmd_flags. It will cause dix=0 in function
> sd_setup_read_write_cmnd() when enabled DIX, which will cause IO
> exception when enabled DIX.
>
> For some IOs such as internal IO from SCSI layer, the parameter bio of
> function blk_mq_get_request() is Null, so need to check bio to
> decise rq->cmd_flags.
>
> Fixes: f9afca4d367b ("blk-mq: pass in request/bio flags to queue mapping")
>
> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
> Reviewed-by: John Garry <john.garry@huawei.com>
> ---
>   block/blk-mq.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 3ba37b9..c4a1c63 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -394,7 +394,7 @@ static struct request *blk_mq_get_request(struct request_queue *q,
>   		return NULL;
>   	}
>   
> -	rq = blk_mq_rq_ctx_init(data, tag, data->cmd_flags);
> +	rq = blk_mq_rq_ctx_init(data, tag, bio ? bio->bi_opf : data->cmd_flags);
>   	if (!op_is_flush(data->cmd_flags)) {
>   		rq->elv.icq = NULL;
>   		if (e && e->type->ops.prepare_request) {

Re: [PATCH] block: set rq->cmd_flags with bio->opf instead of data->cmd_flags when bio is not Null

[Christoph Hellwig]

> > for rq->cmd_flags. It will cause dix=0 in function
> > sd_setup_read_write_cmnd() when enabled DIX, which will cause IO
> > exception when enabled DIX.
> > 
> > For some IOs such as internal IO from SCSI layer, the parameter bio of
> > function blk_mq_get_request() is Null, so need to check bio to
> > decise rq->cmd_flags.

We have data->cmd_flags to deal with the NULL bio case.
blk_mq_make_request initializes data->cmd_flags from bio->bi_opf
just before calling blk_mq_get_request, so I'm really missing what you
are trying to fix here.

Re: [PATCH] block: set rq->cmd_flags with bio->opf instead of data->cmd_flags when bio is not Null

[John Garry]

On 28/01/2019 14:07, Christoph Hellwig wrote:
>>> for rq->cmd_flags. It will cause dix=0 in function
>>> sd_setup_read_write_cmnd() when enabled DIX, which will cause IO
>>> exception when enabled DIX.
>>>
>>> For some IOs such as internal IO from SCSI layer, the parameter bio of
>>> function blk_mq_get_request() is Null, so need to check bio to
>>> decise rq->cmd_flags.
>
> We have data->cmd_flags to deal with the NULL bio case.
> blk_mq_make_request initializes data->cmd_flags from bio->bi_opf
> just before calling blk_mq_get_request, so I'm really missing what you
> are trying to fix here.

As I understood, the problem is the scenario of calling 
blk_mq_make_request()->bio_integrity_prep() where we then allocate a bio 
integrity payload in calling bio_integrity_alloc().

In this case, bio_integrity_alloc() sets bio->bi_opf |= REQ_INTEGRITY, 
which is no longer consistent with data.cmd_flags.

John

>
> .
>

Re: [PATCH] block: set rq->cmd_flags with bio->opf instead of data->cmd_flags when bio is not Null

[Christoph Hellwig]

On Mon, Jan 28, 2019 at 03:36:58PM +0000, John Garry wrote:
> As I understood, the problem is the scenario of calling
> blk_mq_make_request()->bio_integrity_prep() where we then allocate a bio
> integrity payload in calling bio_integrity_alloc().
> 
> In this case, bio_integrity_alloc() sets bio->bi_opf |= REQ_INTEGRITY, which
> is no longer consistent with data.cmd_flags.

I don't see how that could happen:

static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
{
	...

	if (!bio_integrity_prep(bio))
		return BLK_QC_T_NONE;

	...

	data.cmd_flags = bio->bi_opf;
        rq = blk_mq_get_request(q, bio, &data);

Re: [PATCH] block: set rq->cmd_flags with bio->opf instead of data->cmd_flags when bio is not Null

[John Garry]

On 28/01/2019 15:57, Christoph Hellwig wrote:
> On Mon, Jan 28, 2019 at 03:36:58PM +0000, John Garry wrote:
>> As I understood, the problem is the scenario of calling
>> blk_mq_make_request()->bio_integrity_prep() where we then allocate a bio
>> integrity payload in calling bio_integrity_alloc().
>>
>> In this case, bio_integrity_alloc() sets bio->bi_opf |= REQ_INTEGRITY, which
>> is no longer consistent with data.cmd_flags.
>
> I don't see how that could happen:
>
> static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
> {
> 	...
>
> 	if (!bio_integrity_prep(bio))
> 		return BLK_QC_T_NONE;
>
> 	...
>
> 	data.cmd_flags = bio->bi_opf;
>         rq = blk_mq_get_request(q, bio, &data);
>
>

Your code is different to mine, then I see that this has been fixed in 
5.0-rc3:

commit 7809167da5c86fd6bf309b33dee7a797e263342f
Author: Ming Lei <ming.lei@redhat.com>
Date:   Wed Jan 16 19:08:15 2019 +0800

     block: don't lose track of REQ_INTEGRITY flag

     We need to pass bio->bi_opf after bio intergrity preparing, otherwise
     the flag of REQ_INTEGRITY may not be set on the allocated request, then
     breaks block integrity.

     Fixes: f9afca4d367b ("blk-mq: pass in request/bio flags to queue 
mapping")
     Cc: Hannes Reinecke <hare@suse.com>
     Cc: Keith Busch <keith.busch@intel.com>
     Signed-off-by: Ming Lei <ming.lei@redhat.com>
     Signed-off-by: Jens Axboe <axboe@kernel.dk>


Sorry for the noise,
John

> .
>

Re: [PATCH] block: set rq->cmd_flags with bio->opf instead of data->cmd_flags when bio is not Null

[chenxiang (M)]

在 2019/1/28 23:57, Christoph Hellwig 写道:
> On Mon, Jan 28, 2019 at 03:36:58PM +0000, John Garry wrote:
>> As I understood, the problem is the scenario of calling
>> blk_mq_make_request()->bio_integrity_prep() where we then allocate a bio
>> integrity payload in calling bio_integrity_alloc().
>>
>> In this case, bio_integrity_alloc() sets bio->bi_opf |= REQ_INTEGRITY, which
>> is no longer consistent with data.cmd_flags.
> I don't see how that could happen:
>
> static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
> {
> 	...
>
> 	if (!bio_integrity_prep(bio))
> 		return BLK_QC_T_NONE;
>
> 	...
>
> 	data.cmd_flags = bio->bi_opf;
>          rq = blk_mq_get_request(q, bio, &data);

Sorry to disturb, i used kernel 5.0-rc1 which has the issue, and it is 
fixed on linux-next branch.

>
>
> .
>