From: Ben Myers Subject: [PATCH] mountd: use separate lockfiles Date: Thu, 19 Mar 2009 17:14:58 -0500 Message-ID: <20090319221458.GI26378@sgi.com> References: <20090319172841.GF26378@sgi.com> <1237485682.7534.39.camel@heimdal.trondhjem.org> <20090319195357.GH26378@sgi.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Steve Dickson , linux-nfs@vger.kernel.org To: Trond Myklebust Return-path: Received: from relay2.sgi.com ([192.48.179.30]:44125 "EHLO relay.sgi.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753029AbZCSWMr (ORCPT ); Thu, 19 Mar 2009 18:12:47 -0400 In-Reply-To: <20090319195357.GH26378@sgi.com> Sender: linux-nfs-owner@vger.kernel.org List-ID: Hey Trond, On Thu, Mar 19, 2009 at 02:53:57PM -0500, Ben Myers wrote: > On Thu, Mar 19, 2009 at 02:01:21PM -0400, Trond Myklebust wrote: > > Why do you need a second call to open() in the first place? > > I'll give it another try. Here it is again. It works just fine without the second open. Thanks, Ben >From 1687f7a02cd34f76d5e6c461ca7d7bb56f3826d6 Mon Sep 17 00:00:00 2001 From: Ben Myers Date: Thu, 19 Mar 2009 16:44:54 -0500 Subject: [PATCH] Mountd should use separate lockfiles Mountd keeps file descriptors used for locks separate from those used for io and seems to assume that the lock will only be released on close of the file descriptor that was used with fcntl. Actually the lock is released when any file descriptor for that file is closed. When setexportent() is called after xflock() he closes and reopens the io file descriptor and defeats the lock. This patch fixes that by using a separate file for locking, cleaning them up when finished. Signed-off-by: Ben Myers --- support/export/rmtab.c | 2 +- support/export/xtab.c | 24 +++++++++++++----------- support/include/nfslib.h | 9 +++++++++ support/nfs/xio.c | 11 +++++++---- utils/mountd/mountd.c | 10 ++++++++++ utils/mountd/rmtab.c | 8 ++++---- 6 files changed, 44 insertions(+), 20 deletions(-) diff --git a/support/export/rmtab.c b/support/export/rmtab.c index e11a22a..b49e1aa 100644 --- a/support/export/rmtab.c +++ b/support/export/rmtab.c @@ -57,7 +57,7 @@ rmtab_read(void) file. */ int lockid; FILE *fp; - if ((lockid = xflock(_PATH_RMTAB, "w")) < 0) + if ((lockid = xflock(_PATH_RMTABLCK, "w")) < 0) return -1; rewindrmtabent(); if (!(fp = fsetrmtabent(_PATH_RMTABTMP, "w"))) { diff --git a/support/export/xtab.c b/support/export/xtab.c index 510765a..3b1dcce 100644 --- a/support/export/xtab.c +++ b/support/export/xtab.c @@ -23,7 +23,7 @@ static void cond_rename(char *newfile, char *oldfile); static int -xtab_read(char *xtab, int is_export) +xtab_read(char *xtab, char *lockfn, int is_export) { /* is_export == 0 => reading /proc/fs/nfs/exports - we know these things are exported to kernel * is_export == 1 => reading /var/lib/nfs/etab - these things are allowed to be exported @@ -33,7 +33,7 @@ xtab_read(char *xtab, int is_export) nfs_export *exp; int lockid; - if ((lockid = xflock(xtab, "r")) < 0) + if ((lockid = xflock(lockfn, "r")) < 0) return 0; setexportent(xtab, "r"); while ((xp = getexportent(is_export==0, 0)) != NULL) { @@ -66,18 +66,20 @@ xtab_mount_read(void) int fd; if ((fd=open(_PATH_PROC_EXPORTS, O_RDONLY))>=0) { close(fd); - return xtab_read(_PATH_PROC_EXPORTS, 0); + return xtab_read(_PATH_PROC_EXPORTS, + _PATH_PROC_EXPORTS, 0); } else if ((fd=open(_PATH_PROC_EXPORTS_ALT, O_RDONLY) >= 0)) { close(fd); - return xtab_read(_PATH_PROC_EXPORTS_ALT, 0); + return xtab_read(_PATH_PROC_EXPORTS_ALT, + _PATH_PROC_EXPORTS_ALT, 0); } else - return xtab_read(_PATH_XTAB, 2); + return xtab_read(_PATH_XTAB, _PATH_XTABLCK, 2); } int xtab_export_read(void) { - return xtab_read(_PATH_ETAB, 1); + return xtab_read(_PATH_ETAB, _PATH_ETABLCK, 1); } /* @@ -87,13 +89,13 @@ xtab_export_read(void) * fix the auth_reload logic as well... */ static int -xtab_write(char *xtab, char *xtabtmp, int is_export) +xtab_write(char *xtab, char *xtabtmp, char *lockfn, int is_export) { struct exportent xe; nfs_export *exp; int lockid, i; - if ((lockid = xflock(xtab, "w")) < 0) { + if ((lockid = xflock(lockfn, "w")) < 0) { xlog(L_ERROR, "can't lock %s for writing", xtab); return 0; } @@ -124,13 +126,13 @@ xtab_write(char *xtab, char *xtabtmp, int is_export) int xtab_export_write() { - return xtab_write(_PATH_ETAB, _PATH_ETABTMP, 1); + return xtab_write(_PATH_ETAB, _PATH_ETABTMP, _PATH_ETABLCK, 1); } int xtab_mount_write() { - return xtab_write(_PATH_XTAB, _PATH_XTABTMP, 0); + return xtab_write(_PATH_XTAB, _PATH_XTABTMP, _PATH_XTABLCK, 0); } void @@ -139,7 +141,7 @@ xtab_append(nfs_export *exp) struct exportent xe; int lockid; - if ((lockid = xflock(_PATH_XTAB, "w")) < 0) + if ((lockid = xflock(_PATH_XTABLCK, "w")) < 0) return; setexportent(_PATH_XTAB, "a"); xe = exp->m_export; diff --git a/support/include/nfslib.h b/support/include/nfslib.h index a51d79d..9d0d39d 100644 --- a/support/include/nfslib.h +++ b/support/include/nfslib.h @@ -34,18 +34,27 @@ #ifndef _PATH_XTABTMP #define _PATH_XTABTMP NFS_STATEDIR "/xtab.tmp" #endif +#ifndef _PATH_XTABLCK +#define _PATH_XTABLCK NFS_STATEDIR "/.xtab.lock" +#endif #ifndef _PATH_ETAB #define _PATH_ETAB NFS_STATEDIR "/etab" #endif #ifndef _PATH_ETABTMP #define _PATH_ETABTMP NFS_STATEDIR "/etab.tmp" #endif +#ifndef _PATH_ETABLCK +#define _PATH_ETABLCK NFS_STATEDIR "/.etab.lock" +#endif #ifndef _PATH_RMTAB #define _PATH_RMTAB NFS_STATEDIR "/rmtab" #endif #ifndef _PATH_RMTABTMP #define _PATH_RMTABTMP _PATH_RMTAB ".tmp" #endif +#ifndef _PATH_RMTABLCK +#define _PATH_RMTABLCK NFS_STATEDIR "/.rmtab.lock" +#endif #ifndef _PATH_PROC_EXPORTS #define _PATH_PROC_EXPORTS "/proc/fs/nfs/exports" #define _PATH_PROC_EXPORTS_ALT "/proc/fs/nfsd/exports" diff --git a/support/nfs/xio.c b/support/nfs/xio.c index f21f5f0..e126799 100644 --- a/support/nfs/xio.c +++ b/support/nfs/xio.c @@ -17,6 +17,7 @@ #include #include #include +#include #include "xmalloc.h" #include "xlog.h" #include "xio.h" @@ -59,11 +60,12 @@ xflock(char *fname, char *type) int fd; if (readonly) - fd = open(fname, O_RDONLY); + fd = open(fname, (O_RDONLY|O_CREAT), 0600); else - fd = open(fname, (O_RDWR|O_CREAT), mode); + fd = open(fname, (O_RDWR|O_CREAT), 0600); if (fd < 0) { - xlog(L_WARNING, "could not open %s for locking", fname); + xlog(L_WARNING, "could not open %s for locking: %s", + fname, strerror(errno)); return -1; } @@ -74,7 +76,8 @@ xflock(char *fname, char *type) alarm(10); if (fcntl(fd, F_SETLKW, &fl) < 0) { alarm(0); - xlog(L_WARNING, "failed to lock %s", fname); + xlog(L_WARNING, "failed to lock %s: %s", + fname, strerror(errno)); close(fd); fd = 0; } else { diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c index 8084359..25d292b 100644 --- a/utils/mountd/mountd.c +++ b/utils/mountd/mountd.c @@ -88,6 +88,14 @@ unregister_services (void) pmap_unset (MOUNTPROG, MOUNTVERS_NFSV3); } +static void +cleanup_lockfiles (void) +{ + unlink(_PATH_XTABLCK); + unlink(_PATH_ETABLCK); + unlink(_PATH_RMTABLCK); +} + /* Wait for all worker child processes to exit and reap them */ static void wait_for_workers (void) @@ -154,6 +162,7 @@ fork_workers(void) /* in parent */ wait_for_workers(); unregister_services(); + cleanup_lockfiles(); xlog(L_NOTICE, "mountd: no more workers, exiting\n"); exit(0); } @@ -170,6 +179,7 @@ killer (int sig) kill(0, SIGTERM); wait_for_workers(); } + cleanup_lockfiles(); xlog (L_FATAL, "Caught signal %d, un-registering and exiting.", sig); } diff --git a/utils/mountd/rmtab.c b/utils/mountd/rmtab.c index 5787ed6..c371f8d 100644 --- a/utils/mountd/rmtab.c +++ b/utils/mountd/rmtab.c @@ -58,7 +58,7 @@ mountlist_add(char *host, const char *path) int lockid; long pos; - if ((lockid = xflock(_PATH_RMTAB, "a")) < 0) + if ((lockid = xflock(_PATH_RMTABLCK, "a")) < 0) return; setrmtabent("r+"); while ((rep = getrmtabent(1, &pos)) != NULL) { @@ -98,7 +98,7 @@ mountlist_del(char *hname, const char *path) int lockid; int match; - if ((lockid = xflock(_PATH_RMTAB, "w")) < 0) + if ((lockid = xflock(_PATH_RMTABLCK, "w")) < 0) return; if (!setrmtabent("r")) { xfunlock(lockid); @@ -139,7 +139,7 @@ mountlist_del_all(struct sockaddr_in *sin) FILE *fp; int lockid; - if ((lockid = xflock(_PATH_RMTAB, "w")) < 0) + if ((lockid = xflock(_PATH_RMTABLCK, "w")) < 0) return; if (!(hp = gethostbyaddr((char *)&addr, sizeof(addr), AF_INET))) { xlog(L_ERROR, "can't get hostname of %s", inet_ntoa(addr)); @@ -188,7 +188,7 @@ mountlist_list(void) struct in_addr addr; struct hostent *he; - if ((lockid = xflock(_PATH_RMTAB, "r")) < 0) + if ((lockid = xflock(_PATH_RMTABLCK, "r")) < 0) return NULL; if (stat(_PATH_RMTAB, &stb) < 0) { xlog(L_ERROR, "can't stat %s", _PATH_RMTAB); -- 1.6.2.1.214.ge986c.dirty