SPRandom Cache Size Behavior Implementation by cachyyyk · Pull Request #2044 · axboe/fio

cachyyyk · 2026-01-21T22:00:47Z

SPRandom Cache Size Behavior Implementation

Define a new variable spr_cs to set SPRandom Cache Size
Preserve original SPRandom invalidation behavior if spr_cs is
zero(default).
If spr_cs is non-zero then region X invalidations are done
after region X+1 writes are completed instead of before.
This creates an additional region size of distance between
the writes and associated invalidations.
Return an error if the defined cache size is greater than
the region size and tell the user the maximum number of
regions supported with the defined cache size

Attempting to implement #2043

vincentkfu · 2026-01-21T23:16:40Z

bbusacker · 2026-02-06T20:31:18Z

@axboe Any chance we can get someone to review this? The Micron team would like to get it merged if everything looks good.

axboe · 2026-02-06T21:38:56Z

Well, still waiting on @steven-sprouse-sndk to take a look at this. Maybe @tomas-winkler-sndk can help out?

Outside of that, you need to have a single commit in your list of commits, you have for some reason merged master back a few times. So please rebase on master. Secondly, your commit should line break at 72 chars or so, the lines are way too long. Third, and this happens all the time for some reason, people confuse the github PR message for the git commit message. Signed-off-by etc should go in the git commit message, it doesn't mean anything in here.

axboe · 2026-02-06T21:41:06Z

-	free(validity_dist);
-	free(spr_info->invalid_pct);
-	return -ENOMEM;
+	if (validity_dist)


You also have a lot of unrelated changes in your commit. A commit should do one thing, and one thing only. These aren't even needed, doing free(NULL) is perfectly valid.

Removed the unrelated and unnecessary null checks as part of the 2nd commit.

axboe · 2026-02-06T21:42:35Z

 Enabled by default.
 .TP
-.BI (rbd)rbd_encryption_format \fR=\fPstr
-Specifies the encryption format of the RBD image. Supported values are


And same for these - I do agree we should fix white space damage like that, it's a shame that made it into the original. But do these as a separate fix, as they really have nothing at all to do with the change you want to make.

Removed the unrelated whitespace changes as part of the 2nd commit.

axboe · 2026-02-06T21:43:32Z

Please sort out the review comments I posted. As a bonus, this also makes your main change easier to review. As a submitter, you should always make it as easy as possible for people to review. That increases the likelihood that it does get reviewed, and in turn, applied.

axboe · 2026-02-07T14:54:34Z

Don't have changes, and then a revert of some of those changes. If you want to address the whitespace issues, and that would indeed be great, do that first as a separate commit. Then you have a commit with your changes in it. Either that, or you just ignore the whitespace issues in the current documentation. In which case you'd want to squash those two commits into a single commit. And since the last commit is just fixing issues the main commit should've tackled, you should squash those too.

axboe · 2026-02-07T14:56:04Z

Main commit also still has lines that are way too long, and it's missing a Signed-off-by. The other two do have a Signed-off-by, but no empty line between the commit message and the SOB line. Please use git log or similar in a terminal in the fio git repo to see the expected format.

axboe · 2026-02-07T16:03:29Z

I was referring to the commit message... See previous comments on that. Please squash everything, it's turning into quite a mess at this point.

cachyyyk · 2026-02-07T16:17:49Z

Please sort out the review comments I posted. As a bonus, this also makes your main change easier to review. As a submitter, you should always make it as easy as possible for people to review. That increases the likelihood that it does get reviewed, and in turn, applied.

I was referring to the commit message... See previous comments on that. Please squash everything, it's turning into quite a mess at this point.

Thank you for the feedback. I believe I addressed the review comments. Sorry about the mess I haven't used git in some of the requested ways before. So the 3 additional commits should now be squashed into the first, PR is rebased to master, and the only commit here should now have a proper SOB line.

axboe · 2026-02-07T16:26:33Z

Getting closer! But as mentioned a few times, you commit message needs wrapping at 72 chars, or it's unreadable in normal git log. Just go git commit --amend and re-wrap your commit message at 72 chars, then force push that commit. We still need to review the new commit, but at least then the meta data is sane.

axboe · 2026-02-08T19:33:21Z

Just some minor nits, nothing major. I would still like the sprandom folks to sign off on this addition, which is somewhat orthogonal to the actual code in question.

Define a new SPRandom variable to allow for an SSD cache size(spr_cs) to be defined. Preserve original SPRandom invalidation behavior if spr_cs is zero(default). If spr_cs is non-zero then region X invalidations are done after region X+1 writes are completed instead of before. This creates an additional region size of distance between the writes and associated invalidations. Return an error if the defined cache size is greater than the region size and tell the user the maximum number of regions supported with the defined cache size. Attempting to resolve axboe#2043 Signed-off-by: Charles Henry <[email protected]>

steven-sprouse-sndk · 2026-02-08T23:46:57Z

@cachyyyk I understand the proposal and I think it will do what you want. Just to make sure I understand the implementation. You are not requiring any additional data structures, but just the timing of when the first list is replayed is delayed. So the net effect is there will be more memory in use when this new parameter is specified?

cachyyyk · 2026-02-09T01:43:59Z

@cachyyyk I understand the proposal and I think it will do what you want. Just to make sure I understand the implementation. You are not requiring any additional data structures, but just the timing of when the first list is replayed is delayed. So the net effect is there will be more memory in use when this new parameter is specified?

Correct. When spr_cs is non-zero popping from the invalid_buf will not occur until the end of the next region. So we need roughly 2x of the worst case invalidation size worth of space in this buffer to continue to add to the staging portion of the buffer until the commit portion of the buffer is reduced now at the end of the next region. For a 32 TB drive, 100 regions, OP of 15% that would change the total allocation from 145.38 MiB to 290.75 MiB.

steven-sprouse-sndk · 2026-02-09T22:39:13Z

@cachyyyk , @vincentkfu I am good with this feature enhancement.

cachyyyk · 2026-02-10T13:28:07Z

@axboe @steven-sprouse-sndk @vincentkfu Thank you for taking your time to review these changes.

bbusacker · 2026-02-10T15:29:50Z

@axboe @steven-sprouse-sndk @vincentkfu A shout out from the Micron team. Great collaboration and engagement! Thank you so much for your helpfulness as we dotted all the i's and crossed all the t's. Appreciate you guys!

axboe reviewed Feb 6, 2026

View reviewed changes

Comment thread sprandom.c

axboe reviewed Feb 6, 2026

View reviewed changes

Comment thread sprandom.c

axboe reviewed Feb 6, 2026

View reviewed changes

cachyyyk force-pushed the sprandom-cache-implementation branch from cc7c433 to b774517 Compare February 6, 2026 22:19

vincentkfu reviewed Feb 6, 2026

View reviewed changes

Comment thread HOWTO.rst

vincentkfu reviewed Feb 6, 2026

View reviewed changes

Comment thread fio.1 Outdated

cachyyyk force-pushed the sprandom-cache-implementation branch from d721433 to b727ef9 Compare February 7, 2026 16:13

cachyyyk force-pushed the sprandom-cache-implementation branch from b727ef9 to 54cde85 Compare February 7, 2026 16:32