Adding the VFS infrastructure for exchange is useful. Further discussion of adding exchange functionality to UFS can continue in a new review.

Do any file systems besides tmpfs support exchange?

I like the approach that you suggest. In thinking about the implementation it seems like the approach for exchange(a, b) would be:

Having had overnight to think through how to implement AT_RENAME_EXCHANGE in UFS, it will require a journal. If the operation is single threaded or at least kept to a bounded number in parallel, the log can be kept in an area that fits in the disk block holding the superblock. The journal entry simply needs to keep entries for the two names being exchanged. Specifically, the inode number of their resident directory, their name, and the inode number that they should reference. A flag in the superblock can indicate that the journal needs to be checked. If so, fsck can look up each name and verify that it references the correct inode number correcting it if it is wrong.

Oops, sorry, posted to the wrong phabricator review.

Having had overnight to think through how to implement AT_RENAME_EXCHANGE in UFS, it will require a journal. If the operation is single threaded or at least kept to a bounded number in parallel, the log can be kept in an area that fits in the disk block holding the superblock. The journal entry simply needs to keep entries for the two names being exchanged. Specifically, the inode number of their resident directory, their name, and the inode number that they should reference. A flag in the superblock can indicate that the journal needs to be checked. If so, fsck can look up each name and verify that it references the correct inode number correcting it if it is wrong.

In D57658#1331128, @kib wrote:

In D57658#1331127, @mckusick wrote:

This cannot be implemented in UFS since a requirement of its consistency model is that the on-disk image is always consistent. If the directory entries being swapped are in two different disk blocks, then there is no order of writing that will have a consistent filesystem. The only way to ensure consistency is to have a journal that can correct the unwritten directory name with its updated inode number.

In other words, it cannot be implemented for SU, but can for async and for SU+J, do you agree?

This cannot be implemented in UFS since a requirement of its consistency model is that the on-disk image is always consistent. If the directory entries being swapped are in two different disk blocks, then there is no order of writing that will have a consistent filesystem. The only way to ensure consistency is to have a journal that can correct the unwritten directory name with its updated inode number.

Your title on this phabricator review needs to be more specific. There are about 11,000 instances of EINVAL returns and you are not adding EXTERROR for all of them. So being a bit more specific about which subsystem you are doing is needed.

Add time check to loading of inodes in libufs per comment by Andre Albsmeier.

In D57371#1314488, @imp wrote:

I'd be tempted to double the tolerance to 1 minute. Some drives with write caching enable won't flush everything to medium so maybe the prior SB is still on the medium. We don't seem to send a BIO_FLUSH down, but this is from code inspection and maybe I missed something. One could speculate that further writes were lost, but while 30s is a long time to the drive, a few seconds might not be so you may find files on these drives past 30s by a little bit and 1 minute seems like a sensible limit to account for it.

Since I wasn't sure, I asked Claude and it said no too. It knew I asked the wrong question too...

me> when we flush the super block with ffs_sbflush, do we send a BIO_FLUSH down afterwards?
● The function is called ffs_sbupdate, not ffs_sbflush. Let me read the implementation.
● No, ffs_sbupdate does not send a BIO_FLUSH after writing the superblock. It writes the superblock (and summary information) using bwrite() or bawrite() depending on the waitfor flag, but there is no subsequent BIO_FLUSH issued to force the data to stable storage on the device.

The write path is:
1. Copy the superblock into a buffer (getblk + bcopy)
2. Write it with bwrite() (synchronous, waits for I/O completion) or bawrite() (async)

bwrite() waits for the I/O to complete at the BIO layer, but that doesn't guarantee the drive's write cache has been flushed to media — that would require an explicit BIO_FLUSH. None of the callers of   ffs_sbupdate appear to issue one either.

(Note: the function is ffs_sbupdate, not ffs_sbflush — there is no ffs_sbflush in the tree.)

In D57371#1314400, @kib wrote:

ffs_load_inode() is used from ffs_vgetf(), and might set the IN_MODIFIED flag. This breaks suspension for FFS1. Note that despite FF1 not supports snapshots, we still need to suspend it for several VFS ops.

This is pre-existing bug.

This now looks ready to commit.

Have you run your suggested tests?

Still fine with this change.

This is a useful addition. I don't have an opinion on whether unsafe should be in the name.

Sorry for the delay in answering. I am in a remote area of California and the power / Internet has been out since just after I sent my question. Power outages are unfortunately routine enough for you that you can work around them, me not so much.

Why does ufs_bmap_seekdata() cause a panic when it uses an unmapped buffer?

Much more complete and useful output.

Always glad to see more use of EXTERROR.

This now looks complete and correct.

You could also do something similar with the directory filling routine dir_clear_block() and the two functions that call it dir_insert() and dir_extend().

Time to get this committed.

Good to get the consistency.

Good to update the manual page.

Looks good. Not clear that soft updates and GEOM will not work at cross purposes with each other..

This time for sure!

Further error message refinement.

Valid complaint, but not sure what the error message should be for the third case.

Update per your suggestion.

Update per kib's suggestion.

If you don't have kernel strings, you need to print the long form.

Incorporate changes recommended by kib.

This adds clarity for me.

Looks good.

Fix some grammar nits.

Most helpful, thanks.

Agreed, better way to do it.

This is long overdue.

You do not specify what happens if both -U and -u are specified. In practice, whichever one is second wins out. I think it should be flagged as an error.

This is a great step ahead. Looking forward to having it in production.

Coming down the home stretch. Just three more comments.

Providing the file names is a great improvement. I like the way you have automated their generation.

Everything looks good.

Much more useful output to the user.

This LGTM. I will commit it Dec 9th if no objections are raised.

This LGTM.

I have sent you my edit of the manual page in a separate email rather than trying to put all my comments into the above draft.

Tried it out and had no problems and got expected errors.

Unless anyone has further feedback, I will plan to commit this early next week.

This is an impressive performance improvement and one well worth adding to the system.

This approach seems like the right way to get the extended errors back from the underlying devices.

Structurally, this looks good to me. I will await Phil's commentary on its JSON/XML usefulness.

Respond to latest comments.

Get rid of unneeded else statement.

Add comment on why it is not necessary to check for NULL return from prependdevtopath().

Responding to Dag-Erling's comments.

Respond to Dag-Erling's comments.

This looks good to me, but I will await a review from Johan before proceeding.

I will put in this change to fix the tests.
Pleas start a new review to address the additional suggestions.

Indeed this functionality is now embodied in ffs_search.

Fix is correct. Clearly a cut & paste blunder when I wrote it.

In D37615#1212734, @guest-jsollvander wrote:

In D37615#1212613, @guest-svmhdvn wrote:

This patch seems to break several testcases in the sys/geom/class/multipath/misc test suite with this output:

===> sys/geom/class/multipath/misc:add
Result:     failed: ACTIVE !=  (ACTIVE != )

I'll take a look.

I note that the sshd daemon is restarted when it is reinstalled. The sshd daemon is restarted since its update may be security related and thus the security fix should take effect as soon as the updated version is available.

First, sorry for letting this languish. Though I have not dealt with libxo, I assume that you have ensured that this generates the output that you need. Structurally it is properly added to the geom utility. If there are no issues raised by the other reviewers by Monday October 13th, I will commit it.

Mentor approval.

Mentor approval.

Looks ready to go.