Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Benjamin Coddington" <bcodding@redhat.com>
To:     "Trond Myklebust" <trondmy@hammerspace.com>
Cc:     linux-nfs@vger.kernel.org
Subject: Re: [PATCH v7 05/21] NFS: Store the change attribute in the directory
 page cache
Date:   Fri, 25 Feb 2022 15:23:48 -0500
Message-ID: <FDB38E78-8980-46CA-B936-F82C7C104071@redhat.com>
In-Reply-To: <6878D746-0A5E-4815-A520-5CE7CD98A1E2@redhat.com>
References: <20220223211305.296816-1-trondmy@kernel.org>
 <20220223211305.296816-2-trondmy@kernel.org>
 <20220223211305.296816-3-trondmy@kernel.org>
 <20220223211305.296816-4-trondmy@kernel.org>
 <20220223211305.296816-5-trondmy@kernel.org>
 <20220223211305.296816-6-trondmy@kernel.org>
 <0DBE97BF-3A88-49FD-B078-012B5EDA5849@redhat.com>
 <1ca16f7e7be9588d15525e3afa4f7a80eb66b12b.camel@hammerspace.com>
 <9506801a7b7b6330ce2807721da5e03d77cf5c78.camel@hammerspace.com>
 <640B2705-35C6-4E9E-89D2-CC3D0E10EC3A@redhat.com>
 <eb2a551096bb3537a9de7091d203e0cbff8dc6be.camel@hammerspace.com>
 <11744FC6-5EFB-427A-ADB4-D211BA1C74F4@redhat.com>
 <f9ca09baa9e41000ab6286a27de567ca306f6991.camel@hammerspace.com>
 <6878D746-0A5E-4815-A520-5CE7CD98A1E2@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; format=flowed
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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=3D102400k 0 0

and /mnt/localhost is:
localhost:/exports/tmpfs /mnt/localhost/tmpfs nfs4 =

rw,relatime,vers=3D4.1,rsize=3D1048576,wsize=3D1048576,namlen=3D255,hard,=
proto=3Dtcp,timeo=3D600,retrans=3D2,sec=3Dsys,clientaddr=3D127.0.0.1,loca=
l_lock=3Dnone,addr=3D127.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=3D100M tmpfs /exports/tmpfs/
exportfs -ofsid=3D0 *:/exports
exportfs -ofsid=3D1 *:/exports/tmpfs
mount -t nfs -ov4.1,sec=3Dsys localhost:/exports /mnt/localhost
=2E/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 =3D 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 =3D 0; i < 256; i++) {
         sprintf(buf, LOCDIR "/file_%03d", i);
         close(open(buf, O_CREAT, 666));
     }

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

	while (1) {
		nread =3D syscall(SYS_getdents, dir_fd, buf, BUF_SIZE);
		if (nread =3D=3D 0 || nread =3D=3D -1)
			break;
		for (bpos =3D 0; bpos < nread;) {
             d =3D (struct linux_dirent *) (buf + bpos);
             printf("%s\n", d->d_name);
             total++;
             bpos +=3D 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 =3D 127; i < 256; i++) {
         sprintf(buf, LOCDIR "/file_%03d", i);
         unlink(buf);
     }

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

	close(dir_fd);
	return 0;
}