Patchwork mkstemp fails on cifs linux-2.6.31-rc1 / samba-3.3.6

login
register
mail settings
Submitter Jeff Layton
Date July 1, 2009, 11:21 a.m.
Message ID <1246447269.19612.8.camel@tupile.poochiereds.net>
Download mbox | patch
Permalink /patch/29346/
State New
Headers show

Comments

Jeff Layton - July 1, 2009, 11:21 a.m.
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...
Shirish Pargaonkar - July 1, 2009, 11:59 a.m.
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
Jeff Layton - July 1, 2009, 12:13 p.m.
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).
Shirish Pargaonkar - July 1, 2009, 1:17 p.m.
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.
Wilhelm Meier - July 1, 2009, 1:44 p.m.
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
Jeff Layton - July 1, 2009, 1:59 p.m.
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?
Shirish Pargaonkar - July 1, 2009, 2:41 p.m.
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.

Patch

>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