From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Paul E. McKenney" Subject: Re: [patch] fs: aio fix rcu lookup Date: Wed, 19 Jan 2011 20:03:08 -0800 Message-ID: <20110120040308.GD8476@linux.vnet.ibm.com> References: <20110118235236.GA14087@quack.suse.cz> <20110119132123.GC4246@quack.suse.cz> Reply-To: paulmck@linux.vnet.ibm.com Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Jeff Moyer , Jan Kara , Andrew Morton , linux-fsdevel , linux-kernel@vger.kernel.org To: Nick Piggin Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Thu, Jan 20, 2011 at 08:20:00AM +1100, Nick Piggin wrote: > On Thu, Jan 20, 2011 at 8:03 AM, Jeff Moyer wrote= : > > Nick Piggin writes: > > > >> On Thu, Jan 20, 2011 at 7:32 AM, Jeff Moyer wr= ote: > >>> Nick Piggin writes: > >>> > >>>> On Thu, Jan 20, 2011 at 6:46 AM, Jeff Moyer = wrote: > >>>>> Jeff Moyer writes: > >>>>> > >>>>>> Jan Kara writes: > >>>>>> > >>>>>>> =A0But there's the second race I describe making it possible > >>>>>>> for new IO to be created after io_destroy() has waited for al= l IO to > >>>>>>> finish... > >>>>>> > >>>>>> Can't that be solved by introducing memory barriers around the= accesses > >>>>>> to ->dead? > >>>>> > >>>>> Upon further consideration, I don't think so. > >>>>> > >>>>> Given the options, I think adding the synchronize rcu to the io= _destroy > >>>>> path is the best way forward. =A0You're already waiting for a b= unch of > >>>>> queued I/O to finish, so there is no guarantee that you're goin= g to > >>>>> finish that call quickly. > >>>> > >>>> I think synchronize_rcu() is not something to sprinkle around ou= tside > >>>> very slow paths. It can be done without synchronize_rcu. > >>> > >>> I'm not sure I understand what you're saying. =A0Do you mean to i= mply that > >>> io_destroy is not a very slow path? =A0Because it is. =A0I prefer= a solution > >>> that doesn't re-architecht things in order to solve a theoretical= issue > >>> that's never been observed. > >> > >> Even something that happens once per process lifetime, like in for= k/exit > >> is not necessarily suitable for RCU. > > > > Now you've really lost me. =A0;-) =A0Processes which utilize the in= -kernel > > aio interface typically create an ioctx at process startup, use tha= t for > > submitting all of their io, then destroy it on exit. =A0Think of a > > database. =A0Every time you call io_submit, you're doing a lookup o= f the > > ioctx. > > > >> I don't know exactly how all programs use io_destroy -- of the sma= ll > >> number that do, probably an even smaller number would care here. B= ut I > >> don't think it simplifies things enough to use synchronize_rcu for= it. > > > > Above it sounded like you didn't think AIO should be using RCU at a= ll. >=20 > synchronize_rcu of course, not RCU (typo). I think that Nick is suggesting that call_rcu() be used instead. Perhaps also very sparing use of synchronize_rcu_expedited(), which is faster than synchronize_rcu(), but which which uses more CPU time. Thanx, Paul > > Here it sounds like you are just against synchronize_rcu. =A0Which = is it? > > And if the latter, then please tell me in what cases you feel one w= ould > > be justified in calling synchronize_rcu. =A0For now, I simply disag= ree > > with you. =A0As I said before, you're already potentially waiting f= or disk > > I/O to complete. =A0It doesn't get much worse than that for latency= =2E >=20 > I think synchronize_rcu should firstly not be used unless it gives a = good > simplification, or speedup in fastpath. >=20 > When that is satified, then it is a question of exactly what kind of = slow > path it should be used in. I don't think it should be used in process= - > synchronous code (eg syscalls) except for error cases, resource > exhaustion, management syscalls (like module unload). >=20 > For example "it's waiting for IO anyway" is not a good reason, IMO. > Firstly because it may not be waiting for a 10ms disk IO, it may be > waiting for anything up to an in-RAM device. Secondly because it > could be quite slow depending on the RCU model used.