* [U-Boot] [PATCH] tools/fw_env: use fsync to ensure that data is physically stored
@ 2013-05-20 19:34 Michael Heimpold
2013-05-21 12:47 ` Wolfgang Denk
2017-07-25 0:42 ` [U-Boot] " Tom Rini
0 siblings, 2 replies; 9+ messages in thread
From: Michael Heimpold @ 2013-05-20 19:34 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH] tools/fw_env: use fsync to ensure that data is physically stored
2013-05-20 19:34 [U-Boot] [PATCH] tools/fw_env: use fsync to ensure that data is physically stored Michael Heimpold
@ 2013-05-21 12:47 ` Wolfgang Denk
2013-05-21 17:34 ` Michael Heimpold
2017-07-25 0:42 ` [U-Boot] " Tom Rini
1 sibling, 1 reply; 9+ messages in thread
From: Wolfgang Denk @ 2013-05-21 12:47 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH] tools/fw_env: use fsync to ensure that data is physically stored
2013-05-21 12:47 ` Wolfgang Denk
@ 2013-05-21 17:34 ` Michael Heimpold
2013-05-22 14:32 ` Mats Kärrman
2013-05-22 15:33 ` Mark Jackson
0 siblings, 2 replies; 9+ messages in thread
From: Michael Heimpold @ 2013-05-21 17:34 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH] tools/fw_env: use fsync to ensure that data is physically stored
2013-05-21 17:34 ` Michael Heimpold
@ 2013-05-22 14:32 ` Mats Kärrman
2013-05-22 15:33 ` Mark Jackson
1 sibling, 0 replies; 9+ messages in thread
From: Mats Kärrman @ 2013-05-22 14:32 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH] tools/fw_env: use fsync to ensure that data is physically stored
2013-05-21 17:34 ` Michael Heimpold
2013-05-22 14:32 ` Mats Kärrman
@ 2013-05-22 15:33 ` Mark Jackson
2013-05-22 18:32 ` Michael Heimpold
1 sibling, 1 reply; 9+ messages in thread
From: Mark Jackson @ 2013-05-22 15:33 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH] tools/fw_env: use fsync to ensure that data is physically stored
2013-05-22 15:33 ` Mark Jackson
@ 2013-05-22 18:32 ` Michael Heimpold
0 siblings, 0 replies; 9+ messages in thread
From: Michael Heimpold @ 2013-05-22 18:32 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] tools/fw_env: use fsync to ensure that data is physically stored
2013-05-20 19:34 [U-Boot] [PATCH] tools/fw_env: use fsync to ensure that data is physically stored Michael Heimpold
2013-05-21 12:47 ` Wolfgang Denk
@ 2017-07-25 0:42 ` Tom Rini
1 sibling, 0 replies; 9+ messages in thread
From: Tom Rini @ 2017-07-25 0:42 UTC (permalink / raw)
To: u-boot
On Mon, May 20, 2013 at 09:34:42PM +0200, Michael Heimpold wrote:
> 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>
Applied to u-boot/master, thanks!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170724/a1aee0a4/attachment.sig>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH] tools/fw_env: use fsync to ensure that data is physically stored
2017-07-13 13:11 [U-Boot] [PATCH] " Vincent Prince
@ 2017-07-14 9:07 ` Vincent Prince
0 siblings, 0 replies; 9+ messages in thread
From: Vincent Prince @ 2017-07-14 9:07 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH] tools/fw_env: use fsync to ensure that data is physically stored
@ 2017-07-13 13:11 Vincent Prince
2017-07-14 9:07 ` Vincent Prince
0 siblings, 1 reply; 9+ messages in thread
From: Vincent Prince @ 2017-07-13 13:11 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-07-25 0:42 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-20 19:34 [U-Boot] [PATCH] tools/fw_env: use fsync to ensure that data is physically stored Michael Heimpold
2013-05-21 12:47 ` Wolfgang Denk
2013-05-21 17:34 ` Michael Heimpold
2013-05-22 14:32 ` Mats Kärrman
2013-05-22 15:33 ` Mark Jackson
2013-05-22 18:32 ` Michael Heimpold
2017-07-25 0:42 ` [U-Boot] " Tom Rini
2017-07-13 13:11 [U-Boot] [PATCH] " Vincent Prince
2017-07-14 9:07 ` Vincent Prince
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.