HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Allowing user space file-system to (kind-of) keep more than maximum number of files open

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
allowingspacefilemaximumnumberusersystemkeepmorethan

Problem

The linked code below is meant to be used in a user space file-system (fuse). Given that multiple processes may have multiple files opened on the file-system, the files on the specific user space file-system will map each of these open files to an (encrypted) file on a regular file-system, attempting to keep all mapped files open at the same time might fail pretty soon if there are many processes accessing the file-system. Doing an open->action->close on each file-system operation as an alternative would potentially have a rather big overhead.

The below container is meant to allow keeping a large set of files open while possible, hopefully the ones that are most likely to get once again accessed soon, and temporary low-level closing the ones that have been inactive for a while.

Is the design any good, or am I making bad assumptions that would result in trashing behaviour in real life usage scenarios? And if the design is OK, any other feedback on the code would be greatly appreciated.

```
template
class openfilecollection {
nodeType mNull;
uint64_t mLastHandle;
std::map mCollection;
std::map mFullyOpen;
std::deque mOpperQue;
uint64_t getFreeFhNumber() {
while ((mLastHandle == 0) || (mCollection.count(mLastHandle))) {
mLastHandle += 1;
}
return mLastHandle;
}
void cleanupQueFront() {
while ((mFullyOpen.count(mOpperQue.front()) == 0)||
(mFullyOpen[mOpperQue.front()]>1)||
(mOpperQue.size()>maxQueueSize)) {
if (mFullyOpen.count(mOpperQue.front()) != 0) {
mFullyOpen[mOpperQue.front()] -= 1;
if (mFullyOpen[mOpperQue.front()] == 0) {
mCollection[mOpperQue.front()].lowLevelClose();
mFullyOpen.erase(mOpperQue.front());
}
}
mOpperQue.pop_front();
}
}
void tempCloseIfNeeded() {
while (mFullyOpen.size() > maxOpenFiles) {

Solution

From a once-over:

Naming

-
You seem to use all kinds of conventions, all lowercase, sometimes you use an underscore, sometimes you use lowerCamelCase. Perhaps you should stick to the underscore_style.

-
The name mOpperQue is unfortunate, perhaps closing_candidates ?

  • Could be me, but operator seems unfortunate as well, the name does not at all give away that it will actually open the file.



  • tempCloseIfNeeded is unfortunate, I am not sure which part is temp. I would call it force_close_files.



DRY

-
cleanupQueFront and tempCloseIfNeeded share enough code that you could either make it 1 function or extract the common code into a utility function.

-
node_handle operator[](uint64_t fh) { and uint64_t open(Args&& ... args) { share the same code lines and comments, one more opportunity to merge or use a utility function.

Philosophy

-
I would much rather have a library that says Sorry, you can't have this open file, than a library that "willy nilly" can close a file so that I have to keep checking whether my node_handle is still valid.

-
By only checking the front for handlers that might need to be closed, you can create artificial limit problems, closing files that don't need to be closed.

-
If you called tempCloseIfNeeded first and enforced (mFullyOpen.size() > maxOpenFiles - 1 ) and then call lowLevelOpen(), then you would never violate maxOpenFiles.

Context

StackExchange Code Review Q#43782, answer score: 6

Revisions (0)

No revisions yet.