-
Notifications
You must be signed in to change notification settings - Fork 290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rofiles: Add --copyup option #1382
Conversation
Depends: #1378 |
689fe28
to
6e06ee7
Compare
@@ -286,6 +333,9 @@ callback_truncate (const char *path, off_t size) | |||
static int | |||
callback_utimens (const char *path, const struct timespec tv[2]) | |||
{ | |||
/* This one isn't write-verified, we support changing times | |||
* even for hardlinked files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you expand on this? Don't we need to keep bare objects at OSTREE_TIMESTAMP
to be consistent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an interesting point...gets a bit into a philosophical question of whether rofiles-fuse is for "anything using hardlinks" or whether it's really specific to ostree. The "anything using hardlinks" case might be a backup system or non-os-data object store was my thought originally.
For example I could probably have "object semantics" for my ~/Documents
which has a bunch of random PDFs and .txt
and LibreOffice files etc. that should really never be overwritten in place. OTOH I've been thinking about storing all of that in git.
So...hmm. It's not a new issue here and we should think about changing this, but I worry a bit about the backcompat issues. In practice nothing we use rofiles-fuse for should be touching timestamps for files it didn't create at the time I would guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example I could probably have "object semantics" for my ~/Documents which has a bunch of random PDFs and .txt and LibreOffice files etc. that should really never be overwritten in place. OTOH I've been thinking about storing all of that in git.
I've been using git-annex for this for some time now. Good news, it's in fedora.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, git-annex is likely an excellent example of a possible user of rofiles-fuse semantics, specifically its direct mode. Though I didn't look closely.
src/rofiles-fuse/main.c
Outdated
if (opt_copyup) | ||
{ | ||
(void) close (fd); | ||
fd = openat (basefd, path, finfo->flags & ~O_TRUNC, mode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: we can just stop filtering out O_TRUNC
in the copy-up case, right? And only pay the ftruncate
call in the non-copy-up case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW I wish I'd seen this comment while working on this patchset on the plane O_TRUNC
was the source of bugs.
Sadly https://sourceware.org/bugzilla/show_bug.cgi?id=22089 is I think going to actually force us to cave here. Even if we got the glibc patch in today, we need to support the RHEL glibc. See also discussion about fish as part of the general Fedora tracker. This is basically needed to unblock rpm-ostree unified core 🌐: coreos/rpm-ostree#729 Closes: ostreedev#1377
a638c02
to
276f54d
Compare
276f54d
to
ab634a2
Compare
Consumer: coreos/rpm-ostree#1171 |
src/rofiles-fuse/main.c
Outdated
@@ -385,7 +385,12 @@ do_open (const char *path, mode_t mode, struct fuse_file_info *finfo) | |||
if (opt_copyup) | |||
{ | |||
(void) close (fd); | |||
fd = openat (basefd, path, finfo->flags & ~O_TRUNC, mode); | |||
/* Note that unlike the initial open, we will pass through | |||
* O_TRUNC. More ideally in this copyup case we'd avoid copying |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, so then we don't need to ftruncate
down below again, right? Shouldn't the if
condition at 399 be changed to
if (!opt_copyup && (finfo->flags & O_TRUNC))
?
* the whole file in the first place, but eh. It's not like we're | ||
* high performance anyways. | ||
*/ | ||
fd = openat (basefd, path, finfo->flags & ~(O_EXCL|O_CREAT), mode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we filtering out O_EXCL
and O_CREAT
here? This is something that should've been caught by the first openat
, right? Or is this just to not have a TOCTOU race on the file existing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're internally reopening the file we just created. So if the user provided O_EXCL
we need to filter that out. We might as well drop O_CREAT
too.
⚡ Test exempted: merge already tested. |
Sadly https://sourceware.org/bugzilla/show_bug.cgi?id=22089 is I think going to
actually force us to cave here. Even if we got the glibc patch in today, we need
to support the RHEL glibc. See also discussion about fish as part of the general
Fedora tracker.
This is basically needed to unblock rpm-ostree unified core 🌐:
coreos/rpm-ostree#729
Closes: #1377