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

Add handle-specific attr and fsync interfaces #226

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MikaelSmith
Copy link

Getattr and Setattr requests can either happen on a fs.Node or an fs.Handle (depending on the call that initiated it e.g. truncate vs ftruncate or stat vs fstat). Add HandleGetattrer and HandleSetattrer so implementations can provide handle-specific overrides when those operations are performed on an open file descriptor.

Also create HandleFsyncer and deprecate NodeFsyncer to resolve the TODO.

Resolves #225.

Getattr and Setattr requests can either happen on a fs.Node or an
fs.Handle (depending on the call that initiated it e.g. truncate vs
ftruncate or stat vs fstat). Add HandleGetattrer and HandleSetattrer so
implementations can provide handle-specific overrides when those
operations are performed on an open file descriptor.

Also create HandleFsyncer and deprecate NodeFsyncer to resolve the TODO.

Resolves bazil#225.
@tv42
Copy link
Member

tv42 commented Jan 17, 2020

I like the general idea and thank you for contributing.

I'm not yet convinced that NodeFsyncer can be deprecated because I fear we can get handleless sync requests -- would need to experiment with syncfs(2).

All these Handle* variants should probably come with a note to implement the Node* version too because there's plenty of path-based syscalls that don't use a handle -- and that's why I'm still not really convinced these are very useful, as we talked about in #225. This data belongs in the node, even if it's being accessed though a handle, and only something like /proc with it's special files can depart from that (most importantly, can't use pagecache). I see this most valuable to e.g. a bandwidth limited remote FS to try to do fair queueing -- but even for that, uid/gid/pid are probvably more useful. As you said, these theoretically should be there.

Also, are these the first Node* / Handle* interface pairs with identical contents? Maybe that should mean something for the naming, maybe we don't need both. But even worse: Implementing Node and Handle with the same value is legal, and useful for simple FSes. With these interfaces, we can't even tell which one is being called. That sounds weird. (Apart from inspecting req.Handle != 0 but that's really too low-level for Node/Handle method to be looking at.)

Finally, this for sure isn't going in without tests.

@MikaelSmith
Copy link
Author

That all makes sense. In my case I ended up returning an error if you try to call Setattr without a valid handle. And I was able to track handles adequately on things that were modified without any of these changes. So my need for them has gone away.

Having FSync on the node instead of handle still feels weird, but if you're not confident we can deprecate and remove the node version then that's kind of a pointless change as well. Sounds like this isn't worth following up on at the moment.

@tv42
Copy link
Member

tv42 commented Jan 17, 2020

I want to leave this open, just with low priority. This is all good input for future API changes.

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

Successfully merging this pull request may close these issues.

No clear way to use HandleID from Getattrer or Setattrer
2 participants