| Message ID | 20220309220222.20931-2-kch@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | null_blk: replace ida_simple_xxx() | expand |
On 3/10/22 07:02, Chaitanya Kulkarni wrote: > Like various places in kernel replace deprecated ida_simple_get() and > ida_simple_remove with ida_alloc() and ida_free(). > > Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com> > --- > drivers/block/null_blk/main.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c > index 05b1120e6623..e077be800606 100644 > --- a/drivers/block/null_blk/main.c > +++ b/drivers/block/null_blk/main.c > @@ -1724,7 +1724,7 @@ static void null_del_dev(struct nullb *nullb) > > dev = nullb->dev; > > - ida_simple_remove(&nullb_indexes, nullb->index); > + ida_free(&nullb_indexes, nullb->index); > > list_del_init(&nullb->list); > > @@ -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. > dev->index = nullb->index; > mutex_unlock(&lock); >
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
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);
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); > > >
On 3/9/22 18:15, Damien Le Moal wrote: > 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 ? > yes, will send out V2 soon. -ck
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index 05b1120e6623..e077be800606 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -1724,7 +1724,7 @@ static void null_del_dev(struct nullb *nullb) dev = nullb->dev; - ida_simple_remove(&nullb_indexes, nullb->index); + ida_free(&nullb_indexes, nullb->index); list_del_init(&nullb->list); @@ -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); dev->index = nullb->index; mutex_unlock(&lock);
Like various places in kernel replace deprecated ida_simple_get() and ida_simple_remove with ida_alloc() and ida_free(). Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com> --- drivers/block/null_blk/main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)