| Message ID | 4a4634330907010731j9376fe5i93380eab04a21eda@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2009-07-01 at 09:31 -0500, Shirish Pargaonkar wrote: > On Wed, Jul 1, 2009 at 8:59 AM, Jeff Layton<jlayton@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@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@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@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@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@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.
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)