Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

On Mon, Oct 31, 2016 at 10:55:32AM -0700, Junio C Hamano wrote: > > So I guess it's possible that it produces a noticeable effect in some > > cases, but I'm still somewhat doubtful. And actually repacking your > > repository had a greater effect in every case I measured (in addition to > >

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

Jeff King writes: > If you set a probe on touch_atime() in the kernel (which is called for > every attempt to smudge the atime, regardless of mount options, but is > skipped when the descriptor was opened with O_NOATIME), you can see the > impact. Here's a command I picked because

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

Linus Torvalds writes: > On Sat, Oct 29, 2016 at 1:25 AM, Johannes Schindelin > wrote: >> >> Correct. We cannot change an open file handle's state ("FD_CLOEXEC") on >> Windows, but we can ask open() to open said file handle with the

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

On Fri, Oct 28, 2016 at 09:13:41AM -0700, Linus Torvalds wrote: > On Fri, Oct 28, 2016 at 4:11 AM, Johannes Schindelin > wrote: > > > > You guys. I mean: You guys! You sure make my life hard. A brief look at > > mingw.h could have answered your implicit question: > >

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

On Sat, Oct 29, 2016 at 1:25 AM, Johannes Schindelin wrote: > > Correct. We cannot change an open file handle's state ("FD_CLOEXEC") on > Windows, but we can ask open() to open said file handle with the correct > flag ("O_CLOEXEC", which is mapped to O_NOINHERIT on

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

Hi, On Fri, 28 Oct 2016, Junio C Hamano wrote: > Linus Torvalds writes: > > > Apparently windows doesn't even support it, at least not mingw > > Assuming that the above was a misunderstanding, assuming that we can > do O_CLOEXEC (but not FD_CLOEXEC) on

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

Linus Torvalds writes: > Apparently windows doesn't even support it, at least not mingw Assuming that the above was a misunderstanding, assuming that we can do O_CLOEXEC (but not FD_CLOEXEC) on Windows just fine, where stray file descriptors held open in the

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

Linus Torvalds writes: > On Fri, Oct 28, 2016 at 9:48 AM, Junio C Hamano wrote: >> >> Setting FD_CLOEXEC with fcntl(2) may be racy, but in what way >> it is broken to open(2) with O_CLOEXEC? > > Apparently windows doesn't even support it, at

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

On Fri, Oct 28, 2016 at 9:48 AM, Junio C Hamano wrote: > > Excuse me, but care to elaborate a bit more? It just seems entirely pointless to change the games with O_xyz. > Setting FD_CLOEXEC with fcntl(2) may be racy, but in what way > it is broken to open(2) with O_CLOEXEC?

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

Linus Torvalds writes: > On Fri, Oct 28, 2016 at 4:11 AM, Johannes Schindelin > wrote: >> >> You guys. I mean: You guys! You sure make my life hard. A brief look at >> mingw.h could have answered your implicit question: > > So here's

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

On Fri, Oct 28, 2016 at 4:11 AM, Johannes Schindelin wrote: > > You guys. I mean: You guys! You sure make my life hard. A brief look at > mingw.h could have answered your implicit question: So here's what you guys should do: - leave O_NOATIME damn well alone. It

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

Junio C Hamano writes: > Junio C Hamano writes: > >> Hmph. This may not fly well in practice, though. > > We can flip the order around, and then it becomes simpler to later > drop atime support. The first step goes like this. And the second step would

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

Junio C Hamano writes: > Hmph. This may not fly well in practice, though. We can flip the order around, and then it becomes simpler to later drop atime support. The first step goes like this. -- >8 -- From: Junio C Hamano Date: Fri, 28 Oct 2016

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

Hi, On Thu, 27 Oct 2016, Junio C Hamano wrote: > Junio C Hamano writes: > > > Linus Torvalds writes: > > > >> On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano wrote: > >>> > >>> Would the best endgame shape for this function

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

On Thu, Oct 27, 2016 at 03:38:59PM -0700, Linus Torvalds wrote: > On Thu, Oct 27, 2016 at 3:24 AM, Jeff King wrote: > > > > +cc Linus as the original author of 144bde78e9 in case there is > > something subtle I'm missing, but this really just seems like it's > > an outdated

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

Junio C Hamano wrote: > Junio C Hamano writes: > > > Linus Torvalds writes: > > > >> On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano wrote: > >>> > >>> Would the best endgame shape for this function be to

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

Junio C Hamano writes: > Linus Torvalds writes: > >> On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano wrote: >>> >>> Would the best endgame shape for this function be to open with >>> O_NOATIME (and retry without), and then

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

Linus Torvalds writes: > On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano wrote: >> >> Would the best endgame shape for this function be to open with >> O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2) >> but ignoring an error

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano wrote: > > Would the best endgame shape for this function be to open with > O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2) > but ignoring an error from it, I guess? That would be the closest > to what we

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

