Re: [PATCH 1/1] null-blk: replace deprecated ida_simple_xxx()

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 3/10/22 11:02, Chaitanya Kulkarni wrote:
> On 3/9/22 17:57, Chaitanya Kulkarni wrote:
>> On 3/9/22 15:38, Damien Le Moal wrote:
>>> On 3/10/22 07:02, Chaitanya Kulkarni wrote:
>>
>> [..]
>>
>>>> @@ -2044,7 +2044,7 @@ static int null_add_dev(struct nullb_device *dev)
>>>>       blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, nullb->q);
>>>>       mutex_lock(&lock);
>>>> -    nullb->index = ida_simple_get(&nullb_indexes, 0, 0, GFP_KERNEL);
>>>> +    nullb->index = ida_alloc(&nullb_indexes, GFP_KERNEL);
>>>
>>> Do we need error check here ? Not entirely sure if ida_free() tolerates
>>> being passed a failed ida_alloc() nullb_indexes... A quick look at
>>> ida_free() does not show anything obvious, so it may be worth checking
>>> in detail.
>>>
>>
>> Good point, but original code doesn't have error checking, this patch
>> eventually ends up calling same function what original code was doing.
>>
>> Since this is just a replacement patch should we add a 2nd patch on the
>> top of this for error handling ? or you prefer to have it in the same
>> one ?
>>
>> -ck
>>
> 
> Also nullb->index is defined as unsigned int [1] so in order to add
> error handling we need to change the type of variable, so I think it
> makes to make it a separate patch than removing deprecated API, lmk.

One patch to add the missing error check and change the index type, with
cc stable for backport, and a second patch to switch to the new api on
top of the fix, without cc stable. No ?

> 
> -ck
> 
> [1]
> 109 struct nullb {
> 110         struct nullb_device *dev;
> 111         struct list_head list;
> *112         unsigned int index;*
> 113         struct request_queue *q;
> 114         struct gendisk *disk;
> 115         struct blk_mq_tag_set *tag_set;
> 116         struct blk_mq_tag_set __tag_set;
> 117         unsigned int queue_depth;
> 118         atomic_long_t cur_bytes;
> 119         struct hrtimer bw_timer;
> 120         unsigned long cache_flush_pos;
> 121         spinlock_t lock;
> 122
> 123         struct nullb_queue *queues;
> 124         unsigned int nr_queues;
> 125         char disk_name[DISK_NAME_LEN];
> 126 };
> 127
> 128 blk_status_t null_handle_discard(struct nullb_device *dev, sector_t 
> sector,
> 129                                  sector_t nr_sectors);
> 
> 
> 


-- 
Damien Le Moal
Western Digital Research