From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff Layton Subject: Re: [linux-cifs-client] Re: lookup intent patch Date: Mon, 30 Mar 2009 20:29:33 -0400 Message-ID: <20090330202933.0b1b31c9@tleilax.poochiereds.net> References: <4a4634330902271134h3f334febke42b67ceab7e16eb@mail.gmail.com> <4a4634330903191336n54758971r2ff809ba31a80791@mail.gmail.com> <4a4634330903270815j415947f6ja190b884583de055@mail.gmail.com> <20090327141322.6089f72f@tleilax.poochiereds.net> <4a4634330903300857g70f1f91cl98cb6dcfae77d21e@mail.gmail.com> <20090330134509.565eb9e0@tleilax.poochiereds.net> <4a4634330903301307r53c8356flff9dbb9871fc7bca@mail.gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: linux-fsdevel , Steve French , "linux-cifs-client@lists.samba.org" To: Shirish Pargaonkar Return-path: Received: from mx2.redhat.com ([66.187.237.31]:42387 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757348AbZCaA3m convert rfc822-to-8bit (ORCPT ); Mon, 30 Mar 2009 20:29:42 -0400 In-Reply-To: <4a4634330903301307r53c8356flff9dbb9871fc7bca@mail.gmail.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Mon, 30 Mar 2009 15:07:23 -0500 Shirish Pargaonkar wrote: > On Mon, Mar 30, 2009 at 12:45 PM, Jeff Layton wr= ote: > > On Mon, 30 Mar 2009 10:57:32 -0500 > > Shirish Pargaonkar wrote: > >> > >> Jeff, > >> > >> Thanks. =A0Looking into it. I am trying to figure out the need/nec= essity > >> for cifs_lookup to call lookup_instanitate_flip. > >> lookup_instantiate_filp does call dentry_open and if cifs_lookup d= oes > >> not call lookup_instantiate_flip, > >> nameidata_to_filp will call dentry_open. > >> So I am not sure what we loose if dentry_open does not get called > >> between lookup_hash and nameidata_to_flip > >> because of an error between those two calls, specifically how will= the > >> cause of open file getting closed on the > >> server will be served if there was an in-betwen error by calling > >> lookup_instantiate_filp. > >> > > > > I'm not certain since I haven't tested your patch, but you may end = up > > with an inode refcount leak (aka Busy inodes after umount...). You'= re > > doing an open on the file in the lookup and I think that increases = the > > refcount of the inode (i_count). Eventually, that inode gets "put" = when > > you close the file. In the error situation described above though, = that > > put will never occur. As far as the VFS is concerned, the file was > > never actually opened, so it doesn't need to issue a fput(). >=20 > We would still be in do_flip_open and so if there is an error, while = exiting > release_open_intent would get called which would so the cleanup i.e. > call fput(). release_open_intent only calls fput if there is a filp set in the open_intent info. With your patch, you won't have one. Well...you'll have an empty filp, but I'm not sure it'll have all of the fields that are needed to actually make release_open_intent call fput(). In particular, I don't think f_path.dentry will be set. > Let me introduce an error in between to verify whether the data struc= tures > are cleaned up, such as i_count of an inode. >=20 > > > > Properly cleaning up the references is the main reason to make sure > > that you pass the filp back to the caller here. Closing the open fi= le > > on the server is also a nice side benefit since that could block th= e > > granting of oplocks and such. > > >=20 > I think caller is oblivious to the speed-up mechanism that cifs is at= tempting > by taking advantage of lookup intents to reduce network traffic. >=20 Right -- and that's a problem since it won't clean up the references unless it knows this. --=20 Jeff Layton -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel= " in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html