Brian De Wolf wrote:
Here's the patch, as promised.
Many thanks.
Now, instead of radrelay's previous behavior, radrelay will attempt to open and lock <detail>.work. If it can, it will then read/transmit all of the entries and delete <detail>.work. If it can't lock, it will simply try locking later.
The new algorith doesn't require a non-blocking lock in a loop. After the file is renamed rlm_detail will not try to lock it anymore.
If it doesn't exist, it will check if <detail> has entries and if so it will move <detail> to <detail>.work (and make a new <detail>).
rlm_detail will create the new detail file. I think the function detail_move() is not needed anymore. Just use a normal rename().
It seems to move faster, but that's because I removed some of the sleeps to make sure writing had completed since that should be handled by the locks now. Luckily, my relay destinations aren't production yet, so I can keep testing with a decent load.
That's good news.
--- src/main/radrelay.c.orig 2007-03-16 06:22:03.000000000 -0700 +++ src/main/radrelay.c 2007-12-03 12:24:46.000000000 -0800
[...]
@@ -537,11 +514,13 @@ if (rename(from, to) < 0) return -1;
+ oldmask = umask(0); if ((n = open(from, O_CREAT|O_RDWR, st.st_mode)) >= 0) close(n); umask(oldmask);
+ return 0; }
This is only cosmetics.
@@ -607,19 +586,47 @@ if (got_sigterm) state = STATE_SHUTDOWN;
/* - * Open detail file - if needed, and if we can. + * Try to open our work file first. If not, check if the + * actual detail file has stuff in it. If so, open it, + * move it to the work file, and replace it with an empty + * file. Then keep the file handle for locking/reading. */
Maybe re-use the comments from Alan in src/main/detail.c: /* * Open detail.work first, so we don't lose * accounting packets. It's probably better to * duplicate them than to lose them. * * Note that we're not writing to the file, but * we've got to open it for writing in order to * establish the lock, to prevent rlm_detail from * writing to it. */
+ if ((fp = fopen(work, "r+")) == NULL) {
/* * Try reading the detail file. If it * doesn't exist, we can't do anything. * * Doing the stat will tell us if the file * exists, even if we don't have permissions * to read it. */
+ if(!stat(r_args->detail, &st)) { + if(st.st_size > 0) { + detail_move(r_args->detail, work);
As said above, I think you could just use rename() and then you don't need to check st_size > 0.
+ continue;
I have a question about this "continue": please check that radrelay will not use 100% cpu when there is no detail file to read. (as many of the sleep() are gone)
+ } + } }
+ /* + * Try to lock the detail-file. + * If lockf is used we want to lock the _whole_ file, hence the
Please no line of more than 80 characters in comments.
+ * fseek to the start of the file. + * + * If we can't lock the detail file, we close it and + * will try it again on the next iteration. Note that + * we will never unlock the file, on purpose, to stop + * rlm_detail from reading it. + */
The last part is confusing. rlm_detail must not try to acquire the lock anymore after the file is renamed to detail.work.
+ + if(fp) { + fseek(fp, 0L, SEEK_SET); + do { + x = rad_lockfd_nonblock(fileno(fp), 0); + if (x == -1) + ms_sleep(100); + } while (x == -1 && i++ < 20); + + if (x == -1) { + fclose(fp); + fp = NULL; + } else { + state = STATE_BACKLOG; + }
No need to lock in a loop, so you can simplify that part. The lock is only there to catch a corner case where the rename() happened while the current rlm_detail was still writing in the file. (but new rlm_detail execution will create and use a new file) -- Nicolas Baradakis