Diff options
| author | Bryce Kwon <bryce@brycekwon.com> | |
|---|---|---|
| committer | Bryce Kwon <bryce@brycekwon.com> | |
| commit | ||
| parent | ||
| tree | ||
| download | ||
Keep lock contention out of the error log
process_slot logged every failure to lock, so each request that found
its slot being filled left a line, while refresh_slot logged none, so an
expired slot in a directory the server cannot write was served stale for
good without a word. lock_slot now logs the step that failed, as the
scan does, and leaves out only another request at work.
Diffstat (limited to 'source')
| -rw-r--r-- | source/cache.c | 36 |
1 file changed, 25 insertions, 11 deletions
diff --git a/source/cache.c b/source/cache.c index 12151b8..64b7148 100644 --- a/source/cache.c +++ b/source/cache.c @@ -196,26 +196,37 @@ static int is_modified(struct cache_slot *slot) * The lock file becomes the slot once it is renamed, so it has to open with * the key the same way a slot does. The lock is taken without blocking, * because failing to get it is how a second process learns that this slot is - * already being rebuilt, so it returns an errno instead of waiting. + * already being rebuilt, so it returns an errno instead of waiting. Every + * other failure is logged here, for both callers. */ static int lock_slot(struct cache_slot *slot) { struct stat held, named; + int err; // Close-on-exec, so that a program the fill runs gets the file only // as its stdout, and holds the lock no longer than it can write there. slot->lock_fd = open(slot->lock_path, O_RDWR | O_CREAT | O_CLOEXEC, S_IRUSR | S_IWUSR); - if (slot->lock_fd == -1) - return errno; + if (slot->lock_fd == -1) { + err = errno; + fprintf(stderr, "[cgit] Unable to open %s: %s (%d)\n", slot->lock_path, strerror(err), err); + return err; + } // An flock rather than a POSIX record lock. The descriptor is put on // stdout for the fill, and a record lock is dropped as soon as any // descriptor on the file closes, which an exec filter does to stdout, // while an flock holds until the last descriptor closes. if (flock(slot->lock_fd, LOCK_EX | LOCK_NB) < 0) { - int saved_errno = errno; + // A lock held elsewhere only means concurrent requests, which + // is not worth a line in the server log. Any other failure, a + // filesystem without lock support say, silently keeps the slot + // from ever being filled or refreshed and does deserve one. + err = errno; + if (err != EWOULDBLOCK) + fprintf(stderr, "[cgit] Unable to lock %s: %s (%d)\n", slot->lock_path, strerror(err), err); close(slot->lock_fd); slot->lock_fd = -1; - return saved_errno; + return err; } // The lock landed on whatever inode the path named at open. A holder // finishing in between renames that inode into place as the live @@ -233,10 +244,14 @@ static int lock_slot(struct cache_slot *slot) } // A run that died before its rename leaves the lock file behind, so // start from empty now that nobody else can be writing it. - if (ftruncate(slot->lock_fd, 0) < 0) - return errno; - if (write_in_full(slot->lock_fd, slot->key, slot->keylen + 1) < 0) - return errno; + if ( + ftruncate(slot->lock_fd, 0) < 0 || + write_in_full(slot->lock_fd, slot->key, slot->keylen + 1) < 0 + ) { + err = errno; + fprintf(stderr, "[cgit] Unable to write %s: %s (%d)\n", slot->lock_path, strerror(err), err); + return err; + } return 0; } @@ -425,8 +440,7 @@ static int process_slot(struct cache_slot *slot) // straight to the client and the caller is told the request succeeded, // because it did. close_fd(&slot->cache_fd); - if ((err = lock_slot(slot)) != 0) { - fprintf(stderr, "[cgit] Unable to lock slot %s: %s (%d)\n", slot->lock_path, strerror(err), err); + if (lock_slot(slot)) { slot->fn(); return 0; } |