Strange hardlink behavior with CIFS

Strange hardlink behavior with CIFS

[Matt Mackall]

With Linux 2.6.32-35 and either Windows or Samba in nounix mode,
hardlink counts can mysteriously disappear:

- create hardlink pair foo,bar
- stat foo -> nlink = 2
- open foo
- stat foo -> nlink = 1
- close
- wait or sync
- ls -l -> nlink = 2

Original report is here:

http://mercurial.selenic.com/bts/issue1866

-- 
Mathematics is the supreme nostalgia of our time.

Re: Strange hardlink behavior with CIFS

[Suresh Jayaraman]

On 10/19/2010 12:34 AM, Matt Mackall wrote:
> With Linux 2.6.32-35 and either Windows or Samba in nounix mode,
> hardlink counts can mysteriously disappear:
> 
> - create hardlink pair foo,bar
> - stat foo -> nlink = 2
> - open foo
> - stat foo -> nlink = 1
> - close
> - wait or sync
> - ls -l -> nlink = 2
> 
> Original report is here:
> 
> http://mercurial.selenic.com/bts/issue1866
> 

A quick test on 2.6.36-rc4 kernel reveals that the problem is no longer
reproducible. Could you try a more recent kernel and see whether the
problem is reproducible?


Thanks,

-- 
Suresh Jayaraman

Re: Strange hardlink behavior with CIFS

[bjoern]

> A quick test on 2.6.36-rc4 kernel reveals that the problem is no longer
> reproducible. Could you try a more recent kernel and see whether the
> problem is reproducible?

I had this problem on 2.6.32.
I just gave 2.6.36-rc4 a spin and I can't reproduce this issue anymore with the
new kernel. So, all-clear at least from my side.

Reagrds
Björn 

Re: Strange hardlink behavior with CIFS

[Steve French]

On Wed, Oct 20, 2010 at 4:18 AM, bjoern <bjoern-6mE/F8KvGsCzQB+pC5nmwQ@public.gmane.org> wrote:
>
> > A quick test on 2.6.36-rc4 kernel reveals that the problem is no longer
> > reproducible. Could you try a more recent kernel and see whether the
> > problem is reproducible?
>
> I had this problem on 2.6.32.
> I just gave 2.6.36-rc4 a spin and I can't reproduce this issue anymore with the
> new kernel. So, all-clear at least from my side.

It may have been one of these changes:

commit 4065c802da7484fa36f8cdf10f18d087233ecb88
Author: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Date:   Mon May 17 07:18:58 2010 -0400

    cifs: fix noserverino handling when unix extensions are enabled

    The uniqueid field sent by the server when unix extensions are enabled
    is currently used sometimes when it shouldn't be. The readdir codepath
    is correct, but most others are not. Fix it.

commit 84f30c66c3689745abbd3b9ce39816caeb9bec3b
Author: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Date:   Mon May 17 07:18:57 2010 -0400

    cifs: don't update uniqueid in cifs_fattr_to_inode

    We use this value to find an inode within the hash bucket, so we can't
    change this without re-hashing the inode. For now, treat this value
    as immutable.

    Eventually, we should probably use an inode number change on a path
    based operation to indicate that the lookup cache is invalid, but that's
    a bit more code to deal with.


commit db19272edc93661835bf6ec9736cfd0754aa3c62
Author: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Date:   Mon May 17 14:51:49 2010 -0400

    cifs: always revalidate hardlinked inodes when using noserverino


--
Thanks,

Steve

Re: Strange hardlink behavior with CIFS

[Matt Mackall]

On Tue, 2010-10-19 at 15:13 +0530, Suresh Jayaraman wrote:
> On 10/19/2010 12:34 AM, Matt Mackall wrote:
> > With Linux 2.6.32-35 and either Windows or Samba in nounix mode,
> > hardlink counts can mysteriously disappear:
> > 
> > - create hardlink pair foo,bar
> > - stat foo -> nlink = 2
> > - open foo
> > - stat foo -> nlink = 1
> > - close
> > - wait or sync
> > - ls -l -> nlink = 2
> > 
> > Original report is here:
> > 
> > http://mercurial.selenic.com/bts/issue1866
> > 
> 
> A quick test on 2.6.36-rc4 kernel reveals that the problem is no longer
> reproducible. Could you try a more recent kernel and see whether the
> problem is reproducible?

Test continues to fail with 2.6.36.

# uname -a
Linux calx 2.6.36 #71 SMP Tue Oct 26 02:23:34 CDT 2010 x86_64 GNU/Linux
# mount -o nounix //localhost/stuff cifs
           ^^^^^^
# cd cifs
# touch foo
# ln foo bar
# ./h
file: foo nlinks 2
file: foo nlinks 1
file: foo nlinks 1

# cat h.c
#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>

#define FPATH "foo"

int main(void)
{
	int rc, fh;
	struct stat st;

	rc = lstat(FPATH, &st);
	printf("file: %s nlinks %d\n", FPATH, st.st_nlink);

	fh = open(FPATH, 0, O_RDONLY);

	rc = lstat(FPATH, &st);
	printf("file: %s nlinks %d\n", FPATH, st.st_nlink);

	close(fh);

	rc = lstat(FPATH, &st);
	printf("file: %s nlinks %d\n", FPATH, st.st_nlink);

	return 0;
}




-- 
Mathematics is the supreme nostalgia of our time.

Re: Strange hardlink behavior with CIFS

[Jeff Layton]

On Wed, 27 Oct 2010 12:31:26 -0500
Matt Mackall <mpm-VDJrAJ4Gl5ZBDgjK7y7TUQ@public.gmane.org> wrote:

> On Tue, 2010-10-19 at 15:13 +0530, Suresh Jayaraman wrote:
> > On 10/19/2010 12:34 AM, Matt Mackall wrote:
> > > With Linux 2.6.32-35 and either Windows or Samba in nounix mode,
> > > hardlink counts can mysteriously disappear:
> > > 
> > > - create hardlink pair foo,bar
> > > - stat foo -> nlink = 2
> > > - open foo
> > > - stat foo -> nlink = 1
> > > - close
> > > - wait or sync
> > > - ls -l -> nlink = 2
> > > 
> > > Original report is here:
> > > 
> > > http://mercurial.selenic.com/bts/issue1866
> > > 
> > 
> > A quick test on 2.6.36-rc4 kernel reveals that the problem is no longer
> > reproducible. Could you try a more recent kernel and see whether the
> > problem is reproducible?
> 
> Test continues to fail with 2.6.36.
> 
> # uname -a
> Linux calx 2.6.36 #71 SMP Tue Oct 26 02:23:34 CDT 2010 x86_64 GNU/Linux
> # mount -o nounix //localhost/stuff cifs
>            ^^^^^^
> # cd cifs
> # touch foo
> # ln foo bar
> # ./h
> file: foo nlinks 2
> file: foo nlinks 1
> file: foo nlinks 1
> 
> # cat h.c
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <unistd.h>
> #include <fcntl.h>
> #include <stdio.h>
> 
> #define FPATH "foo"
> 
> int main(void)
> {
> 	int rc, fh;
> 	struct stat st;
> 
> 	rc = lstat(FPATH, &st);
> 	printf("file: %s nlinks %d\n", FPATH, st.st_nlink);
> 
> 	fh = open(FPATH, 0, O_RDONLY);
> 
> 	rc = lstat(FPATH, &st);
> 	printf("file: %s nlinks %d\n", FPATH, st.st_nlink);
> 
> 	close(fh);
> 
> 	rc = lstat(FPATH, &st);
> 	printf("file: %s nlinks %d\n", FPATH, st.st_nlink);
> 
> 	return 0;
> }
> 
> 
> 
> 

Ok, I can reproduce this too. The problem is this nastiness in CIFSSMBOpen:

        if (rc) {
                cFYI(1, "Error in Open = %d", rc);
        } else {
                *pOplock = pSMBr->OplockLevel; /* 1 byte no need to le_to_cpu */
                *netfid = pSMBr->Fid;   /* cifs fid stays in le */
                /* Let caller know file was created so we can set the mode. */
                /* Do we care about the CreateAction in any other cases? */
                if (cpu_to_le32(FILE_CREATE) == pSMBr->CreateAction)
                        *pOplock |= CIFS_CREATE_ACTION;
                if (pfile_info) {
                        memcpy((char *)pfile_info, (char *)&pSMBr->CreationTime,
                                36 /* CreationTime to Attributes */);
                        /* the file_info buf is endian converted by caller */
                        pfile_info->AllocationSize = pSMBr->AllocationSize;
                        pfile_info->EndOfFile = pSMBr->EndOfFile;
                        pfile_info->NumberOfLinks = cpu_to_le32(1);
                        pfile_info->DeletePending = 0;
                }
        }

In particular, the NumberOfLinks being set to 1 there. That probably
just needs to go. I'll look over it when I have time.


-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: Strange hardlink behavior with CIFS

[Matt Mackall]

On Wed, 2010-10-27 at 16:32 -0400, Jeff Layton wrote:
> On Wed, 27 Oct 2010 12:31:26 -0500
> Matt Mackall <mpm-VDJrAJ4Gl5ZBDgjK7y7TUQ@public.gmane.org> wrote:
> 
> > On Tue, 2010-10-19 at 15:13 +0530, Suresh Jayaraman wrote:
> > > On 10/19/2010 12:34 AM, Matt Mackall wrote:
> > > > With Linux 2.6.32-35 and either Windows or Samba in nounix mode,
> > > > hardlink counts can mysteriously disappear:
> > > > 
> > > > - create hardlink pair foo,bar
> > > > - stat foo -> nlink = 2
> > > > - open foo
> > > > - stat foo -> nlink = 1
> > > > - close
> > > > - wait or sync
> > > > - ls -l -> nlink = 2
> > > > 
> > > > Original report is here:
> > > > 
> > > > http://mercurial.selenic.com/bts/issue1866
> > > > 
> > > 
> > > A quick test on 2.6.36-rc4 kernel reveals that the problem is no longer
> > > reproducible. Could you try a more recent kernel and see whether the
> > > problem is reproducible?
> > 
> > Test continues to fail with 2.6.36.
> > 
> > # uname -a
> > Linux calx 2.6.36 #71 SMP Tue Oct 26 02:23:34 CDT 2010 x86_64 GNU/Linux
> > # mount -o nounix //localhost/stuff cifs
> >            ^^^^^^
> > # cd cifs
> > # touch foo
> > # ln foo bar
> > # ./h
> > file: foo nlinks 2
> > file: foo nlinks 1
> > file: foo nlinks 1
> > 
> > # cat h.c
> > #include <sys/types.h>
> > #include <sys/stat.h>
> > #include <unistd.h>
> > #include <fcntl.h>
> > #include <stdio.h>
> > 
> > #define FPATH "foo"
> > 
> > int main(void)
> > {
> > 	int rc, fh;
> > 	struct stat st;
> > 
> > 	rc = lstat(FPATH, &st);
> > 	printf("file: %s nlinks %d\n", FPATH, st.st_nlink);
> > 
> > 	fh = open(FPATH, 0, O_RDONLY);
> > 
> > 	rc = lstat(FPATH, &st);
> > 	printf("file: %s nlinks %d\n", FPATH, st.st_nlink);
> > 
> > 	close(fh);
> > 
> > 	rc = lstat(FPATH, &st);
> > 	printf("file: %s nlinks %d\n", FPATH, st.st_nlink);
> > 
> > 	return 0;
> > }
> > 
> > 
> > 
> > 
> 
> Ok, I can reproduce this too. The problem is this nastiness in CIFSSMBOpen:
> 
>         if (rc) {
>                 cFYI(1, "Error in Open = %d", rc);
>         } else {
>                 *pOplock = pSMBr->OplockLevel; /* 1 byte no need to le_to_cpu */
>                 *netfid = pSMBr->Fid;   /* cifs fid stays in le */
>                 /* Let caller know file was created so we can set the mode. */
>                 /* Do we care about the CreateAction in any other cases? */
>                 if (cpu_to_le32(FILE_CREATE) == pSMBr->CreateAction)
>                         *pOplock |= CIFS_CREATE_ACTION;
>                 if (pfile_info) {
>                         memcpy((char *)pfile_info, (char *)&pSMBr->CreationTime,
>                                 36 /* CreationTime to Attributes */);
>                         /* the file_info buf is endian converted by caller */
>                         pfile_info->AllocationSize = pSMBr->AllocationSize;
>                         pfile_info->EndOfFile = pSMBr->EndOfFile;
>                         pfile_info->NumberOfLinks = cpu_to_le32(1);
>                         pfile_info->DeletePending = 0;
>                 }
>         }
> 
> In particular, the NumberOfLinks being set to 1 there. That probably
> just needs to go. I'll look over it when I have time.

I should have noted that the behavior is* different be .35 and .36 - the
link count reappears quite quickly after the test in .36 so a manual
test is unlikely to show the problem now. But back-to-back close/stat
still shows the problem.

-- 
Mathematics is the supreme nostalgia of our time.

Re: Strange hardlink behavior with CIFS

[Suresh Jayaraman]

On 10/28/2010 02:02 AM, Jeff Layton wrote:
> On Wed, 27 Oct 2010 12:31:26 -0500
> Matt Mackall <mpm-VDJrAJ4Gl5ZBDgjK7y7TUQ@public.gmane.org> wrote:
> 
>> On Tue, 2010-10-19 at 15:13 +0530, Suresh Jayaraman wrote:
>>> On 10/19/2010 12:34 AM, Matt Mackall wrote:
>>>> With Linux 2.6.32-35 and either Windows or Samba in nounix mode,
>>>> hardlink counts can mysteriously disappear:
>>>>
>>>> - create hardlink pair foo,bar
>>>> - stat foo -> nlink = 2
>>>> - open foo
>>>> - stat foo -> nlink = 1
>>>> - close
>>>> - wait or sync
>>>> - ls -l -> nlink = 2
>>>>
>>>> Original report is here:
>>>>
>>>> http://mercurial.selenic.com/bts/issue1866
>>>>
>>>
>>> A quick test on 2.6.36-rc4 kernel reveals that the problem is no longer
>>> reproducible. Could you try a more recent kernel and see whether the
>>> problem is reproducible?
>>
>> Test continues to fail with 2.6.36.
>>
>> # uname -a
>> Linux calx 2.6.36 #71 SMP Tue Oct 26 02:23:34 CDT 2010 x86_64 GNU/Linux
>> # mount -o nounix //localhost/stuff cifs
>>            ^^^^^^
>> # cd cifs
>> # touch foo
>> # ln foo bar
>> # ./h
>> file: foo nlinks 2
>> file: foo nlinks 1
>> file: foo nlinks 1
>>
>> # cat h.c
>> #include <sys/types.h>
>> #include <sys/stat.h>
>> #include <unistd.h>
>> #include <fcntl.h>
>> #include <stdio.h>
>>
>> #define FPATH "foo"
>>
>> int main(void)
>> {
>> 	int rc, fh;
>> 	struct stat st;
>>
>> 	rc = lstat(FPATH, &st);
>> 	printf("file: %s nlinks %d\n", FPATH, st.st_nlink);
>>
>> 	fh = open(FPATH, 0, O_RDONLY);
>>
>> 	rc = lstat(FPATH, &st);
>> 	printf("file: %s nlinks %d\n", FPATH, st.st_nlink);
>>
>> 	close(fh);
>>
>> 	rc = lstat(FPATH, &st);
>> 	printf("file: %s nlinks %d\n", FPATH, st.st_nlink);
>>
>> 	return 0;
>> }
>>
>>
>>
>>
> 
> Ok, I can reproduce this too. The problem is this nastiness in CIFSSMBOpen:
> 
>         if (rc) {
>                 cFYI(1, "Error in Open = %d", rc);
>         } else {
>                 *pOplock = pSMBr->OplockLevel; /* 1 byte no need to le_to_cpu */
>                 *netfid = pSMBr->Fid;   /* cifs fid stays in le */
>                 /* Let caller know file was created so we can set the mode. */
>                 /* Do we care about the CreateAction in any other cases? */
>                 if (cpu_to_le32(FILE_CREATE) == pSMBr->CreateAction)
>                         *pOplock |= CIFS_CREATE_ACTION;
>                 if (pfile_info) {
>                         memcpy((char *)pfile_info, (char *)&pSMBr->CreationTime,
>                                 36 /* CreationTime to Attributes */);
>                         /* the file_info buf is endian converted by caller */
>                         pfile_info->AllocationSize = pSMBr->AllocationSize;
>                         pfile_info->EndOfFile = pSMBr->EndOfFile;
>                         pfile_info->NumberOfLinks = cpu_to_le32(1);
>                         pfile_info->DeletePending = 0;
>                 }
>         }
> 
> In particular, the NumberOfLinks being set to 1 there. That probably
> just needs to go. I'll look over it when I have time.
> 

Looks like bogus value is being set temporarily here. The only call I
see that gets us nlink is SMBQpathInfo.

One way of addressing this is to pass NULL FILE_ALL_INFO buffer to
CIFSSMBOpen from cifs_create()/cifs_open()/cifs_mknod() so that
cifs_get_file_info will call SMBQPathInfo to populate FILE_ALL_INFO, but
it means additional QPATHINFO call. Another way is to get nlink value
from CIFSSMBQPathInfo response by adding nlink to cifsInodeInfo struct..
Neither sounds quite convincing to me..

Any thoughts, other possible approaches?


Thanks,


-- 
Suresh Jayaraman

Re: Strange hardlink behavior with CIFS

[Jeff Layton]

On Thu, 28 Oct 2010 16:38:28 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:

> On 10/28/2010 02:02 AM, Jeff Layton wrote:
> > On Wed, 27 Oct 2010 12:31:26 -0500
> > Matt Mackall <mpm-VDJrAJ4Gl5ZBDgjK7y7TUQ@public.gmane.org> wrote:
> > 
> >> On Tue, 2010-10-19 at 15:13 +0530, Suresh Jayaraman wrote:
> >>> On 10/19/2010 12:34 AM, Matt Mackall wrote:
> >>>> With Linux 2.6.32-35 and either Windows or Samba in nounix mode,
> >>>> hardlink counts can mysteriously disappear:
> >>>>
> >>>> - create hardlink pair foo,bar
> >>>> - stat foo -> nlink = 2
> >>>> - open foo
> >>>> - stat foo -> nlink = 1
> >>>> - close
> >>>> - wait or sync
> >>>> - ls -l -> nlink = 2
> >>>>
> >>>> Original report is here:
> >>>>
> >>>> http://mercurial.selenic.com/bts/issue1866
> >>>>
> >>>
> >>> A quick test on 2.6.36-rc4 kernel reveals that the problem is no longer
> >>> reproducible. Could you try a more recent kernel and see whether the
> >>> problem is reproducible?
> >>
> >> Test continues to fail with 2.6.36.
> >>
> >> # uname -a
> >> Linux calx 2.6.36 #71 SMP Tue Oct 26 02:23:34 CDT 2010 x86_64 GNU/Linux
> >> # mount -o nounix //localhost/stuff cifs
> >>            ^^^^^^
> >> # cd cifs
> >> # touch foo
> >> # ln foo bar
> >> # ./h
> >> file: foo nlinks 2
> >> file: foo nlinks 1
> >> file: foo nlinks 1
> >>
> >> # cat h.c
> >> #include <sys/types.h>
> >> #include <sys/stat.h>
> >> #include <unistd.h>
> >> #include <fcntl.h>
> >> #include <stdio.h>
> >>
> >> #define FPATH "foo"
> >>
> >> int main(void)
> >> {
> >> 	int rc, fh;
> >> 	struct stat st;
> >>
> >> 	rc = lstat(FPATH, &st);
> >> 	printf("file: %s nlinks %d\n", FPATH, st.st_nlink);
> >>
> >> 	fh = open(FPATH, 0, O_RDONLY);
> >>
> >> 	rc = lstat(FPATH, &st);
> >> 	printf("file: %s nlinks %d\n", FPATH, st.st_nlink);
> >>
> >> 	close(fh);
> >>
> >> 	rc = lstat(FPATH, &st);
> >> 	printf("file: %s nlinks %d\n", FPATH, st.st_nlink);
> >>
> >> 	return 0;
> >> }
> >>
> >>
> >>
> >>
> > 
> > Ok, I can reproduce this too. The problem is this nastiness in CIFSSMBOpen:
> > 
> >         if (rc) {
> >                 cFYI(1, "Error in Open = %d", rc);
> >         } else {
> >                 *pOplock = pSMBr->OplockLevel; /* 1 byte no need to le_to_cpu */
> >                 *netfid = pSMBr->Fid;   /* cifs fid stays in le */
> >                 /* Let caller know file was created so we can set the mode. */
> >                 /* Do we care about the CreateAction in any other cases? */
> >                 if (cpu_to_le32(FILE_CREATE) == pSMBr->CreateAction)
> >                         *pOplock |= CIFS_CREATE_ACTION;
> >                 if (pfile_info) {
> >                         memcpy((char *)pfile_info, (char *)&pSMBr->CreationTime,
> >                                 36 /* CreationTime to Attributes */);
> >                         /* the file_info buf is endian converted by caller */
> >                         pfile_info->AllocationSize = pSMBr->AllocationSize;
> >                         pfile_info->EndOfFile = pSMBr->EndOfFile;
> >                         pfile_info->NumberOfLinks = cpu_to_le32(1);
> >                         pfile_info->DeletePending = 0;
> >                 }
> >         }
> > 
> > In particular, the NumberOfLinks being set to 1 there. That probably
> > just needs to go. I'll look over it when I have time.
> > 
> 
> Looks like bogus value is being set temporarily here. The only call I
> see that gets us nlink is SMBQpathInfo.
> 
> One way of addressing this is to pass NULL FILE_ALL_INFO buffer to
> CIFSSMBOpen from cifs_create()/cifs_open()/cifs_mknod() so that
> cifs_get_file_info will call SMBQPathInfo to populate FILE_ALL_INFO, but
> it means additional QPATHINFO call. Another way is to get nlink value
> from CIFSSMBQPathInfo response by adding nlink to cifsInodeInfo struct..
> Neither sounds quite convincing to me..
> 
> Any thoughts, other possible approaches?
> 

Well...the open call returns some values that we can use to update the
inode. OTOH, the attribute cache timeout is 1s, so on an open we will
almost always have recently updated the attribute cache via QPathInfo
call.

I think we ought to not have CIFSSMBOpen try to fake up values since
it's clearly incorrect. The easiest way is just to get rid of the
places that try to do that (like this one).

If we feel that it's important to try and use the values that are
returned from this call (and others that fake up a QPathInfo response
like this), then they ought to changed so that they put the info into a
cifs_fattr struct and then update the inode values from that.

To do that however, you'll need to fix cifs_fattr_to_inode to only
update the values that have been set in the cifs_fattr so that you're
not faking up other values. That may mean adding a "valid" field of
some sort and setting flags that show which fields should be copied to
the inode.

Another approach would be to copy the fields from the open response to
a cifs_fattr and then populate the remaining fields with the existing
values in the inode. That might be ok, but could be more racy than the
first approach.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: Strange hardlink behavior with CIFS

[Suresh Jayaraman]

On 10/28/2010 05:16 PM, Jeff Layton wrote:
> On Thu, 28 Oct 2010 16:38:28 +0530
> Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
> 
>> On 10/28/2010 02:02 AM, Jeff Layton wrote:
>>> On Wed, 27 Oct 2010 12:31:26 -0500
>>> Matt Mackall <mpm-VDJrAJ4Gl5ZBDgjK7y7TUQ@public.gmane.org> wrote:
>>>
>>>> On Tue, 2010-10-19 at 15:13 +0530, Suresh Jayaraman wrote:
>>>>> On 10/19/2010 12:34 AM, Matt Mackall wrote:
>>>>>> With Linux 2.6.32-35 and either Windows or Samba in nounix mode,
>>>>>> hardlink counts can mysteriously disappear:
>>>>>>
>>>>>> - create hardlink pair foo,bar
>>>>>> - stat foo -> nlink = 2
>>>>>> - open foo
>>>>>> - stat foo -> nlink = 1
>>>>>> - close
>>>>>> - wait or sync
>>>>>> - ls -l -> nlink = 2
>>>>>>

> 
> I think we ought to not have CIFSSMBOpen try to fake up values since
> it's clearly incorrect. The easiest way is just to get rid of the
> places that try to do that (like this one).
> 
> If we feel that it's important to try and use the values that are
> returned from this call (and others that fake up a QPathInfo response
> like this), then they ought to changed so that they put the info into a
> cifs_fattr struct and then update the inode values from that.
> 
> To do that however, you'll need to fix cifs_fattr_to_inode to only
> update the values that have been set in the cifs_fattr so that you're
> not faking up other values. That may mean adding a "valid" field of
> some sort and setting flags that show which fields should be copied to
> the inode.
> 

Great idea, Thanks.

Looks like we could overload fattr->cf_flags. Something like this?
(Only compile tested as I screwed up my VM setup)


 fs/cifs/cifsglob.h |    1 +
 fs/cifs/cifssmb.c  |    2 +-
 fs/cifs/inode.c    |    8 ++++++--
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index f259e4d..bdb508f 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -558,6 +558,7 @@ struct dfs_info3_param {
 #define CIFS_FATTR_DELETE_PENDING	0x2
 #define CIFS_FATTR_NEED_REVAL		0x4
 #define CIFS_FATTR_INO_COLLISION	0x8
+#define CIFS_FATTR_NLINK_NOT_SET	0x10	/* number of hardlinks unknown */
 
 struct cifs_fattr {
 	u32		cf_flags;
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 2f2632b..6a5455f 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -1326,7 +1326,7 @@ openRetry:
 			/* the file_info buf is endian converted by caller */
 			pfile_info->AllocationSize = pSMBr->AllocationSize;
 			pfile_info->EndOfFile = pSMBr->EndOfFile;
-			pfile_info->NumberOfLinks = cpu_to_le32(1);
+			pfile_info->NumberOfLinks = 0; /* not known */
 			pfile_info->DeletePending = 0;
 		}
 	}
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 39869c3..7ab9a62 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -128,7 +128,8 @@ cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)
 	inode->i_mtime = fattr->cf_mtime;
 	inode->i_ctime = fattr->cf_ctime;
 	inode->i_rdev = fattr->cf_rdev;
-	inode->i_nlink = fattr->cf_nlink;
+	if (!(fattr->cf_flags & CIFS_FATTR_NLINK_NOT_SET))
+		inode->i_nlink = fattr->cf_nlink;
 	inode->i_uid = fattr->cf_uid;
 	inode->i_gid = fattr->cf_gid;
 
@@ -531,7 +532,10 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
 			fattr->cf_mode &= ~(S_IWUGO);
 	}
 
-	fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
+	if (info->NumberOfLinks)
+		fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
+	else
+		fattr->cf_flags |= CIFS_FATTR_NLINK_NOT_SET;
 
 	fattr->cf_uid = cifs_sb->mnt_uid;
 	fattr->cf_gid = cifs_sb->mnt_gid;

Re: Strange hardlink behavior with CIFS

[Jeff Layton]

On Fri, 29 Oct 2010 16:21:41 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:

> On 10/28/2010 05:16 PM, Jeff Layton wrote:
> > On Thu, 28 Oct 2010 16:38:28 +0530
> > Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
> > 
> >> On 10/28/2010 02:02 AM, Jeff Layton wrote:
> >>> On Wed, 27 Oct 2010 12:31:26 -0500
> >>> Matt Mackall <mpm-VDJrAJ4Gl5ZBDgjK7y7TUQ@public.gmane.org> wrote:
> >>>
> >>>> On Tue, 2010-10-19 at 15:13 +0530, Suresh Jayaraman wrote:
> >>>>> On 10/19/2010 12:34 AM, Matt Mackall wrote:
> >>>>>> With Linux 2.6.32-35 and either Windows or Samba in nounix mode,
> >>>>>> hardlink counts can mysteriously disappear:
> >>>>>>
> >>>>>> - create hardlink pair foo,bar
> >>>>>> - stat foo -> nlink = 2
> >>>>>> - open foo
> >>>>>> - stat foo -> nlink = 1
> >>>>>> - close
> >>>>>> - wait or sync
> >>>>>> - ls -l -> nlink = 2
> >>>>>>
> 
> > 
> > I think we ought to not have CIFSSMBOpen try to fake up values since
> > it's clearly incorrect. The easiest way is just to get rid of the
> > places that try to do that (like this one).
> > 
> > If we feel that it's important to try and use the values that are
> > returned from this call (and others that fake up a QPathInfo response
> > like this), then they ought to changed so that they put the info into a
> > cifs_fattr struct and then update the inode values from that.
> > 
> > To do that however, you'll need to fix cifs_fattr_to_inode to only
> > update the values that have been set in the cifs_fattr so that you're
> > not faking up other values. That may mean adding a "valid" field of
> > some sort and setting flags that show which fields should be copied to
> > the inode.
> > 
> 
> Great idea, Thanks.
> 
> Looks like we could overload fattr->cf_flags. Something like this?
> (Only compile tested as I screwed up my VM setup)
> 
> 
>  fs/cifs/cifsglob.h |    1 +
>  fs/cifs/cifssmb.c  |    2 +-
>  fs/cifs/inode.c    |    8 ++++++--
>  3 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index f259e4d..bdb508f 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -558,6 +558,7 @@ struct dfs_info3_param {
>  #define CIFS_FATTR_DELETE_PENDING	0x2
>  #define CIFS_FATTR_NEED_REVAL		0x4
>  #define CIFS_FATTR_INO_COLLISION	0x8
> +#define CIFS_FATTR_NLINK_NOT_SET	0x10	/* number of hardlinks unknown */
>  
>  struct cifs_fattr {
>  	u32		cf_flags;
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 2f2632b..6a5455f 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -1326,7 +1326,7 @@ openRetry:
>  			/* the file_info buf is endian converted by caller */
>  			pfile_info->AllocationSize = pSMBr->AllocationSize;
>  			pfile_info->EndOfFile = pSMBr->EndOfFile;
> -			pfile_info->NumberOfLinks = cpu_to_le32(1);
> +			pfile_info->NumberOfLinks = 0; /* not known */
>  			pfile_info->DeletePending = 0;
>  		}
>  	}
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 39869c3..7ab9a62 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -128,7 +128,8 @@ cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)
>  	inode->i_mtime = fattr->cf_mtime;
>  	inode->i_ctime = fattr->cf_ctime;
>  	inode->i_rdev = fattr->cf_rdev;
> -	inode->i_nlink = fattr->cf_nlink;
> +	if (!(fattr->cf_flags & CIFS_FATTR_NLINK_NOT_SET))
> +		inode->i_nlink = fattr->cf_nlink;
>  	inode->i_uid = fattr->cf_uid;
>  	inode->i_gid = fattr->cf_gid;
>  
> @@ -531,7 +532,10 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
>  			fattr->cf_mode &= ~(S_IWUGO);
>  	}
>  
> -	fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
> +	if (info->NumberOfLinks)
> +		fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
> +	else
> +		fattr->cf_flags |= CIFS_FATTR_NLINK_NOT_SET;
>  
>  	fattr->cf_uid = cifs_sb->mnt_uid;
>  	fattr->cf_gid = cifs_sb->mnt_gid;

No, I don't think that's going to do it...

With that you're basically just saying "when the server reports zero
nlinks, just ignore it". That not really correct, at least not with
samba. I'm not sure how that would work with windows semantics, but
it's probably best not to do this.

Suppose I hold a file open and something unlinks the dentry. Then I
do a QFileInfo against the open filehandle. The file will still exist,
but it'll have 0 nlinks. This approach will also make the metadata
wrong.

What you really want to do is fix CIFSSMBOpen to take a cifs_fattr
pointer instead of a buffer, populate that with the values from the open
response, and set flags to make cifs_fattr_to_inode ignore everything
else.

You'll then want to make cifs_open just call cifs_fattr_to_inode
directly instead of calling cifs_get_inode_info. You can probably also
eliminate cifs_open_inode_helper in the process, though you'll need to
look at the logic carefully and may need to move some of it into
cifs_open.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>