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
