Skip to content

Commit

Permalink
lockfile: Remove lockfile_lock timeout argument
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
Leo Alterman committed Aug 9, 2012
1 parent b6d729a commit 4770e79
Show file tree
Hide file tree
Showing 7 changed files with 45 additions and 114 deletions.
46 changes: 11 additions & 35 deletions lib/lockfile.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion lib/lockfile.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
2 changes: 1 addition & 1 deletion ovsdb/file.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion ovsdb/log.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions ovsdb/ovsdb-tool.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
2 changes: 0 additions & 2 deletions tests/lockfile.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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])
101 changes: 29 additions & 72 deletions tests/test-lockfile.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand All @@ -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);
}

Expand All @@ -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);
}

Expand All @@ -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);
}

Expand Down Expand Up @@ -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);
}
}
Expand All @@ -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);
}
}
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
}
Expand All @@ -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),
Expand Down

0 comments on commit 4770e79

Please sign in to comment.