| Message ID | 20221030100714.876891-2-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] block: drop the duplicate check in elv_register | expand |
On 10/30/22 4:07 AM, Christoph Hellwig wrote: > We have less than a handful of elevators, and if someone adds a duplicate > one it simply will never be found but other be harmless. That isn't quite parseable... > diff --git a/block/elevator.c b/block/elevator.c > index d26aa787e29f0..ef9af17293ffb 100644 > --- a/block/elevator.c > +++ b/block/elevator.c > @@ -545,13 +545,7 @@ int elv_register(struct elevator_type *e) > return -ENOMEM; > } > > - /* register, don't allow duplicate names */ > spin_lock(&elv_list_lock); > - if (elevator_find(e->elevator_name, 0)) { > - spin_unlock(&elv_list_lock); > - kmem_cache_destroy(e->icq_cache); > - return -EBUSY; > - } > list_add_tail(&e->list, &elv_list); > spin_unlock(&elv_list_lock); What's the idea behind this? Yes it'll be harmless and list ordering will dictate which one will be found, leaving the other(s) dead, but why not catch this upfront? I agree likelihood of this ever happening to be tiny, but seems like a good idea to catch and return BUSY for this case.
On Mon, Oct 31, 2022 at 07:50:02AM -0600, Jens Axboe wrote: > > list_add_tail(&e->list, &elv_list); > > spin_unlock(&elv_list_lock); > > What's the idea behind this? Yes it'll be harmless and list ordering > will dictate which one will be found, leaving the other(s) dead, but why > not catch this upfront? I agree likelihood of this ever happening to be > tiny, but seems like a good idea to catch and return BUSY for this case. Because it's just not very useful code bloat here that I stumbled upon. But I can just drop it if you prefer.
diff --git a/block/elevator.c b/block/elevator.c index d26aa787e29f0..ef9af17293ffb 100644 --- a/block/elevator.c +++ b/block/elevator.c @@ -545,13 +545,7 @@ int elv_register(struct elevator_type *e) return -ENOMEM; } - /* register, don't allow duplicate names */ spin_lock(&elv_list_lock); - if (elevator_find(e->elevator_name, 0)) { - spin_unlock(&elv_list_lock); - kmem_cache_destroy(e->icq_cache); - return -EBUSY; - } list_add_tail(&e->list, &elv_list); spin_unlock(&elv_list_lock);
We have less than a handful of elevators, and if someone adds a duplicate one it simply will never be found but other be harmless. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/elevator.c | 6 ------ 1 file changed, 6 deletions(-)