nfs: handling empty files within the cache

Description

First off, thanks for this project!

Now this might be bit of a convoluted edge case, but I wanted to validate my understanding.

Currently, nfs read-cache blob accounting treats empty file contents as size 0, because blobCache.EntrySize is defined as len(value) https://github.com/tailscale/gomodfs/blob/dfea0c215707c9d2893af2e9bb8c5457be3fbc6d/nfs.go#L601-L603

As a result, empty files contribute nothing to blobCache.Size(), while eviction is triggered only when:
fs.blobCache.Size() > fs.GetFileCacheSize()
https://github.com/tailscale/gomodfs/blob/dfea0c215707c9d2893af2e9bb8c5457be3fbc6d/nfs.go#L637

This means empty-file cache entries can accumulate (entCache grows infinitely) without creating eviction pressure from byte accounting.

I have a small test that validates this:

func TestNFSReadCacheEmptyFileAccounting(t *testing.T) {
	gitCacheDir := testGitDir(t)
	nh := testNFSHandler(t, gitCacheDir)

	nh.fs.FileCacheSize = 1

	for i := 0; i < 3; i++ {
		nh.fs.setReadCache(handle{}, nil, nil) // empty file
	}

	if got := nh.fs.blobCache.Size(); got != 0 {
		t.Fatalf("blob cache size = %d; want 0 for empty file blobs", got)
	}

	if got := nh.fs.MetricBlobEntrySize.Value(); got != 0 {
		t.Fatalf("MetricBlobEntrySize = %d; want 0 for empty file blobs", got)
	}

	if got := nh.fs.entCache.Len(); got != 0 {
		t.Fatalf("entCache length = %d after caching empty files; want 0", got)
	}

	if got := nh.fs.MetricFileEntryCount.Value(); got != 0 {
		t.Fatalf("MetricFileEntryCount = %d after caching empty files; want 0", got)
	}
}

Considerations

Should we skip caching empty files?

The change is straightforward as shown here

This makes the above test pass but introduces another question, what if there's an actual reliance on empty files? Not caching them would obviously degrade performance whenever they're accessed.

If we do want to cache, should we define a min size of each blob?

If we decide to do this, would there be other implications when it comes to distinguishing an actual file with minSize vs an empty file with the now, same minSize?

Improving eviction logic

Another alternative is to improve the eviction logic where the size isn't the only condition to evict. This could potentially look as follows.

	maxEntries := int((maxSize / 64) + 100)
	for fs.blobCache.Size() > maxSize || fs.entCache.Len() > maxEntries {
		_, v, ok := fs.entCache.DeleteOldest()
		if !ok {
			break
		}
		fs.decrBlobCountLocked(v.blobHash)
	}

We would still prefer the current eviction strategy of evicting by size but in addition adding the secondary check would ensure entCache doesn't grow infinitely.

Open to ideas!

Notes

While going through the constraints defined on files, I didn't observe a minimum size requirement. So having empty files is certainly possible.

---

Signed-off-by: Simar simar@linux.com

Start in nfs.go around blobCache.EntrySize, setReadCache, and the eviction check, then run or add the proposed TestNFSReadCacheEmptyFileAccounting. Compare empty-file behavior across blobCache and entCache, and confirm that the chosen resolution keeps cache accounting and eviction bounded without breaking valid empty-file reads.