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

changed fds to array of pointers #21

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

nagyation
Copy link
Contributor

@nagyation nagyation commented May 22, 2018

added fd_count to count the number of childs or others who are using the fd, so won't deallocate it unless no one is using it.

still working on it to enhance it

@nagyation nagyation closed this May 22, 2018
@nagyation nagyation reopened this May 22, 2018
@thethumbler
Copy link
Owner

You can't use inode->ref because multiple file descriptors can point to the same inode, use file->ref instead. Some parts need to be reworked, but overall, good work.

And Nagy, consistency, please :"D

@nagyation
Copy link
Contributor Author

Yea I was going through the code to understand more, at first I used fd_count, but okay I will rename it to ref if that matches the consistency 😂
And really I try my best to match with your code style,
And we can make a test for that if possible would be great :'D

@nagyation
Copy link
Contributor Author

I was thinking about caching the fd instead of kfree it, for not wasting a lot of time allocating and deallocating

@thethumbler
Copy link
Owner

thethumbler commented May 23, 2018

@nagyation caching is only effective if the resource is used frequently, applications don't usually open/close files 1000 times a second, so caching would be an over complication, and mostly a waste.

Also we would use SLxB allocator later, the caching would be embedded in the allocator itself for many object types.

About ref, it's literally there :"D I was gonna do it but had more important stuff to work on :"D

@nagyation
Copy link
Contributor Author

@mohamed-anwar check it now 🤔

@thethumbler
Copy link
Owner

@nagyation checked it, will pull it when I push my local changes. Nice work!

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

Successfully merging this pull request may close these issues.

2 participants