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.

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?