Re: new regression from cifs open on lookup patches

On Fri, May 22, 2009 at 9:29 AM, Jeff Layton <[email protected]> wrote:
> On Fri, 22 May 2009 08:42:35 -0500
> Shirish Pargaonkar <[email protected]> wrote:
>
>> On Fri, May 22, 2009 at 6:48 AM, Jeff Layton <[email protected]> wrote:
>> > I've run across a new regression due to the open on lookup patches when
>> > working against a server with functioning posix open calls. Prior to
>> > those patches, opening a directory would simply result in a
>> > revalidation of the dir, at most this would mean a QPathInfo call would
>> > go out on the wire.
>> >
>> > Since the introduction of these patches, opening a directory causes an
>> > actual open call to go out on the wire for the directory. In general,
>> > the server will then return a status of NT_STATUS_FILE_IS_A_DIRECTORY
>> > that gets translated to -EACCES.
>> >
>> > The main place I've noticed this is when trying recursively remove a
>> > directory (rm -rf). rm will openat(AT_FDCWD, ...) the dir, and then try
>> > to walk the subdirectories. That fails. Backing out the open intent
>> > patches fixes the problem.
>> >
>> > This will definitely need to be fixed before 2.6.30 is released, as I'm
>> > certain this will break more than "rm". Attaching a testcase that can
>> > be used to test this.
>> >
>> > --
>> > Jeff Layton <[email protected]>
>> >
>>
>> Jeff,
>>
>> Looking into this, but as I see, with this check in cifs_lookup
>>  !(nd->flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY))
>> posix open will not work on a directory, only on a file.
>
> In fact, this new code in cifs_lookup does not look right to me:
>
>    if (!((nd->intent.open.flags & O_CREAT) && (nd->intent.open.flags & O_EXCL))) {
>
> ...why are we skipping this for O_EXCL? We send SMB_O_EXCL on the wire,
> so if the file already exists then the server should just return an
> error, correct?
>
> --
> Jeff Layton <[email protected]>
>

Jeff,

you are correct on both counts, LOOKUP_DIRECTORY and O_CREAT |  O_EXCL.

I was going to change the exclusive create eventually but right now, we do not
break any functionality, we just take the non-lookup_intent route for
exclusive creates.

The opendir case needs to be handled.  One thought is to go the
non-lookup-intent route
in case of EACCESS, so we will be wasting posix open call/response in
case of directories
and files that are not accessible (finding it out again).