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

util.c: add fsync_close() helper, use where appropriate #268

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

Villemoes
Copy link

Using genimage directly on an eMMC on target, I'm seeing that the BLKRRPART ioctl fails with EBUSY, presumably because there's still lots of data in flight, and then subsequent parts of my bootstrap procedure fail because the expected partitions can't be found.

Whenever we've written some data, we really should ensure that all data has hit the disk before proceeding, and close() itself is not synchronous.

Add a fsync_close() helper that does exactly what it says on the tin. Use that wherever we close an fd that has been open for writing and actually written to (i.e., no point in doing that in the reload_partitions() function).

Currently, the return value of these close() calls are ignored, so at least for now continue to do that, but at least we do see an error message in case something went wrong.

@Villemoes
Copy link
Author

So the failing tests seem to be due to a "wrong" expected disk usage. But I do wonder how an added fsync() can change that - it's not really clear where the expected numbers come from. And in fact, when I just tried building the hdimage test case with current master, looked at du -B 1, I got the 61440 figure, but then simply going for a coffee and returning, running du -B 1 again shows me the 65536 that the test is now failing with; so the kernel has simply done its thing in the background and that has changed the disk usage. So I think the tests currently pass mostly because the du -B is run so shortly after genimage, but it doesn't really reflect what the "long term" disk usage is. IOW, I think adding this fsync() and adapting the test case numbers should also make the tests more robust and accurate.

Using genimage directly on an eMMC on target, I'm seeing that the
BLKRRPART ioctl fails with EBUSY, presumably because there's still
lots of data in flight, and then subsequent parts of my bootstrap
procedure fail because the expected partitions can't be found.

Whenever we've written some data, we really should ensure that all
data has hit the disk before proceeding, and close() itself is not
synchronous.

Add a fsync_close() helper that does exactly what it says on the
tin. Use that wherever we close an fd that has been open for writing
and actually written to (i.e., no point in doing that in the
reload_partitions() function).

Currently, the return value of these close() calls are ignored, so at
least for now continue to do that, but at least we do see an error
message in case something went wrong.

A test case is updated to reflect the new, and more accurate, disk
usage. It turns out that the lack of this fsync'ing has really created
images which later (long after genimage is done and the test framework
has accepted them) would increase in disk usage, i.e. once the kernel
gets around to write out any buffered data. With current master, one
can observe this:

  $ mkdir images input tmp
  $ dd if=/dev/zero of=input/part1.img bs=512 count=7 && dd if=/dev/zero of=input/part2.img bs=512 count=11 && touch input/part3.img
  $ ./genimage --outputpath=images --inputpath=input --rootpath=/no/such/dir --tmppath=tmp --config test/hdimage.config
  $ date ; du -B 1 images/test.hdimage-2
  Fri Nov  1 08:20:39 PM CET 2024
  61440   images/test.hdimage-2
  # Let time pass...
  $ date ; du -B 1 images/test.hdimage-2
  Fri Nov  1 08:21:10 PM CET 2024
  65536   images/test.hdimage-2

Signed-off-by: Rasmus Villemoes <[email protected]>
@Villemoes
Copy link
Author

I amended the commit to update the test case, and added a description of how to reproduce the current somewhat unexpected behaviour.

Copy link
Member

@michaelolbrich michaelolbrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, I did not expect an effect like this with the file size.

Anyways, this looks good.

@michaelolbrich michaelolbrich merged commit d785239 into pengutronix:master Nov 8, 2024
4 checks passed
@Villemoes Villemoes deleted the fsync branch November 8, 2024 15:08
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.

3 participants