From 0f6cb5cb0329f00581bc631211cdea8100dbce09 Mon Sep 17 00:00:00 2001 From: Erez Zadok Date: Fri, 14 Apr 2006 19:51:43 +0000 Subject: [PATCH] * libamu/mount_fs.c (compute_nfs_attrcache_flags): use new hasmntvalerr() function to set attribute cache values only if they were set (regardless whether they were set to zero or a non-zero value). Before, we were unable to distinguish between an error to parse an option, and a user who actually wanted to set an attribute-cache value to 0. This now fixes an important performance bug that Amd was turning off the attribute caches even for regular (non-automounter) NFS mounts. * libamu/mtab.c (hasmntvalerr): new function to set the value of an option into an integer, but ONLY if that options was set and parsed correctly. This function returns 1 on error, 0 on success (instead of always setting the option value to 0). (hasmntval): wrapper function around hasmntvalerr, which maintains backwards compatibility (always sets option value to 0, even on error to parse the option). * amd/nfs_subr.c (fh_to_mp3): use long int printf format for fhh_pid. --- ChangeLog | 22 +++++++++++ NEWS | 2 + amd/nfs_subr.c | 2 +- include/am_utils.h | 1 + libamu/mount_fs.c | 58 +++++++++++++++++------------ libamu/mtab.c | 92 ++++++++++++++++++++++++++++++++-------------- 6 files changed, 125 insertions(+), 52 deletions(-) diff --git a/ChangeLog b/ChangeLog index 236c405a..53064fed 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,25 @@ +2006-04-14 Erez Zadok + + * libamu/mount_fs.c (compute_nfs_attrcache_flags): use new + hasmntvalerr() function to set attribute cache values only if they + were set (regardless whether they were set to zero or a non-zero + value). Before, we were unable to distinguish between an error to + parse an option, and a user who actually wanted to set an + attribute-cache value to 0. This now fixes an important + performance bug that Amd was turning off the attribute caches even + for regular (non-automounter) NFS mounts. + + * libamu/mtab.c (hasmntvalerr): new function to set the value of + an option into an integer, but ONLY if that options was set and + parsed correctly. This function returns 1 on error, 0 on success + (instead of always setting the option value to 0). + (hasmntval): wrapper function around hasmntvalerr, which maintains + backwards compatibility (always sets option value to 0, even on + error to parse the option). + + * amd/nfs_subr.c (fh_to_mp3): use long int printf format for + fhh_pid. + 2006-04-05 Christos Zoulas * amd/amfs_generic.c (amfs_lookup_mntfs): fix use-after-free bug diff --git a/NEWS b/NEWS index 6cd03877..df8ae55a 100644 --- a/NEWS +++ b/NEWS @@ -27,6 +27,8 @@ better tune Amd's responsiveness under heavy scheduler loads. * handle old-style filehandles correctly (for mount points longer than 28 chars) * use-after-free bug in amfs_lookup_mntfs (Coverity) + * don't turn off attribute cache for regular NFS mounts (improves + performance) *** Notes specific to am-utils version 6.2a1 diff --git a/amd/nfs_subr.c b/amd/nfs_subr.c index 80c52d18..d80cd6a7 100644 --- a/amd/nfs_subr.c +++ b/amd/nfs_subr.c @@ -658,7 +658,7 @@ fh_to_mp3(am_nfs_fh *fhp, int *rp, int vop) * which is now out of date. */ if (fp->u.s.fhh_pid != get_server_pid()) { - dlog("fh_to_mp3: wrong pid %d != my pid %ld", fp->u.s.fhh_pid, get_server_pid()); + dlog("fh_to_mp3: wrong pid %ld != my pid %ld", fp->u.s.fhh_pid, get_server_pid()); goto drop; } diff --git a/include/am_utils.h b/include/am_utils.h index 99205e20..355c55c2 100644 --- a/include/am_utils.h +++ b/include/am_utils.h @@ -293,6 +293,7 @@ extern int compute_mount_flags(mntent_t *); extern u_long get_amd_program_number(void); extern int getcreds(struct svc_req *, uid_t *, gid_t *, SVCXPRT *); extern int hasmntval(mntent_t *, char *); +extern unsigned int hasmntvalerr(mntent_t *, char *, int *); extern char *hasmntstr(mntent_t *, char *); extern char *hasmnteq(mntent_t *, char *); extern char *haseq(char *); diff --git a/libamu/mount_fs.c b/libamu/mount_fs.c index 5519d246..80891bbb 100644 --- a/libamu/mount_fs.c +++ b/libamu/mount_fs.c @@ -336,13 +336,15 @@ static void compute_nfs_attrcache_flags(nfs_args_t *nap, mntent_t *mntp) { int acval = 0; + int err_acval = 1; /* 1 means we found no 'actimeo' value */ + int err_acrdmm; /* for ac{reg,dir}{min,max} */ /************************************************************************/ /*** ATTRIBUTE CACHES ***/ /************************************************************************/ /* * acval is set to 0 at the top of the function. If actimeo mount option - * exists and defined in mntopts, then it acval is set to it. + * exists and defined in mntopts, then its acval is set to it. * If the value is non-zero, then we set all attribute cache fields to it. * If acval is zero, it means it was never defined in mntopts or the * actimeo mount option does not exist, in which case we check for @@ -351,81 +353,91 @@ compute_nfs_attrcache_flags(nfs_args_t *nap, mntent_t *mntp) * on the values of the attribute caches. */ #ifdef MNTTAB_OPT_ACTIMEO - acval = hasmntval(mntp, MNTTAB_OPT_ACTIMEO); /* attr cache timeout (sec) */ + err_acval = hasmntvalerr(mntp, MNTTAB_OPT_ACTIMEO, &acval); /* attr cache timeout (sec) */ #endif /* MNTTAB_OPT_ACTIMEO */ /*** acregmin ***/ #ifdef HAVE_NFS_ARGS_T_ACREGMIN - if (acval) { + err_acrdmm = 1; /* 1 means we found no acregmin value */ + if (!err_acval) { nap->acregmin = acval; /* min ac timeout for reg files (sec) */ } else { # ifdef MNTTAB_OPT_ACREGMIN - nap->acregmin = hasmntval(mntp, MNTTAB_OPT_ACREGMIN); + err_acrdmm = hasmntvalerr(mntp, MNTTAB_OPT_ACREGMIN, &nap->acregmin); # else /* not MNTTAB_OPT_ACREGMIN */ nap->acregmin = 0; # endif /* not MNTTAB_OPT_ACREGMIN */ } - /* set this flag, because if we got here, then we changed acregmin */ + /* set this flag iff we changed acregmin (possibly to zero) */ # ifdef MNT2_NFS_OPT_ACREGMIN - nap->flags |= MNT2_NFS_OPT_ACREGMIN; + if (!err_acval || !err_acrdmm) + nap->flags |= MNT2_NFS_OPT_ACREGMIN; # endif /* MNT2_NFS_OPT_ACREGMIN */ #endif /* HAVE_NFS_ARGS_T_ACREGMIN */ /*** acregmax ***/ #ifdef HAVE_NFS_ARGS_T_ACREGMAX - if (acval) { - nap->acregmax = acval; /* min ac timeout for reg files (sec) */ + err_acrdmm = 1; /* 1 means we found no acregmax value */ + if (!err_acval) { + nap->acregmax = acval; /* max ac timeout for reg files (sec) */ } else { # ifdef MNTTAB_OPT_ACREGMAX - nap->acregmax = hasmntval(mntp, MNTTAB_OPT_ACREGMAX); + err_acrdmm = hasmntvalerr(mntp, MNTTAB_OPT_ACREGMAX, &nap->acregmax); # else /* not MNTTAB_OPT_ACREGMAX */ nap->acregmax = 0; # endif /* not MNTTAB_OPT_ACREGMAX */ } - /* set this flag, because if we got here, then we changed acregmax */ + /* set this flag iff we changed acregmax (possibly to zero) */ # ifdef MNT2_NFS_OPT_ACREGMAX - nap->flags |= MNT2_NFS_OPT_ACREGMAX; + if (!err_acval || !err_acrdmm) + nap->flags |= MNT2_NFS_OPT_ACREGMAX; # endif /* MNT2_NFS_OPT_ACREGMAX */ #endif /* HAVE_NFS_ARGS_T_ACREGMAX */ /*** acdirmin ***/ #ifdef HAVE_NFS_ARGS_T_ACDIRMIN - if (acval) { - nap->acdirmin = acval; /* min ac timeout for reg files (sec) */ + err_acrdmm = 1; /* 1 means we found no acdirmin value */ + if (!err_acval) { + nap->acdirmin = acval; /* min ac timeout for dirs (sec) */ } else { # ifdef MNTTAB_OPT_ACDIRMIN - nap->acdirmin = hasmntval(mntp, MNTTAB_OPT_ACDIRMIN); + err_acrdmm = hasmntvalerr(mntp, MNTTAB_OPT_ACDIRMIN, &nap->acdirmin); # else /* not MNTTAB_OPT_ACDIRMIN */ nap->acdirmin = 0; # endif /* not MNTTAB_OPT_ACDIRMIN */ } - /* set this flag, because if we got here, then we changed acdirmin */ + /* set this flag iff we changed acdirmin (possibly to zero) */ # ifdef MNT2_NFS_OPT_ACDIRMIN - nap->flags |= MNT2_NFS_OPT_ACDIRMIN; + if (!err_acval || !err_acrdmm) + nap->flags |= MNT2_NFS_OPT_ACDIRMIN; # endif /* MNT2_NFS_OPT_ACDIRMIN */ #endif /* HAVE_NFS_ARGS_T_ACDIRMIN */ /*** acdirmax ***/ #ifdef HAVE_NFS_ARGS_T_ACDIRMAX - if (acval) { - nap->acdirmax = acval; /* min ac timeout for reg files (sec) */ + err_acrdmm = 1; /* 1 means we found no acdirmax value */ + if (!err_acval) { + nap->acdirmax = acval; /* max ac timeout for dirs (sec) */ } else { # ifdef MNTTAB_OPT_ACDIRMAX - nap->acdirmax = hasmntval(mntp, MNTTAB_OPT_ACDIRMAX); + err_acrdmm = hasmntvalerr(mntp, MNTTAB_OPT_ACDIRMAX, &nap->acdirmax); # else /* not MNTTAB_OPT_ACDIRMAX */ nap->acdirmax = 0; # endif /* not MNTTAB_OPT_ACDIRMAX */ } - /* set this flag, because if we got here, then we changed acdirmax */ + /* set this flag iff we changed acdirmax (possibly to zero) */ # ifdef MNT2_NFS_OPT_ACDIRMAX - nap->flags |= MNT2_NFS_OPT_ACDIRMAX; + if (!err_acval || !err_acrdmm) + nap->flags |= MNT2_NFS_OPT_ACDIRMAX; # endif /* MNT2_NFS_OPT_ACDIRMAX */ #endif /* HAVE_NFS_ARGS_T_ACDIRMAX */ -#ifdef MNTTAB_OPT_NOAC /* don't cache attributes */ + + /* don't cache attributes */ +#if defined(MNTTAB_OPT_NOAC) && defined(MNT2_NFS_OPT_NOAC) if (amu_hasmntopt(mntp, MNTTAB_OPT_NOAC) != NULL) nap->flags |= MNT2_NFS_OPT_NOAC; -#endif /* MNTTAB_OPT_NOAC */ +#endif /* defined(MNTTAB_OPT_NOAC) && defined(MNT2_NFS_OPT_NOAC) */ } diff --git a/libamu/mtab.c b/libamu/mtab.c index 0070c51d..cf583864 100644 --- a/libamu/mtab.c +++ b/libamu/mtab.c @@ -137,44 +137,80 @@ hasmnteq(mntent_t *mnt, char *opt) /* - * Utility routine which determines the value of a - * numeric option in the mount options (such as port=%d). - * Returns 0 if the option is not specified. + * Wrapper around hasmntvalerr(), which retains backwards compatibiliy with + * older use of hasmntval(). + * + * XXX: eventually, all use of hasmntval() should be replaced with + * hasmntvalerr(). */ int hasmntval(mntent_t *mnt, char *opt) +{ + int err, val = 0; + + err = hasmntvalerr(mnt, opt, &val); + if (!err) /* if there was an error */ + return 0; /* redundant: val==0 above, but leave here for clarity */ + /* otherwise there was no error */ + return val; +} + + +/* + * Utility routine which determines the value of a numeric option in the + * mount options (such as port=%d), and fills in the value in the argument + * valp (argument won't be touched if no value is set, for example due to an + * error). + * + * Returns non-zero (1) on error; returns 0 on success. + * + * XXX: eventually, all use of hasmntval() should be replaced with + * hasmntvalerr(). + */ +unsigned int +hasmntvalerr(mntent_t *mnt, char *opt, int *valp) { char *str = amu_hasmntopt(mnt, opt); + int err = 1; /* 1 means no good value was set (an error) */ + char *eq, *endptr; + long int i; - if (str) { /* The option was there */ + /* exit if no option specificed */ + if (!str) { + goto out; + } - char *eq = hasmnteq(mnt, opt); + eq = hasmnteq(mnt, opt); - if (eq) { /* and had an = after it */ + if (!eq) { /* no argument to option ('=' sign was missing) */ + plog(XLOG_MAP, "numeric option to \"%s\" missing", opt); + goto out; + } - char *endptr = NULL; - long int i = strtol(eq, &endptr, 0); /* hex and octal allowed ;-) */ - - if (!endptr || - /* - * endptr set means strtol saw a non-digit. If the - * non-digit is a comma, it's probably the start of the next - * option. If the comma is the first char though, complain about - * it (foo=,bar is made noticeable by this). - * - * Similar reasoning for '\0' instead of comma, it's the end - * of the string. - */ - (endptr != eq && (*endptr == ',' || *endptr == '\0'))) - return((int) i); - /* whatever was after the '=' sign wasn't a number */ - plog(XLOG_MAP, "invalid numeric option in \"%s\": \"%s\"", opt, str); - } else { - /* No argument to option ('=' sign was missing) */ - plog(XLOG_MAP, "numeric option to \"%s\" missing", opt); - } + /* if got here, then we had an '=' after option name */ + endptr = NULL; + i = strtol(eq, &endptr, 0); /* hex and octal allowed ;-) */ + if (!endptr || + (endptr != eq && (*endptr == ',' || *endptr == '\0'))) { + /* + * endptr set means strtol saw a non-digit. If the non-digit is a + * comma, it's probably the start of the next option. If the comma is + * the first char though, complain about it (foo=,bar is made + * noticeable by this). + * + * Similar reasoning for '\0' instead of comma, it's the end of the + * string. + */ + *valp = (int) i; /* set good value */ + err = 0; /* no error */ + } else { + /* whatever was after the '=' sign wasn't a number */ + plog(XLOG_MAP, "invalid numeric option in \"%s\": \"%s\"", opt, str); + /* fall through to error/exit processing */ } - return 0; + + out: + return err; } -- 2.43.0