[linux-cifs-client] Re: new regression from cifs open on lookup patches

[Jeff Layton]

On Fri, 22 May 2009 09:40:27 -0500
Shirish Pargaonkar <shirishpargaonkar at gmail.com> wrote:

> On Fri, May 22, 2009 at 9:29 AM, Jeff Layton <jlayton at redhat.com> wrote:
> > On Fri, 22 May 2009 08:42:35 -0500
> > Shirish Pargaonkar <shirishpargaonkar at gmail.com> wrote:
> >
> >> On Fri, May 22, 2009 at 6:48 AM, Jeff Layton <jlayton at redhat.com> 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 <jlayton at redhat.com>
> >> >
> >>
> >> 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 <jlayton at redhat.com>
> >
> 
> 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).

You could also change the mapping for NT_STATUS_FILE_IS_A_DIRECTORY
(maybe to -EISDIR) to distinguish it from files that aren't accessible.
The question is whether changing that mapping will break other codepaths
that depend on it.

At this point, we need to consider what to do for 2.6.30. We're looking
at the 3rd regression from these patches. I'm strongly of the opinion
that committing them was premature, and that they should be pulled and
deferred until they are better tested.

Open on lookup is a nice optimization but ultimately this is something
that we can live without until it's working correctly.

-- 
Jeff Layton <jlayton at redhat.com>