Re: [PATCH 02/21] block: add flag for internal commands

Re: [PATCH 02/21] block: add flag for internal commands

[John Garry]

On 03/07/2020 14:01, Hannes Reinecke wrote:

+linux-block

I figure that linux-block should be cc'ed here

> Some drivers require to allocate requests for internal command
> submission. These request will never be passed through the block
> layer, but nevertheless require a valid tag to avoid them clashing
> with normal I/O commands.
> This patch adds a new request flag REQ_INTERNAL to mark such
> requests and a terminates any such commands in blk_execute_rq_nowait()
> with a WARN_ON_ONCE to signal such an invalid usage.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   block/blk-exec.c          | 5 +++++
>   include/linux/blk_types.h | 2 ++
>   include/linux/blkdev.h    | 5 +++++
>   3 files changed, 12 insertions(+)
> 
> diff --git a/block/blk-exec.c b/block/blk-exec.c
> index 85324d53d072..6869877e0d21 100644
> --- a/block/blk-exec.c
> +++ b/block/blk-exec.c
> @@ -55,6 +55,11 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
>   	rq->rq_disk = bd_disk;
>   	rq->end_io = done;
>   
> +	if (WARN_ON_ONCE(blk_rq_is_internal(rq))) {
> +		blk_mq_end_request(rq, BLK_STS_NOTSUPP);
> +		return;
> +	}
> +
>   	blk_account_io_start(rq);
>   
>   	/*
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index ccb895f911b1..e386c43e4d77 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -360,6 +360,7 @@ enum req_flag_bits {
>   	/* for driver use */
>   	__REQ_DRV,
>   	__REQ_SWAP,		/* swapping request. */
> +	__REQ_INTERNAL,		/* driver-internal command */
>   	__REQ_NR_BITS,		/* stops here */
>   };
>   
> @@ -384,6 +385,7 @@ enum req_flag_bits {
>   
>   #define REQ_DRV			(1ULL << __REQ_DRV)
>   #define REQ_SWAP		(1ULL << __REQ_SWAP)
> +#define REQ_INTERNAL		(1ULL << __REQ_INTERNAL)
>   
>   #define REQ_FAILFAST_MASK \
>   	(REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT | REQ_FAILFAST_DRIVER)
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 8fd900998b4e..d09210d4591e 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -273,6 +273,11 @@ static inline bool blk_rq_is_passthrough(struct request *rq)
>   	return blk_rq_is_scsi(rq) || blk_rq_is_private(rq);
>   }
>   
> +static inline bool blk_rq_is_internal(struct request *rq)
> +{
> +	return rq->cmd_flags & REQ_INTERNAL;
> +}
> +
>   static inline bool bio_is_passthrough(struct bio *bio)
>   {
>   	unsigned op = bio_op(bio);
> 

Re: [PATCH 02/21] block: add flag for internal commands

[Jens Axboe]

On 7/8/20 3:27 AM, John Garry wrote:
> On 03/07/2020 14:01, Hannes Reinecke wrote:
> 
> +linux-block
> 
> I figure that linux-block should be cc'ed here
> 
>> Some drivers require to allocate requests for internal command
>> submission. These request will never be passed through the block
>> layer, but nevertheless require a valid tag to avoid them clashing
>> with normal I/O commands.
>> This patch adds a new request flag REQ_INTERNAL to mark such
>> requests and a terminates any such commands in blk_execute_rq_nowait()
>> with a WARN_ON_ONCE to signal such an invalid usage.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   block/blk-exec.c          | 5 +++++
>>   include/linux/blk_types.h | 2 ++
>>   include/linux/blkdev.h    | 5 +++++
>>   3 files changed, 12 insertions(+)
>>
>> diff --git a/block/blk-exec.c b/block/blk-exec.c
>> index 85324d53d072..6869877e0d21 100644
>> --- a/block/blk-exec.c
>> +++ b/block/blk-exec.c
>> @@ -55,6 +55,11 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
>>   	rq->rq_disk = bd_disk;
>>   	rq->end_io = done;
>>   
>> +	if (WARN_ON_ONCE(blk_rq_is_internal(rq))) {
>> +		blk_mq_end_request(rq, BLK_STS_NOTSUPP);
>> +		return;
>> +	}
>> +
>>   	blk_account_io_start(rq);

The whole concept seems very odd, and then there's this seemingly
randomly placed check and error condition. As I haven't seen the
actual use case for this, hard to make suggestions though.

-- 
Jens Axboe

Re: [PATCH 02/21] block: add flag for internal commands

[John Garry]

On 09/07/2020 21:02, Jens Axboe wrote:
> On 7/8/20 3:27 AM, John Garry wrote:
>> On 03/07/2020 14:01, Hannes Reinecke wrote:
>>
>> +linux-block
>>
>> I figure that linux-block should be cc'ed here
>>
>>> Some drivers require to allocate requests for internal command
>>> submission. These request will never be passed through the block
>>> layer, but nevertheless require a valid tag to avoid them clashing
>>> with normal I/O commands.
>>> This patch adds a new request flag REQ_INTERNAL to mark such
>>> requests and a terminates any such commands in blk_execute_rq_nowait()
>>> with a WARN_ON_ONCE to signal such an invalid usage.

So if these requests will never be passed through the block layer - as 
you say - then why add a check for this? Surely you will find out in 
nasty and obvious ways that it should be done.

Thanks,
John

>>>
>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>> ---
>>>    block/blk-exec.c          | 5 +++++
>>>    include/linux/blk_types.h | 2 ++
>>>    include/linux/blkdev.h    | 5 +++++
>>>    3 files changed, 12 insertions(+)
>>>
>>> diff --git a/block/blk-exec.c b/block/blk-exec.c
>>> index 85324d53d072..6869877e0d21 100644
>>> --- a/block/blk-exec.c
>>> +++ b/block/blk-exec.c
>>> @@ -55,6 +55,11 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
>>>    	rq->rq_disk = bd_disk;
>>>    	rq->end_io = done;
>>>    
>>> +	if (WARN_ON_ONCE(blk_rq_is_internal(rq))) {
>>> +		blk_mq_end_request(rq, BLK_STS_NOTSUPP);
>>> +		return;
>>> +	}
>>> +
>>>    	blk_account_io_start(rq);
> 
> The whole concept seems very odd, and then there's this seemingly
> randomly placed check and error condition. As I haven't seen the
> actual use case for this, hard to make suggestions though.
>