Re: mkstemp fails on cifs linux-2.6.31-rc1 / samba-3.3.6

On Wed, Jul 1, 2009 at 8:59 AM, Jeff Layton<[email protected]> wrote:
> On Wed, 2009-07-01 at 08:17 -0500, Shirish Pargaonkar wrote:
>> On Wed, Jul 1, 2009 at 7:13 AM, Jeff Layton<[email protected]> wrote:
>> > On Wed, 2009-07-01 at 06:59 -0500, Shirish Pargaonkar wrote:
>> >> On Wed, Jul 1, 2009 at 6:21 AM, Jeff Layton<[email protected]> 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 <[email protected]>
>> >> >
>> >>
>> >> 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 <[email protected]>
>> >
>> >
>>
>> 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 <[email protected]>
>
>

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,
                                        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)
                        rc = cifs_get_inode_info_unix(&newInode, full_path,

_______________________________________________
linux-cifs-client mailing list
[email protected]
https://lists.samba.org/mailman/listinfo/linux-cifs-client

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)
 			rc = cifs_get_inode_info_unix(&newInode, full_path,