From 4770e795f51cdff3b43325474997088f540c9b18 Mon Sep 17 00:00:00 2001 From: Leo Alterman Date: Wed, 8 Aug 2012 17:40:43 -0700 Subject: [PATCH 1/1] lockfile: Remove lockfile_lock timeout argument lockfile_lock() accepts a timeout argument but, aside from unit tests pertaining to timeout, its value is always 0. Since this feature relies on a periodic SIGALRM signal, which is not a given if we're not caching time, the cleanest solution is just to remove it. Signed-off-by: Leo Alterman --- lib/lockfile.c | 46 +++++-------------- lib/lockfile.h | 2 +- ovsdb/file.c | 2 +- ovsdb/log.c | 2 +- ovsdb/ovsdb-tool.c | 4 +- tests/lockfile.at | 2 - tests/test-lockfile.c | 101 ++++++++++++------------------------------ 7 files changed, 45 insertions(+), 114 deletions(-) diff --git a/lib/lockfile.c b/lib/lockfile.c index 7ac34659b..db84aebb4 100644 --- a/lib/lockfile.c +++ b/lib/lockfile.c @@ -55,8 +55,7 @@ struct lockfile { static struct hmap lock_table = HMAP_INITIALIZER(&lock_table); static void lockfile_unhash(struct lockfile *); -static int lockfile_try_lock(const char *name, bool block, - struct lockfile **lockfilep); +static int lockfile_try_lock(const char *name, struct lockfile **lockfilep); /* Returns the name of the lockfile that would be created for locking a file * named 'filename_'. The caller is responsible for freeing the returned name, @@ -87,56 +86,33 @@ lockfile_name(const char *filename_) /* Locks the configuration file against modification by other processes and * re-reads it from disk. * - * The 'timeout' specifies the maximum number of milliseconds to wait for the - * config file to become free. Use 0 to avoid waiting or INT_MAX to wait - * forever. - * * Returns 0 on success, otherwise a positive errno value. On success, * '*lockfilep' is set to point to a new "struct lockfile *" that may be * unlocked with lockfile_unlock(). On failure, '*lockfilep' is set to - * NULL. */ + * NULL. Will not block if the lock cannot be immediately acquired. */ int -lockfile_lock(const char *file, int timeout, struct lockfile **lockfilep) +lockfile_lock(const char *file, struct lockfile **lockfilep) { /* Only exclusive ("write") locks are supported. This is not a problem * because the Open vSwitch code that currently uses lock files does so in * stylized ways such that any number of readers may access a file while it * is being written. */ - long long int warn_elapsed = 1000; - long long int start, elapsed; char *lock_name; int error; COVERAGE_INC(lockfile_lock); lock_name = lockfile_name(file); - time_refresh(); - start = time_msec(); - - do { - error = lockfile_try_lock(lock_name, timeout > 0, lockfilep); - time_refresh(); - elapsed = time_msec() - start; - if (elapsed > warn_elapsed) { - warn_elapsed *= 2; - VLOG_WARN("%s: waiting for lock file, %lld ms elapsed", - lock_name, elapsed); - } - } while (error == EINTR && (timeout == INT_MAX || elapsed < timeout)); - - if (error == EINTR) { - COVERAGE_INC(lockfile_timeout); - VLOG_WARN("%s: giving up on lock file after %lld ms", - lock_name, elapsed); - error = ETIMEDOUT; - } else if (error) { + + error = lockfile_try_lock(lock_name, lockfilep); + + if (error) { COVERAGE_INC(lockfile_error); if (error == EACCES) { error = EAGAIN; } - VLOG_WARN("%s: failed to lock file " - "(after %lld ms, with %d-ms timeout): %s", - lock_name, elapsed, timeout, strerror(error)); + VLOG_WARN("%s: failed to lock file: %s", + lock_name, strerror(error)); } free(lock_name); @@ -225,7 +201,7 @@ lockfile_register(const char *name, dev_t device, ino_t inode, int fd) } static int -lockfile_try_lock(const char *name, bool block, struct lockfile **lockfilep) +lockfile_try_lock(const char *name, struct lockfile **lockfilep) { struct flock l; struct stat s; @@ -268,7 +244,7 @@ lockfile_try_lock(const char *name, bool block, struct lockfile **lockfilep) l.l_len = 0; time_disable_restart(); - error = fcntl(fd, block ? F_SETLKW : F_SETLK, &l) == -1 ? errno : 0; + error = fcntl(fd, F_SETLK, &l) == -1 ? errno : 0; time_enable_restart(); if (!error) { diff --git a/lib/lockfile.h b/lib/lockfile.h index 202134b41..3e6b54c38 100644 --- a/lib/lockfile.h +++ b/lib/lockfile.h @@ -19,7 +19,7 @@ struct lockfile; char *lockfile_name(const char *file); -int lockfile_lock(const char *file, int timeout, struct lockfile **); +int lockfile_lock(const char *file, struct lockfile **); void lockfile_unlock(struct lockfile *); void lockfile_postfork(void); diff --git a/ovsdb/file.c b/ovsdb/file.c index 9e2dd5079..43fcb9564 100644 --- a/ovsdb/file.c +++ b/ovsdb/file.c @@ -649,7 +649,7 @@ ovsdb_file_compact(struct ovsdb_file *file) /* Lock temporary file. */ tmp_name = xasprintf("%s.tmp", file->file_name); - retval = lockfile_lock(tmp_name, 0, &tmp_lock); + retval = lockfile_lock(tmp_name, &tmp_lock); if (retval) { error = ovsdb_io_error(retval, "could not get lock on %s", tmp_name); goto exit; diff --git a/ovsdb/log.c b/ovsdb/log.c index 9b882dc49..b79535ac8 100644 --- a/ovsdb/log.c +++ b/ovsdb/log.c @@ -80,7 +80,7 @@ ovsdb_log_open(const char *name, enum ovsdb_log_open_mode open_mode, locking = open_mode != OVSDB_LOG_READ_ONLY; } if (locking) { - int retval = lockfile_lock(name, 0, &lockfile); + int retval = lockfile_lock(name, &lockfile); if (retval) { error = ovsdb_io_error(retval, "%s: failed to lock lockfile", name); diff --git a/ovsdb/ovsdb-tool.c b/ovsdb/ovsdb-tool.c index f31bdd106..6b75f4971 100644 --- a/ovsdb/ovsdb-tool.c +++ b/ovsdb/ovsdb-tool.c @@ -229,14 +229,14 @@ compact_or_convert(const char *src_name_, const char *dst_name_, /* Lock the source, if we will be replacing it. */ if (in_place) { - retval = lockfile_lock(src_name, 0, &src_lock); + retval = lockfile_lock(src_name, &src_lock); if (retval) { ovs_fatal(retval, "%s: failed to lock lockfile", src_name); } } /* Get (temporary) destination and lock it. */ - retval = lockfile_lock(dst_name, 0, &dst_lock); + retval = lockfile_lock(dst_name, &dst_lock); if (retval) { ovs_fatal(retval, "%s: failed to lock lockfile", dst_name); } diff --git a/tests/lockfile.at b/tests/lockfile.at index 602cfab44..877cc872f 100644 --- a/tests/lockfile.at +++ b/tests/lockfile.at @@ -15,8 +15,6 @@ CHECK_LOCKFILE([lock_blocks_same_process_twice], [0]) CHECK_LOCKFILE([lock_blocks_other_process], [1]) CHECK_LOCKFILE([lock_twice_blocks_other_process], [1]) CHECK_LOCKFILE([lock_and_unlock_allows_other_process], [1]) -CHECK_LOCKFILE([lock_timeout_gets_the_lock], [1]) -CHECK_LOCKFILE([lock_timeout_runs_out], [1]) CHECK_LOCKFILE([lock_multiple], [0]) CHECK_LOCKFILE([lock_symlink], [0]) CHECK_LOCKFILE([lock_symlink_to_dir], [0]) diff --git a/tests/test-lockfile.c b/tests/test-lockfile.c index 412010b30..b37fd225b 100644 --- a/tests/test-lockfile.c +++ b/tests/test-lockfile.c @@ -54,7 +54,7 @@ run_lock_and_unlock(void) { struct lockfile *lockfile; - CHECK(lockfile_lock("file", 0, &lockfile), 0); + CHECK(lockfile_lock("file", &lockfile), 0); lockfile_unlock(lockfile); } @@ -63,10 +63,10 @@ run_lock_and_unlock_twice(void) { struct lockfile *lockfile; - CHECK(lockfile_lock("file", 0, &lockfile), 0); + CHECK(lockfile_lock("file", &lockfile), 0); lockfile_unlock(lockfile); - CHECK(lockfile_lock("file", 0, &lockfile), 0); + CHECK(lockfile_lock("file", &lockfile), 0); lockfile_unlock(lockfile); } @@ -75,8 +75,8 @@ run_lock_blocks_same_process(void) { struct lockfile *lockfile; - CHECK(lockfile_lock("file", 0, &lockfile), 0); - CHECK(lockfile_lock("file", 0, &lockfile), EDEADLK); + CHECK(lockfile_lock("file", &lockfile), 0); + CHECK(lockfile_lock("file", &lockfile), EDEADLK); lockfile_unlock(lockfile); } @@ -85,9 +85,9 @@ run_lock_blocks_same_process_twice(void) { struct lockfile *lockfile; - CHECK(lockfile_lock("file", 0, &lockfile), 0); - CHECK(lockfile_lock("file", 0, &lockfile), EDEADLK); - CHECK(lockfile_lock("file", 0, &lockfile), EDEADLK); + CHECK(lockfile_lock("file", &lockfile), 0); + CHECK(lockfile_lock("file", &lockfile), EDEADLK); + CHECK(lockfile_lock("file", &lockfile), EDEADLK); lockfile_unlock(lockfile); } @@ -118,10 +118,10 @@ run_lock_blocks_other_process(void) * this function that does the wait() call. */ static struct lockfile *lockfile; - CHECK(lockfile_lock("file", 0, &lockfile), 0); + CHECK(lockfile_lock("file", &lockfile), 0); if (do_fork() == CHILD) { lockfile_unlock(lockfile); - CHECK(lockfile_lock("file", 0, &lockfile), EAGAIN); + CHECK(lockfile_lock("file", &lockfile), EAGAIN); exit(11); } } @@ -131,10 +131,10 @@ run_lock_twice_blocks_other_process(void) { struct lockfile *lockfile, *dummy; - CHECK(lockfile_lock("file", 0, &lockfile), 0); - CHECK(lockfile_lock("file", 0, &dummy), EDEADLK); + CHECK(lockfile_lock("file", &lockfile), 0); + CHECK(lockfile_lock("file", &dummy), EDEADLK); if (do_fork() == CHILD) { - CHECK(lockfile_lock("file", 0, &dummy), EAGAIN); + CHECK(lockfile_lock("file", &dummy), EAGAIN); exit(11); } } @@ -144,72 +144,31 @@ run_lock_and_unlock_allows_other_process(void) { struct lockfile *lockfile; - CHECK(lockfile_lock("file", 0, &lockfile), 0); + CHECK(lockfile_lock("file", &lockfile), 0); lockfile_unlock(lockfile); if (do_fork() == CHILD) { - CHECK(lockfile_lock("file", 0, &lockfile), 0); + CHECK(lockfile_lock("file", &lockfile), 0); exit(11); } } -static void -run_lock_timeout_gets_the_lock(void) -{ - struct lockfile *lockfile; - - CHECK(lockfile_lock("file", 0, &lockfile), 0); - - if (do_fork() == CHILD) { - lockfile_unlock(lockfile); - CHECK(lockfile_lock("file", TIME_UPDATE_INTERVAL * 3, &lockfile), 0); - exit(11); - } else { - long long int now = time_msec(); - while (time_msec() < now + TIME_UPDATE_INTERVAL) { - pause(); - } - lockfile_unlock(lockfile); - } -} - -static void -run_lock_timeout_runs_out(void) -{ - struct lockfile *lockfile; - - CHECK(lockfile_lock("file", 0, &lockfile), 0); - - if (do_fork() == CHILD) { - lockfile_unlock(lockfile); - CHECK(lockfile_lock("file", TIME_UPDATE_INTERVAL, &lockfile), - ETIMEDOUT); - exit(11); - } else { - long long int now = time_msec(); - while (time_msec() < now + TIME_UPDATE_INTERVAL * 3) { - pause(); - } - lockfile_unlock(lockfile); - } -} - static void run_lock_multiple(void) { struct lockfile *a, *b, *c, *dummy; - CHECK(lockfile_lock("a", 0, &a), 0); - CHECK(lockfile_lock("b", 0, &b), 0); - CHECK(lockfile_lock("c", 0, &c), 0); + CHECK(lockfile_lock("a", &a), 0); + CHECK(lockfile_lock("b", &b), 0); + CHECK(lockfile_lock("c", &c), 0); lockfile_unlock(a); - CHECK(lockfile_lock("a", 0, &a), 0); - CHECK(lockfile_lock("a", 0, &dummy), EDEADLK); + CHECK(lockfile_lock("a", &a), 0); + CHECK(lockfile_lock("a", &dummy), EDEADLK); lockfile_unlock(a); lockfile_unlock(b); - CHECK(lockfile_lock("a", 0, &a), 0); + CHECK(lockfile_lock("a", &a), 0); lockfile_unlock(c); lockfile_unlock(a); @@ -231,14 +190,14 @@ run_lock_symlink(void) CHECK(stat(".b.~lock~", &s), -1); CHECK(errno, ENOENT); - CHECK(lockfile_lock("a", 0, &a), 0); - CHECK(lockfile_lock("a", 0, &dummy), EDEADLK); - CHECK(lockfile_lock("b", 0, &dummy), EDEADLK); + CHECK(lockfile_lock("a", &a), 0); + CHECK(lockfile_lock("a", &dummy), EDEADLK); + CHECK(lockfile_lock("b", &dummy), EDEADLK); lockfile_unlock(a); - CHECK(lockfile_lock("b", 0, &b), 0); - CHECK(lockfile_lock("b", 0, &dummy), EDEADLK); - CHECK(lockfile_lock("a", 0, &dummy), EDEADLK); + CHECK(lockfile_lock("b", &b), 0); + CHECK(lockfile_lock("b", &dummy), EDEADLK); + CHECK(lockfile_lock("a", &dummy), EDEADLK); lockfile_unlock(b); CHECK(lstat(".a.~lock~", &s), 0); @@ -268,12 +227,12 @@ run_lock_symlink_to_dir(void) CHECK(S_ISLNK(s.st_mode) != 0, 1); /* Lock 'a'. */ - CHECK(lockfile_lock("a", 0, &a), 0); + CHECK(lockfile_lock("a", &a), 0); CHECK(lstat("dir/.b.~lock~", &s), 0); CHECK(S_ISREG(s.st_mode) != 0, 1); CHECK(lstat(".a.~lock~", &s), -1); CHECK(errno, ENOENT); - CHECK(lockfile_lock("dir/b", 0, &dummy), EDEADLK); + CHECK(lockfile_lock("dir/b", &dummy), EDEADLK); lockfile_unlock(a); } @@ -300,8 +259,6 @@ static const struct test tests[] = { TEST(lock_blocks_other_process), TEST(lock_twice_blocks_other_process), TEST(lock_and_unlock_allows_other_process), - TEST(lock_timeout_gets_the_lock), - TEST(lock_timeout_runs_out), TEST(lock_multiple), TEST(lock_symlink), TEST(lock_symlink_to_dir), -- 2.45.2