On Mon, Jan 07, 2008 at 12:39:25AM -0500, Wendy Cheng wrote:
> +#define DEBUG 0
> +#define fo_printk(x...) ((void)(DEBUG && printk(x)))
Please don't introduce more debugging helpers but use the existing
ones.
> +extern __u32 in_aton(const char *str);
This is properly declared in <linux/inet.h>
> + return (nfsd_fo_cmd(where, fo_path, 0));
no braces around the return values please. (happens multiple times)
> +int
> +nfsd_fo_cmd(int cmd, char *datap, int grace_period)
> +{
> + struct nameidata nd;
> + void *objp = (void *)datap;
> + int rc=0;
> +
> + if (cmd == NFSD_FO_PATH) {
> + rc = path_lookup((const char *)datap, 0, &nd);
> + if (rc) {
> + fo_printk("nfsd: nfsd_fo path (%s) not found\n", datap);
> + return rc;
> + }
> + fo_printk("nfsd: nfsd_fo lookup path = (0x%p,0x%p)\n",
> + nd.mnt, nd.dentry);
> + objp = (void *) &nd;
> + }
> + return (nlmsvc_fo_cmd(cmd, objp, grace_period));
this has nothing in common for the two cases except for the final
function call. Please just inline this function into the caller which
gives you quite a bit of nice cleanup by not passing all the parameters
in odd ways aswell.
And btw, I think this code has quite a bit too much debug printks,
almost more than code. I'd be better readable by reducing that.
> +static inline int
> +nlmsvc_fo_unlock_match(void *datap, struct nlm_file *file)
> +{
> + nlm_fo_cmd *fo_cmd = (nlm_fo_cmd *) datap;
> + int cmd = fo_cmd->cmd;
> + struct path *f_path;
> +
> + fo_printk("nlm_fo_unlock_match cmd=%d\n", cmd);
> +
> + if (cmd == NFSD_FO_VIP) {
Please split this into two separate functions for the NFSD_FO_VIP/
NFSD_FO_PATH cases as there's just about nothing in common for the two.
> {
> + /* Cluster failover has timing constraints. There is a slight
> + * performance hit if nlm_fo_unlock_match() is implemented as
> + * a match fn (since it will be invoked for each block, share,
> + * and lock later when the lists are traversed). Instead, we
> + * add path-matching logic into the following unlikely clause.
> + * If matches, the dummy nlmsvc_fo_match will always return
> + * true.
> + */
> + dprintk("nlm_inspect_files: file=%p\n", file);
> + if (unlikely(match == nlmsvc_fo_match)) {
> + if (!nlmsvc_fo_unlock_match((void *)host, file))
> + return 0;
> + fo_printk("nlm_fo find lock file entry (0x%p)\n", file);
> + }
That's a quite nast hack. Did you benchmark the the match fn variant
to see if there is actually any mesurable difference? Also no need
to downcast pointers to void *, it's implicit in C.
> + /* "if" place holder for NFSD_FO_RESUME */
> + {
no need for such placeholders.
> +/* cluster failover support */
> +
> +typedef struct {
> + int cmd;
> + int stat;
> + int gp;
> + void *datap;
> +} nlm_fo_cmd;
please don't introduce typedefs for struct types.
