Subject: Re: Fwd: Re: A new look at old NFS readdir() problems?

From: Tom Lane <[email-masked]>
Date: Thu, 02 Jan 2025 18:20:15 -0500

Thomas Munro <[email-masked]> writes:
> For what little it's worth, I'm not quite convinced yet that FreeBSD's
> client isn't more broken than it needs to be.

I'm suspicious of that too.  The wireshark trace you described is hard
to read any other way than that FreeBSD went out of its way to deliver
incorrect information.  I'm prepared to believe that we can't work
correctly on NFS servers that don't do the stable-cookie thing, but
why isn't it succeeding on ones that do?

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Thu, 02 Jan 2025 20:58:01 -0500

> I'm suspicious of that too.

I poked at this a little further.  I made the attached stand-alone
test case (you don't need any more than "cc -o rmtree rmtree.c"
to build it, then point the script at some NFS-mounted directory).
This fails with my NAS at least as far back as FreeBSD 11.0.
I also tried it on NetBSD 9.2 which seems fine.

			regards, tom lane

#! /bin/sh

set -e

TESTDIR="$1"

mkdir "$TESTDIR"

i=0
while [ $i -lt 1000 ]
do
  touch "$TESTDIR/$i"
  i=`expr $i + 1`
done

./rmtree "$TESTDIR"
/*-------------------------------------------------------------------------
 *
 * rmtree.c
 *
 * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
 * Portions Copyright (c) 1994, Regents of the University of California
 *
 * IDENTIFICATION
 *	  src/common/rmtree.c
 *
 *-------------------------------------------------------------------------
 */
#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>
#include <string.h>
#include <unistd.h>
#include <errno.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <dirent.h>

typedef enum PGFileType
{
	PGFILETYPE_ERROR,
	PGFILETYPE_UNKNOWN,
	PGFILETYPE_REG,
	PGFILETYPE_DIR,
	PGFILETYPE_LNK,
} PGFileType;

static void *
palloc(size_t size)
{
	void	   *tmp;

	/* Avoid unportable behavior of malloc(0) */
	if (size == 0)
		size = 1;
	tmp = malloc(size);
	if (tmp == NULL)
	{
		fprintf(stderr, "out of memory\n");
		exit(1);
	}
	return tmp;
}

static void *
repalloc(void *ptr, size_t size)
{
	void	   *tmp;

	/* Avoid unportable behavior of realloc(NULL, 0) */
	if (ptr == NULL && size == 0)
		size = 1;
	tmp = realloc(ptr, size);
	if (!tmp)
	{
		fprintf(stderr, "out of memory\n");
		exit(1);
	}
	return tmp;
}

static char *
pstrdup(const char *in)
{
	char	   *tmp;

	if (!in)
	{
		fprintf(stderr,
				"cannot duplicate null pointer (internal error)\n");
		exit(1);
	}
	tmp = strdup(in);
	if (!tmp)
	{
		fprintf(stderr, "out of memory\n");
		exit(1);
	}
	return tmp;
}

/*
 * Return the type of a directory entry.
 */
static PGFileType
get_dirent_type(const char *path,
				const struct dirent *de,
				bool look_through_symlinks)
{
	PGFileType	result;

	/*
	 * Some systems tell us the type directly in the dirent struct, but that's
	 * a BSD and Linux extension not required by POSIX.  Even when the
	 * interface is present, sometimes the type is unknown, depending on the
	 * filesystem.
	 */
#if defined(DT_REG) && defined(DT_DIR) && defined(DT_LNK)
	if (de->d_type == DT_REG)
		result = PGFILETYPE_REG;
	else if (de->d_type == DT_DIR)
		result = PGFILETYPE_DIR;
	else if (de->d_type == DT_LNK && !look_through_symlinks)
		result = PGFILETYPE_LNK;
	else
		result = PGFILETYPE_UNKNOWN;
#else
	result = PGFILETYPE_UNKNOWN;
#endif

	if (result == PGFILETYPE_UNKNOWN)
	{
		struct stat fst;
		int			sret;

		if (look_through_symlinks)
			sret = stat(path, &fst);
		else
			sret = lstat(path, &fst);

		if (sret < 0)
		{
			result = PGFILETYPE_ERROR;
			fprintf(stderr, "could not stat file \"%s\": %m\n", path);
		}
		else if (S_ISREG(fst.st_mode))
			result = PGFILETYPE_REG;
		else if (S_ISDIR(fst.st_mode))
			result = PGFILETYPE_DIR;
		else if (S_ISLNK(fst.st_mode))
			result = PGFILETYPE_LNK;
	}

	return result;
}

/*
 *	rmtree
 *
 *	Delete a directory tree recursively.
 *	Assumes path points to a valid directory.
 *	Deletes everything under path.
 *	If rmtopdir is true deletes the directory too.
 *	Returns true if successful, false if there was any problem.
 *	(The details of the problem are reported already, so caller
 *	doesn't really have to say anything more, but most do.)
 */
static bool
rmtree(const char *path, bool rmtopdir)
{
	char		pathbuf[8192];
	DIR		   *dir;
	struct dirent *de;
	bool		result = true;
	size_t		dirnames_size = 0;
	size_t		dirnames_capacity = 8;
	char	  **dirnames;

	dir = opendir(path);
	if (dir == NULL)
	{
		fprintf(stderr, "could not open directory \"%s\": %m\n", path);
		return false;
	}

	dirnames = (char **) palloc(sizeof(char *) * dirnames_capacity);

	while (errno = 0, (de = readdir(dir)))
	{
		if (strcmp(de->d_name, ".") == 0 ||
			strcmp(de->d_name, "..") == 0)
			continue;
		snprintf(pathbuf, sizeof(pathbuf), "%s/%s", path, de->d_name);
		switch (get_dirent_type(pathbuf, de, false))
		{
			case PGFILETYPE_ERROR:
				/* already logged, press on */
				break;
			case PGFILETYPE_DIR:

				/*
				 * Defer recursion until after we've closed this directory, to
				 * avoid using more than one file descriptor at a time.
				 */
				if (dirnames_size == dirnames_capacity)
				{
					dirnames = repalloc(dirnames,
										sizeof(char *) * dirnames_capacity * 2);
					dirnames_capacity *= 2;
				}
				dirnames[dirnames_size++] = pstrdup(pathbuf);
				break;
			default:
				if (unlink(pathbuf) != 0 && errno != ENOENT)
				{
					fprintf(stderr, "could not remove file \"%s\": %m\n", pathbuf);
					result = false;
				}
				break;
		}
	}

	if (errno != 0)
	{
		fprintf(stderr, "could not read directory \"%s\": %m\n", path);
		result = false;
	}

	closedir(dir);

	/* Now recurse into the subdirectories we found. */
	for (size_t i = 0; i < dirnames_size; ++i)
	{
		if (!rmtree(dirnames[i], true))
			result = false;
		free(dirnames[i]);
	}

	if (rmtopdir)
	{
		if (rmdir(path) != 0)
		{
			fprintf(stderr, "could not remove directory \"%s\": %m\n", path);
			result = false;
		}
	}

	free(dirnames);

	return result;
}

int
main(int argc, char **argv)
{
	if (argc != 2)
	{
		fprintf(stderr, "usage: %s target-directory\n", argv[0]);
		exit(1);
	}

	if (!rmtree(argv[1], true))
	{
		fprintf(stderr, "rmtree failed\n");
		exit(1);
	}

	return 0;
}

---

From: Peter Eisentraut <[email-masked]>
Date: Fri, 3 Jan 2025 18:31:37 +0100

> I also tried it on NetBSD 9.2 which seems fine.

If you use some GUI file manager, point at a directory, and select 
"remove this directory with everything in it", what does it do 
internally?  Surely it runs a readdir() loop and unlinks the files as it 
gets to them?  Or does it indeed slurp the whole directory tree into 
memory before starting the deletion?  It seems like we're probably not 
the first to have this use case, so there should be some information or 
problem reports about this somewhere out there.  Or if not, then I'd be 
tempted to think, someone broke it recently, even if the NFS hacker 
argues that it cannot possibly work and never should have worked.

---

From: Tom Lane <[email-masked]>
Date: Fri, 03 Jan 2025 13:12:43 -0500

> memory before starting the deletion?

One thing I noticed while testing yesterday is that "rm -rf foo"
worked even in cases where "rmtree foo" didn't.  I did not look
into FreeBSD's rm code, but I'll bet it has the sort of retry
logic that was recommended to us upthread.  I agree with your
point though that it's hard to believe that everyone does that
in every case where it would matter.

As for pre-existing bug reports, I found

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=57696

but there's basically no new information there, other than
the not-so-surprising fact that renaming directory entries
triggers the failure as efficiently as unlinking does.

			regards, tom lane

---

From: Thomas Munro <[email-masked]>
Date: Sat, 4 Jan 2025 12:39:38 +1300

On Sat, Jan 4, 2025 at 7:12 AM Tom Lane <[email-masked]> wrote:
> logic that was recommended to us upthread.

It punts the directory scan to the fts_read() etc functions (directory
traversal helpers widely inherited from ancestral BSD but not in any
standard), which buffers the scan and probably hides this issue:

