Hi Tejun, On Tue, Sep 18, 2018 at 05:49:09AM -0700, Tejun Heo wrote: > Hello, Ming. > > Sorry about the delay. > > On Thu, Sep 13, 2018 at 06:11:40AM +0800, Ming Lei wrote: > > > Yeah but what guards ->release() starting to run and then the ref > > > being switched to percpu mode? Or maybe that doesn't matter? > > > > OK, we may add synchronize_rcu() just after clearing the DEAD flag in > > the new introduced helper to avoid the race. > > That doesn't make sense to me. How is synchronize_rcu() gonna change > anything there? As you saw in the new post, synchronize_rcu() isn't used for avoiding the race. Instead, it is done by grabbing one extra ref on atomic part. > > > > > 4) after the queue is recovered(or the controller is reset successfully), it > > > > isn't necessary to wait until the refcount drops zero, since it is fine to > > > > reinit it by clearing DEAD and switching back to percpu mode from atomic mode. > > > > And waiting for the refcount dropping to zero in the reset handler may trigger > > > > IO hang if IO timeout happens again during reset. > > > > > > Does the recovery need the in-flight commands actually drained or does > > > it just need to block new issues for a while. If latter, why is > > > > The recovery needn't to drain the in-flight commands actually. > > Is it just waiting till confirm_kill is called? So that new ref is > not given away? If synchronization like that is gonna work, the > percpu ref operations on the reader side must be wrapped in a larger > critical region, which brings up two issues. > > 1. Callers of percpu_ref must not depend on what internal > synchronization construct percpu_ref uses. Again, percpu_ref > doesn't even use regular RCU. > > 2. If there is already an outer RCU protection around ref operation, > that RCU critical section can and should be used for > synchronization, not percpu_ref. I guess the above doesn't apply any more because there isn't new synchronize_rcu() introduced in my new post. > > > > percpu_ref even being used? > > > > Just for avoiding to invent a new wheel, especially .q_usage_counter > > has served for this purpose for long time. > > It sounds like this was more of an abuse. So, basically what you want > is sth like the following. > > READER > > rcu_read_lock(); > if (can_issue_new_commands) > issue; > else > abort; > rcu_read_unlock(); > > WRITER > > can_issue_new_commands = false; > synchronize_rcu(); > // no new command will be issued anymore > > Right? There isn't much wheel to reinvent here and using percpu_ref > for the above is likely already incorrect due to the different RCU > type being used. No RCU story any more, :-) It might work, but still a reinvented wheel since perpcu-refcount does provide same function. Not mention the inter-action between the two mechanism may have to be considered. Also there is still cost introduced in WRITER side, and the synchronize_rcu() often takes a bit long, especially there might be lots of namespaces, each need to run one synchronize_rcu(). We have learned lessons in converting to blk-mq for scsi, in which synchronize_rcu() introduces long delay in booting. Thanks, Ming
