On Wed, May 13, 2020 at 05:27:53AM -0700, Christoph Hellwig wrote: > On Wed, May 13, 2020 at 05:54:37PM +0800, Ming Lei wrote: > > .commit_rqs() is supposed to handle partial dispatch when driver may not > > see .last of flag passed to .queue_rq(). > > > > We have added .commit_rqs() in case of partial dispatch and all consumers > > of bd->last have implemented .commit_rqs() callback, so it is perfect to > > pass real .last flag of the request list to .queue_rq() instead of faking > > it by trying to allocate driver tag for next request in the batching list. > > The current case still seems like a nice optimization to avoid an extra > indirect function call. So if you want to get rid of it I think it at > least needs a better rationale. Forget to mention, trying to predicate the last request via allocating tag for next request can't avoid extra .commit_rqs() because this indirect call is always called when the rq list isn't done. Also no matter .last is set or not, every implementation of .commit_rqs always grabs one lock, so looks this patch can get real win without any performance loss. On the other side, .commit_rqs() can be avoided iff the last queued(successful) rq is marked as .last, and the cost is to keep current estimate on .last. However, why is .commit_rqs() required? Why doesn't .queue_rq() handle the batching submission before non-STS_OK is returned? And the inline handling can be quite efficient because one more spin lock acquire can be avoided usually. Then .commit_rqs() can be killed. Thanks, Ming
