Skip to content

Log errors for block device close() #2188

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

Closed
alindima opened this issue Oct 15, 2020 · 5 comments
Closed

Log errors for block device close() #2188

alindima opened this issue Oct 15, 2020 · 5 comments
Labels
Priority: Low Indicates that an issue or pull request should be resolved behind issues or pull requests labelled `

Comments

@alindima
Copy link
Contributor

alindima commented Oct 15, 2020

The file backing Firecracker's block device, like any other rust File is closed when going out of scope or when calling drop on the File, without any error handling: https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/fd.rs#L284.

This may be a design flaw of rust's File struct, since the linux man page states that:

Not checking the return value of close() is a common but nevertheless serious programming error. It is quite possible that errors on a previous write(2) operation are first reported at the final close(). Not checking the return value when closing the file may lead to silent loss of data.

One thing we can do to ensure that the subsequent close() will run successfully is calling fsync() on the file descriptor in advance.
This can be done as a Drop implementation for the DiskProperties member struct of the block device.

As far as I can tell, there is not much we can do to handle the potential error and data loss, apart from logging it, which may still be useful.

@alindima alindima added Quality: Improvement Priority: Low Indicates that an issue or pull request should be resolved behind issues or pull requests labelled ` labels Oct 15, 2020
@jkshtj
Copy link

jkshtj commented Oct 26, 2020

Hi, I'd be interested in working on this issue.

@alindima
Copy link
Contributor Author

alindima commented Oct 27, 2020

Hi and thank you for your interest. I will assign this issue to you.
If you need any guidance/help feel free to reach out to me on this issue or on Slack.

@berciuliviu
Copy link

Hey @Jain98! I wonder if you are still looking/working on this issue? If not, I could tackle it since I've got some time on my hands.
Thanks!

@jkshtj
Copy link

jkshtj commented Jan 12, 2021

@berciuliviu feel free to take up the issue.

@alindima alindima assigned berciuliviu and unassigned jkshtj Jan 12, 2021
@berciuliviu berciuliviu removed their assignment Jan 14, 2021
@berciuliviu
Copy link

Closed as duplicate to issue 2207

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Low Indicates that an issue or pull request should be resolved behind issues or pull requests labelled `
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants