Conversation
An AsyncIo might outlive the device it is created from, so it is necessary for it to have a copy of the underlying file descriptor. The previous code used RawFd instead of BorrowedFd and so the problem was not detected. Also get rid of the BorrowedDiskFd struct. Its only purpose was to work around mutex.lock().unwrap().as_fd() returning a temporary BorrowedFd that is immediately invalid. The error from rustc is in fact correct here, as code with a mutable reference to what the mutex is protecting can replace it and thus drop the underlying file descriptor. This does not happen in VhdxSync, but the compiler cannot know this. Therefore, use mem::transmute to cast the lifetime to a longer one. Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
Generally AsRawFd is an antipattern outside of FFI. Use AsFd instead where possible. Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
phip1611
left a comment
There was a problem hiding this comment.
I originally authored struct BorrowedDiskFd as a pragmatic solution. Your proposed design is cleaner - thank you! Left some remarks
|
|
||
| impl AsFd for VhdxDiskSync { | ||
| fn as_fd(&self) -> BorrowedFd<'_> { | ||
| let wrong_lifetime_fd = self.vhdx_file.lock().unwrap(); |
There was a problem hiding this comment.
Generally, I think this PR is going into a great direction and I see why you are doing this here this way. This impl however feels a little to much like a hack 🤔
There was a problem hiding this comment.
What do you suggest instead?
There was a problem hiding this comment.
Store a dup'd OwnedFd in VhdxDiskSync at construction time:
lock_fd: OwnedFd, // file.as_fd().try_clone_to_owned()?
Then as_fd() / fd() just returns self.lock_fd.as_fd(). No mutex, no transmute, no unsafe. One extra fd per instance.
Thanks
|
|
||
| impl AsFd for VhdxDiskSync { | ||
| fn as_fd(&self) -> BorrowedFd<'_> { | ||
| let wrong_lifetime_fd = self.vhdx_file.lock().unwrap(); |
There was a problem hiding this comment.
Generally, I think this PR is going into a great direction and I see why you are doing this here this way. This impl however feels a little to much like a hack 🤔
| } | ||
| } | ||
|
|
||
| impl AsFd for VhdxDiskSync { |
There was a problem hiding this comment.
from commit message:
An AsyncIo might outlive the device it is created from, so it is
necessary for it to have a copy of the underlying file descriptor.
Please explain in the commit message why/when this is the case.
There was a problem hiding this comment.
I don’t know if it actually is the case! I suspect it is not, but this is a global invariant. This means that the block APIs are unsound.
weltling
left a comment
There was a problem hiding this comment.
Thanks for working on this. The direction is sound and the core fix for OwnedFd in AsyncIo is valuable. This also overlaps or goes in the same direction with the current refactoring efforts, while I was more on keeping things more general and reusable with various existing and upcoming disk formats.
IMO it would be better to keep AsFd off DiskFile. Semantically, a QCOW or VHDX image isn't a file descriptor and DiskFd names the purpose explicitly. The currently existing approach is close to the right design, it can be switched from BorrowedDiskFd+RawFd to the proper BorrowedFd. Fixing DiskFd to use BorrowedFd is cleaner than deleting it.
Thanks
The block layer did not use I/O safety as much as it could have.