In D45987#1051603, @olce wrote:

In D45987#1051237, @mckusick wrote:

I do not remember why the change to allow the removing of directories with whiteouts became allowed. The changes in commit 996c772f581f56248 are quite extensive. If you can let me know which file(s) in that commit have the relevant changes, I can go back through the changes made in those files for the Lite/2 release to see if any explanation shows up.

Sure, I'm referring to:

--- a/sys/ufs/ufs/ufs_lookup.c
+++ b/sys/ufs/ufs/ufs_lookup.c
@@ -906,7 +961,7 @@ ufs_dirempty(ip, parentino, cred)
                if (dp->d_reclen == 0)
                        return (0);
                /* skip empty entries */
-               if (dp->d_ino == 0)
+               if (dp->d_ino == 0 || dp->d_ino == WINO)
                        continue;
                /* accept only "." and ".." */
 #              if (BYTE_ORDER == LITTLE_ENDIAN)

part of function ufs_dirempty(), which is (now, but if I'm not mistaken also was then) called only by ufs_rename() and ufs_rmdir().

Barring other reasons I can't see now, I think we'd better revert that behavior, both for logical reasons (the directory references no file but still has whiteout entries that we are going to lose silently) and practical ones (e.g., on Linux, whiteouts are implemented as device files, and thus rmdir() on directories holding some will fail with ENOTEMPTY; this is also to ease consistency in ZFS' code).

I'm unsure about this

I'm unsure about this

In D45987#1052150, @olce wrote:

In D45987#1052043, @jah wrote:

Yes, discarding the whiteout data does strike me as odd, I've already mentioned that.

Sure, but explaining the why is important, else there's no discussion possible. I don't think anyone an architecture for unionfs where all behaviors are "unsurprising" to users at first exists. What I am seeking, though, is to have consistent, understandable, and most of the time unsurprising behaviors, at least in connection to security, data loss, etc.

But whiteouts themselves are quite odd: as you mentioned they're not directory metadata. They might be thought of as file metadata for files contained within the lower directory, but they are file metadata whose purpose is to allow the user and other parts of the system to pretend those files don't exist. Given that, a directory whose upper FS component contains only whiteouts that completely negate the contents of its lower FS component is meant to be seen as logically empty by the user.

That's some common thinking for these but I'm in clear disagreement with it. It conflates what is visible through the union view and how the latter is concretely built out of the (upper and lower) layers. Once one recognizes them as conceptually separate, there is really no reason to see a directory holding whiteouts as empty, even if these whiteouts are meant to "delete" some lower layer's files *in the union view*. Even more, considering such directories empty, in the context of the upper layer on its own, is basically discarding the existence of these whiteouts in a context where the operator should be in a position to tweak the pieces that finally make the union view at will, and thus should be presented with all of them. For people not using UFS at all for unions, or using it only as a layer backing store that they never tweak directly, the chosen behavior doesn't matter at all, since they will never have to deal with whiteouts.

In D45987#1052043, @jah wrote:

Yes, discarding the whiteout data does strike me as odd, I've already mentioned that.

Nothing except zfs_fallthrough is needed.

It does undo the ACCESS_ONCE part of it, but doesn't undo the proper uintptr_t casting part. Oh, but I see the const cast is there for READ...
I can omit that bit too. OpenZFS really just reads READ_ONCE and WRITE_ONCE anyway. It
suffers from defining this stuff in too many different places :(.

In D46151#1052141, @andrew wrote:

In D46151#1052139, @kib wrote:

Am I right that this revision does not fix the issue, but covers it with more bits from the unused space in the head index? It is still possible that head wraps around, just less likely now that full 2^32 iterations needs to occur under it?

It's still possible, just very unlikely. Even if the value wrapped it would need to be the correct value for the cmpset to succeed.

I'd prefer a bunch of small, self-contained commits. I'd tag them all with this one review. Phabricator isn't ideal with that, but it's the best we have.
That way, we can MFC them piecemeal if need be, and people trying to 'merge all FOO driver changes' or whatever will see the right thing.
If you're worried, push a branch to github and I can take a look.

In D46151#1052139, @kib wrote:

Am I right that this revision does not fix the issue, but covers it with more bits from the unused space in the head index? It is still possible that head wraps around, just less likely now that full 2^32 iterations needs to occur under it?

The mtx code appears to be a check that the correct mutex is being held by the kernel so only one thread can access the single-consumer functions at a time. In userspace the tests I wrote don't use a mutex as only one consumer thread is created when testing these functions.

Am I right that this revision does not fix the issue, but covers it with more bits from the unused space in the head index? It is still possible that head wraps around, just less likely now that full 2^32 iterations needs to occur under it?

If this is useful in kernel, why hen why not use mutex from either normal libpthread or mtx from libstdthread? (The later seems to not have mtx_owned analog)