From patchwork Wed Jul 1 14:31:11 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: mkstemp fails on cifs linux-2.6.31-rc1 / samba-3.3.6 Date: Wed, 01 Jul 2009 04:31:11 -0000 From: Shirish Pargaonkar X-Patchwork-Id: 29350 Message-Id: <4a4634330907010731j9376fe5i93380eab04a21eda@mail.gmail.com> To: Jeff Layton Cc: wilhelm.meier@fh-kl.de, linux-cifs-client@lists.samba.org On Wed, Jul 1, 2009 at 8:59 AM, Jeff Layton wrote: > On Wed, 2009-07-01 at 08:17 -0500, Shirish Pargaonkar wrote: >> On Wed, Jul 1, 2009 at 7:13 AM, Jeff Layton wrote: >> > On Wed, 2009-07-01 at 06:59 -0500, Shirish Pargaonkar wrote: >> >> On Wed, Jul 1, 2009 at 6:21 AM, Jeff Layton 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 >> >> > >> >> >> >> 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 >> > >> > >> >> 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 > > This is the patch, this is what we had it before in dir.c in cifs_lookup() rc = cifs_get_inode_info_unix(&newInode, full_path, 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, parent_dir_inode->i_sb, nd->intent.open.create_mode, nd->intent.open.flags, &oplock, &fileHandle, xid); - /* - * The check below works around a bug in POSIX - * open in samba versions 3.3.1 and earlier where - * open could incorrectly fail with invalid parameter. - * If either that or op not supported returned, follow - * the normal lookup. - */ - if ((rc == 0) || (rc == -ENOENT)) - posix_open = true; - else if ((rc == -EINVAL) || (rc != -EOPNOTSUPP)) - pTcon->broken_posix_open = true; + /* + * The check below works around a bug in POSIX + * open in samba versions 3.3.1 and earlier + * where open could incorrectly fail with + * invalid parameter. If either that or op not + * supported returned, follow the normal lookup. + */ + if ((rc == 0) || (rc == -ENOENT)) + posix_open = true; + else if ((rc == -EINVAL) || (rc != -EOPNOTSUPP)) + pTcon->broken_posix_open = true; + } } if (!posix_open)