On Tue, Nov 06, 2018 at 01:35:09PM -0500, Scott Mayhew wrote:
> This will allow the reclaim_str_hashtbl to store either the recovery
> directory names used by the legacy client tracking code or the full
> client strings used by the nfsdcld client tracking code.
>
> Signed-off-by: Scott Mayhew <smayhew@xxxxxxxxxx>
> ---
> fs/nfsd/nfs4recover.c | 79 +++++++++++++++++++++++++++++++++++++------
> fs/nfsd/nfs4state.c | 30 ++++++++--------
> fs/nfsd/state.h | 8 ++---
> 3 files changed, 87 insertions(+), 30 deletions(-)
>
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index 5188f9f70c78..b288b14273ff 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -178,6 +178,7 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
> struct nfs4_client_reclaim *crp;
> int status;
> struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
> + struct xdr_netobj name;
>
> if (test_and_set_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
> return;
> @@ -222,9 +223,18 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
> inode_unlock(d_inode(dir));
> if (status == 0) {
> if (nn->in_grace) {
> - crp = nfs4_client_to_reclaim(dname, nn);
> - if (crp)
> - crp->cr_clp = clp;
> + name.data = kmemdup(dname, HEXDIR_LEN, GFP_KERNEL);
> + if (name.data) {
> + name.len = HEXDIR_LEN;
> + crp = nfs4_client_to_reclaim(name, nn);
> + if (crp)
> + crp->cr_clp = clp;
> + else
> + kfree(name.data);
> + } else {
> + dprintk("%s: failed to allocate memory for name.data!\n",
> + __func__);
> + }
The usual pattern for error handling is
try_something
if failed
goto error cleanup
try_something_else
if failed
goto error cleanup
...
and I find that a little easier to follow than this sort of:
try_something
if succeeded
try something_else
if succeeded
...
Also, nfsd4_create_clid is getting a little long.
But, I don't know, I haven't tried, if the alternatives don't look like
clear improvements then I can live with it.
The basic idea of the patch looks fine to me.
--b.
> }
> vfs_fsync(nn->rec_file, 0);
> } else {
> @@ -353,6 +363,7 @@ nfsd4_remove_clid_dir(struct nfs4_client *clp)
> char dname[HEXDIR_LEN];
> int status;
> struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
> + struct xdr_netobj name;
>
> if (!nn->rec_file || !test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
> return;
> @@ -376,9 +387,17 @@ nfsd4_remove_clid_dir(struct nfs4_client *clp)
> vfs_fsync(nn->rec_file, 0);
> if (nn->in_grace) {
> /* remove reclaim record */
> - crp = nfsd4_find_reclaim_client(dname, nn);
> - if (crp)
> - nfs4_remove_reclaim_record(crp, nn);
> + name.data = kmemdup(dname, HEXDIR_LEN, GFP_KERNEL);
> + if (name.data) {
> + name.len = HEXDIR_LEN;
> + crp = nfsd4_find_reclaim_client(name, nn);
> + kfree(name.data);
> + if (crp)
> + nfs4_remove_reclaim_record(crp, nn);
> + } else {
> + dprintk("%s: failed to allocate memory for name.data!\n",
> + __func__);
> + }
> }
> }
> out_drop_write:
> @@ -393,14 +412,31 @@ static int
> purge_old(struct dentry *parent, struct dentry *child, struct nfsd_net *nn)
> {
> int status;
> + struct xdr_netobj name;
>
> - if (nfs4_has_reclaimed_state(child->d_name.name, nn))
> + if (child->d_name.len != HEXDIR_LEN - 1) {
> + printk("%s: illegal name %pd in recovery directory\n",
> + __func__, child);
> + /* Keep trying; maybe the others are OK: */
> return 0;
> + }
> + name.data = kmemdup_nul(child->d_name.name, child->d_name.len, GFP_KERNEL);
> + if (!name.data) {
> + dprintk("%s: failed to allocate memory for name.data!\n",
> + __func__);
> + goto out;
> + }
> + name.len = HEXDIR_LEN;
> + if (nfs4_has_reclaimed_state(name, nn))
> + goto out_free;
>
> status = vfs_rmdir(d_inode(parent), child);
> if (status)
> printk("failed to remove client recovery directory %pd\n",
> child);
> +out_free:
> + kfree(name.data);
> +out:
> /* Keep trying, success or failure: */
> return 0;
> }
> @@ -430,13 +466,24 @@ nfsd4_recdir_purge_old(struct nfsd_net *nn)
> static int
> load_recdir(struct dentry *parent, struct dentry *child, struct nfsd_net *nn)
> {
> + struct xdr_netobj name;
> +
> if (child->d_name.len != HEXDIR_LEN - 1) {
> - printk("nfsd4: illegal name %pd in recovery directory\n",
> - child);
> + printk("%s: illegal name %pd in recovery directory\n",
> + __func__, child);
> /* Keep trying; maybe the others are OK: */
> return 0;
> }
> - nfs4_client_to_reclaim(child->d_name.name, nn);
> + name.data = kmemdup_nul(child->d_name.name, child->d_name.len, GFP_KERNEL);
> + if (!name.data) {
> + dprintk("%s: failed to allocate memory for name.data!\n",
> + __func__);
> + goto out;
> + }
> + name.len = HEXDIR_LEN;
> + if (!nfs4_client_to_reclaim(name, nn))
> + kfree(name.data);
> +out:
> return 0;
> }
>
> @@ -616,6 +663,7 @@ nfsd4_check_legacy_client(struct nfs4_client *clp)
> char dname[HEXDIR_LEN];
> struct nfs4_client_reclaim *crp;
> struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
> + struct xdr_netobj name;
>
> /* did we already find that this client is stable? */
> if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
> @@ -628,13 +676,22 @@ nfsd4_check_legacy_client(struct nfs4_client *clp)
> }
>
> /* look for it in the reclaim hashtable otherwise */
> - crp = nfsd4_find_reclaim_client(dname, nn);
> + name.data = kmemdup(dname, HEXDIR_LEN, GFP_KERNEL);
> + if (!name.data) {
> + dprintk("%s: failed to allocate memory for name.data!\n",
> + __func__);
> + goto out_enoent;
> + }
> + name.len = HEXDIR_LEN;
> + crp = nfsd4_find_reclaim_client(name, nn);
> + kfree(name.data);
> if (crp) {
> set_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
> crp->cr_clp = clp;
> return 0;
> }
>
> +out_enoent:
> return -ENOENT;
> }
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index f093fbe47133..777fbb0d2761 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1058,9 +1058,9 @@ static unsigned int clientid_hashval(u32 id)
> return id & CLIENT_HASH_MASK;
> }
>
> -static unsigned int clientstr_hashval(const char *name)
> +static unsigned int clientstr_hashval(struct xdr_netobj name)
> {
> - return opaque_hashval(name, 8) & CLIENT_HASH_MASK;
> + return opaque_hashval(name.data, 8) & CLIENT_HASH_MASK;
> }
>
> /*
> @@ -2039,11 +2039,6 @@ compare_blob(const struct xdr_netobj *o1, const struct xdr_netobj *o2)
> return memcmp(o1->data, o2->data, o1->len);
> }
>
> -static int same_name(const char *n1, const char *n2)
> -{
> - return 0 == memcmp(n1, n2, HEXDIR_LEN);
> -}
> -
> static int
> same_verf(nfs4_verifier *v1, nfs4_verifier *v2)
> {
> @@ -6449,7 +6444,7 @@ alloc_reclaim(void)
> }
>
> bool
> -nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn)
> +nfs4_has_reclaimed_state(struct xdr_netobj name, struct nfsd_net *nn)
> {
> struct nfs4_client_reclaim *crp;
>
> @@ -6459,20 +6454,24 @@ nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn)
>
> /*
> * failure => all reset bets are off, nfserr_no_grace...
> + *
> + * The caller is responsible for freeing name.data if NULL is returned (it
> + * will be freed in nfs4_remove_reclaim_record in the normal case).
> */
> struct nfs4_client_reclaim *
> -nfs4_client_to_reclaim(const char *name, struct nfsd_net *nn)
> +nfs4_client_to_reclaim(struct xdr_netobj name, struct nfsd_net *nn)
> {
> unsigned int strhashval;
> struct nfs4_client_reclaim *crp;
>
> - dprintk("NFSD nfs4_client_to_reclaim NAME: %.*s\n", HEXDIR_LEN, name);
> + dprintk("NFSD nfs4_client_to_reclaim NAME: %.*s\n", name.len, name.data);
> crp = alloc_reclaim();
> if (crp) {
> strhashval = clientstr_hashval(name);
> INIT_LIST_HEAD(&crp->cr_strhash);
> list_add(&crp->cr_strhash, &nn->reclaim_str_hashtbl[strhashval]);
> - memcpy(crp->cr_recdir, name, HEXDIR_LEN);
> + crp->cr_name.data = name.data;
> + crp->cr_name.len = name.len;
> crp->cr_clp = NULL;
> nn->reclaim_str_hashtbl_size++;
> }
> @@ -6483,6 +6482,7 @@ void
> nfs4_remove_reclaim_record(struct nfs4_client_reclaim *crp, struct nfsd_net *nn)
> {
> list_del(&crp->cr_strhash);
> + kfree(crp->cr_name.data);
> kfree(crp);
> nn->reclaim_str_hashtbl_size--;
> }
> @@ -6506,16 +6506,16 @@ nfs4_release_reclaim(struct nfsd_net *nn)
> /*
> * called from OPEN, CLAIM_PREVIOUS with a new clientid. */
> struct nfs4_client_reclaim *
> -nfsd4_find_reclaim_client(const char *recdir, struct nfsd_net *nn)
> +nfsd4_find_reclaim_client(struct xdr_netobj name, struct nfsd_net *nn)
> {
> unsigned int strhashval;
> struct nfs4_client_reclaim *crp = NULL;
>
> - dprintk("NFSD: nfs4_find_reclaim_client for recdir %s\n", recdir);
> + dprintk("NFSD: nfs4_find_reclaim_client for name %.*s\n", name.len, name.data);
>
> - strhashval = clientstr_hashval(recdir);
> + strhashval = clientstr_hashval(name);
> list_for_each_entry(crp, &nn->reclaim_str_hashtbl[strhashval], cr_strhash) {
> - if (same_name(crp->cr_recdir, recdir)) {
> + if (compare_blob(&crp->cr_name, &name) == 0) {
> return crp;
> }
> }
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 6aacb325b6a0..95dd298c4620 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -367,7 +367,7 @@ struct nfs4_client {
> struct nfs4_client_reclaim {
> struct list_head cr_strhash; /* hash by cr_name */
> struct nfs4_client *cr_clp; /* pointer to associated clp */
> - char cr_recdir[HEXDIR_LEN]; /* recover dir */
> + struct xdr_netobj cr_name; /* recovery dir name */
> };
>
> /* A reasonable value for REPLAY_ISIZE was estimated as follows:
> @@ -619,7 +619,7 @@ void nfs4_put_stid(struct nfs4_stid *s);
> void nfs4_inc_and_copy_stateid(stateid_t *dst, struct nfs4_stid *stid);
> void nfs4_remove_reclaim_record(struct nfs4_client_reclaim *, struct nfsd_net *);
> extern void nfs4_release_reclaim(struct nfsd_net *);
> -extern struct nfs4_client_reclaim *nfsd4_find_reclaim_client(const char *recdir,
> +extern struct nfs4_client_reclaim *nfsd4_find_reclaim_client(struct xdr_netobj name,
> struct nfsd_net *nn);
> extern __be32 nfs4_check_open_reclaim(clientid_t *clid,
> struct nfsd4_compound_state *cstate, struct nfsd_net *nn);
> @@ -634,9 +634,9 @@ extern void nfsd4_destroy_callback_queue(void);
> extern void nfsd4_shutdown_callback(struct nfs4_client *);
> extern void nfsd4_shutdown_copy(struct nfs4_client *clp);
> extern void nfsd4_prepare_cb_recall(struct nfs4_delegation *dp);
> -extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
> +extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(struct xdr_netobj name,
> struct nfsd_net *nn);
> -extern bool nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn);
> +extern bool nfs4_has_reclaimed_state(struct xdr_netobj name, struct nfsd_net *nn);
>
> struct nfs4_file *find_file(struct knfsd_fh *fh);
> void put_nfs4_file(struct nfs4_file *fi);
> --
> 2.17.1
