use more FDs, avoid race conditions on active fs by ThomasWaldmann · Pull Request #4043 · borgbackup/borg

ThomasWaldmann · 2018-08-12T22:53:26Z

guess this is mostly done, fixes issues mentioned in #908 and #1038.

codecov-io · 2018-08-13T00:51:50Z

Codecov Report

Merging #4043 into master will increase coverage by 0.1%.
The diff coverage is 86.09%.

@@            Coverage Diff            @@
##           master    #4043     +/-   ##
=========================================
+ Coverage   84.29%   84.39%   +0.1%     
=========================================
  Files          37       37             
  Lines        9430     9467     +37     
  Branches     1572     1573      +1     
=========================================
+ Hits         7949     7990     +41     
+ Misses       1036     1034      -2     
+ Partials      445      443      -2

Impacted Files	Coverage Δ
src/borg/archiver.py	`81.93% <80%> (+0.38%)`	⬆️
src/borg/helpers/fs.py	`91.11% <81.25%> (-3.39%)`	⬇️
src/borg/archive.py	`84.06% <93.75%> (+0.34%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 091bd2b...23eeded. Read the comment docs.

src/borg/archive.py

ThomasWaldmann · 2018-08-15T16:05:11Z

this is how a borg create (py36) with the code of this PR currently looks like:

# 6 is the fd of the "docs" directory, now backing up "3rd_party" dir and contents:

newfstatat(6, "3rd_party", {st_mode=S_IFDIR|0775, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
openat(6, "3rd_party", O_RDONLY|O_NOFOLLOW|O_NOATIME|O_CLOEXEC|O_DIRECTORY) = 7
fstat(7, {st_mode=S_IFDIR|0775, st_size=4096, ...}) = 0
flistxattr(7, "", 4096)                 = 0
ioctl(7, FS_IOC_GETFLAGS, 0x7ffe824aff1c) = 0
fgetxattr(7, "system.posix_acl_access", NULL, 0) = -1 ENODATA (No data available)
fgetxattr(7, "system.posix_acl_default", NULL, 0) = -1 ENODATA (No data available)
openat(AT_FDCWD, "docs/3rd_party", O_RDONLY|O_NONBLOCK|O_CLOEXEC|O_DIRECTORY) = 9
fstat(9, {st_mode=S_IFDIR|0775, st_size=4096, ...}) = 0
getdents(9, /* 6 entries */, 32768)     = 160
getdents(9, /* 0 entries */, 32768)     = 0
close(9)                                = 0

newfstatat(7, "README", {st_mode=S_IFREG|0664, st_size=215, ...}, AT_SYMLINK_NOFOLLOW) = 0
openat(7, "README", O_RDONLY|O_NOCTTY|O_NONBLOCK|O_NOATIME|O_CLOEXEC) = 9
fstat(9, {st_mode=S_IFREG|0664, st_size=215, ...}) = 0
read(9, "Here we store 3rd party document"..., 8388608) = 215
fadvise64(9, 0, 0, POSIX_FADV_DONTNEED) = 0
read(9, "", 8388393)                    = 0
fadvise64(9, 0, 0, POSIX_FADV_DONTNEED) = 0
flistxattr(9, "", 4096)                 = 0
ioctl(9, FS_IOC_GETFLAGS, 0x7ffe824afc7c) = 0
fgetxattr(9, "system.posix_acl_access", NULL, 0) = -1 ENODATA (No data available)
fgetxattr(9, "system.posix_acl_default", NULL, 0) = -1 ENODATA (No data available)
close(9)                                = 0

# (... all the stuff in there ...)

close(7)                                = 0

ThomasWaldmann · 2018-08-15T16:16:16Z

same with py37 (using the new fd-based scandir()):

newfstatat(6, "3rd_party", {st_mode=S_IFDIR|0775, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
openat(6, "3rd_party", O_RDONLY|O_NOFOLLOW|O_NOATIME|O_CLOEXEC|O_DIRECTORY) = 7
fstat(7, {st_mode=S_IFDIR|0775, st_size=4096, ...}) = 0
flistxattr(7, "", 4096)                 = 0
ioctl(7, FS_IOC_GETFLAGS, 0x7fff0a3e595c) = 0
fgetxattr(7, "system.posix_acl_access", NULL, 0) = -1 ENODATA (No data available)
fgetxattr(7, "system.posix_acl_default", NULL, 0) = -1 ENODATA (No data available)
fcntl(7, F_DUPFD_CLOEXEC, 0)            = 9
fstat(9, {st_mode=S_IFDIR|0775, st_size=4096, ...}) = 0
fcntl(9, F_GETFL)                       = 0x78000 (flags O_RDONLY|O_LARGEFILE|O_NOFOLLOW|O_NOATIME|O_DIRECTORY)
fcntl(9, F_SETFD, FD_CLOEXEC)           = 0
getdents(9, /* 6 entries */, 32768)     = 160
getdents(9, /* 0 entries */, 32768)     = 0
lseek(9, 0, SEEK_SET)                   = 0
close(9)                                = 0

newfstatat(7, "README", {st_mode=S_IFREG|0664, st_size=215, ...}, AT_SYMLINK_NOFOLLOW) = 0
openat(7, "README", O_RDONLY|O_NOCTTY|O_NONBLOCK|O_NOATIME|O_CLOEXEC) = 9
fstat(9, {st_mode=S_IFREG|0664, st_size=215, ...}) = 0
read(9, "Here we store 3rd party document"..., 8388608) = 215
fadvise64(9, 0, 0, POSIX_FADV_DONTNEED) = 0
read(9, "", 8388393)                    = 0
fadvise64(9, 0, 0, POSIX_FADV_DONTNEED) = 0
flistxattr(9, "", 4096)                 = 0
ioctl(9, FS_IOC_GETFLAGS, 0x7fff0a3e56dc) = 0
fgetxattr(9, "system.posix_acl_access", NULL, 0) = -1 ENODATA (No data available)
fgetxattr(9, "system.posix_acl_default", NULL, 0) = -1 ENODATA (No data available)
close(9)                                = 0

st param was only given at the root paths of the recursion. we can just drop that and make the code simpler.

always open the file and then do all operations with the fd: - fstat - read - get xattrs, acls, bsdflags

also: - add and use OsOpen context manager - add O_NONBLOCK, O_NOFOLLOW, O_NOCTTY (inspired by gnu tar)

we'll add/remove some args soon, so many pos args would be just bad.

races via changing path components can be avoided by opening the parent directory and using parent_fd + file_name combination with *at style functions to access the directories' contents.

if scandir does not get a path, it can't prefix it in front of the filename in the direntries it returns, so dirent.path == dirent.name. thus, we just only use dirent.name and construct the full path.

acl_get: remove assumption that having an FD means it is a regular file, we try to use FDs a much as possible. only get the default acl for directories - other fs objects are not expected to have a default acl. the path needs to be encoded also for the case when we have an fd, it is needed to get the default acl for directories. also: micro-opt: encode path later, not needed for ISLNK check. acl_set: remove the "if False" branch, it is the same here: the fd-based api only supports access ACLs, but not default ACLs, so we always need to use the path-based api here.

for fd-based operations, we would have to open the file, but for char / block devices this has unwanted effects, even if we do not read from the device. thus, we use path (or dir_fd + name) based ops here.

on linux, acls are based on xattrs, so do these closeby: 1. listxattr -> keys (without acl related keys) 2. for all keys: getxattr 3. acl-related getxattr by acl library

scenario: - x is a regular file - borg does stat on x: is a regular file - so borg dispatches to process_file - attack: x gets replaced by a symlink (mv symlink x) - in process_file, borg opens x and must not follow the symlink nor continue processing as a normal file, but rather error in open() due to NOFOLLOW.

ThomasWaldmann · 2019-02-18T07:40:43Z

@verygreen ah, right. guess one can not change the file type of an existing inode (except with a sector editor maybe? :-) ).

is there any way / chance to kill the old inode and get a new one (with different file type) with the same inode number?

if not, considering this code is called once per file (fs item), I'll remove the file type check again.

ThomasWaldmann · 2019-02-18T07:42:51Z

(we also need to consider fs with synthetic inode numbers, like network fs for that)

verygreen · 2019-02-18T07:59:40Z

yes, technically inode numbers could be resused, so if you want to be 100% sure this did not happen, you need to look at inode number and generation, but generation apparently is not exposed outside of the kernel for whatever reason.

That said conditions of inode reuse in relatively short time should still be very rare even on synthetic fses (esp. there since there they typically just do like a constantly increasing counter).

Here is a real world example of inode reuse: GoogleCloudPlatform/gcsfuse#57

On ext3/4 the fs tries to avoid reusing recently deleted inodes unless you really don't have any other left. Probably many other filesystems do this too.

we must avoid a handler processing a fs item of wrong file type, so check if it has changed.

also: added a test for this.

ThomasWaldmann force-pushed the use-more-fds branch 2 times, most recently from 9879189 to 0bdeaf4 Compare August 13, 2018 00:51

ThomasWaldmann commented Aug 13, 2018

View reviewed changes

src/borg/archive.py Outdated Show resolved Hide resolved

ThomasWaldmann force-pushed the use-more-fds branch from 9d382b3 to 0559cdc Compare November 16, 2018 00:02

ThomasWaldmann mentioned this pull request Dec 16, 2018

borg 1.1.6 on macOS 10.12.6 (Sierra) is much slower than borg 1.0.12 - why? #3974

Closed

ThomasWaldmann force-pushed the use-more-fds branch from 0559cdc to 379303c Compare December 23, 2018 19:58

ThomasWaldmann force-pushed the use-more-fds branch from c394deb to a257690 Compare January 25, 2019 20:17

This was referenced Feb 12, 2019

speed up xattrs / acls / bsdflags back up #3955

Closed

should print a warning if file changed during backup #1750

Closed

ThomasWaldmann force-pushed the use-more-fds branch from a257690 to f1fb9d7 Compare February 12, 2019 13:42

ThomasWaldmann added 8 commits February 14, 2019 09:20

_process: remove an infrequently used micro-opt

dca04ba

st param was only given at the root paths of the recursion. we can just drop that and make the code simpler.

process_file: avoid race condition: stat data vs. content

677102f

always open the file and then do all operations with the fd: - fstat - read - get xattrs, acls, bsdflags

move/refactor Archive._open_rb function to helpers.os_open

8220c6e

also: - add and use OsOpen context manager - add O_NONBLOCK, O_NOFOLLOW, O_NOCTTY (inspired by gnu tar)

_process / process_*: change to kwargs only

ad5b9a1

we'll add/remove some args soon, so many pos args would be just bad.

use *at style functions (e.g. openat, statat) to avoid races

833c49f

races via changing path components can be avoided by opening the parent directory and using parent_fd + file_name combination with *at style functions to access the directories' contents.

when scandir gets called with an FD, dirent.path is not usable

66dd25e

if scandir does not get a path, it can't prefix it in front of the filename in the direntries it returns, so dirent.path == dirent.name. thus, we just only use dirent.name and construct the full path.

opening device files is troublesome, don't do it

85b711f

for fd-based operations, we would have to open the file, but for char / block devices this has unwanted effects, even if we do not read from the device. thus, we use path (or dir_fd + name) based ops here.

ThomasWaldmann force-pushed the use-more-fds branch from f1fb9d7 to 85b711f Compare February 14, 2019 08:20

ThomasWaldmann added 2 commits February 17, 2019 02:46

micro-opt: get xattrs directly before acls

39922e8

on linux, acls are based on xattrs, so do these closeby: 1. listxattr -> keys (without acl related keys) 2. for all keys: getxattr 3. acl-related getxattr by acl library

ThomasWaldmann mentioned this pull request Feb 17, 2019

Races between readdir/getdents64 and name access. #908

Closed

ThomasWaldmann added 2 commits February 20, 2019 09:16

check for stat race conditions, see borgbackup#908

ec17f0a

we must avoid a handler processing a fs item of wrong file type, so check if it has changed.

fix --read-special behaviour: follow symlinks pointing to special files

23eeded

also: added a test for this.

ThomasWaldmann force-pushed the use-more-fds branch from 4cda43d to 23eeded Compare February 20, 2019 09:13

ThomasWaldmann changed the title ~~WIP: use more FDs~~ use more FDs, avoid race conditions on active fs Feb 20, 2019

ThomasWaldmann merged commit d644323 into borgbackup:master Feb 22, 2019

ThomasWaldmann deleted the use-more-fds branch February 22, 2019 11:54

ThomasWaldmann mentioned this pull request Feb 22, 2019

Open file/dirs once and use fildes-based APIs instead of path-based APIs #1038

Closed

ThomasWaldmann mentioned this pull request Apr 6, 2019

1.1-maint vs master performance difference #4498

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

use more FDs, avoid race conditions on active fs#4043

use more FDs, avoid race conditions on active fs#4043
ThomasWaldmann merged 12 commits intoborgbackup:masterfrom
ThomasWaldmann:use-more-fds

ThomasWaldmann commented Aug 12, 2018 •

edited

Loading

Uh oh!

codecov-io commented Aug 13, 2018 •

edited

Loading

Uh oh!

Uh oh!

ThomasWaldmann commented Aug 15, 2018

Uh oh!

ThomasWaldmann commented Aug 15, 2018 •

edited

Loading

Uh oh!

ThomasWaldmann commented Feb 18, 2019 •

edited

Loading

Uh oh!

ThomasWaldmann commented Feb 18, 2019

Uh oh!

verygreen commented Feb 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

ThomasWaldmann commented Aug 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

ThomasWaldmann commented Aug 15, 2018

Uh oh!

ThomasWaldmann commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ThomasWaldmann commented Feb 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ThomasWaldmann commented Feb 18, 2019

Uh oh!

verygreen commented Feb 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ThomasWaldmann commented Aug 12, 2018 •

edited

Loading

codecov-io commented Aug 13, 2018 •

edited

Loading

ThomasWaldmann commented Aug 15, 2018 •

edited

Loading

ThomasWaldmann commented Feb 18, 2019 •

edited

Loading