Note that update of manual page date fell out on corrective update.

Looks good.

As noted, date at top of manual pages needs to be updated (which I will admit, I failed to do when I added underscores).

Update the intro(2) manual page to better reflect the meaning as suggested by imp, cem, etal.

In D18765#401317, @imp wrote:

In D18765#401272, @mckusick wrote:

At a minimum EINTEGRITY is useful for mount since it allows the mount command to notify the user that the mount failed because of an integrity error which very likely can be fixed with a run of fsck versus an I/O error which probably cannot.

We should make mount return a different exit value for this perhaps... We could use that to automate a running of fsck in some cases.

EINTEGRITY means the end-to-end notion of what should be there isn't. This is similar to EIO (can't read the sector). The integrity error, though, means that some can't fail sanity check failed, which is a larger class of errors.

At a minimum EINTEGRITY is useful for mount since it allows the mount command to notify the user that the mount failed because of an integrity error which very likely can be fixed with a run of fsck versus an I/O error which probably cannot.

Based on feedback from Ed Maste and Ed Schouten do not add EINTEGRITY to CloudABI. Rather map it to EINVAL as is done in Linux compatibility.

It is useful to the application or user to distinguish EINTEGRITY from EIO. With EIO they know that the underlying media has failed and there is really nothing they can do. With EINTEGRITY the individual file has become corrupted, but if they still have its contents, they can remove it and rewrite it as the filesystem media itself is still working. Alternatively they can request the system administrator to run a check of the filesystem to attempt to clean up the file.

Mount is just one of the places that can return EINTEGRITY. It can also come back when accessing a file or directory whose meta information fails a check-hash.

Update diffs based on reviewer comments:
gnn - corrections to File05 cddl/lib/libdtrace/errno.d
dim - drop unneeded changes to sanitizer, previously File14 and File15
Drop dteske as reviewer as gnn has reviewed the DTrace change

When a mount fails because of a hash-check failure it is helpful to get "Integrity check failed" rather than "Invalid Argument" back. There are numerous reasons that you can get "Invalid argument" including miss-typed disk or mount-point names and other administrative errors. Knowing that it was due to an integrity check gives a strong indication that a run of fsck is needed. Actually in the case of mount we have the ability to print a more complete message, but the low-level functions in the filesystems need to be able to return something other than EINVAL so that the mount-layer function can know how to formulate its error message.

In D18765#400620, @dim wrote:

The libc++ changes look OK to me, and I'll take care of sending them upstream.

In D18765#400403, @cem wrote:

This proposal is to create a new EINTEGRITY error number. EINTEGRITY's purpose is to respond to errors where an integrity check such as a check-hash or a cross-correlation has failed. For example, an uncorrectable error has been found in a filesystem or a disk subsystem such as gmirror(8) or graid3(8).

How do you envision making use of this error code? Does it meaningfully differ from EIO? In the case of gmirror in particular, AFAIK it cannot determine bitrot and relies on the underlying volume to signal a bad block with EIO (which it then restarts on any remaining component, I think).

(I believe we could use a tighter integration between UFS and underlying geoms like gmirror/graid, I'm just not sure how this error number fits in to that.)

Thanks!

I'll take a look at the lua bits soon.

This change now looks good to me.

Given that this plausibly works only with two different /etc/fstab files, I think your change should also include an update to the manual page to explain how the update keyword is intended to be used.

This change does not seem wrong, though the only way I can see it being usable is to have the read-only mount's in /etc/fstab, then a special fstab that has the desired update mount lines in it. Because if the update mount's are in /etc/fstab, then for 'mount -a' the read-only mount will happen immediately followed by the read-write mount. Though I suppose that the update mount line could appear first (and fail) followed by the read-only mount which would presumably succeed. Then on the second 'mount -a' the update would succeed and the read-only would be skipped because it was already mounted. But this seems like a really obscure way of doing things. Plus you would have to change mount so that would not exit when the initial "ro,update" failed.

This now looks reasonable to me.

This now looks good to me. I concur with kib's followup comments.

Generally this looks reasonable.

OK, keeping it as currently specified is fine. I was concerned that using it as an lseek parameter might be an issue for ZFS,and OneFS, but given that it appears that these filesystems are fine with that tells me that the above implementation works fine.

My intent when adding the d_off field was that it be used for this purpose, so I am glad to see someone actually making it happen.

I prefer not to do things halfway. Either we use lseek/read or we use pread. Having it half & half is just crappy coding style. It is possible to write some tests that will exercise the error cases, so if you want to make this change, then I would like you to do it completely.

In answer to kib:

This change looks fine to me.

This seems like the correct approach to me.

The idea of channelling everything through b_iodone is aesthetically pleasing, but impractical. Adding an extra indirect call to every buffer operation introduces a lot of overhead for a very rare event. It also requires changes to every filesystem that uses the buffer cache (which is most of them). So, the idea of adding a function that gets called only when a disk goes offline that can have a default function that keeps the current (albeit broken) behavior of redirtying the buffer is both efficient and requires far less code churn.

As kib has pointed out, much of UFS is not prepared to deal with failure.

On further thought, I realize that while we (correctly) deal with the B_PAGING panic, we simply replace it with the "dangling dependency" panic in UFS along with about 100 UFS metadata corruption panics that occur if the I/O fails when writing a UFS metadata block. So, at a minimum we need to make a callback to the filesystem when we are going to fail a write so that it can take some sort of defensive action (such as forcible unmount). This can be done here where we know that we are about to start the cascade or perhaps in buf_free where we should call buf_deallocate if the reason for the buffer being freed is that it had a failed write.

I am fine with this change.

I am not the right person to review the ZFS-specific parts of this change. But the approach of avoiding recursive invocations of getnewvnode is needed and this change accomplishes that requirement.

This looks good to me. I'll change the M_TEMP to M_TRIM when I check my changes into the tree.