On 10 Apr 2014, at 12:45, Stefan Winter <stefan.winter@restena.lu> wrote:
Hi,
Oh, sorry... It's fixed!
I.e. could you point me to a commit to cherry-pick?
https://github.com/FreeRADIUS/freeradius-server/commit/a2a04cd15bc3886ceb7d1...
I cherry-picked this one and recompiled. Didn't truly solve the issue.
I have apporx 5 writes into the file per second, and got garbled statements 2-3 times a day after the fix.
My colleague Bruno investigated this further. It looks like the fcntl() locking doesn't catch all corner cases. He writes:
c.f.: man 2 fcntl
As well as being removed by an explicit F_UNLCK, record locks are automatically released when the process terminates or if it closes any file descriptor referring to a file on which locks are held. This is bad: it means that a process can lose the locks on a file like /etc/passwd or /etc/mtab when for some reason a library function decides to open, read and close it.
This means that rad_lockfd() might possibly have no effect:
-thread1- -thread2- -thread3- open(logfile) . . rad_lockfd() . . write(query) . . write(";\n") . . . open(logfile) open(logfile) . rad_lockfd() . . rad_lockfd( . write(query) . close() . ) . . write(query) . write(";\n") write(";\n") . close() close()
We think we fixed it locally (Bruno's patch attached) by combining both strings into one write() as I suggested earlier.
We're aware that this is also no /guarantee/ that the string is written atomically, but the cases where it's not are limited to corner conditions (out of memory, thrashing, ...) and so shouldn't occur during normal operations.
Take the patch or leave it, it does seem to work for us :-)
It's quite inefficient, you have to strdup the entire buffer just to add two characters. fcntl locks are completely irrelevant in this case, there should now be a mutex around that call preventing multiple writes to the file by different threads... fcntl locks are not mirrored back to threads within the same process, so if one thread locks the file, and another thread attempts to acquire the lock it will succeed. They're not at all useful for synchronisation within a process, only for notifying other processes that the file, or certain areas of the file, are locked. Please check the correct fix was cherry picked. Note: If you're using multiple instances of rlm_sql, and having them all log to the same log file, you'll still experience issues. The only fix for this is some kind of central locking API within the server core. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team FD31 3077 42EC 7FCD 32FE 5EE2 56CF 27F9 30A8 CAA2