From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shirish Pargaonkar Subject: Re: [linux-cifs-client] Re: lookup intent patch Date: Tue, 31 Mar 2009 04:25:55 -0500 Message-ID: <4a4634330903310225u29095872le0dd414f4a7c34f5@mail.gmail.com> 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> <20090330202933.0b1b31c9@tleilax.poochiereds.net> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: linux-fsdevel , Steve French , "linux-cifs-client@lists.samba.org" To: Jeff Layton Return-path: Received: from an-out-0708.google.com ([209.85.132.242]:5558 "EHLO an-out-0708.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750975AbZCaJZ6 (ORCPT ); Tue, 31 Mar 2009 05:25:58 -0400 Received: by an-out-0708.google.com with SMTP id d14so1906630and.1 for ; Tue, 31 Mar 2009 02:25:55 -0700 (PDT) In-Reply-To: <20090330202933.0b1b31c9@tleilax.poochiereds.net> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Mon, Mar 30, 2009 at 7:29 PM, Jeff Layton wrote: > On Mon, 30 Mar 2009 15:07:23 -0500 > Shirish Pargaonkar wrote: > >> On Mon, Mar 30, 2009 at 12:45 PM, Jeff Layton wrote: >> > On Mon, 30 Mar 2009 10:57:32 -0500 >> > Shirish Pargaonkar wrote: >> >> >> >> Jeff, >> >> >> >> Thanks. Looking into it. I am trying to figure out the need/necessity >> >> for cifs_lookup to call lookup_instanitate_flip. >> >> lookup_instantiate_filp does call dentry_open and if cifs_lookup does >> >> 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(). >> >> 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 structures >> are cleaned up, such as i_count of an inode. >> >> > >> > 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 file >> > on the server is also a nice side benefit since that could block the >> > granting of oplocks and such. >> > >> >> I think caller is oblivious to the speed-up mechanism that cifs is attempting >> by taking advantage of lookup intents to reduce network traffic. >> > > Right -- and that's a problem since it won't clean up the references > unless it knows this. > > -- > Jeff Layton > OK, let me make those changes and I will re-post the patch on a different thread with appropriate subject line. Regards, Shirish