[PATCH 1/1] Make os.fsync() failure non-fatal. Needed on mounted CIFS file systems.

[Rob Browning]

From: Dov Grobgeld <dov.gr...@gmail.com>

Signed-off-by: Dov Grobgeld <dov.gr...@gmail.com>
---

Thanks for the help. Redirected to bup-list as-per HACKING from
pull request https://github.com/bup/bup/pull/38

lib/bup/git.py | 2 ++
1 file changed, 2 insertions(+)

diff --git a/lib/bup/git.py b/lib/bup/git.py
index f39e9a34..288c3f89 100644
--- a/lib/bup/git.py
+++ b/lib/bup/git.py
@@ -812,6 +812,8 @@ class PackWriter:
os.rename(self.filename + '.idx', nameprefix + '.idx')
try:
os.fsync(self.parentfd)
+ except:
+ pass
finally:
os.close(self.parentfd)

--
2.15.1

[Rob Browning]

Rob Browning <r...@defaultvalue.org> writes:

> --- a/lib/bup/git.py
> +++ b/lib/bup/git.py
> @@ -812,6 +812,8 @@ class PackWriter:
> os.rename(self.filename + '.idx', nameprefix + '.idx')
> try:
> os.fsync(self.parentfd)
> + except:
> + pass
> finally:
> os.close(self.parentfd)

Hmm, offhand I suspect we might not want to ignore all exceptions (and
we might also want to controlling the suppression via an option of some
kind if we can't reliably detect the situation).

But first, if this is easy for you to reproduce, what does the exception
look like?

Thanks
--
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

[dov.gr...@gmail.com]

Here is a trace back of doing this manually:

In [7]: os.fsync(fd)
---------------------------------------------------------------------------
OSError                                   Traceback (most recent call last)
<ipython-input-7-fa52f1dffc25> in <module>()
----> 1 os.fsync(fd)

OSError: [Errno 22] Invalid argument

So I can narrow down my patch to only catch OSError if you prefer? Or do you want me to narrow it down further to only errno=22?

Thanks!

[Rob Browning]

dov.gr...@gmail.com writes:

> So I can narrow down my patch to only catch OSError if you prefer? Or do
> you want me to narrow it down further to only errno=22?

Looks like both Linux and POSIX agree that EINVAL just indicates that
the underlying filesystem "can't do it", and Linux may also report EROFS
if the filesystem is read-only:

POSIX:
http://pubs.opengroup.org/onlinepubs/009695399/functions/fsync.html

Linux:
EBADF fd is not a valid open file descriptor.
EIO An error occurred during synchronization.
EROFS, EINVAL
fd is bound to a special file (e.g., a pipe, FIFO, or socket)
which does not support synchronization.

So it sounds plausible to quash EINVAL (and if it exists, EROFS). One
possibliity might be something like:

if hasattr(errno, 'EROFS'):
fsync_notsup_errnos = (errno.EINVAL, errno.EROFS)
else
fsync_notsup_errnos = (errno.EINVAL,)

...


if ex.errno in fsync_notsup_errnos:
...

(Oh, and you're welcome to update your PR branch if you prefer that to
posting the changes here (in which case, I'll forward them for you).)

[Greg Troxel]

Rob Browning <r...@defaultvalue.org> writes:

> So it sounds plausible to quash EINVAL (and if it exists, EROFS). One
> possibliity might be something like:
>
> if hasattr(errno, 'EROFS'):
> fsync_notsup_errnos = (errno.EINVAL, errno.EROFS)
> else
> fsync_notsup_errnos = (errno.EINVAL,)
>

Why would we doing fsync on a filesystem that we can't write to?
Wouldn't a previous write have failed? Or do we fsync after we might
have written, so this can arise if we didn't actually write?

[Rob Browning]

Greg Troxel <g...@lexort.com> writes:

> Why would we doing fsync on a filesystem that we can't write to?
> Wouldn't a previous write have failed? Or do we fsync after we might
> have written, so this can arise if we didn't actually write?

Doh -- of course, you're completely right; we should just accommodate
EINVAL and rethrow EROFS like anything else.