[U-Boot] [PATCH] tools/fw_env: use fsync to ensure that data is physically stored

[U-Boot] [PATCH] tools/fw_env: use fsync to ensure that data is physically stored

[Vincent Prince]

Hi all,

We had the same issue on our custom board, and this patch fixed it.

We use fw_setenv for updating our BSP, and before the patch, 100 boards
over 170 didn't work,
and after, 170/170 did get the update.

This patches worked well for us,
Thanks Michael

[U-Boot] [PATCH] tools/fw_env: use fsync to ensure that data is physically stored

[Vincent Prince]

For completness, here is the original post url :

https://patchwork.ozlabs.org/patch/245097/
https://lists.denx.de/pipermail/u-boot/2013-May/154752.html



2017-07-13 15:11 GMT+02:00 Vincent Prince <vincent.prince.fr@gmail.com>:

> Hi all,
>
> We had the same issue on our custom board, and this patch fixed it.
>
> We use fw_setenv for updating our BSP, and before the patch, 100 boards
> over 170 didn't work,
> and after, 170/170 did get the update.
>
> This patches worked well for us,
> Thanks Michael
>

[U-Boot] [PATCH] tools/fw_env: use fsync to ensure that data is physically stored

[Michael Heimpold]

Closing a file descriptor does not guarantee that the data has been
successfully saved to disk, as the kernel might defer the write.

Signed-off-by: Michael Heimpold <mhei@heimpold.de>
---
 tools/env/fw_env.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index 428e374..0420495 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -1033,7 +1033,19 @@ static int flash_io (int mode)
 
 		rc = flash_write (fd_current, fd_target, dev_target);
 
+		if (fsync (fd_current)) {
+			fprintf (stderr,
+				 "fsync failed on %s: %s\n",
+				 DEVNAME (dev_current), strerror (errno));
+		}
+
 		if (HaveRedundEnv) {
+			if (fsync (fd_target)) {
+				fprintf (stderr,
+					 "fsync failed on %s: %s\n",
+					 DEVNAME (dev_current), strerror (errno));
+			}
+
 			if (close (fd_target)) {
 				fprintf (stderr,
 					"I/O error on %s: %s\n",
-- 
1.7.10.4

[U-Boot] [PATCH] tools/fw_env: use fsync to ensure that data is physically stored

[Wolfgang Denk]

Dear Michael Heimpold,

In message <1369078482-5863-1-git-send-email-mhei@heimpold.de> you wrote:
> Closing a file descriptor does not guarantee that the data has been
> successfully saved to disk, as the kernel might defer the write.

What is the exact problem you are trying to fix?

I mean, when exactly does adding the sync play a role?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Old programmers never die, they just branch to a new address.

[U-Boot] [PATCH] tools/fw_env: use fsync to ensure that data is physically stored

[Michael Heimpold]

Hi Wolfgang Denx,

> > Closing a file descriptor does not guarantee that the data has been
> > successfully saved to disk, as the kernel might defer the write.
> 
> What is the exact problem you are trying to fix?
> 
> I mean, when exactly does adding the sync play a role?

I'm using fw_setenv during system update process. The sequence
of such a shell script is something like (much simplified):

...
fw_setenv state=2
dd if=... of=/dev/mmcblk0...
fw_setenv state=1
...
reboot

The (redundant) environment is stored in a eMMC flash.
The env var 'state' gives a hint to U-Boot whether/where the process
was interrupted.

So my intension is to be absolutely sure, that when fw_setenv returns,
the environment is written out to disk.

Best regards,
Michael

[U-Boot] [PATCH] tools/fw_env: use fsync to ensure that data is physically stored

[Mats Kärrman]

Hi Michael,

Michael Heimpold [mhei at heimpold.de] wrote:
> fw_setenv state=2
> dd if=... of=/dev/mmcblk0...
> fw_setenv state=1

How about:

fw_setenv state 1 && sync

BR // Mats

[U-Boot] [PATCH] tools/fw_env: use fsync to ensure that data is physically stored

[Mark Jackson]

On 21/05/13 18:34, Michael Heimpold wrote:
> Hi Wolfgang Denx,
> 
>>> Closing a file descriptor does not guarantee that the data has been
>>> successfully saved to disk, as the kernel might defer the write.
>>
>> What is the exact problem you are trying to fix?
>>
>> I mean, when exactly does adding the sync play a role?
> 
> I'm using fw_setenv during system update process. The sequence
> of such a shell script is something like (much simplified):
> 
> ...
> fw_setenv state=2
> dd if=... of=/dev/mmcblk0...
> fw_setenv state=1
> ...
> reboot

Not sure what final "OS" environment you're running, but I would think
that "reboot" would sync for you ?

For instance, under BusyBox, we have:-

# reboot --help
BusyBox v1.14.0 (2012-02-15 10:28:26 GMT) multi-call binary

Usage: reboot [-d delay] [-n] [-f]

Reboot the system

Options:
	-d	Delay interval for rebooting
	-n	No call to sync()
	-f	Force reboot (don't go through init)

... and under Ubuntu, we have ...

$ reboot --help
Usage: reboot [OPTION]...
Reboot the system.

Options:
  -n, --no-sync               don't sync before reboot or halt
...

So by default, reboot would (should ?) call sync automatically.

This might point to some other issue ?

Mark J.

[U-Boot] [PATCH] tools/fw_env: use fsync to ensure that data is physically stored

[Michael Heimpold]

Hi,

> > ...
> > fw_setenv state=2
> > dd if=... of=/dev/mmcblk0...
> > fw_setenv state=1
> > ...
> > reboot
> 
> Not sure what final "OS" environment you're running, but I would think
> that "reboot" would sync for you ?

I'm using OpenWRT and reboot links to the busybox implementation.
This implemenetation calls sync when I traced it correctly.

According to "man 2 sync":

<quote>
DESCRIPTION
       sync() causes all buffered modifications to file metadata and data to be written to the underlying file systems.
</quote>

When I use fw_setenv with /dev/mmcblk0, that means with a block device directly,
then I have a problem matching the "filesystem layer" of the description above with
the "block layer" which I am using.

Futhermore another quote from the very same man page:
<quote>
BUGS
       ...sync() schedules the writes, but may return before the actual writing is done.  However, since version  1.3.20  Linux
       does actually wait.  (This still does not guarantee data integrity: modern disks have large caches.)
</quote>

So it seems to me, that calling "sync" doesn't do the job.

When looking at "man 2 fsync" I read 
<quote>
... This includes writing through or flushing a disk  cache  if
present.  The call blocks until the device reports that the transfer has completed....
</quote>

This looks much better.

However, I did not trace the call chain in linux kernel down to the block layer yet.
Maybe I should.

BR, Michael