From 56850593049e792c1775047481f814dd444042c5 Mon Sep 17 00:00:00 2001 From: Doug Ledford Date: Tue, 20 Jul 2010 17:42:26 -0400 Subject: Bugfix: mapfile locking is broken/racy While we attempt to use a lockfile to grant exclusive access to the mapfile, our implementation is buggy. Specifically, we create a lockfile, then lock it, at which point new instances can open the lockfile and attempt to lock it, which will cause them to block. However, when we are ready to unlock it, we unlink the file. This causes existing lock waiters to get a lock on an unlinked inode while a different instance may now create a new lockfile and get an exclusive lock on it. There are two possible fixes: don't unlink the lock file or require that the open of the lockfile include the O_CREAT|O_EXCL flags and on EEXIST retry the open until we either fail for a different error or actually create the lock file. I implemented the later on the assumption that the md device map file and therefore the lockfile are on local, early read write storage (probably a tmpfs of some sort) and therefore not subject to the problem that it doesn't work over nfs2 or earlier. In addition add warnings if we ever fail to get a lock when we request it. Signed-off-by: Doug Ledford --- Incremental.c | 8 ++++++-- mapfile.c | 16 +++++----------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/Incremental.c b/Incremental.c index 35490e7..a8a072d 100644 --- a/Incremental.c +++ b/Incremental.c @@ -294,7 +294,9 @@ int Incremental(char *devname, int verbose, int runstop, /* 4/ Check if array exists. */ - map_lock(&map); + if (map_lock(&map)) + fprintf(stderr, Name ": failed to get exclusive lock on " + "mapfile\n"); mp = map_by_uuid(&map, info.uuid); if (mp) mdfd = open_dev(mp->devnum); @@ -793,7 +795,9 @@ int Incremental_container(struct supertype *st, char *devname, int verbose, struct mdinfo *ra; struct map_ent *map = NULL; - map_lock(&map); + if (map_lock(&map)) + fprintf(stderr, Name ": failed to get exclusive lock on " + "mapfile\n"); for (ra = list ; ra ; ra = ra->next) { int mdfd; diff --git a/mapfile.c b/mapfile.c index 79a6bd8..69f78b1 100644 --- a/mapfile.c +++ b/mapfile.c @@ -58,7 +58,7 @@ char *mapname[2][3] = { }; char *mapdir[2] = { MAP_DIR, NULL }; -int mapmode[3] = { O_RDONLY, O_RDWR|O_CREAT, O_RDWR|O_CREAT|O_TRUNC }; +int mapmode[3] = { O_RDONLY, O_RDWR|O_CREAT, O_RDWR|O_CREAT|O_EXCL }; char *mapsmode[3] = { "r", "w", "w"}; FILE *open_map(int modenum, int *choice) @@ -120,14 +120,11 @@ static int lwhich = 0; int map_lock(struct map_ent **melp) { if (lf == NULL) { - lf = open_map(2, &lwhich); + do { + lf = open_map(2, &lwhich); + } while (lf == NULL && errno == EEXIST); if (lf == NULL) return -1; - if (flock(fileno(lf), LOCK_EX) != 0) { - fclose(lf); - lf = NULL; - return -1; - } } if (*melp) map_free(*melp); @@ -137,10 +134,7 @@ int map_lock(struct map_ent **melp) void map_unlock(struct map_ent **melp) { - if (lf) { - flock(fileno(lf), LOCK_UN); - fclose(lf); - } + fclose(lf); unlink(mapname[lwhich][2]); lf = NULL; } -- cgit