search

DataDog / glommio

Glommio is a thread-per-core crate that makes writing highly parallel asynchronous applications in a thread-per-core architecture easier for rustaceans.
Other
2.93k stars 161 forks source link

don't test for file size on resize_dma_buffer #626

Closed glommer closed 7 months ago

glommer commented 7 months ago

We already test that the write is successful, and we already test that we read what we write.

In my local machine on an ext4 filesystem, the file size never changes after we write this. This is a metadata operation and the filesystem is free to do this optimization AFAIR, which it likely does, especially since this is a tmpfile that was not closed. We'd have to have a named file and fsync() the directory to make sure the metadata always gets updated, but I don't see the value in testing that.

What does this PR do?

A brief description of the change being made with this pull request.

Motivation

What inspired you to submit this pull request?

Related issues

A list of issues either fixed, containing architectural discussions, otherwise relevant for this Pull Request.

Additional Notes

Anything else we should know when reviewing?

Checklist

[] I have added unit tests to the code I am submitting [] My unit tests cover both failure and success scenarios [] If applicable, I have discussed my architecture

glommer commented 7 months ago

@vlovich FYI

vlovich commented 7 months ago

I believe you but my impression has always been that fsync is only a synchronization barrier for durability and otherwise has no effect on any operations unless there’s a kernel reboot involved. Also all these operations are on direct I/o fds so it’s not like any kernel caches impacted by sync should be involved right? I believe the size in particular does not need a directory fsync anyway as it’s considered what I call “data metadata” associated with the file itself (eg the man page says the file size is synchronized by fdatasync which makes sense for many reasons). Does adding an fdatasync before the file retrieval fix your issue? I feel like I’d like to better understand what’s happening here as it’s something subtle and potentially a lesson about filesystem stuff I haven’t learned yet, and it’s super annoying I have trouble reproducing this locally.

glommer commented 7 months ago

I'd have to look into ext4 is doing to comment properly. I think most likely this is due to the file being a temporary file. A test I could do is use a named file, then reopen it. At worst, this is a kernel bug on my particular version. But one way or another, I don't think the assert adds a lot of value, so removing is the way to go.