| Message ID | 1246447269.19612.8.camel@tupile.poochiereds.net |
|---|---|
| State | New |
| Headers | show |
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
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).
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.
Am Mittwoch, 1. Juli 2009 15:17:20 schrieb Shirish Pargaonkar: > 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. Well, I don't know nothing about the internals of cifs, but in this case lookup and create must be atomic together ... > _______________________________________________ > linux-cifs-client mailing list > linux-cifs-client@lists.samba.org > https://lists.samba.org/mailman/listinfo/linux-cifs-client
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?
On Wed, Jul 1, 2009 at 8:44 AM, Wilhelm Meier<wilhelm.meier@fh-kl.de> wrote: > Am Mittwoch, 1. Juli 2009 15:17:20 schrieb Shirish Pargaonkar: >> 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. > > Well, I don't know nothing about the internals of cifs, but in this case > lookup and create must be atomic together ... > >> _______________________________________________ >> linux-cifs-client mailing list >> linux-cifs-client@lists.samba.org >> https://lists.samba.org/mailman/listinfo/linux-cifs-client > > -- > Wilhelm > Wilhelm, I do not know how to maintain atomicity in an operation like exclusive creates for network file systems, I mean, between a lookup and create from client, there is a chance that file gets created on the server. With cifs_posix_open in lookup, we can gurantee that atomicity but with the current vfs implementation, that is not possible (calling cifs_posix_open within cifs_lookup for atomic operations like exclusive create) on a Samba server but not on a Windows server.
>From 7edbbd8dfa17b7865a25c97f3b586fefbf2a73b3 Mon Sep 17 00:00:00 2001 From: Jeff Layton <jlayton@redhat.com> Date: Wed, 1 Jul 2009 07:12:03 -0400 Subject: [PATCH] cifs: fix regression with O_EXCL creates and optimize away lookup Signed-off-by: Jeff Layton <jlayton@redhat.com> --- fs/cifs/dir.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-) diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c index 7dc6b74..0ac4859 100644 --- a/fs/cifs/dir.c +++ b/fs/cifs/dir.c @@ -643,6 +643,15 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry, } } + /* + * O_EXCL: optimize away the lookup, but don't hash the dentry. Let + * the VFS handle the create. + */ + if (nd->flags & LOOKUP_EXCL) { + d_instantiate(direntry, NULL); + return 0; + } + /* can not grab the rename sem here since it would deadlock in the cases (beginning of sys_rename itself) in which we already have the sb rename sem */ -- 1.6.0.6