[PATCH] Silence fsync() EINVAL error when saving on CIFS

[xeyow...@noekeon.org]

Hello,

Here a patch to fix the EINVAL error during 'bup save' when bup dir is
on a samba/cifs share:

bup save -r mybupserver:/path/to/bup -n some-backup /some/path
#
# Reading index: 115, done.
# Saving: 100.00% (327721/327721k, 115/115 files), done.
# Traceback (most recent call last):
# File "/usr/local/lib/bup/cmd/bup-server", line 209, in <module>
# cmd(conn, rest)
# File "/usr/local/lib/bup/cmd/bup-server", line 91, in
receive_objects_v2
# fullpath = w.close(run_midx=not dumb_server_mode)
# File "/usr/local/lib/bup/bup/git.py", line 790, in close
# return self._end(run_midx=run_midx)
# File "/usr/local/lib/bup/bup/git.py", line 776, in _end
# os.fsync(self.parentfd)
# OSError: [Errno 22] Invalid argument
# Traceback (most recent call last):
# File "/usr/local/lib/bup/cmd/bup-save", line 460, in <module>
# w.close() # must close before we can update the ref
# File "/usr/local/lib/bup/bup/client.py", line 317, in close
# id = self._end()
# File "/usr/local/lib/bup/bup/client.py", line 314, in _end
# return self.suggest_packs() # Returns last idx received
# File "/usr/local/lib/bup/bup/client.py", line 229, in _suggest_packs
# self.check_ok()
# File "/usr/local/lib/bup/bup/client.py", line 134, in check_ok
# % rv)
# bup.client.ClientError: server exited unexpectedly with code 1

This problem was first reported here:

https://groups.google.com/d/msg/bup-list/5rqPrlo_6h8/XYENSGjoAwAJ

Note that BorgBackup had the same issue, and was patched the same way
(see https://github.com/borgbackup/borg/issues/1287,
https://github.com/borgbackup/borg/pull/1288/commits/0005023a7354856fafcea144110bcc2b42accd40).


Regards,
Michaël

[Rob Browning]

xeyow...@noekeon.org writes:

> This problem was first reported here:
>
> https://groups.google.com/d/msg/bup-list/5rqPrlo_6h8/XYENSGjoAwAJ

(cf. id:377cc366-389b-454b...@googlegroups.com)

Ahh, right, thanks for tracking down the references.

> --- a/lib/bup/git.py
> +++ b/lib/bup/git.py
> @@ -774,6 +774,10 @@ class PackWriter:
> os.rename(self.filename + '.idx', nameprefix + '.idx')
> try:
> os.fsync(self.parentfd)
> + except OSError as e:
> + # Ignore EINVAL (*only*) since some fs don't support this (e.g. cifs).
> + if e.errno != errno.EINVAL:
> + raise

This does still seem plausible. Anyone have objections?

Of course we could log something as well if we wanted to be more
cautious, or -- fancier, we could fstat parentfd to get its device id,
and only log once per device per run, e.g.:

Filesystem containing ... may not allow durable renames (ignoring rejected fsyncs)

via something like

_fstat_rejection_warnings = set()
...
global _fstat_rejection_warnings
...
if e.errno != errno.EINVAL:
raise
st = os.fstat(self.parentfd)
if st.st_dev not in _fstat_rejection_warnings:
...etc...

But that's only worthwhile if there are situations where unexpected
EINVALs will mask cases we haven't hit yet that matter.

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