On Thu, May 14, 2020 at 10:09:55AM +0800, Ming Lei wrote: > 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. The only chance we need .commit_rqs() should be: - requests are queued successfully, and the last queued rq isn't marked as last - running out of budget or driver tag before queueing one new request I think we need to document the interfaces(.commit_rqs & .queue_rq) clearly. thanks, Ming
