Re: [PATCH] fs: aio: Transition from Linked List to Hash Table for Active Request Management in AIO

[Jeff Moyer]

Jan Kara <jack@xxxxxxx> writes:

> Hi!
>
> On Tue 22-10-24 12:33:27, Mohammed Anees wrote:
>> > Benchmarks, please.  Look at what operations are done on this list.
>> > It's not at all obvious to me that what you've done here will improve
>> > performance of any operation.
>> 
>> This patch aims to improve this operation in io_cancel() syscall,
>> currently this iterates through all the requests in the Linked list,
>> checking for a match, which could take a significant time if the 
>> requests are high and once it finds one it deletes it. Using a hash
>> table will significant reduce the search time, which is what the comment
>> suggests as well.
>> 
>> /* TODO: use a hash or array, this sucks. */
>> 	list_for_each_entry(kiocb, &ctx->active_reqs, ki_list) {
>> 		if (kiocb->ki_res.obj == obj) {
>> 			ret = kiocb->ki_cancel(&kiocb->rw);
>> 			list_del_init(&kiocb->ki_list);
>> 			break;
>> 		}
>> 	}
>> 
>> I have tested this patch and believe it doesn’t affect the 
>> other functions. As for the io_cancel() syscall, please let 
>> me know exactly how you’d like me to test it so I can benchmark 
>> it accordingly.
>
> Well, I'd say that calling io_cancel() isn't really frequent operation. Or
> are you aware of any workload that would be regularly doing that? Hence
> optimizing performance for such operation isn't going to bring much benefit
> to real users. On the other hand the additional complexity of handling
> hashtable for requests in flight (although it isn't big on its own) is
> going to impact everybody using AIO. Hence I agree with Matthew that
> changes like you propose are not a clear win when looking at the bigger
> picture and need good justification.

... and cancelation is only supported by usb gadgetfs.  I'd say submit a
patch that gets rid of that todo so nobody else wastes time on it.

Cheers,
Jeff