Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dispose of/replace the cache #930

Closed
uweseimet opened this issue Oct 22, 2022 · 0 comments
Closed

Dispose of/replace the cache #930

uweseimet opened this issue Oct 22, 2022 · 0 comments
Labels
enhancement New feature or request major effort New feature/change with significant implementation effort

Comments

@uweseimet
Copy link
Contributor

uweseimet commented Oct 22, 2022

The current cache implementation (DiskCache/DiskTrack/CdTrack) does not appear to be efficient, and in some respects is counter-productive and blocks progress:

  • The cache is sector-based, which blocks resolving issues like Add support for READ LONG(10) and WRITE LONG(10) ? #479 and probably also ScsiController should not know anything about SCBR and SCDP #189, which deal with transfers of non sector-based data. BlueSCSI has no issue with READ/WRITE LONG because it does not appear to have a sector-based cache.
  • The cache cannot be switched off (raSCSI needs to periodically check disk write cache(s) #335)
  • The cache is inefficient, at least that's the result of some simple (!) tests I made: Without any cache and with directly reading data from the image file, reading is only about 3% slower. On the other hand, just adding a very naive (!) read-ahead cache of 4 sectors (see code snippet below) results in about 10% more speed. Strange: A more recent test shows that the current cache is more effective, probably related to the memory-mapped file.
  • The cache memory-maps the image file. This is not portable and might even make things slower, especially on a Pi with only 512 MB of memory. If you memory-map an image file of 512 MB, but only have 512 MB of RAM, and parts of this RAM are used as a cache (and for the OS etc., of course), what does this mean? Resolution: Digging deeper into this revealed that memory-mapping the file results in zero-copy semantics when reading/saving data.

Note: This ticket is not primarily about adding a new cache, but about discarding the existing one and maybe adding a very simple replacement.
A more sophisticated new cache might be added later, if it really makes a difference. This cache should be intelligent, e.g. it should have a configurable size. It might be a read-ahead or LRU cache. It might be a read/write or only a read cache. The caching policy might be configurable.
It should also be considered to completely remove the sector-oriented approach currently implemented, even though this is a major change. Data are read sector-by-sector, and a transfer of several sectors is split into several transfers of single sectors. Changing the code to work on byte sequences (in a separate ticket) would make it more flexible, would remove the overhead of splitting transfers, and may also make the code simpler.

A very naive POC of a read-ahead cache (4 sectors) used for testing:

unordered_map<uint32_t, vector<BYTE>> cache;

void StorageDevice::Read(vector<BYTE>& buf, uint64_t offset, uint32_t count)
{
	const auto& it = cache.find(offset);
	if (it != cache.end()) {
		memcpy(buf.data(), it->second.data(), count);
		cache.erase(it);
		return;
	}

	try {
		if (!in.is_open()) {
			in.open(filename, ios::binary);
		}

		in.seekg(offset);
		in.read((char *)buf.data(), count);

		// TODO Do not read beyond the end of the file, use zero-copy semantics
		cache.clear();
		vector<BYTE> next(count);
		in.read((char *)next.data(), count);
		offset += count;
		cache[offset] = next;
		in.read((char *)next.data(), count);
		offset += count;
		cache[offset] = next;
		in.read((char *)next.data(), count);
		offset += count;
		cache[offset] = next;
	}
	catch (const ifstream::failure&) {
		throw io_exception("Error reading " + to_string(count) + " byte(s) at offset " + to_string(offset)
				+ " of '" + filename + "'");
	}
}
@uweseimet uweseimet changed the title Discard the cache Dispose of the cache Oct 22, 2022
@uweseimet uweseimet added enhancement New feature or request architecture Software architecture and code quality labels Oct 22, 2022
@uweseimet uweseimet changed the title Dispose of the cache Dispose of/replace the cache Oct 23, 2022
@uweseimet uweseimet removed the architecture Software architecture and code quality label Oct 23, 2022
@uweseimet uweseimet added the major effort New feature/change with significant implementation effort label Nov 8, 2022
@uweseimet uweseimet closed this as not planned Won't fix, can't repro, duplicate, stale Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request major effort New feature/change with significant implementation effort
Projects
None yet
Development

No branches or pull requests

1 participant