| Message ID | mvmpnhwvx68.fsf@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | Always do locking when accessing streams (bug 15142) | expand |
* Andreas Schwab: > During exit, skip files that are currently locked to avoid deadlock. Which locks are taken during the deadlock? Does this fix or re-open bug 15142? > + /* We want to skip locked streams. Some threads might use streams but > + that is their problem, we don't flush those. */ > + int result = _IO_flush_all_lockp (true); Is this still conforming to POSIX? Sorry, it's not clear to me how this patch improves matters, and if the direction is ultimately the right one. Thanks, Florian
On Nov 21 2019, Florian Weimer wrote: > * Andreas Schwab: > >> During exit, skip files that are currently locked to avoid deadlock. > > Which locks are taken during the deadlock? Presumably any FILE locks, if there are other threads still running. > Does this fix or re-open bug 15142? Why re-open? >> + /* We want to skip locked streams. Some threads might use streams but >> + that is their problem, we don't flush those. */ >> + int result = _IO_flush_all_lockp (true); > > Is this still conforming to POSIX? Are deadlocks conforming to POSIX? Andreas.
* Andreas Schwab: > On Nov 21 2019, Florian Weimer wrote: > >> * Andreas Schwab: >> >>> During exit, skip files that are currently locked to avoid deadlock. >> >> Which locks are taken during the deadlock? > > Presumably any FILE locks, if there are other threads still running. Okay, so the problem is simply lack of progress after calling exit? >> Does this fix or re-open bug 15142? > > Why re-open? Lack of flushing of streams on process exit. >>> + /* We want to skip locked streams. Some threads might use streams but >>> + that is their problem, we don't flush those. */ >>> + int result = _IO_flush_all_lockp (true); >> >> Is this still conforming to POSIX? > > Are deadlocks conforming to POSIX? I think the last time we discussed this, the conclusion was that lack of progress was conforming to POSIX (actually required by it). What has changed since then? Thanks, Florian
On Nov 21 2019, Florian Weimer wrote: >>> Does this fix or re-open bug 15142? >> >> Why re-open? > > Lack of flushing of streams on process exit. I don't understand what you are trying to tell me. > I think the last time we discussed this, Do you have a pointer? Andreas.
* Andreas Schwab: > On Nov 21 2019, Florian Weimer wrote: > >>>> Does this fix or re-open bug 15142? >>> >>> Why re-open? >> >> Lack of flushing of streams on process exit. > > I don't understand what you are trying to tell me. Hmm. Bug 15142 is not the bug, but don't we have a bug somewhere about not flushing streams on exit (as opposed to abort)? >> I think the last time we discussed this, > > Do you have a pointer? I can't find the discussion. But I think we discussed it somewhere. The main question is whether it is acceptable for programs to hang indefinitely if a flushable stream is locked for some reason. POSIX actually seems to require that behavior, but I'm not sure how backwards-compatible it is. There are probably quite a few libraries which store a FILE * internally, use flockfile to get performance, and synchronize access to the file by other means. Thanks, Florian
On Nov 25 2019, Florian Weimer wrote: > * Andreas Schwab: > >> On Nov 21 2019, Florian Weimer wrote: >> >>>>> Does this fix or re-open bug 15142? >>>> >>>> Why re-open? >>> >>> Lack of flushing of streams on process exit. >> >> I don't understand what you are trying to tell me. > > Hmm. Bug 15142 is not the bug, but don't we have a bug somewhere about > not flushing streams on exit (as opposed to abort)? Bug 14697 perhaps? Andreas.
* Andreas Schwab: > On Nov 25 2019, Florian Weimer wrote: > >> * Andreas Schwab: >> >>> On Nov 21 2019, Florian Weimer wrote: >>> >>>>>> Does this fix or re-open bug 15142? >>>>> >>>>> Why re-open? >>>> >>>> Lack of flushing of streams on process exit. >>> >>> I don't understand what you are trying to tell me. >> >> Hmm. Bug 15142 is not the bug, but don't we have a bug somewhere about >> not flushing streams on exit (as opposed to abort)? > > Bug 14697 perhaps? Yes, that's it. Thanks, Florian
diff --git a/libio/genops.c b/libio/genops.c index f871e7751b..30daa169ef 100644 --- a/libio/genops.c +++ b/libio/genops.c @@ -681,8 +681,8 @@ _IO_adjust_column (unsigned start, const char *line, int count) } libc_hidden_def (_IO_adjust_column) -int -_IO_flush_all_lockp (int do_lock) +static int +_IO_flush_all_lockp (bool skip_locked) { int result = 0; FILE *fp; @@ -695,7 +695,16 @@ _IO_flush_all_lockp (int do_lock) for (fp = (FILE *) _IO_list_all; fp != NULL; fp = fp->_chain) { run_fp = fp; - if (do_lock) + if (skip_locked) + { + /* Skip files that are currently locked. */ + if (_IO_ftrylockfile (fp)) + { + run_fp = NULL; + continue; + } + } + else _IO_flockfile (fp); if (((fp->_mode <= 0 && fp->_IO_write_ptr > fp->_IO_write_base) @@ -706,8 +715,7 @@ _IO_flush_all_lockp (int do_lock) && _IO_OVERFLOW (fp, EOF) == EOF) result = EOF; - if (do_lock) - _IO_funlockfile (fp); + _IO_funlockfile (fp); run_fp = NULL; } @@ -724,7 +732,7 @@ int _IO_flush_all (void) { /* We want locking. */ - return _IO_flush_all_lockp (1); + return _IO_flush_all_lockp (false); } libc_hidden_def (_IO_flush_all) @@ -789,6 +797,14 @@ _IO_unbuffer_all (void) for (fp = (FILE *) _IO_list_all; fp; fp = fp->_chain) { + run_fp = fp; + /* Skip files that are currently locked. */ + if (_IO_ftrylockfile (fp)) + { + run_fp = NULL; + continue; + } + int legacy = 0; #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1) @@ -800,18 +816,6 @@ _IO_unbuffer_all (void) /* Iff stream is un-orientated, it wasn't used. */ && (legacy || fp->_mode != 0)) { -#ifdef _IO_MTSAFE_IO - int cnt; -#define MAXTRIES 2 - for (cnt = 0; cnt < MAXTRIES; ++cnt) - if (fp->_lock == NULL || _IO_lock_trylock (*fp->_lock) == 0) - break; - else - /* Give the other thread time to finish up its use of the - stream. */ - __sched_yield (); -#endif - if (! legacy && ! dealloc_buffers && !(fp->_flags & _IO_USER_BUF)) { fp->_flags |= _IO_USER_BUF; @@ -825,17 +829,15 @@ _IO_unbuffer_all (void) if (! legacy && fp->_mode > 0) _IO_wsetb (fp, NULL, NULL, 0); - -#ifdef _IO_MTSAFE_IO - if (cnt < MAXTRIES && fp->_lock != NULL) - _IO_lock_unlock (*fp->_lock); -#endif } /* Make sure that never again the wide char functions can be used. */ if (! legacy) fp->_mode = -1; + + _IO_funlockfile (fp); + run_fp = NULL; } #ifdef _IO_MTSAFE_IO @@ -861,9 +863,9 @@ libc_freeres_fn (buffer_free) int _IO_cleanup (void) { - /* We do *not* want locking. Some threads might use streams but - that is their problem, we flush them underneath them. */ - int result = _IO_flush_all_lockp (0); + /* We want to skip locked streams. Some threads might use streams but + that is their problem, we don't flush those. */ + int result = _IO_flush_all_lockp (true); /* We currently don't have a reliable mechanism for making sure that C++ static destructors are executed in the correct order. diff --git a/libio/libio.h b/libio/libio.h index bed324e57d..3e660bd454 100644 --- a/libio/libio.h +++ b/libio/libio.h @@ -291,11 +291,15 @@ libc_hidden_proto (_IO_sgetn) if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_lock_lock (*(_fp)->_lock) # define _IO_funlockfile(_fp) \ if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_lock_unlock (*(_fp)->_lock) +# define _IO_ftrylockfile(_fp) \ + (((_fp)->_flags & _IO_USER_LOCK) == 0 ? _IO_lock_trylock (*(_fp)->_lock) : 0) # else # define _IO_flockfile(_fp) \ if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_flockfile (_fp) # define _IO_funlockfile(_fp) \ if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_funlockfile (_fp) +# define _IO_ftrylockfile(_fp) \ + (((_fp)->_flags & _IO_USER_LOCK) == 0 ? _IO_ftrylockfile (_fp) : 0) # endif #endif /* _IO_MTSAFE_IO */ diff --git a/libio/libioP.h b/libio/libioP.h index 3787605cfb..2187b8edbe 100644 --- a/libio/libioP.h +++ b/libio/libioP.h @@ -487,7 +487,6 @@ extern int _IO_new_do_write (FILE *, const char *, size_t); extern int _IO_old_do_write (FILE *, const char *, size_t); extern int _IO_wdo_write (FILE *, const wchar_t *, size_t); libc_hidden_proto (_IO_wdo_write) -extern int _IO_flush_all_lockp (int); extern int _IO_flush_all (void); libc_hidden_proto (_IO_flush_all) extern int _IO_cleanup (void);