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

[Martin Buchholz]

These flags have been part of POSIX since 2001, so it could be time to
assume they work unconditionally, but I like maximal portability.
Just because O_CLOEXEC is defined at compile time doesn't mean it will work
at runtime.  I checked both.
It seems like a serious bug in AIX if using O_CLOEXEC could return EINVAL
for some file systems but not others.
If that's true then you would need a fallback using fcntl always.
But maybe that's a good idea.
Could we write a shared implementation of this for all posix platforms?
Perhaps.
Try to use O_CLOEXEC.  first time it fails with EINVAL, remember that and
from then on, fall back to fcntl with FD_CLOEXEC.
Humans are not good at getting this sort of code correct!

On Tue, Feb 11, 2020 at 6:40 AM Baesken, Matthias <matthias.baesken at sap.com>
wrote:

> Hello,  new webrev :
>
>
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8237830.3/
>
>
>
>    - Only touching  os_aix.cpp  ( I leave os_bsd.cpp as it is , might
>    make the discussion   more  simple )
>    - Added handling of  failing O_CLOEXEC   ( O_CLOEXEC_is_known_to_work
>    == -1  … case )
>
>
>
> Best regards, Matthias
>
>
>
>
>
> *From:* Baesken, Matthias
> *Sent:* Dienstag, 11. Februar 2020 14:06
> *To:* 'Thomas Stüfe' <thomas.stuefe at gmail.com>; 'Martin Buchholz' <
> martinrb at google.com>
> *Cc:* 'David Holmes' <david.holmes at oracle.com>; '
> hotspot-dev at openjdk.java.net' <hotspot-dev at openjdk.java.net>
> *Subject:* RE: Question about JDK-8043780 (was: RFR: 8237830: support
> O_CLOEXEC in os::open on other OS than Linux)
>
>
>
>
>
> >I wonder whether we could shorten the coding to a simple, unconditional
>
> >
>
> >::fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
>
>
>
>
>
> Looks like we would need to call first  fcntl with   F_GETFD  AND then F_SETFD
> (or where  would flags come from?) :
>
>
>
>     int flags = ::fcntl(fd, F_GETFD);
>
>     ::fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
>
>
>
> That’s 2  calls  that are not really needed .
>
>
>
>
>
> Best regards, Matthias
>
>
>
>
>
> *From:* Baesken, Matthias
> *Sent:* Dienstag, 11. Februar 2020 10:09
> *To:* 'Thomas Stüfe' <thomas.stuefe at gmail.com>; Martin Buchholz <
> martinrb at google.com>
> *Cc:* David Holmes <david.holmes at oracle.com>; hotspot-dev at openjdk.java.net
> *Subject:* RE: Question about JDK-8043780 (was: RFR: 8237830: support
> O_CLOEXEC in os::open on other OS than Linux)
>
>
>
> Ping – any comments on this ?
>
>
>
> https://bugs.openjdk.java.net/browse/JDK-8043780
>
>
>
> mentions :  „Backward compatibility needs to be provided via #ifdef and
> checking (once) whether O_CLOEXEC works.”
>
>
>
> So it was intentional that just ***once*** it is checked whether  O_CLOEXEC
> works (in the linux version) .
>
>
>
>
>
> Best regards, Matthias
>
>
>
>
>
> *From:* Thomas Stüfe <thomas.stuefe at gmail.com>
> *Sent:* Donnerstag, 6. Februar 2020 11:30
> *To:* Martin Buchholz <martinrb at google.com>
> *Cc:* David Holmes <david.holmes at oracle.com>; hotspot-dev at openjdk.java.net;
> Baesken, Matthias <matthias.baesken at sap.com>
> *Subject:* Question about JDK-8043780 (was: RFR: 8237830: support
> O_CLOEXEC in os::open on other OS than Linux)
>
>
>
> 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
>
>