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

View: New views
20 Messages — Rating Filter:   Alert me  
< Prev | 1 - 2 | Next >

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

by Wilhelm Meier-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

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.


--
Wilhelm

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

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

by Jeff Layton-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

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 <jlayton@...>

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

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

by Shirish Pargaonkar :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Tue, Jun 30, 2009 at 8:01 AM, Jeff Layton<jlayton@...> 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 <jlayton@...>
>
>

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!

Regards,

Shirish
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

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

by Jeff Layton-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Tue, 2009-06-30 at 08:27 -0500, Shirish Pargaonkar wrote:

> On Tue, Jun 30, 2009 at 8:01 AM, Jeff Layton<jlayton@...> 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 <jlayton@...>
> >
> >
>
> 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 <jlayton@...>

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

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

by Shirish Pargaonkar :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Tue, Jun 30, 2009 at 6:00 AM, Wilhelm Meier<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.
>
>
> --
> Wilhelm
>
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client@...
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>

Wilhelm,

How exactly can I recreate the problem?  You mount a Samba share and
create files and then run
 mktemp -p . <list_of_files> 2>&1 | more

I have not dealt with mktemp command, will spend more time later in
the afternoon.

Need to address Jeff's comments/concerns about exclusive create too.

Regards,

Shirish
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

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

by Wilhelm Meier-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

Am Dienstag, 30. Juni 2009 15:44:36 schrieben Sie:
> On Tue, Jun 30, 2009 at 6:00 AM, Wilhelm Meier<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.
> >
> >
> > --
> > Wilhelm
> >
> > _______________________________________________
> > linux-cifs-client mailing list
> > linux-cifs-client@...
> > https://lists.samba.org/mailman/listinfo/linux-cifs-client
>
> Wilhelm,
>
> How exactly can I recreate the problem?  You mount a Samba share

yes, versions see above

> and
> create files and then run
>  mktemp -p . <list_of_files> 2>&1 | more

excatly, but keep the 6 'X', they are replaced by other chars to form an
unique filename:

mktemp -p . abcXXXXXX

>
> I have not dealt with mktemp command, will spend more time later in
> the afternoon.

look at mkstemp() libc-function

>
> Need to address Jeff's comments/concerns about exclusive create too.
>
> Regards,
>
> Shirish

--
Wilhelm
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

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

by Jeff Layton-4 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Tue, 2009-06-30 at 08:44 -0500, Shirish Pargaonkar wrote:

> On Tue, Jun 30, 2009 at 6:00 AM, Wilhelm Meier<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.
> >
> >
> > --
> > Wilhelm
> >
> > _______________________________________________
> > linux-cifs-client mailing list
> > linux-cifs-client@...
> > https://lists.samba.org/mailman/listinfo/linux-cifs-client
> >
>
> Wilhelm,
>
> How exactly can I recreate the problem?  You mount a Samba share and
> create files and then run
>  mktemp -p . <list_of_files> 2>&1 | more
>
> I have not dealt with mktemp command, will spend more time later in
> the afternoon.
>
> Need to address Jeff's comments/concerns about exclusive create too.
>

I was able to reproduce it by just running this while cd'ed into the
share:
$ strace mktemp -p . abcXXXXXX

You'll see it go into a near infinite loop trying to create different
filenames. Each one returns -EEXIST. I'm pretty sure the O_EXCL flag is
related to the problem since mktemp uses that.

--
Jeff Layton <jlayton@...>

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

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

by Shirish Pargaonkar :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Tue, Jun 30, 2009 at 8:40 AM, Jeff Layton<jlayton@...> wrote:

> On Tue, 2009-06-30 at 08:27 -0500, Shirish Pargaonkar wrote:
>> On Tue, Jun 30, 2009 at 8:01 AM, Jeff Layton<jlayton@...> 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 <jlayton@...>
>> >
>> >
>>
>> 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 <jlayton@...>
>
>

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,
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

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

by Shirish Pargaonkar :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Tue, Jun 30, 2009 at 1:36 PM, Shirish
Pargaonkar<shirishpargaonkar@...> wrote:

> On Tue, Jun 30, 2009 at 8:40 AM, Jeff Layton<jlayton@...> wrote:
>> On Tue, 2009-06-30 at 08:27 -0500, Shirish Pargaonkar wrote:
>>> On Tue, Jun 30, 2009 at 8:01 AM, Jeff Layton<jlayton@...> 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 <jlayton@...>
>>> >
>>> >
>>>
>>> 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 <jlayton@...>
>>
>>
>
> 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.
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

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

by Shirish Pargaonkar :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Tue, Jun 30, 2009 at 1:36 PM, Shirish
Pargaonkar<shirishpargaonkar@...> wrote:

> On Tue, Jun 30, 2009 at 8:40 AM, Jeff Layton<jlayton@...> wrote:
>> On Tue, 2009-06-30 at 08:27 -0500, Shirish Pargaonkar wrote:
>>> On Tue, Jun 30, 2009 at 8:01 AM, Jeff Layton<jlayton@...> 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 <jlayton@...>
>>> >
>>> >
>>>
>>> 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 <jlayton@...>
>>
>>
>
> 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,
>

This is a problem at vfs level/interaction.  I see posix open
returning without any error and wireshark
trace says the same, set_path_info with info level SMB_POSIX_OPEN succeeds.
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

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

by Wilhelm Meier-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

Am Mittwoch, 1. Juli 2009 13:01:04 schrieb Shirish Pargaonkar:

> On Tue, Jun 30, 2009 at 1:36 PM, Shirish
>
> Pargaonkar<shirishpargaonkar@...> wrote:
> > On Tue, Jun 30, 2009 at 8:40 AM, Jeff Layton<jlayton@...> wrote:
> >> On Tue, 2009-06-30 at 08:27 -0500, Shirish Pargaonkar wrote:
> >>> On Tue, Jun 30, 2009 at 8:01 AM, Jeff Layton<jlayton@...> 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 <jlayton@...>
> >>>
> >>> 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 <jlayton@...>
> >
> > 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.  

Exactly!
But the file is actualy created on the server-side, so we end up with tons of
files.

> The
> looping is I guess
> because mktemp then attempts to create another file and receives EEXIST and
> it goes on forever.

You may look into libc: it will try 62^3 attempts (or all comibinations of
char for the six 'X').

> So the basic issues is why EEXIST since mktemp is attempting a file
> which does not
> exist on the server.

yes

> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client@...
> https://lists.samba.org/mailman/listinfo/linux-cifs-client

--
Wilhelm
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

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

by Jeff Layton-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

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@...>

[0001-cifs-fix-regression-with-O_EXCL-creates-and-optimiz.patch]

>From 7edbbd8dfa17b7865a25c97f3b586fefbf2a73b3 Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@...>
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@...>
---
 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



_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

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

by Shirish Pargaonkar :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Wed, Jul 1, 2009 at 6:21 AM, Jeff Layton<jlayton@...> 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@...>
>

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
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

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

by Jeff Layton-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Wed, 2009-07-01 at 06:59 -0500, Shirish Pargaonkar wrote:

> On Wed, Jul 1, 2009 at 6:21 AM, Jeff Layton<jlayton@...> 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@...>
> >
>
> 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@...>

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

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

by Shirish Pargaonkar :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Wed, Jul 1, 2009 at 7:13 AM, Jeff Layton<jlayton@...> 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@...> 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@...>
>> >
>>
>> 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@...>
>
>

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.
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

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

by Wilhelm Meier-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

Am Mittwoch, 1. Juli 2009 15:17:20 schrieb Shirish Pargaonkar:

> On Wed, Jul 1, 2009 at 7:13 AM, Jeff Layton<jlayton@...> 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@...> 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@...>
> >>
> >> 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@...>
>
> 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@...
> https://lists.samba.org/mailman/listinfo/linux-cifs-client

--
Wilhelm
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

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

by Jeff Layton-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Wed, 2009-07-01 at 08:17 -0500, Shirish Pargaonkar wrote:

> On Wed, Jul 1, 2009 at 7:13 AM, Jeff Layton<jlayton@...> 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@...> 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@...>
> >> >
> >>
> >> 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@...>
> >
> >
>
> 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@...>

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

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

by Shirish Pargaonkar :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Wed, Jul 1, 2009 at 8:59 AM, Jeff Layton<jlayton@...> 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@...> 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@...> 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@...>
>> >> >
>> >>
>> >> 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@...>
>> >
>> >
>>
>> 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@...>
>
>
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,
                                        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)
                        rc = cifs_get_inode_info_unix(&newInode, full_path,


_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

excl_create.patch (2K) Download Attachment

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

by Shirish Pargaonkar :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Wed, Jul 1, 2009 at 8:44 AM, Wilhelm Meier<wilhelm.meier@...> wrote:

> Am Mittwoch, 1. Juli 2009 15:17:20 schrieb Shirish Pargaonkar:
>> On Wed, Jul 1, 2009 at 7:13 AM, Jeff Layton<jlayton@...> 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@...> 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@...>
>> >>
>> >> 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@...>
>>
>> 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@...
>> 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.
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

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

by Jeff Layton-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Wed, 2009-07-01 at 09:31 -0500, Shirish Pargaonkar wrote:

> On Wed, Jul 1, 2009 at 8:59 AM, Jeff Layton<jlayton@...> 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@...> 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@...> 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@...>
> >> >> >
> >> >>
> >> >> 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@...>
> >> >
> >> >
> >>
> >> 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@...>
> >
> >
>
> 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.

--
Jeff Layton <jlayton@...>

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client
< Prev | 1 - 2 | Next >