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

[Shirish Pargaonkar]

On Mon, Mar 30, 2009 at 7:29 PM, Jeff Layton <jlayton at redhat.com> wrote:
> 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>
>

OK, let me make those changes and I will re-post the patch
on a different thread with appropriate subject line.

Regards,

Shirish