[linux-cifs-client] Re: lookup intent patch

[Jeff Layton]

On Mon, 30 Mar 2009 15:07:23 -0500
Shirish Pargaonkar <shirishpargaonkar at gmail.com> wrote:

> On Mon, Mar 30, 2009 at 12:45 PM, Jeff Layton <jlayton at redhat.com> wrote:
> > On Mon, 30 Mar 2009 10:57:32 -0500
> > Shirish Pargaonkar <shirishpargaonkar at gmail.com> 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 <jlayton at redhat.com>