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

On Tue, Jun 30, 2009 at 1:36 PM, Shirish
Pargaonkar<[email protected]> wrote:
> On Tue, Jun 30, 2009 at 8:40 AM, Jeff Layton<[email protected]> wrote:
>> On Tue, 2009-06-30 at 08:27 -0500, Shirish Pargaonkar wrote:
>>> On Tue, Jun 30, 2009 at 8:01 AM, Jeff Layton<[email protected]> wrote:
>>> > On Tue, 2009-06-30 at 13:00 +0200, Wilhelm Meier wrote:
>>> >> Hi,
>>> >>
>>> >> I made a simple test with cifs in linux-2.6.31-rc1 to see if the so called
>>> >> kmail-problem (cifs-user-homes are totally unusable for kmail-mail-cache)
>>> >> still remains. Than I ran into a strange problem using "sed -i <command> <file-
>>> >> on-cifs>". "sed" uses mkstemp libc-funktion and fails with EEXIST, writing
>>> >> therefore tons of files onto the cifs-share.
>>> >>
>>> >> You can reproduce it with:
>>> >>
>>> >> strace mktemp -p . abcXXXXXX 2>&1 | more
>>> >>
>>> >> giving
>>> >>
>>> >> stat64(".", {st_mode=S_IFDIR|0700, st_size=0, ...}) = 0
>>> >> open("./abcUJcKWM", O_RDWR|O_CREAT|O_EXCL, 0600) = -1 EEXIST (File exists)
>>> >> stat64(".", {st_mode=S_IFDIR|0700, st_size=0, ...}) = 0
>>> >> open("./abcUGJaiA", O_RDWR|O_CREAT|O_EXCL, 0600) = -1 EEXIST (File exists)
>>> >> stat64(".", {st_mode=S_IFDIR|0700, st_size=0, ...}) = 0
>>> >> open("./abcWfKacs", O_RDWR|O_CREAT|O_EXCL, 0600) = -1 EEXIST (File exists)
>>> >> stat64(".", {st_mode=S_IFDIR|0700, st_size=0, ...}) = 0
>>> >> open("./abcBaGjWM", O_RDWR|O_CREAT|O_EXCL, 0600) = -1 EEXIST (File exists)
>>> >>
>>> >> This is the same on 2.6.31-rc1-git6.
>>> >>
>>> >> This is not the case in 2.6.26-2-vserver-686 (debian).
>>> >>
>>> >> The Samba-Server is lenny with samba-enterprise:
>>> >> kmux-fs:/# dpkg -l | grep samba
>>> >> ii  sernet-samba                    3.3.6-24                 a LanManager-like
>>> >> file and printer server fo
>>> >> ii  sernet-samba-common             3.3.6-24                 Samba common files
>>> >> used by both the server a
>>> >> ii  sernet-samba-keyring            1.1                      GnuPG archive
>>> >> keys of the SerNet Samba archi
>>> >>
>>> >> If I change Samba to the lenny-version
>>> >>
>>> >> ii  samba                           2:3.2.5-4lenny6          a LanManager-like
>>> >> file and printer server fo
>>> >> ii  samba-common                    2:3.2.5-4lenny6          Samba common files
>>> >> used by both the server a
>>> >>
>>> >> the described test-case is fine.
>>> >>
>>> >>
>>> >
>>> > I can reproduce this too.
>>> >
>>> > Looks like another regression from the create-on-lookup patches. I
>>> > commented the relevant chunk out of cifs_lookup and that fixes the
>>> > issue.
>>> >
>>> > Shirish, thoughts?
>>> >
>>> > --
>>> > Jeff Layton <[email protected]>
>>> >
>>> >
>>>
>>> Jeff, Wilhelm,
>>>
>>> I will spend some time later in the day on this but one thought came to mind,
>>> Jeff, we took out a line where if there was exclusive create, we would let
>>> vfs handle it, not sure if putting it back would help!
>>>
>>
>> Probably...it does seem to be related to O_EXCL. Rather than just
>> blindly putting that back though, I'd like to see some understanding
>> about why this is failing, and a consistent idea about how this is code
>> is intended to work.
>>
>> Skipping create on lookup for the O_EXCL case seems ass-backwards to me.
>> Doing it on lookup should help atomicity, and that's just what you want
>> for O_EXCL.
>>
>> --
>> Jeff Layton <[email protected]>
>>
>>
>
> So adding this back helps, this is what I had initially.  The reason I
> did put this was because I
> was not sure how to handle exclusive create then when I added lookup
> intent code to cifs.
> It sounds correct that a Samba server would handle the case of
> exclusive create for cifs
> and return an appropriate code (EEXISTS) in the response, may be cifs
> is not handling it.
>
> This is what I get with this patch in
>
> # mktemp -p . abcXXXXXX
> ./abcWpeApL
>
> I am debugging further to identify what the exact problem is.
>
>
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index 7dc6b74..d6f9ecd 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -673,6 +673,8 @@ 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)) {
> +                       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,
> @@ -689,6 +691,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct
> dentry *direntry,
>                                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,
>

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.