+2006-04-14 Erez Zadok <ezk@cs.sunysb.edu>
+
+ * 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 <christos@zoulas.com>
* amd/amfs_generic.c (amfs_lookup_mntfs): fix use-after-free bug
* 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
* 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;
}
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 *);
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
* 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) */
}
/*
- * 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;
}
projects / am-utils-6.1.git / commitdiff