From patchwork Wed Jul 1 14:31:11 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Shirish Pargaonkar X-Patchwork-Id: 29350 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.samba.org (mail.samba.org [66.70.73.150]) by bilbo.ozlabs.org (Postfix) with ESMTP id 64C53B7119 for ; Thu, 2 Jul 2009 00:31:58 +1000 (EST) Received: from dp.samba.org (localhost [127.0.0.1]) by lists.samba.org (Postfix) with ESMTP id F0111163CCC for ; Wed, 1 Jul 2009 14:31:20 +0000 (GMT) X-Spam-Checker-Version: SpamAssassin 3.1.7 (2006-10-05) on dp.samba.org X-Spam-Level: X-Spam-Status: No, score=-3.5 required=3.8 tests=AWL,BAYES_00, DNS_FROM_RFC_POST,SPF_PASS autolearn=no version=3.1.7 X-Original-To: linux-cifs-client@lists.samba.org Delivered-To: linux-cifs-client@lists.samba.org Received: from mail-yx0-f199.google.com (mail-yx0-f199.google.com [209.85.210.199]) by lists.samba.org (Postfix) with ESMTP id 44F30163BD6 for ; Wed, 1 Jul 2009 14:30:37 +0000 (GMT) Received: by yxe37 with SMTP id 37so1498648yxe.10 for ; Wed, 01 Jul 2009 07:31:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:in-reply-to:references :date:message-id:subject:from:to:cc:content-type; bh=EEn8cOUJ3JSKeJq0x5UhRNCTtIFmG9MoA928+3NbQFM=; b=W2TdopxNrlPUImQIZkwZbo99DS6RHzbEFyEyJOQjp3sHy2bqKfHcLmQ+/bjwJZKHxZ hGWAWuFtsNyCwoNdy0iZjOj+x4MrD1avc8l24FLy9DK8fJBUknp23kiM2l6tbGG7efq+ Nji57s0vdEgU7Jr98gcpmrv4mMBfBjzUyW2EI= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; b=ShatmaAXgjWMTsSX0V7oQyca117nNK5xhnKBaMldNr3a7zlIIRTcOOHc/Uj5vEcJ38 vu6pjbXSU0KWkQ9Cz+DLpj1vA7CGMYPrBNUUguMgViJz1iH+yWoaOMxnvnUdAyNtZk0i YC/W6rG7TxjpwYhnuhHjqi/1Aw9Mg5EvHuT54= MIME-Version: 1.0 Received: by 10.100.94.13 with SMTP id r13mr13558717anb.80.1246458671333; Wed, 01 Jul 2009 07:31:11 -0700 (PDT) In-Reply-To: <1246456768.3941.18.camel@tlielax.poochiereds.net> References: <200906301300.13865.wilhelm.meier@fh-kl.de> <4a4634330906300627y271d0f36v1c608789a1235185@mail.gmail.com> <1246369203.13212.18.camel@tupile.poochiereds.net> <4a4634330906301136l426b2ecifdeb10ac731394a4@mail.gmail.com> <4a4634330907010401x5b3208c2mf3490b577cbd35f9@mail.gmail.com> <1246447269.19612.8.camel@tupile.poochiereds.net> <4a4634330907010459j432f84efp3afd28e97537bcd0@mail.gmail.com> <1246450411.19612.15.camel@tupile.poochiereds.net> <4a4634330907010617x75701a2epa624f4d22d45beee@mail.gmail.com> <1246456768.3941.18.camel@tlielax.poochiereds.net> Date: Wed, 1 Jul 2009 09:31:11 -0500 Message-ID: <4a4634330907010731j9376fe5i93380eab04a21eda@mail.gmail.com> Subject: Re: [linux-cifs-client] mkstemp fails on cifs linux-2.6.31-rc1 / samba-3.3.6 From: Shirish Pargaonkar To: Jeff Layton Cc: wilhelm.meier@fh-kl.de, linux-cifs-client@lists.samba.org X-BeenThere: linux-cifs-client@lists.samba.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: The Linux CIFS VFS client List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-cifs-client-bounces+incoming=patchwork.ozlabs.org@lists.samba.org Errors-To: linux-cifs-client-bounces+incoming=patchwork.ozlabs.org@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)