Thanks for the quick response! On 12/03/07 16:40, Nicolas Baradakis wrote:
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.
Well, I thought the lock was not to prevent rlm_detail from writing, but to prevent radrelay from reading before rlm_detail was finished writing.
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().
I left that in there because there was an exit condition if radrelay couldn't open either the work file or the original file. I eventually took out that exit condition altogether, but didn't take out the file creation for some reason...
This is only cosmetics.
Oops...I commented out the file creation, but as mentioned above, put it back in for the time being. Out it goes again...
Maybe re-use the comments from Alan in src/main/detail.c:
That sounds a lot better than mine, so sure!
+ 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.
I was thinking about that, but I didn't want to make a loop where <detail> kept appearing as an empty file and I kept moving it away.
+ 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)
I used continue so that, once <detail> was renamed to <detail>.work, the loop would start over and open <detail>.work. However, this makes assumptions that this block of code will always be at the beginning of the loop, so I'm going to change 'continue' to the fopen that I was taking the long way to get to.
Please no line of more than 80 characters in comments.
Oops, sorry about that. My co-worker just showed me the "gq" command in vim, though, so it shouldn't happen any more...
+ * 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.
Oops, that's not confusing, that's just wrong altogether :). I meant to say lock and write. However, this seems to be well explained by the comment from src/main/detail.c.
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)
Good point, revised. I thought about making this a blocking lock (rlm_detail should release it quickly, I would hope) but it's probably better to let radrelay run through the other parts of the loop while waiting. I've attached a revised patch. I've dropped it in, and it works as expected. It doesn't peg the CPU at all, I assume it spends most of its time in the select's waiting on network traffic. I almost want to add some more sleeping though, it's hard to see the files appear and disappear, they move so fast... :) I'll see how it responds during the day tomorrow, load is lighter right now (wireless users). --- src/main/radrelay.c.orig 2007-03-16 06:22:03.000000000 -0700 +++ src/main/radrelay.c 2007-12-03 18:31:20.000000000 -0800 @@ -140,7 +140,6 @@ int read_one(FILE *fp, struct relay_request *req); int do_recv(struct relay_misc *r_args); int do_send(struct relay_request *r, char *secret); -int detail_move(char *from, char *to); void loop(struct relay_misc *r_args); int find_shortname(char *shortname, char **host, char **secret); void usage(void); @@ -202,8 +201,6 @@ /* * Read one request from the detail file. - * Note that the file is locked during the read, and that - * we return *with the file locked* if we reach end-of-file. * * STATE_EMPTY: Slot is empty. * STATE_BUSY1: Looking for start of a detail record (timestamp) @@ -219,8 +216,6 @@ char key[32], val[32]; int skip; long fpos; - int x; - unsigned int i = 0; /* Never happens */ if (r_req->state == STATE_FULL) @@ -230,21 +225,7 @@ r_req->state = STATE_BUSY1; } - /* - * Try to lock the detail-file. - * If lockf is used we want to lock the _whole_ file, hence the - * fseek to the start of the file. - */ fpos = ftell(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) - return 0; redo: s = NULL; @@ -359,11 +340,6 @@ return EOF; } - fpos = ftell(fp); - fseek(fp, 0L, SEEK_SET); - rad_unlockfd(fileno(fp), 0); - fseek(fp, fpos, SEEK_SET); - return 0; } @@ -523,30 +499,6 @@ } /* - * Rename a file, then recreate the old file with the - * same permissions and zero size. - */ -int detail_move(char *from, char *to) -{ - struct stat st; - int n; - int oldmask; - - if (stat(from, &st) < 0) - return -1; - 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; -} - - -/* * Open detail file, collect records, send them to the * remote accounting server, yadda yadda yadda. * @@ -567,11 +519,10 @@ struct relay_stats stats; fd_set readfds; char work[1030]; - time_t now, uptime, last_rename = 0; + time_t now, uptime; int i, n; int state = STATE_RUN; int id; - long fpos; strNcpy(work, r_args->detail, sizeof(work) - 6); strcat(work, ".work"); @@ -607,19 +558,40 @@ if (got_sigterm) state = STATE_SHUTDOWN; /* - * Open detail file - if needed, and if we can. + * 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 (state == STATE_RUN && fp == NULL) { - if ((fp = fopen(work, "r+")) != NULL) - state = STATE_BACKLOG; - else - fp = fopen(r_args->detail, "r+"); - if (fp == NULL) { - fprintf(stderr, "%s: Unable to open detail file - %s\n", progname, r_args->detail); - perror("fopen"); - return; + if ((fp = fopen(work, "r+")) == NULL) { + /* + * Try moving the detail file. If it + * doesn't exist, we can't do anything. + */ + if(rename(r_args->detail, work) != -1) + fp = fopen(work, "r+"); } + + if(fp) { + /* + * Try to lock the detail-file. If lockf is + * used we want to lock the _whole_ file, hence + * the fseek to the start of the file. + */ + fseek(fp, 0L, SEEK_SET); + if(rad_lockfd_nonblock(fileno(fp), 0) == -1) { + fclose(fp); + fp = NULL; + } else { + state = STATE_BACKLOG; + } + } } /* @@ -640,43 +612,6 @@ state = STATE_CLOSE; break; } - - /* - * End of file. See if the file has - * any size, and if we renamed less - * than 10 seconds ago or not. - */ - now = time(NULL); - if (ftell(fp) == 0 || now < last_rename + 10) { - fpos = ftell(fp); - fseek(fp, 0L, SEEK_SET); - rad_unlockfd(fileno(fp), 0); - fseek(fp, fpos, SEEK_SET); - break; - } - last_rename = now; - - /* - * We rename the file to <file>.work - * and create an empty new file. - */ - if (detail_move(r_args->detail, work) == 0) { - if (debug_flag > 0) - fprintf(stderr, "Moving %s to %s\n", - r_args->detail, work); - /* - * rlm_detail might still write - * something to <detail>.work if - * it opens <detail> before it is - * renamed (race condition) - */ - ms_sleep(1000); - state = STATE_BACKLOG; - } - fpos = ftell(fp); - fseek(fp, 0L, SEEK_SET); - rad_unlockfd(fileno(fp), 0); - fseek(fp, fpos, SEEK_SET); } while(0); if (r_args->records_print && state == STATE_RUN){ stats.records_read++;