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

Do not update inode stats in real time - collect during dump #4337

Open
mykaul opened this issue Apr 18, 2024 · 4 comments
Open

Do not update inode stats in real time - collect during dump #4337

mykaul opened this issue Apr 18, 2024 · 4 comments

Comments

@mykaul
Copy link
Contributor

mykaul commented Apr 18, 2024

Just like other counters, I think it's the wrong approach. Count when you dump. All those 'inode->table->active_size++;' and 'inode->table->active_size--;' are not needed - as we take a lock when dumping anyway, let's just count the number of entries in the purge, lru and active lists.

Originally posted by @mykaul in #4335 (comment)

CC @mohit84

@jkroonza
Copy link
Contributor

Plus unless it's locked it's wrong anyway as it could be raced unless it's atomic_t types (which does a memory bus lock at time of read-update-store), and those you don't increment and decrement using ++ and/or -- operators.

@mykaul
Copy link
Contributor Author

mykaul commented Apr 18, 2024

Plus unless it's locked it's wrong anyway as it could be raced unless it's atomic_t types (which does a memory bus lock at time of read-update-store), and those you don't increment and decrement using ++ and/or -- operators.

But it done under lock. See

ret = pthread_mutex_trylock(&itable->lock);

@jkroonza
Copy link
Contributor

I think we do need the LRU inode entries in table kept as a counter because we also have LRU limit, which this counter is checked against to determine if we need to invalidate some entries. So in my opinion rather sack the extra cycle or two on insert/remove rather than re-counting every time we need to know how many entries there are (if we have 2m table size that's 32MB of memory being traversed minimum, trashing page caches on the CPU as we go, and that's without traversing the individual chains).

@mykaul
Copy link
Contributor Author

mykaul commented Apr 18, 2024

I was not suggesting to remove all counters. Only those that are relevant for dump time only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants