[linux-cifs-client] mkstemp fails on cifs linux-2.6.31-rc1 / samba-3.3.6

[Jeff Layton]

On Wed, 2009-07-01 at 09:31 -0500, Shirish Pargaonkar wrote:
> On Wed, Jul 1, 2009 at 8:59 AM, Jeff Layton<jlayton at redhat.com> wrote:
> > On Wed, 2009-07-01 at 08:17 -0500, Shirish Pargaonkar wrote:
> >> On Wed, Jul 1, 2009 at 7:13 AM, Jeff Layton<jlayton at redhat.com> wrote:
> >> > On Wed, 2009-07-01 at 06:59 -0500, Shirish Pargaonkar wrote:
> >> >> On Wed, Jul 1, 2009 at 6:21 AM, Jeff Layton<jlayton at redhat.com> wrote:
> >> >> > On Wed, 2009-07-01 at 06:01 -0500, Shirish Pargaonkar wrote:
> >> >> >>
> >> >> >> I think the problem is, why EEXIST since file does not exist.  The
> >> >> >> looping is I guess
> >> >> >> because mktemp then attempts to create another file and receives EEXIST and it
> >> >> >> goes on forever.
> >> >> >> So the basic issues is why EEXIST since mktemp is attempting a file
> >> >> >> which does not
> >> >> >> exist on the server.
> >> >> >
> >> >> > The problem is that the lookup is returning a positive dentry since it
> >> >> > just created the file. This makes the VFS turn around and return -EEXIST
> >> >> > (see the second O_EXCL check in do_filp_open). So we do have to skip the
> >> >> > create on lookup for O_EXCL or it won't work.
> >> >> >
> >> >> > I think what's needed is something like this patch. It seems to fix the
> >> >> > testcase for me. That said, this is not adequately regression tested and
> >> >> > should not be committed until it is.
> >> >> >
> >> >> > On that note, I'd like to reiterate my earlier sentiment that all of
> >> >> > this create-on-lookup code was committed prematurely and should be
> >> >> > backed out until it's more fully baked.
> >> >> >
> >> >> > Just my USD$.02...
> >> >> >
> >> >> > --
> >> >> > Jeff Layton <jlayton at redhat.com>
> >> >> >
> >> >>
> >> >> meant to send this to everybody but ended up sending only to Jeff,
> >> >> so here it is again...
> >> >>
> >> >> I think that is why it is not possible to exclusive create within
> >> >> lookup intent code.
> >> >> So, IMHO, we should put the check back in the cifs_lookup.
> >> >> That is what NFS does, and cifs should do the same
> >> >
> >> > Right, but do we really need to do the lookup in that case? It seems
> >> > like the old check didn't optimize it away (but maybe I'm remembering
> >> > incorrectly).
> >> >
> >> > --
> >> > Jeff Layton <jlayton at redhat.com>
> >> >
> >> >
> >>
> >> With the check, we are back to what was before lookup intent for
> >> exclusive create.  I think for exclusive create, a lookup can result in
> >> not create if the file exists, so for a command like mktemp, it
> >> probably does not help, there will be a lookup and there will be a create.
> >
> > I'm sorry, I don't understand what you're saying here. Can you rephrase
> > it? Or better yet, maybe post the patch that you're proposing to fix
> > this?
> >
> > --
> > Jeff Layton <jlayton at redhat.com>
> >
> >
> 
> This is the patch, this is what we had it before in dir.c in cifs_lookup()
> 
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index 7dc6b74..29d5642 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -673,22 +673,26 @@ cifs_lookup(struct inode *parent_dir_inode,
> struct dentry *direntry,
>                 if (!(nd->flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY)) &&
>                      (nd->flags & LOOKUP_OPEN) && !pTcon->broken_posix_open &&
>                      (nd->intent.open.flags & O_CREAT)) {
> -                       rc = cifs_posix_open(full_path, &newInode,
> +                       /* skip posix open if exclusive create */
> +                       if (!((nd->intent.open.flags & O_CREAT) &&
> +                                       (nd->intent.open.flags & O_EXCL))) {
> +                               rc = cifs_posix_open(full_path, &newInode,

The problem here is that when O_EXCL is set, you're doing a lookup
anyway. That breaks atomicity. It's quite possible that something like
this occurs:

Client 1	Client 2
========	=========
LOOKUP
		DELETE
CREATE

...it would be much better to skip the lookup on an O_EXCL create and
simply attempt to create the file. The patch I sent does this in the
same way that NFS does. It returns an unhashed, negative dentry on the
lookup and lets the VFS attempt the create.

That said, as you no doubt understand, this code is extremely delicate
and we need to take extra care to make sure that new regressions are not
introduced by any further patches.

This is why I've been counseling from the beginning to remove this code
pending more thorough testing. If this is worth doing at all, it's worth
waiting and making sure that it's done right.

-- 
Jeff Layton <jlayton at redhat.com>