https://github.com/freebsd/freebsd-src/blob/main/bin/rm/rm.c
https://github.com/freebsd/freebsd-src/blob/main/lib/libc/gen/fts.c

I doubt that hides all potential problems though, if I have understood
the vague outline of this bug correctly: perhaps if you ran large
enough rm -r, and you unlinked a file concurrently with that loop, you
could break it, that is, cause it to skip innocent files other than
the one you unlinked?

glibc also has an implementation of the BSD fts functions, and uses
them in its rm:

https://github.com/coreutils/coreutils/blob/master/src/rm.c
https://github.com/coreutils/coreutils/blob/master/src/remove.c

> triggers the failure as efficiently as unlinking does.

FWIW I am discussing this off-list with Rick, I *think* we can
distinguish between "gripes abouts NFS that we can't do much about"
and "extra thing going wrong here".  The fog of superstition around
NFS is thick because so many investigations end at a vendor
boundary/black box, but here we can understand the cookie scheme and
trace through all the layers here...

---

From: Tom Lane <[email-masked]>
Date: Fri, 03 Jan 2025 20:01:12 -0500

> could break it, that is, cause it to skip innocent files other than

Yeah.  The thing that makes this untenably bad for us is that it's not
only your own process's actions that can break readdir(), it's the
actions of some unrelated process.  So even "buffer the whole
directory contents" isn't a reliable fix, because someone else could
unlink a file while you're reading the directory.

One way to "fix" this from our side is to institute some system-wide
interlock that prevents file deletions (and renames, and maybe even
creations?) while any process is executing a readdir loop.  That's
several steps beyond awful from a performance standpoint, and what's
worse is that it's still not reliable.  It only takes one bit of
code that isn't on board with the locking protocol to put you right
back at square one.  It might not even be code that's part of Postgres
proper, so long as it has access to PGDATA.  As an example: it'd be
unsafe to modify postgresql.conf with emacs, or any other editor that
likes to make a backup file.  So IMV that's no fix at all.

The only other thing I can think of is to read and buffer the whole
directory (no matter how large), and then do it *again*, and repeat
till we get the same results twice in a row.  That's likewise just
horrid from a performance standpoint.  Worse, I'm not entirely
convinced that it's a 100% fix: it seems to rely on rather debatable
assumptions about the possible consequences of concurrent directory
mods.

> trace through all the layers here...

I wouldn't have any problem with saying that we don't support NFS
implementations that don't have stable cookies.  But so far I haven't
found any supported platform except FreeBSD that fails the rmtree test
against my Synology NAS.  I think the circumstantial evidence that
FreeBSD is doing something wrong, or wronger than necessary, is
strong.

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Sat, 04 Jan 2025 03:15:51 -0500

I wrote:
> against my Synology NAS.

To expand on that: I've now found that Linux, macOS, NetBSD, OpenBSD,
and illumos/OpenIndiana[1] pass that test.  I don't believe that Windows
does NFS.  That means that FreeBSD is the only one of our supported
platforms that fails.  I think we should simply document that NFS on
FreeBSD is broken, and await somebody getting annoyed enough to
fix that brokenness.

			regards, tom lane

[1] Getting OpenIndiana to work under qemu is not a task for those
easily discouraged.

---

From: Robert Haas <[email-masked]>
Date: Mon, 6 Jan 2025 09:20:18 -0500

> FreeBSD is broken, and await somebody getting annoyed enough to

Yeah, that seems like very strong evidence against FreeBSD, but I
think Thomas Munro's point about CIFS is worth considering. That is
rather widely used, and if the same workarounds would help both that
and FreeBSD's NFS, we might want to adopt it even if it's not a
complete fix.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Tom Lane <[email-masked]>
Date: Mon, 06 Jan 2025 09:49:22 -0500

> and FreeBSD's NFS, we might want to adopt it even if it's not a

TBH, I am happy that PG is now failing in a fairly visible way on
filesystems that are broken in this fashion.  I think that is better
than silent data corruption in obscure circumstances, which is where
we were before and would be again if we band-aid rmtree() and do
nothing else.  Nor do I think it's worth the effort to try to become
fully bulletproof on the point.

I think we should document that CIFS is unsupported.  The docs
could say something like:

	Storing databases on filesystems with unreliable readdir() is
	not supported and can lead to data corruption.  If you observe
	warnings like "directory is not empty" when trying to drop a
	database, that is strong evidence that the filesystem has
	unreliable readdir().  Filesystems known to have this problem
	include NFS on FreeBSD and CIFS on all platforms.

We could s/FreeBSD/older FreeBSD/ if something comes of Thomas'
efforts to fix that situation.

			regards, tom lane