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.

This all looks good to me. Glad to see more of Giant retired.

I concur that this change should be made.

A couple of points that I would like to add to this discussion:

In response to brooks. You are correct that this always allocates one block and initialises it to zero. Thus all zero-length directories will sort to the front of the list so that they will all be reported in a block.

See https://reviews.freebsd.org/D14163

I agree with the problem, but propose this new diff as the correct solution.

I concur with the problem, but differ on the solution. When I tried to upload my proposed change it created a whole new report https://reviews.freebsd.org/D14177. How do I just attach a new diff here?

I agree that the dependency needs to be added though I am not sure precisely how it should be done. I suggest added kib@ as a reviewer because he is up-to-date with the way that these sorts of dependencies should be indicated.

This is the correct fix.

Approve of latest change.

This looks like a sensible and long overdue change.

Given the issues that have been raised by adding the checkhash, I am thinking that it may be better not to enable it by default. If it is (for now) only enabled optionally, then folks that want to test it can do so and the rest of the users do not have to deal with the testing shake-out. I want to add other checkhashes for things like superblocks, inodes and possibly indirect blocks. Once these are working as a whole, then we can expand them to the larger population. In particular, I want to add the ability to detect when a filesystem is run on an earlier kernel so that we will expect the checkhashes to be wrong. I have just one bit to work with here, so I want to defer adding this until I can use it to cover all the checkhashes. But I think that this ability to detect running on earlier kernels is important to have for the wider population of users.

Using pfatal means that fsck -p (preen mode) will fail causing a reboot to drop into single user mode with a request that you run fsck manually. If you use pwarn, then a message will be generated but the system will continue to multi-user. I believe that pwarn is more appropriate in this case since you are not in danger of corrupting your filesystem when the checksum is wrong.

I believe that the correct solution is to make barrier writes work correctly as they can be used to solve a multitude of problems. Indeed I would use them more extensively in UFS if I believed that they worked. That said, I suggest a comment over the definition of the doasyncinodeinit that explains that synchronous inode initialization is only needed when barrier writes do not work.

This looks like an excellent improvement. I have not reviewed every line of code, but the concept is sound.