On Thursday September 14, [email protected] wrote:
> This change enables per NFS-export entry lockd grace period. The
> implementation is based on a double linked list fo_fsid_list that
> contains entries of fsid info. It is expected this would not be a
> frequent event. The fo_fsid_list is short and the entries expire within
> a maximum of 50 seconds. The grace period setting follows the existing
> NLM grace period handling logic and is triggered via echoing the NFS
> export filesystem id into nfsd procfs entry as:
>
> shell> echo 1234 > /proc/fs/nfsd/nlm_set_igrace
What is the 'i' for?
How about /proc/fs/nfsd/nlm_set_grace_for_fsid ??
> --- linux-1/include/linux/lockd/lockd.h 2006-09-03 21:51:41.000000000 -0400
> +++ linux-2/include/linux/lockd/lockd.h 2006-09-13 22:48:00.000000000 -0400
> @@ -107,6 +107,17 @@ struct nlm_file {
> int f_hash; /* hash of f_handle */
> };
>
> +#define NLM_FO_MAX_FSID_GP 127
Why do you need this limit?
> + *
> + * Also, please don't ask why using opencoded list manipulation,
> + * instead of <linux/list.h>, unless you can point to me where
> + * in that file have existing macro and/or functions that can do
> + * single linked list.
I don't think this comment is appropriate any more. You are using
list.h lists, not open coding them.
> + /* check to see whether this_fsid is in fo_fsid_list list */
> + list_for_each_safe(p, tlist, &fo_fsid_list) {
> + e_this = list_entry(p, struct fo_fsid, g_list);
> + if (time_before(e_this->g_expire, jiffies)) {
> + printk("lockd: fsid=%d grace period expires\n",
> + e_this->g_fsid);
> + list_del(p);
> + fo_fsid_cnt--;
> + kfree(e_this);
> + } else if (e_this->g_fsid == this_fsid) {
> + if (!e_this->g_flag) {
> + e_this->g_flag = 1;
> + printk("lockd: fsid=%d in grace period\n",
> + e_this->g_fsid);
> + }
> + rc = 1;
I assume this 'g_flag' was just for debugging. Can it be take out
now?
Also, I though you should break out of the loop once you set rc = 1.
> * Obtain client and file from arguments
> */
> @@ -89,7 +102,7 @@ nlm4svc_proc_test(struct svc_rqst *rqstp
> resp->cookie = argp->cookie;
>
> /* Don't accept test requests during grace period */
> - if (nlmsvc_grace_period) {
> + if ((nlmsvc_grace_period) || (nlm4svc_fo_grace_period(argp))) {
> resp->status = nlm_lck_denied_grace_period;
> return rpc_success;
> }
I think this (and the rest) would look cleaner if you put the
'nlmsvc_grace_period' test into the nlm4svc_fo_grace_period function.
NeilBrown
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs