by Trond Myklebust

[permalink] [raw]

Subject: Re: [PATCH v7 05/21] NFS: Store the change attribute in the directory page cache

On Fri, 2022-02-25 at 15:23 -0500, Benjamin Coddington wrote:
> On 25 Feb 2022, at 10:34, Benjamin Coddington wrote:
> > Ok, so I'm reading that further proof is required, and I'm happy to
> > do
> > the
> > work. Thanks for the replies here and elsewhere.
>
> Here's an example of this problem on a tmpfs export using v8 of your
> patchset with the fix to set the change_attr in
> nfs_readdir_page_init_array().
>
> I'm using tmpfs, because it reliably orders cookies in reverse order
> of
> creation (or perhaps sorted by name).
>
> The program drives both the client-side and server-side - so on this
> one
> system, /exports/tmpfs is:
> tmpfs /exports/tmpfs tmpfs rw,seclabel,relatime,size=102400k 0 0
>
> and /mnt/localhost is:
> localhost:/exports/tmpfs /mnt/localhost/tmpfs nfs4
> rw,relatime,vers=4.1,rsize=1048576,wsize=1048576,namlen=255,hard,prot
> o=tcp,timeo=600,retrans=2,sec=sys,clientaddr=127.0.0.1,local_lock=non
> e,addr=127.0.0.1
> 0 0
>
> The program creates 256 files on the server, walks through them once
> on
> the
> client, deletes the last 127 on the server, drops the first page from
> the
> pagecache, and walks through them again on the client.
>
> The second listing produces 124 duplicate entries.
>
> I just have to say again: this behavior is _new_ (but not new to me),
> and it
> is absolutely going to crop up on our customer's systems that are
> walking
> through millions of directory entries on loaded servers under memory
> pressure. The directory listings as a whole become very likely to be
> nonsense at random times. I realize they are not /supposed/ to be
> coherent,
> but what we're getting here is going to be far far less coherent, and
> its
> going to be a mess.
>
> There are other scenarios that are worse when the cookies aren't
> ordered,
> you can end up with EOF, or get into repeating patterns.
>
> Please compare this with v3, and before this patchset, and tell me if
> I'm
> not justified playing chicken little.
>
> Here's what I do to run this:
>
> mount -t tmpfs -osize=100M tmpfs /exports/tmpfs/
> exportfs -ofsid=0 *:/exports
> exportfs -ofsid=1 *:/exports/tmpfs
> mount -t nfs -ov4.1,sec=sys localhost:/exports /mnt/localhost
> ./getdents2
>
> Compare "Listing 1" with "Listing 2".
>
> I would also do a "rm -f /export/tmpfs/*" between each run.
>
> Thanks again for your time and work.
>
> Ben
>
> #define _GNU_SOURCE
> #include <stdio.h>
> #include <unistd.h>
> #include <fcntl.h>
> #include <sched.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <sys/syscall.h>
> #include <string.h>
>
> #define NFSDIR "/mnt/localhost/tmpfs"
> #define LOCDIR "/exports/tmpfs"
> #define BUF_SIZE 4096
>
> int main(int argc, char **argv)
> {
>         int i, dir_fd, bpos, total = 0;
>      size_t nread;
>         struct linux_dirent {
>                 long           d_ino;
>                 off_t          d_off;
>                 unsigned short d_reclen;
>                 char           d_name[];
>         };
>      struct linux_dirent *d;
>         char buf[BUF_SIZE];
>
>      /* create files: */
>      for (i = 0; i < 256; i++) {
>          sprintf(buf, LOCDIR "/file_%03d", i);
>          close(open(buf, O_CREAT, 666));
>      }
>
>         dir_fd = open(NFSDIR,
> O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC);
>         if (dir_fd < 0) {
>                 perror("cannot open dir");
>                 return 1;
>         }
>
>         while (1) {
>                 nread = syscall(SYS_getdents, dir_fd, buf, BUF_SIZE);
>                 if (nread == 0 || nread == -1)
>                         break;
>                 for (bpos = 0; bpos < nread;) {
>              d = (struct linux_dirent *) (buf + bpos);
>              printf("%s\n", d->d_name);
>              total++;
>              bpos += d->d_reclen;
>          }
>      }
>      printf("Listing 1: %d total dirents\n", total);
>
>      /* rewind */
>      lseek(dir_fd, 0, SEEK_SET);
>
>      /* drop the first page */
>      posix_fadvise(dir_fd, 0, 4096, POSIX_FADV_DONTNEED);
>
>      /* delete the last 127 files: */
>      for (i = 127; i < 256; i++) {
>          sprintf(buf, LOCDIR "/file_%03d", i);
>          unlink(buf);
>      }
>
>      total = 0;
>         while (1) {
>                 nread = syscall(SYS_getdents, dir_fd, buf, BUF_SIZE);
>                 if (nread == 0 || nread == -1)
>                         break;
>                 for (bpos = 0; bpos < nread;) {
>              d = (struct linux_dirent *) (buf + bpos);
>              printf("%s\n", d->d_name);
>              total++;
>              bpos += d->d_reclen;
>          }
>      }
>      printf("Listing 2: %d total dirents\n", total);
>
>         close(dir_fd);
>         return 0;
> }

tmpfs is broken on the server. It doesn't provide stable cookies, and
knfsd doesn't use the verifier to tell you that the cookie assignment
has changed.

Re-export of tmpfs has never worked reliably.

--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]

2022-02-26 02:36:28

by Benjamin Coddington

[permalink] [raw]

Subject: Re: [PATCH v7 05/21] NFS: Store the change attribute in the directory page cache

On 25 Feb 2022, at 15:41, Trond Myklebust wrote:

> On Fri, 2022-02-25 at 15:23 -0500, Benjamin Coddington wrote:
>> On 25 Feb 2022, at 10:34, Benjamin Coddington wrote:
>>> Ok, so I'm reading that further proof is required, and I'm happy to
>>> do
>>> the
>>> work. Thanks for the replies here and elsewhere.
>>
>> Here's an example of this problem on a tmpfs export using v8 of your
>> patchset with the fix to set the change_attr in
>> nfs_readdir_page_init_array().
>>
>> I'm using tmpfs, because it reliably orders cookies in reverse order
>> of
>> creation (or perhaps sorted by name).
>>
>> The program drives both the client-side and server-side - so on this
>> one
>> system, /exports/tmpfs is:
>> tmpfs /exports/tmpfs tmpfs rw,seclabel,relatime,size=102400k 0 0
>>
>> and /mnt/localhost is:
>> localhost:/exports/tmpfs /mnt/localhost/tmpfs nfs4
>> rw,relatime,vers=4.1,rsize=1048576,wsize=1048576,namlen=255,hard,prot
>> o=tcp,timeo=600,retrans=2,sec=sys,clientaddr=127.0.0.1,local_lock=non
>> e,addr=127.0.0.1
>> 0 0
>>
>> The program creates 256 files on the server, walks through them once
>> on
>> the
>> client, deletes the last 127 on the server, drops the first page from
>> the
>> pagecache, and walks through them again on the client.
>>
>> The second listing produces 124 duplicate entries.
>>
>> I just have to say again: this behavior is _new_ (but not new to me),
>> and it
>> is absolutely going to crop up on our customer's systems that are
>> walking
>> through millions of directory entries on loaded servers under memory
>> pressure. The directory listings as a whole become very likely to
>> be
>> nonsense at random times. I realize they are not /supposed/ to be
>> coherent,
>> but what we're getting here is going to be far far less coherent, and
>> its
>> going to be a mess.
>>
>> There are other scenarios that are worse when the cookies aren't
>> ordered,
>> you can end up with EOF, or get into repeating patterns.
>>
>> Please compare this with v3, and before this patchset, and tell me if
>> I'm
>> not justified playing chicken little.
>>
>> Here's what I do to run this:
>>
>> mount -t tmpfs -osize=100M tmpfs /exports/tmpfs/
>> exportfs -ofsid=0 *:/exports
>> exportfs -ofsid=1 *:/exports/tmpfs
>> mount -t nfs -ov4.1,sec=sys localhost:/exports /mnt/localhost
>> ./getdents2
>>
>> Compare "Listing 1" with "Listing 2".
>>
>> I would also do a "rm -f /export/tmpfs/*" between each run.
>>
>> Thanks again for your time and work.
>>
>> Ben
>>
>> #define _GNU_SOURCE
>> #include <stdio.h>
>> #include <unistd.h>
>> #include <fcntl.h>
>> #include <sched.h>
>> #include <sys/types.h>
>> #include <sys/stat.h>
>> #include <sys/syscall.h>
>> #include <string.h>
>>
>> #define NFSDIR "/mnt/localhost/tmpfs"
>> #define LOCDIR "/exports/tmpfs"
>> #define BUF_SIZE 4096
>>
>> int main(int argc, char **argv)
>> {
>>         int i, dir_fd, bpos, total = 0;
>>      size_t nread;
>>         struct linux_dirent {
>>                 long           d_ino;
>>                 off_t          d_off;
>>                 unsigned short d_reclen;
>>                 char           d_name[];
>>         };
>>      struct linux_dirent *d;
>>         char buf[BUF_SIZE];
>>
>>      /* create files: */
>>      for (i = 0; i < 256; i++) {
>>          sprintf(buf, LOCDIR "/file_%03d", i);
>>          close(open(buf, O_CREAT, 666));
>>      }
>>
>>         dir_fd = open(NFSDIR,
>> O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC);
>>         if (dir_fd < 0) {
>>                 perror("cannot open dir");
>>                 return 1;
>>         }
>>
>>         while (1) {
>>                 nread = syscall(SYS_getdents, dir_fd,
>> buf, BUF_SIZE);
>>                 if (nread == 0 || nread == -1)
>>                         break;
>>                 for (bpos = 0; bpos < nread;) {
>>              d = (struct linux_dirent *) (buf + bpos);
>>              printf("%s\n", d->d_name);
>>              total++;
>>              bpos += d->d_reclen;
>>          }
>>      }
>>      printf("Listing 1: %d total dirents\n", total);
>>
>>      /* rewind */
>>      lseek(dir_fd, 0, SEEK_SET);
>>
>>      /* drop the first page */
>>      posix_fadvise(dir_fd, 0, 4096, POSIX_FADV_DONTNEED);
>>
>>      /* delete the last 127 files: */
>>      for (i = 127; i < 256; i++) {
>>          sprintf(buf, LOCDIR "/file_%03d", i);
>>          unlink(buf);
>>      }
>>
>>      total = 0;
>>         while (1) {
>>                 nread = syscall(SYS_getdents, dir_fd,
>> buf, BUF_SIZE);
>>                 if (nread == 0 || nread == -1)
>>                         break;
>>                 for (bpos = 0; bpos < nread;) {
>>              d = (struct linux_dirent *) (buf + bpos);
>>              printf("%s\n", d->d_name);
>>              total++;
>>              bpos += d->d_reclen;
>>          }
>>      }
>>      printf("Listing 2: %d total dirents\n", total);
>>
>>         close(dir_fd);
>>         return 0;
>> }
>
>
> tmpfs is broken on the server. It doesn't provide stable cookies, and
> knfsd doesn't use the verifier to tell you that the cookie assignment
> has changed.
>
>
> Re-export of tmpfs has never worked reliably.

In this case, the cookies are stable, they can be verified with a wire
capture.

I've just adapted the program slightly to ext4 below. In this case the
Listing 2 shows files "file_125 file_126" that don't exist on the server
and
leave out files "elif_125 elif_126". It would take me more time to
produce
more dramatic results, but I'm sure I could produce them.

And its not hard to understand either, so I'm not sure why so much proof
is
needed.

#define _GNU_SOURCE
#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <sched.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/syscall.h>
#include <string.h>

#define NFSDIR "/mnt/localhost/ext4"
#define LOCDIR "/exports/ext4"
#define BUF_SIZE 4096

int main(int argc, char **argv)
{
int i, dir_fd, bpos, total = 0;
size_t nread;
struct linux_dirent {
long d_ino;
off_t d_off;
unsigned short d_reclen;
char d_name[];
};
struct linux_dirent *d;
char buf[BUF_SIZE];

/* create files: */
for (i = 0; i < 256; i++) {
sprintf(buf, LOCDIR "/file_%03d", i);
close(open(buf, O_CREAT, 666));
}

dir_fd = open(NFSDIR, O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC);
if (dir_fd < 0) {
perror("cannot open dir");
return 1;
}

while (1) {
nread = syscall(SYS_getdents, dir_fd, buf, BUF_SIZE);
if (nread == 0 || nread == -1)
break;
for (bpos = 0; bpos < nread;) {
d = (struct linux_dirent *) (buf + bpos);
printf("%s\n", d->d_name);
total++;
bpos += d->d_reclen;
}
}
printf("Listing 1: %d total dirents\n", total);

/* rewind */
lseek(dir_fd, 0, SEEK_SET);

/* drop the first page */
posix_fadvise(dir_fd, 0, 4096, POSIX_FADV_DONTNEED);

/* delete the first 127 files: */
for (i = 0; i < 127; i++) {
sprintf(buf, LOCDIR "/file_%03d", i);
unlink(buf);
}

/* create 127 more: */
for (i = 0; i < 127; i++) {
sprintf(buf, LOCDIR "/elif_%03d", i);
close(open(buf, O_CREAT, 666));
}

total = 0;
while (1) {
nread = syscall(SYS_getdents, dir_fd, buf, BUF_SIZE);
if (nread == 0 || nread == -1)
break;
for (bpos = 0; bpos < nread;) {
d = (struct linux_dirent *) (buf + bpos);
printf("%s\n", d->d_name);
total++;
bpos += d->d_reclen;
}
}
printf("Listing 2: %d total dirents\n", total);

close(dir_fd);
return 0;
}