{"thread":{"id":"55282","subject":"[RFC PATCH] merge-recursive: create new files with O_EXCL","startedAt":"2021-03-10T07:58:26Z","lastAt":"2021-03-13T01:09:06Z","messageCount":6,"participants":["Ephrim Khong","Junio C Hamano","Elijah Newren","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"418681","messageId":"f6cd9386-8a58-ee52-4c7b-60d9bd14a51d@gmail.com","threadId":"55282","inReplyTo":null,"subject":"[RFC PATCH] merge-recursive: create new files with O_EXCL","fromName":"Ephrim Khong","fromEmail":"dr.khong@gmail.com","sentAt":"2021-03-10T07:57:43Z","receivedAt":"2021-03-10T07:58:26Z","isPatch":true,"sender":{"key":"dr.khong@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2847265?v=4"},"body":"When re-writing the content of a file during a merge, the file\nis first unlinked and then re-created. Since it is a new file\nat the time of the open call, O_EXCL should be passed to clarify\nthat the file is expected to be new.\n\nSigned-off-by: Ephrim Khong \n---\nThis is actually a fix for an issue we ran into on an nfs4\nmount. Files created with O_TRUNC instead of O_EXCL sometimes\nhad their permissions wrong. However, it appears to be a safe\nthing to change this, especially since other parts of the\ngit codebase also prefer the O_EXCL flag.\n\n merge-recursive.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex f736a0f632..f72a376c5b 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -974,7 +974,7 @@ static int update_file_flags(struct merge_options *opt,\n \t\t\tint fd;\n \t\t\tint mode = (contents->mode & 0100 ? 0777 : 0666);\n\n-\t\t\tfd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode);\n+\t\t\tfd = open(path, O_WRONLY | O_EXCL | O_CREAT, mode);\n \t\t\tif (fd < 0) {\n \t\t\t\tret = err(opt, _(\"failed to open '%s': %s\"),\n \t\t\t\t\t path, strerror(errno));\n-- \n2.30.1.1.g07d5ea6f42.dirty\n\n"},{"id":"418748","messageId":"xmqqblbqipeh.fsf@gitster.g","threadId":"55282","inReplyTo":"f6cd9386-8a58-ee52-4c7b-60d9bd14a51d@gmail.com","subject":"Re: [RFC PATCH] merge-recursive: create new files with O_EXCL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-10T23:01:58Z","receivedAt":"2021-03-10T23:02:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ephrim Khong writes:\n\n> When re-writing the content of a file during a merge, the file\n> is first unlinked and then re-created. Since it is a new file\n> at the time of the open call, O_EXCL should be passed to clarify\n> that the file is expected to be new.\n>\n> Signed-off-by: Ephrim Khong \n> ---\n> This is actually a fix for an issue we ran into on an nfs4\n> mount. Files created with O_TRUNC instead of O_EXCL sometimes\n> had their permissions wrong. However, it appears to be a safe\n> thing to change this, especially since other parts of the\n> git codebase also prefer the O_EXCL flag.\n\nInteresting. \n\nIt seems that this part of the code has always used O_TRUNC since\nits inception at 6d297f81 (Status update on merge-recursive in C,\n2006-07-08) when two thieves smuggled it into our codebase. Back\nthen, this was the only use of O_TRUNC|O_WRONLY|O_CREAT combination\nin the production codebase (as you observed, O_CREAT|O_EXCL|O_WRONLY\nwas what all the other codepaths use). But the use of O_TRUNC has\nspread over time to other parts of the codebase, perhaps cargo\nculted. If you look at hits from\n\n $ git grep -e O_TRUNC -e O_EXCL\n\nyou see the combination of CREAT/WRONLY/TRUNC used all over the\nplace [*], especially in newer parts of the code.\n\nSo it becomes very curious why this one location needs to be so\nspecial and you are not patching other uses of O_TRUNC.\n\nI do not think we mind fixing the use of O_TRUNC with \"remove and\nthen create with O_EXCL\", but we'd probably want to\n\n * understand why only this place matters, or perhaps other uses of\n O_TRUNC needs the same fix to work \"correctly\" with your NFS\n mounts, in which case we'd need all of them addressed in the same\n series of patches, and\n\n * understand why your NFS mount is broken and give a better\n explanation as to why we need to have a workaround in our code.\n\nbefore doing so.\n\nThanks.\n\n\n[Footnote]\n\n* It is understandable that CREAT/WRONLY/TRUNC are used in\n combination, as that is documented as a synonym for creat().\n\n\n\n\n> merge-recursive.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index f736a0f632..f72a376c5b 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -974,7 +974,7 @@ static int update_file_flags(struct merge_options *opt,\n> \t\t\tint fd;\n> \t\t\tint mode = (contents->mode & 0100 ? 0777 : 0666);\n>\n> -\t\t\tfd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode);\n> +\t\t\tfd = open(path, O_WRONLY | O_EXCL | O_CREAT, mode);\n> \t\t\tif (fd < 0) {\n> \t\t\t\tret = err(opt, _(\"failed to open '%s': %s\"),\n> \t\t\t\t\t path, strerror(errno));\n"},{"id":"418835","messageId":"cd7c6682-7409-f72c-8751-02b70a423f83@gmail.com","threadId":"55282","inReplyTo":"xmqqblbqipeh.fsf@gitster.g","subject":"Re: [RFC PATCH] merge-recursive: create new files with O_EXCL","fromName":"Ephrim Khong","fromEmail":"dr.khong@gmail.com","sentAt":"2021-03-11T09:54:41Z","receivedAt":"2021-03-11T09:55:35Z","isPatch":true,"sender":{"key":"dr.khong@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2847265?v=4"},"body":"On 11.03.2021 00:01, Junio C Hamano wrote:\n> Ephrim Khong writes:\n> \n>> When re-writing the content of a file during a merge, the file\n>> is first unlinked and then re-created. Since it is a new file\n>> at the time of the open call, O_EXCL should be passed to clarify\n>> that the file is expected to be new.\n>\n> But the use of O_TRUNC has\n> spread over time to other parts of the codebase, perhaps cargo\n> culted. If you look at hits from\n> \n> $ git grep -e O_TRUNC -e O_EXCL\n> \n> you see the combination of CREAT/WRONLY/TRUNC used all over the\n> place [*], especially in newer parts of the code.\n> \n> So it becomes very curious why this one location needs to be so\n> special and you are not patching other uses of O_TRUNC.\n\nThe main difference is that this is the only place where the file mode\nmatters, since we want the executable bit set for some files in the\nworking directory. All other locations create the files with mode 0600\nor 0666 and are happy as long as the user has rw rights, which appears\nto be the case even when running into my nfs-issue.\n\n> I do not think we mind fixing the use of O_TRUNC with \"remove and\n> then create with O_EXCL\", but we'd probably want to\n> \n> * understand why only this place matters, or perhaps other uses of\n> O_TRUNC needs the same fix to work \"correctly\" with your NFS\n> mounts, in which case we'd need all of them addressed in the same\n> series of patches, and\n\nI'd say that is up to you. Personally I'd rather err on the side of\ncaution and leave the other code as is, especially since it is not\nreally required for the file mode issue as described above. But I'd\nhappily patch those places as well if you find that to be a better idea.\nMaybe refactoring this (fstat - unlink - open) into a common function\nwould make sense then.\n\n> * understand why your NFS mount is broken and give a better\n> explanation as to why we need to have a workaround in our code.\n\nI'll work on this, but unfortunately have no idea of how to properly\ndebug it. Since it is a company server without administrative rights and\nthe backend is some IBM storage system, the options are limited and\nprocesses are slow. What I did find out so far is that it is not a race\ncondition with unlink. A simple openat() without O_EXCL already produces\nthe wrong file mode.\n\n(I fully understand that this is not a bug on git's side, and I found no\ndocumentation indicating that O_EXCL would be recommended or have any\neffect in this way. Hopefully, others that run into similar issues would\nbenefit from this as well, there are a few reports online of people\nrunning into \"failed to refresh\" errors.)\n\nAs a side-note, the strace on the affected file also shows that git\nwrites that file twice during the merge, with the same content. There\nmight be some potential to further optimize merges to avoid such\ndouble-writes. A small example to reproduce, note how \"b\" is opened\ntwice during the merge:\n\n git init\n echo \"foo\" > a\n git add a\n git commit -m \"Initial commit\"\n\n git mv a b\n git commit -m \"File moved\"\n\n git checkout -b other_branch HEAD~\n touch c && git add c && git commit -m \"Some other commit\"\n strace -otrace git merge master -m \"merge message\"\n grep '\"b\"' trace\n\nThanks\n- Eph\n\n"},{"id":"418886","messageId":"xmqqlfatefwt.fsf@gitster.g","threadId":"55282","inReplyTo":"cd7c6682-7409-f72c-8751-02b70a423f83@gmail.com","subject":"Re: [RFC PATCH] merge-recursive: create new files with O_EXCL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-11T17:52:50Z","receivedAt":"2021-03-11T17:53:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ephrim Khong writes:\n\n>> I do not think we mind fixing the use of O_TRUNC with \"remove and\n>> then create with O_EXCL\", but we'd probably want to\n>> \n>> * understand why only this place matters, or perhaps other uses of\n>> O_TRUNC needs the same fix to work \"correctly\" with your NFS\n>> mounts, in which case we'd need all of them addressed in the same\n>> series of patches, and\n>\n> I'd say that is up to you. Personally I'd rather err on the side of\n> caution and leave the other code as is, especially since it is not\n> really required for the file mode issue as described above.\n\nOK.\n\nEspecially the other places are prepared to deal with a leftover\nstale file, there is no reason to add extra cost to unlink before\nrecreating. If we know this place we will never have an existing\nfile (because we unlinked), then TRUNC -> EXCL is a no-cost change\nfor us.\n\nIf some of these other places turn out to be problem for you and\nother users who are affected by the same NFS mount problem, we may\nneed to replace these places' open(CREAT|TRUNC) with a wrapper that\nconditionally does an unlink+open(CREAT|EXCL) with a Makefile knob.\n\nBut before doing any of that, we'd need to see ...\n\n>> * understand why your NFS mount is broken and give a better\n>> explanation as to why we need to have a workaround in our code.\n>\n> I'll work on this, but unfortunately have no idea of how to properly\n> debug it. Since it is a company server without administrative rights and\n> the backend is some IBM storage system, the options are limited and\n> processes are slow. What I did find out so far is that it is not a race\n> condition with unlink. A simple openat() without O_EXCL already produces\n> the wrong file mode.\n\n... this resolved, I would think.\n\nThanks.\n"},{"id":"418890","messageId":"CABPp-BFPj4E=pOK_f=w9k_zGA+=A1F0Hv-PgYyNDdN30mXMBuw@mail.gmail.com","threadId":"55282","inReplyTo":"cd7c6682-7409-f72c-8751-02b70a423f83@gmail.com","subject":"Re: [RFC PATCH] merge-recursive: create new files with O_EXCL","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-03-11T18:01:50Z","receivedAt":"2021-03-11T18:03:00Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Mar 11, 2021 at 1:58 AM Ephrim Khong wrote:\n>\n> On 11.03.2021 00:01, Junio C Hamano wrote:\n> > Ephrim Khong writes:\n\n> As a side-note, the strace on the affected file also shows that git\n> writes that file twice during the merge, with the same content. There\n> might be some potential to further optimize merges to avoid such\n> double-writes. A small example to reproduce, note how \"b\" is opened\n> twice during the merge:\n>\n> git init\n> echo \"foo\" > a\n> git add a\n> git commit -m \"Initial commit\"\n>\n> git mv a b\n> git commit -m \"File moved\"\n>\n> git checkout -b other_branch HEAD~\n> touch c && git add c && git commit -m \"Some other commit\"\n> strace -otrace git merge master -m \"merge message\"\n> grep '\"b\"' trace\n\nYeah, this is somewhat fundamental to merge-recursive's implementation\ndesign; fixing it essentially requires a rewrite. That rewrite is\nnearing completion; so this double-write issue will be fixed when\nmerge-ort is complete (or for anyone who applies the patches and tries\nit out now).\n\nIn fact, if you don't see the permissions problems when switching\nbranches (because branch switching uses O_EXCL?), then merge-ort\nalmost certainly incidentally fixes that problem for you as well.\n"},{"id":"419022","messageId":"YEwQe0qMUtlI5PCl@camp.crustytoothpaste.net","threadId":"55282","inReplyTo":"cd7c6682-7409-f72c-8751-02b70a423f83@gmail.com","subject":"Re: [RFC PATCH] merge-recursive: create new files with O_EXCL","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-03-13T01:08:11Z","receivedAt":"2021-03-13T01:09:06Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-03-11 at 09:54:41, Ephrim Khong wrote:\n> On 11.03.2021 00:01, Junio C Hamano wrote:\n> > * understand why your NFS mount is broken and give a better\n> > explanation as to why we need to have a workaround in our code.\n> \n> I'll work on this, but unfortunately have no idea of how to properly\n> debug it. Since it is a company server without administrative rights and\n> the backend is some IBM storage system, the options are limited and\n> processes are slow. What I did find out so far is that it is not a race\n> condition with unlink. A simple openat() without O_EXCL already produces\n> the wrong file mode.\n\nThis reminds me of a NFS bug that we saw in the past[0]. I don't know\nif you're using that same type of system in this case, but if so, it\ncould be part of the problem.\n\nSince buggy NFS implementations seem to be a problem specifically with\nopen(2) and I need to reroll my series to add some entries to the FAQ,\nand I'll document that we require NFS servers to support POSIX open\nsemantics, including permissions and O_EXCL.\n\n> (I fully understand that this is not a bug on git's side, and I found no\n> documentation indicating that O_EXCL would be recommended or have any\n> effect in this way. Hopefully, others that run into similar issues would\n> benefit from this as well, there are a few reports online of people\n> running into \"failed to refresh\" errors.)\n\nThis does tend to frequently affect Git, but it can also affect other\nprograms as well, and you're probably going to be better off filing a\nbug report with IBM about their NFS server than trying to work around it\nin every situation.\n\n[0] https://lore.kernel.org/git/CAPx1GvfKxu8gwbp0Gn2dBf9th874skKjD-echeAFr7_77o8FYw@mail.gmail.com/T/#mead6be6c92f0ab29cf9fd600781dea7315e87411\n-- \nbrian m. carlson (he/him or they/them)\nHouston, Texas, US\n"}]}