Linus Torvalds writes: > But the basic issue still remains - I'd really prefer to have NOATIME > stay around for all those poor misguided souls that for some reason > don't like "relatime" or run old kernels. But whether it is with > O_NOATIME at open time or with

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

On Thu, Oct 27, 2016 at 4:09 PM, Linus Torvalds wrote: > > That said, now that I think about it, I should double-check: maybe > open() doesn't actually set atime at all, and we *could* do NOATIME > with SETFL after all. Checked. Yup. O_NOATIME could easily be done

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

On Thu, Oct 27, 2016 at 3:56 PM, Junio C Hamano wrote: > > I also thought O_NOATIME shouldn't be an effective fcntl(2) thing, > for the reasons you stated above, when I was studying the area > because of the discussion on these patches. I was surprised to see > that

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

Linus Torvalds writes: > Note that you can *not* do the same thing with O_NOATIME, since the > whole point of O_NOATIME is that it changes the behavior of the open > itself (unlike O_CLOEXEC which changes _later_ behavior, and can > always be replaced by FD_CLOEXEC

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

On Thu, Oct 27, 2016 at 3:24 AM, Jeff King wrote: > > +cc Linus as the original author of 144bde78e9 in case there is > something subtle I'm missing, but this really just seems like it's > an outdated optimization. I'd *really* like to keep O_NOATIME if at all possible. It made a

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

Jeff King writes: > +cc Linus as the original author of 144bde78e9 in case there is > something subtle I'm missing, but this really just seems like it's > an outdated optimization. > > -- >8 -- > Subject: [PATCH] sha1_file: stop opening files with O_NOATIME > > When we open object

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

On Wed, Oct 26, 2016 at 02:15:33PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > On Wed, Oct 26, 2016 at 10:52:41AM -0700, Junio C Hamano wrote: > > > >> > I actually wonder if it is worth carrying around the O_NOATIME hack at > >> > all. > >> > >> Yes, I share the

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

Jeff King writes: > On Wed, Oct 26, 2016 at 10:52:41AM -0700, Junio C Hamano wrote: > >> > I actually wonder if it is worth carrying around the O_NOATIME hack at >> > all. >> >> Yes, I share the thought. We no longer have too many loose objects >> to matter. >> >> I do not mind

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

On Wed, Oct 26, 2016 at 10:52:41AM -0700, Junio C Hamano wrote: > > I actually wonder if it is worth carrying around the O_NOATIME hack at > > all. > > Yes, I share the thought. We no longer have too many loose objects > to matter. > > I do not mind flipping the order, but I'd prefer to cook

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

Jeff King writes: > Of the two flags, I would say CLOEXEC is the more important one to > respect because it may actually impact correctness (e.g., leaking > descriptors to sub-processes). Whereas O_NOATIME is purely a performance > optimization. I tend to agree. > I actually

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

On Wed, Oct 26, 2016 at 09:23:21AM -0700, Junio C Hamano wrote: > >> + /* Might the failure be due to O_NOATIME? */ > >> + if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) { > >> + sha1_file_open_flag &= ~O_NOATIME; > >> + continue; >

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

Jeff King writes: >> +/* Try again w/o O_CLOEXEC: the kernel might not support it */ >> +if ((sha1_file_open_flag & O_CLOEXEC) && errno == EINVAL) { >> +sha1_file_open_flag &= ~O_CLOEXEC; >> continue; >>

Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

On Tue, Oct 25, 2016 at 11:16:20AM -0700, Junio C Hamano wrote: > diff --git a/sha1_file.c b/sha1_file.c > index 5d2bcd3ed1..09045df1dc 100644 > --- a/sha1_file.c > +++ b/sha1_file.c > @@ -1571,12 +1571,17 @@ int git_open(const char *name) > if (fd >= 0) >

[PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

From: Lars Schneider All processes that the Git main process spawns inherit the open file descriptors of the main process. These leaked file descriptors can cause problems. Use the O_CLOEXEC flag similar to 05d1ed61 to fix the leaked file descriptors. Signed-off-by: