On 11 Apr 2014, at 07:24, Alan DeKok <aland@deployingradius.com> wrote:
Stefan Winter wrote:
Whoa. Why would that be a configuration error?
Because we can't guarantee that the writes occur properly.
I have multiple instances of rlm_sql_null, all writing different SQL statements, and all writing into the same file for forwarding to the SQL server by radsqlrelay.
Which doesn't work with POSIX locks, as you pointed out. It might work accidentally, but different threads writing to the same file will cause problems. One thread closing a file descriptor will cause another thread to magically lose its lock.
See this is why I wasn't quite sure why you did this on a per module basis. My suggestion would be to use a global hash of filenames -> FDs, which is shared between all things that write to log files on the server. That's sort of the point of a separate FD API, to add a layer on top of the broken POSIX locking to reflect lock states back into things within the same process space. My other suggestion to use multiple FDs would also work alleviate some of the contention. Assuming hash table is already initialised. typedef struct fr_fd_t { fr_pool_t parent; int fd; struct fr_fd_t next; } fr_fd_t; typedef struct fr_pool_t { int in_use; //!< How many file descriptors are in use int in_pool; //!< How many file descriptors are open for this file mutex mutex; //!< Protect access to the pool. fr_fd_t fd; //!< Head of FD list } fr_pool_t; get_fd(filename, bytes_to_lock) { lock(hash) pool = find(filename) if (!pool) { /* new pool */ } lock(pool) unlock(hash) pool->in_use++ /* We don't want this to be removed whilst were using it */ for (fd = pool->fd; fd->next && fd->in_use; fd = fd->next); if (fd->in_use) fd = new_fd() /* add a new fd to the pool for this file (maybe limit this to n FDs) */ lseek(fd, pool->offset, SEEK_CUR) fcntl() /* add range lock */ fd->in_use = true pool->offset += bytes_to_lock /* Set new offset */ unlock(pool) return fd } release_fd(fd) { pool = fd->parent lock(pool) fctnl() /* release range lock */ fd->in_use = false; pool->in_use-- } You can just use one mutex, but two would probably be better. You really want to limit the critical region for the one global mutex, and locking another mutex is probably quicker than doing a bunch of system calls. There's also an issue one what you do if there are no more FDs available. You probably don't want to drop the message... Maybe the max number of FDs should equal the max number of threads + 1. You can't close them whilst something is using any of the FDs in the pool. So this has to be managed carefully. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team FD31 3077 42EC 7FCD 32FE 5EE2 56CF 27F9 30A8 CAA2