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

Junio C Hamano [off-list ref] writes:

Linus Torvalds [off-list ref] writes:

quoted

On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano [off-list ref] wrote:

quoted

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 historically had, I would think.

I think that's the best model.

OK, so perhaps like this.

Hmph.  This may not fly well in practice, though.  

To Unix folks, CLOEXEC is not a huge correctness issue.  A child
process may hold onto an open file descriptor a bit longer than the
lifetime of the parent but as long as the child eventually exits,
nothing is affected.  Over there, things are different.  The parent
cannot even rename(2) or unlink(2) a file it created and closed
while the child is still holding the file descriptor open and the
lack of CLOEXEC will make the parent fail.  I do not know how well
fcntl(2) emulation works on Windows, but I would not be surprised
if J6t or Dscho comes back and says that FD_CLOEXEC given to F_SETFD
would not work while O_CLOEXEC given to open(2) does.

Junio C Hamano [off-list ref] wrote:

Junio C Hamano [off-list ref] writes:

quoted

Linus Torvalds [off-list ref] writes:

quoted

On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano [off-list ref] wrote:

quoted

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 historically had, I would think.

I think that's the best model.

Actually, I would flip the order of flags.  O_CLOEXEC is more
important from a correctness standpoint.

quoted

OK, so perhaps like this.

Hmph.  This may not fly well in practice, though.  

To Unix folks, CLOEXEC is not a huge correctness issue.  A child
process may hold onto an open file descriptor a bit longer than the
lifetime of the parent but as long as the child eventually exits,

I'm not too familiar with C internals of git; but I know we use
threads in some places, and fork+execve in others.

If our usage of threads and execve intersects, and we run
untrusted code in an execve-ed child, then only having cloexec
on open() will save us time when auditing for leaking FDs.

fcntl(fd, F_SETFD, O_CLOEXEC) is racy in if there are other
threads doing execve; so I wouldn't rely on it as a first
choice.

So I suppose something like this:

	static int noatime = 1;
	int fd = open(... | O_CLOEXEC);
	...error checking and retrying...

	if (fd >= 0 && noatime && fcntl(fd, F_SETFL, O_NOATIME) != 0)
		noatime = 0;

	return fd;

Hi,

On Thu, 27 Oct 2016, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

Linus Torvalds [off-list ref] writes:

quoted

On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano [off-list ref] wrote:

quoted

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 historically had, I would think.

I think that's the best model.

OK, so perhaps like this.

Hmph.  This may not fly well in practice, though.  

To Unix folks, CLOEXEC is not a huge correctness issue.  A child
process may hold onto an open file descriptor a bit longer than the
lifetime of the parent but as long as the child eventually exits,
nothing is affected.  Over there, things are different.  The parent
cannot even rename(2) or unlink(2) a file it created and closed
while the child is still holding the file descriptor open and the
lack of CLOEXEC will make the parent fail.  I do not know how well
fcntl(2) emulation works on Windows, but I would not be surprised
if J6t or Dscho comes back and says that FD_CLOEXEC given to F_SETFD
would not work while O_CLOEXEC given to open(2) does.

You guys. I mean: You guys! You sure make my life hard. A brief look at
mingw.h could have answered your implicit question:

	static inline int fcntl(int fd, int cmd, ...)
	{
		if (cmd == F_GETFD || cmd == F_SETFD)
			return 0;
		errno = EINVAL;
		return -1;
	}

So while you discuss in your Linux Ivory Tower how to optimize Git for
Linux, and Linux only, I'll have to drop everything else and spend the
rest of my Friday trying to find a way to adjust a file handle
*immediately after opening it with undesired flags* (when it could have
been opened with the desired flags, as suggested, to begin with).

Ciao,
Johannes

On Fri, Oct 28, 2016 at 4:11 AM, Johannes Schindelin
[off-list ref] 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 works. It has worked for 10+
years. Stop arguing against it, people who do.

 - get rid of all O_CLOEXEC games. They don't work. If you want to
close file descriptors at execve(), you - gasp - close the file
descriptor before doing an execve.

So O_CLOEXEC or FD_CLOEXEC is broken.

DO NOT BREAK O_NOATIME JUST TO ADD COMPLETELY NEW BREAKAGE.

                 Linus

On Fri, Oct 28, 2016 at 09:13:41AM -0700, Linus Torvalds wrote:

On Fri, Oct 28, 2016 at 4:11 AM, Johannes Schindelin
[off-list ref] wrote:

quoted

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 works. It has worked for 10+
years. Stop arguing against it, people who do.

For some definition of worked, perhaps.

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 it reads a lot of objects (run
on my git.git clone):

  $ perf stat -e probe:touch-atime git log -Sfoo >/dev/null

And the probe:touch_atime counts before (stock git) and after (a patch
to drop O_NOATIME):

  before: 22,235
   after: 22,362

So that's only half a percent difference. And it's on a reasonably messy
clone that is partway to triggering an auto-repack:

  $ git count-objects -v
  count: 6167
  size: 61128
  in-pack: 275773
  packs: 18
  size-pack: 86857
  prune-packable: 25
  garbage: 0
  size-garbage: 0

Running "git gc" drops the probe count to 21,733.

It makes a bigger difference for some commands (it's more like 10% for
git-status). And smaller for others ("git log -p" triggers it over
100,000 times).

One thing missing in that count is how many of those calls would have
resulted in an actual disk write. Looking at strace, most of the
filesystem activity is opening .gitattributes files, and we end up
opening the same ones repeatedly (e.g., t/.gitattributes in git.git).
Multiple hits for a given inode in the same second get coalesced into at
most a single disk write.

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
providing other speedups).

Like I said, I'm OK keeping O_NOATIME. It's just not that much code. But
if you really care about the issue of dirtying inodes via atime, you
should look into vastly increasing our use of O_NOATIME. Or possibly
looking at caching more in the attribute code.

-Peff