Question about JDK-8043780 (was: RFR: 8237830: support O_CLOEXEC in os::open on other OS than Linux)

[Thomas Stüfe]

Hi Martin,

do you remember https://bugs.openjdk.java.net/browse/JDK-8043780 ?

Some questions came up about your patch:

Your original coding does this
check-O_CLOEXEC-after-open-and-set-if-missing-dance, but if it worked ar
least once it will stop doing that. However, if it fails to work it will
retest over and over again. Was this intentional? Do we expect it to start
working at some point? If yes, could it also stop working at some point?

I wonder whether we could shorten the coding to a simple, unconditional

::fcntl(fd, F_SETFD, flags | FD_CLOEXEC);

to be done after open.

If open(CLOEXEC) worked, this would be a unnecessary noop. If it did not,
we set it now. We'd always pay for a fcntl call but I think it is cheap and
open is not called that often anyway.

--

But then, I wonder whether we can scratch that coding altogether. O_CLOEXEC
has been in the kernel since 2.6.23. This is 13 years old. 2.6 line topped
out at 2.6.39 in 2011, and I would expect anyone still running on 2.6 to
have at least a recent kernel.

What do you think?

Thank you!

Cheers, Thomas


On Thu, Feb 6, 2020 at 8:55 AM Baesken, Matthias <matthias.baesken at sap.com>
wrote:

> Hi Thomas , the coding was borrowed from linux, see :
>
>
>
>
> https://hg.openjdk.java.net/jdk/jdk/file/7b57401deb0c/src/hotspot/os/linux/os_linux.cpp#l5616
>
>
>
> I would be fine with  always calling fcntl (your suggestion 1) ,  do you
> think  it was done otherwise  back then   in os_linux because of
> performance issues ?
>
> Probably os::open in os_linux  should be adjusted then  to the  new
> “always call fcntl” approach too, right ?
>
>
>
>
>
> Best regards, Matthias
>
>
>
>
>
>
>
> Hi Matthias:
>
>
>
> + // Validate that the use of the O_CLOEXEC flag on open above worked.
>
> +  static sig_atomic_t O_CLOEXEC_is_known_to_work = 0;
>
> +  if (!O_CLOEXEC_is_known_to_work) {
>
>      int flags = ::fcntl(fd, F_GETFD);
>
> -    if (flags != -1)
>
> +    if (flags != -1) {
>
> +      if ((flags & FD_CLOEXEC) != 0) {
>
> +        O_CLOEXEC_is_known_to_work = 1;
>
> +      } else {
>
>        ::fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
>
>    }
>
> -#endif
>
> +    }
>
> +  }
>
>
>
> This is either too complicated or not complicated enough :)
>
>
> if we know for a fact that open(..O_CLOEXEC) did work at least once, we
> never again check.
> if we know for a fact that open(..O_CLOEXEC) failed to work, we recheck
> again and again. Do we expect it to suddenly start working? If yes, could
> it also suddenly start failing? Should we then not check always?
>
> I propose the following change:
>
> 1) If open(..O_CLOEXEC) could sometimes fail and sometimes not, or we are
> just not sure, I would do the fcntl test every time. Or, even just always
> set the flag again with fcntl. So, just remove the
> O_CLOEXEC_is_known_to_work conditional variable.
>
> Which would reduce the code to a one liner:
>
> + ::fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
>
> 2) If you are sure that it either always works or fails, you need a
> conditional with three states: unknown, works, does not work. And only
> check if unknown.
>
> I personally would prefer (1) because it is way simpler and fcntl is not
> such an expensive operation that we have to save time (also, how often do
> we open files anyway?).
>
> ----
>
>
>
> Otherwise looks fine. This is a useful fix.
>
>
> Cheers, Thomas
>
>
>
>
>
>
>
>
>
> On Tue, Jan 28, 2020 at 12:32 PM Baesken, Matthias <
> matthias.baesken at sap.com> wrote:
>
> New webrev :
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8237830.1/
>
> (one 'c' got lost somehow in the older webrev)
>
> Best regards, Matthias
>
>