arc_read()/arc_access() refactoring and cleanup.

[amotin]

ARC code was many times significantly modified over the years, that created significant amount of tangled and potentially broken code. This should make arc_access()/arc_read() code some more readable.

• Decouple prefetch status tracking from b_refcnt. It made sense originally, but became highly cryptic over the years. Move all the logic into arc_access(). While there, clean up and comment state transitions in arc_access(). Some transitions were weird IMO.
• Unify arc_access() calls to arc_read() instead of sometimes calling it from arc_read_done(). To avoid extra state changes and checks add one more b_refcnt for ARC_FLAG_IO_IN_PROGRESS.
• Reimplement ARC_FLAG_WAIT in case of ARC_FLAG_IO_IN_PROGRESS with the same callback mechanism to not falsely account them as hits. Count those as "iohits", an intermediate between "hits" and "misses". While there, call read callbacks in original request order, that should be good for fairness and random speculations/allocations/aggregations.
• Introduce additional statistic counters for prefetch, accounting predictive vs prescient and hits vs iohits vs misses.
• Remove hash_lock argument from functions not needing it.
• Remove ARC_FLAG_PREDICTIVE_PREFETCH, since it should be opposite to ARC_FLAG_PRESCIENT_PREFETCH if ARC_FLAG_PREFETCH is set. We may wish to add ARC_FLAG_PRESCIENT_PREFETCH to few more places.
• Fix few false positive tests found in the process.

How Has This Been Tested?

Lots of code reading, basic read/write tests with ZFS_DEBUG and lots of ZTS runs.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[shodanshok]

As this patch refactors arc_read() and arc_access(), any clue if it would affect #14120 ?

[amotin]

As this patch refactors arc_read() and arc_access(), any clue if it would affect #14120 ?

I saw that report of yours, but I am not certain about its causes. I don't think this patch should change MRU/MFU distribution (arc_p), especially after you disabled prefetch, that is mostly affected by this PR. So I don't think it would, but its hard to say for sure, its a quite tangled area.

[amotin]

@mmaybee @ahrens Could you please take a look on this PR? And in particular, could you explain me why prefetched read in arc_mru_ghost and especially arc_mfu_ghost states promoted buffer to arc_mru instead of arc_mfu? Don't you remember what thinking was behind it 16 years ago? Unfortunately message in the illumos commit 13506d1eefb is not descriptive.

[ahrens]

could you explain me why prefetched read in arc_mru_ghost and especially arc_mfu_ghost states promoted buffer to arc_mru instead of arc_mfu?

I think a prefetched read in arc_mru_ghost means that the block was read by a predictive prefetch, and then evicted before being demand-accessed. In that case it seems reasonable for a subsequent access to move it to arc_mru, since it was only demand-accessed once. However, I see that if we get a hit on a prefetched block in arc_mru, we move it to mfu. It seems odd to me that we would have a block that was only requested once by the user (but happens to have been correctly predictively prefetched) to be in the MFU.

It definitely make sense that a hit on a prefetched block in arc_mru_ghost or arc_mru should go to the same state, so the current logic seems wrong. But I would think it should go to arc_mru, not arc_mfu, because it was only "hit" once.

[amotin]

I think a prefetched read in arc_mru_ghost means that the block was read by a predictive prefetch, and then evicted before being demand-accessed. In that case it seems reasonable for a subsequent access to move it to arc_mru, since it was only demand-accessed once.

Thanks, @ahrens. I should have noticed that part myself. Fixed.

However, I see that if we get a hit on a prefetched block in arc_mru, we move it to mfu. It seems odd to me that we would have a block that was only requested once by the user (but happens to have been correctly predictively prefetched) to be in the MFU.

Neither my nor I think the existing code promotes previously prefetched block from MRU to MFU on demand hit. Only on following access, whether it will be another demand or another prefetch after the current demand. It may look like promotes only on demand requests could be right, but from anon or ghost we promote to MRU on any request, including prefetch, so the current logic is at least consistent.

[amotin]

As promised during the Developer Summit, I am working on uncached prefetch implementation. Since those two patches are going to overlap in arc.c, I'd appreciate some reviews for this one meanwhile. Thanks in advance.

[ghost]

The refactoring and cleanup and test changes look good. I have not so much expertise in the area to comment on the behavioral changes.

[behlendorf]

This all looks pretty reasonable and it's held up well in my local stress testing. I'll keep throwing interesting workloads at it to be certain, but right now this LGTM.

With the addition of the new iostat kstat entries we'll want to update the arcstat and arc_summary commands. Otherwise, we'll end up reporting slightly skewed results due to the ignored "iohit" counters.

[amotin]

Added sync_pool to couple more unstable tests.

[amotin]

Removed hash_lock argument from arc_evict_hdr(), noticed by @ryao.

[amotin]

Slightly refactor beginning of arc_access() as proposed by @ryao, and fix minor stats issues I spotted while at it.

[grwilson]

Why do we need to add an additional reference here? Maybe a comment indicating why.

[amotin]

To stop arc_access() from putting this header without any buffers and so other references but obviously nonevictable onto the evictable list of MRU or MFU state. Added comment.

[grwilson]

How can we guarantee that an I/O is not in progress?

[amotin]

Because header having reference will never appear on evictable headers list of any state. That is why I added references for in-progress headers.

[grwilson]

Thanks for the explanation.

[grwilson]

Why do we need two accesses to the callback?

[amotin]

To efficiently implement FIFO to keep original order. First pointer points to the first element, second -- to the acb_next of the last element.

[grwilson]

Is the ordering critical here? Just wondering about preserving the ordering vs the additional memory requirement for each l1hdr.

[amotin]

I think it may be. In many benchmarks I saw that requests arriving in order from different threads and blocked on the same indirect block are reaching the data in wrong order, confusing speculative prefetcher and potentially I/O scheduler and disks. Some time ago I made prefetcher to do speculations for read before reading indirect blocks, that significantly helped reads, but for writes it appeared difficult. I hope this change should if not fix the problem, but may be improve it a bit.

[grwilson]

I see. It is probably more efficient to make arc_callback_t have an extra pointer for the previous element so that we can process the callback list in reverse order in arc_read_done. This would mean that the memory penalty would only apply when we have a read in progress and not to every l1hdr and gives you the ordering you need. Would that suffice?

[amotin]

It could be cheaper for the header, indeed, but still could add one more traverse. I'll think about it closer tomorrow morning. But if we care so much about this structure, we could probably save much more by hiding b_freeze_* under ZFS_DEBUG.

[amotin]

Implemented. First traversal left reversed to not traverse extra time. Hope there will be no significant downsides. The callbacks though should be called in right order, that should be more important.

[grwilson]

Thanks! I also like the idea of putting b_freeze_* behind a debug flag to reduce the memory.