Re: [PATCH v2 02/15] blk-ioprio: Modify fewer bio->bi_ioprio bits

[Bart Van Assche <bvanassche@acm.org>]

On 10/5/23 23:36, Kanchan Joshi wrote:
> On Thu, Oct 05, 2023 at 12:40:48PM -0700, Bart Van Assche wrote:
>> A later patch will store the data lifetime in the bio->bi_ioprio member
>> before the blk-ioprio policy is applied. Make sure that this policy 
>> doesn't
>> clear more bits than necessary.
>>
>> Cc: Damien Le Moal <dlemoal@kernel.org>
>> Cc: Niklas Cassel <niklas.cassel@wdc.com>
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>> block/blk-ioprio.c | 9 +++++----
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/block/blk-ioprio.c b/block/blk-ioprio.c
>> index 4051fada01f1..2db86f153b6d 100644
>> --- a/block/blk-ioprio.c
>> +++ b/block/blk-ioprio.c
>> @@ -202,7 +202,8 @@ void blkcg_set_ioprio(struct bio *bio)
>>          * to achieve this.
>>          */
>>         if (IOPRIO_PRIO_CLASS(bio->bi_ioprio) != IOPRIO_CLASS_RT)
>> -            bio->bi_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_RT, 4);
>> +            ioprio_set_class_and_level(&bio->bi_ioprio,
>> +                    IOPRIO_PRIO_VALUE(IOPRIO_CLASS_RT, 4));
>>         return;
>>     }
>>
>> @@ -213,10 +214,10 @@ void blkcg_set_ioprio(struct bio *bio)
>>      * If the bio I/O priority equals IOPRIO_CLASS_NONE, the cgroup I/O
>>      * priority is assigned to the bio.
>>      */
>> -    prio = max_t(u16, bio->bi_ioprio,
>> +    prio = max_t(u16, bio->bi_ioprio & IOPRIO_CLASS_LEVEL_MASK,
>>             IOPRIO_PRIO_VALUE(blkcg->prio_policy, 0));
> 
> All 9 bits (including CDL) are not taking part in this decision making
> now. Maybe you want to exclude only lifetime bits.
> 
>> -    if (prio > bio->bi_ioprio)
>> -        bio->bi_ioprio = prio;
>> +    if (prio > (bio->bi_ioprio & IOPRIO_CLASS_LEVEL_MASK))
>> +        ioprio_set_class_and_level(&bio->bi_ioprio, prio);
> 
> Same as above.

Hi Kanchan,

It is intentional that the CDL bits are left out from these decisions.
I think the decisions made in this policy should be based on the I/O
priority class and level only and not on the CDL value.

Thanks,

Bart.