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.
-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);
