From: Benny Halevy <bhalevy@panasas.com>
To: Fred Isaman <iisaman@netapp.com>, Andy Adamson <andros@netapp.com>
Cc: linux-nfs@vger.kernel.org
Subject: Re: [PATCH 01/18] NFSv4.1: Callback share session between ops
Date: Wed, 10 Nov 2010 15:37:49 +0200 [thread overview]
Message-ID: <4CDAA02D.20204@panasas.com> (raw)
In-Reply-To: <1288884151-11128-2-git-send-email-iisaman@netapp.com>
On 2010-11-04 17:22, Fred Isaman wrote:
> From: Andy Adamson <andros@netapp.com>
>
> The NFSv4.1 session found in cb_sequence needs to be shared by other
> callback operations in the same cb_compound.
> Hold a reference to the session's nfs_client throughout the cb_compound
> processing.
>
> Move NFS4ERR_RETRY_UNCACHED_REP processing into nfs4_callback_sequence.
>
> Signed-off-by: Andy Adamson <andros@netapp.com>
> Signed-off-by: Fred Isaman <iisaman@netapp.com>
> ---
> fs/nfs/callback.h | 24 ++++++--
> fs/nfs/callback_proc.c | 138 ++++++++++++++++++++++++++++--------------------
> fs/nfs/callback_xdr.c | 29 +++++-----
> 3 files changed, 113 insertions(+), 78 deletions(-)
>
> diff --git a/fs/nfs/callback.h b/fs/nfs/callback.h
> index 2ce61b8..89fee05 100644
> --- a/fs/nfs/callback.h
> +++ b/fs/nfs/callback.h
> @@ -34,6 +34,11 @@ enum nfs4_callback_opnum {
> OP_CB_ILLEGAL = 10044,
> };
>
> +struct cb_process_state {
> + __be32 drc_status;
> + struct nfs4_session *session;
> +};
> +
> struct cb_compound_hdr_arg {
> unsigned int taglen;
> const char *tag;
> @@ -104,7 +109,8 @@ struct cb_sequenceres {
> };
>
> extern unsigned nfs4_callback_sequence(struct cb_sequenceargs *args,
> - struct cb_sequenceres *res);
> + struct cb_sequenceres *res,
> + struct cb_process_state *cps);
>
> extern int nfs41_validate_delegation_stateid(struct nfs_delegation *delegation,
> const nfs4_stateid *stateid);
> @@ -125,14 +131,17 @@ struct cb_recallanyargs {
> uint32_t craa_type_mask;
> };
>
> -extern unsigned nfs4_callback_recallany(struct cb_recallanyargs *args, void *dummy);
> +extern unsigned nfs4_callback_recallany(struct cb_recallanyargs *args,
> + void *dummy,
> + struct cb_process_state *cps);
>
> struct cb_recallslotargs {
> struct sockaddr *crsa_addr;
> uint32_t crsa_target_max_slots;
> };
> extern unsigned nfs4_callback_recallslot(struct cb_recallslotargs *args,
> - void *dummy);
> + void *dummy,
> + struct cb_process_state *cps);
>
> struct cb_layoutrecallargs {
> struct sockaddr *cbl_addr;
> @@ -147,12 +156,15 @@ struct cb_layoutrecallargs {
>
> extern unsigned nfs4_callback_layoutrecall(
> struct cb_layoutrecallargs *args,
> - void *dummy);
> + void *dummy, struct cb_process_state *cps);
>
> #endif /* CONFIG_NFS_V4_1 */
>
> -extern __be32 nfs4_callback_getattr(struct cb_getattrargs *args, struct cb_getattrres *res);
> -extern __be32 nfs4_callback_recall(struct cb_recallargs *args, void *dummy);
> +extern __be32 nfs4_callback_getattr(struct cb_getattrargs *args,
> + struct cb_getattrres *res,
> + struct cb_process_state *cps);
> +extern __be32 nfs4_callback_recall(struct cb_recallargs *args, void *dummy,
> + struct cb_process_state *cps);
>
> #ifdef CONFIG_NFS_V4
> extern int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt);
> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
> index 6b560ce..84c5a1b 100644
> --- a/fs/nfs/callback_proc.c
> +++ b/fs/nfs/callback_proc.c
> @@ -20,8 +20,10 @@
> #ifdef NFS_DEBUG
> #define NFSDBG_FACILITY NFSDBG_CALLBACK
> #endif
> -
> -__be32 nfs4_callback_getattr(struct cb_getattrargs *args, struct cb_getattrres *res)
> +
> +__be32 nfs4_callback_getattr(struct cb_getattrargs *args,
> + struct cb_getattrres *res,
> + struct cb_process_state *cps)
> {
> struct nfs_client *clp;
> struct nfs_delegation *delegation;
> @@ -30,9 +32,13 @@ __be32 nfs4_callback_getattr(struct cb_getattrargs *args, struct cb_getattrres *
>
> res->bitmap[0] = res->bitmap[1] = 0;
> res->status = htonl(NFS4ERR_BADHANDLE);
> - clp = nfs_find_client(args->addr, 4);
> - if (clp == NULL)
> - goto out;
> + if (cps->session) { /* set in cb_sequence */
> + clp = cps->session->clp;
> + } else {
> + clp = nfs_find_client(args->addr, 4);
> + if (clp == NULL)
> + goto out;
> + }
How about extracting this code out into a helper function?
It's repeated also in nfs4_callback_recall().
>
> dprintk("NFS: GETATTR callback request from %s\n",
> rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_ADDR));
> @@ -60,22 +66,28 @@ out_iput:
> rcu_read_unlock();
> iput(inode);
> out_putclient:
> - nfs_put_client(clp);
> + if (!cps->session)
> + nfs_put_client(clp);
> out:
> dprintk("%s: exit with status = %d\n", __func__, ntohl(res->status));
> return res->status;
> }
>
> -__be32 nfs4_callback_recall(struct cb_recallargs *args, void *dummy)
> +__be32 nfs4_callback_recall(struct cb_recallargs *args, void *dummy,
> + struct cb_process_state *cps)
> {
> struct nfs_client *clp;
> struct inode *inode;
> __be32 res;
>
> res = htonl(NFS4ERR_BADHANDLE);
> - clp = nfs_find_client(args->addr, 4);
> - if (clp == NULL)
> - goto out;
> + if (cps->session) { /* set in cb_sequence */
> + clp = cps->session->clp;
> + } else {
> + clp = nfs_find_client(args->addr, 4);
> + if (clp == NULL)
> + goto out;
> + }
>
> dprintk("NFS: RECALL callback request from %s\n",
> rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_ADDR));
> @@ -99,9 +111,11 @@ __be32 nfs4_callback_recall(struct cb_recallargs *args, void *dummy)
> }
> iput(inode);
> }
> - clp = nfs_find_client_next(prev);
> - nfs_put_client(prev);
> - } while (clp != NULL);
> + if (!cps->session) {
> + clp = nfs_find_client_next(prev);
> + nfs_put_client(prev);
> + }
> + } while (!cps->session && clp != NULL);
I.e.,
if (cps->session)
break;
(I think this is simpler)
> out:
> dprintk("%s: exit with status = %d\n", __func__, ntohl(res));
> return res;
> @@ -346,46 +360,40 @@ static int pnfs_recall_all_layouts(struct nfs_client *clp)
> }
>
> __be32 nfs4_callback_layoutrecall(struct cb_layoutrecallargs *args,
> - void *dummy)
> + void *dummy, struct cb_process_state *cps)
> {
> struct nfs_client *clp;
> struct inode *inode = NULL;
> __be32 res;
> int status;
> - unsigned int num_client = 0;
>
> dprintk("%s: -->\n", __func__);
>
> res = cpu_to_be32(NFS4ERR_OP_NOT_IN_SESSION);
> - clp = nfs_find_client(args->cbl_addr, 4);
> - if (clp == NULL)
> + if (cps->session) /* set in cb_sequence */
> + clp = cps->session->clp;
> + else
> goto out;
>
> - res = cpu_to_be32(NFS4ERR_NOMATCHING_LAYOUT);
> - do {
> - struct nfs_client *prev = clp;
> - num_client++;
> - /* the callback must come from the MDS personality */
> - if (!(clp->cl_exchange_flags & EXCHGID4_FLAG_USE_PNFS_MDS))
> - goto loop;
> - /* In the _ALL or _FSID case, we need the inode to get
> - * the nfs_server struct.
> - */
> - inode = nfs_layoutrecall_find_inode(clp, args);
> - if (!inode)
> - goto loop;
> - status = pnfs_async_return_layout(clp, inode, args);
> - if (status)
> - res = cpu_to_be32(NFS4ERR_DELAY);
> - iput(inode);
> -loop:
> - clp = nfs_find_client_next(prev);
> - nfs_put_client(prev);
> - } while (clp != NULL);
> + /* the callback must come from the MDS personality */
> + res = cpu_to_be32(NFS4ERR_NOTSUPP);
> + if (!(clp->cl_exchange_flags & EXCHGID4_FLAG_USE_PNFS_MDS))
> + goto out;
>
> + res = cpu_to_be32(NFS4ERR_NOMATCHING_LAYOUT);
> + /*
> + * In the _ALL or _FSID case, we need the inode to get
> + * the nfs_server struct.
> + */
> + inode = nfs_layoutrecall_find_inode(clp, args);
> + if (!inode)
> + goto out;
> + status = pnfs_async_return_layout(clp, inode, args);
> + if (status)
> + res = cpu_to_be32(NFS4ERR_DELAY);
> + iput(inode);
> out:
> - dprintk("%s: exit with status = %d numclient %u\n",
> - __func__, ntohl(res), num_client);
> + dprintk("%s: exit with status = %d\n", __func__, ntohl(res));
> return res;
> }
>
> @@ -552,12 +560,15 @@ out:
> }
>
> __be32 nfs4_callback_sequence(struct cb_sequenceargs *args,
> - struct cb_sequenceres *res)
> + struct cb_sequenceres *res,
> + struct cb_process_state *cps)
> {
> struct nfs_client *clp;
> int i;
> __be32 status;
>
> + cps->session = NULL;
> +
> status = htonl(NFS4ERR_BADSESSION);
> clp = find_client_with_session(args->csa_addr, 4, &args->csa_sessionid);
> if (clp == NULL)
> @@ -583,21 +594,27 @@ __be32 nfs4_callback_sequence(struct cb_sequenceargs *args,
> res->csr_slotid = args->csa_slotid;
> res->csr_highestslotid = NFS41_BC_MAX_CALLBACKS - 1;
> res->csr_target_highestslotid = NFS41_BC_MAX_CALLBACKS - 1;
> + cps->session = clp->cl_session; /* caller must put nfs_client */
>
> -out_putclient:
> - nfs_put_client(clp);
> out:
> for (i = 0; i < args->csa_nrclists; i++)
> kfree(args->csa_rclists[i].rcl_refcalls);
> kfree(args->csa_rclists);
>
> - if (status == htonl(NFS4ERR_RETRY_UNCACHED_REP))
> + if (status == htonl(NFS4ERR_RETRY_UNCACHED_REP)) {
> res->csr_status = 0;
> - else
> + cps->drc_status = status;
> + status = 0;
> + } else
> res->csr_status = status;
> +
> dprintk("%s: exit with status = %d res->csr_status %d\n", __func__,
> ntohl(status), ntohl(res->csr_status));
> return status;
> +
> +out_putclient:
> + nfs_put_client(clp);
> + goto out;
> }
>
> static inline bool
> @@ -624,24 +641,31 @@ validate_bitmap_values(const unsigned long *mask)
> return false;
> }
>
> -__be32 nfs4_callback_recallany(struct cb_recallanyargs *args, void *dummy)
> +__be32 nfs4_callback_recallany(struct cb_recallanyargs *args, void *dummy,
> + struct cb_process_state *cps)
> {
> struct nfs_client *clp;
> __be32 status;
> fmode_t flags = 0;
>
> status = cpu_to_be32(NFS4ERR_OP_NOT_IN_SESSION);
> - clp = nfs_find_client(args->craa_addr, 4);
> - if (clp == NULL)
> + if (cps->session) /* set in cb_sequence */
> + clp = cps->session->clp;
> + else
> goto out;
>
> dprintk("NFS: RECALL_ANY callback request from %s\n",
> rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_ADDR));
>
> + /* the callback must come from the MDS personality */
> + status = cpu_to_be32(NFS4ERR_NOTSUPP);
> + if (!(clp->cl_exchange_flags & EXCHGID4_FLAG_USE_PNFS_MDS))
> + goto out;
> +
wouldn't it be simpler to do that once in cb_sequence?
I'll send a patch as reply to this message with my proposals...
Benny
> status = cpu_to_be32(NFS4ERR_INVAL);
> if (!validate_bitmap_values((const unsigned long *)
> &args->craa_type_mask))
> - goto out_put;
> + goto out;
>
> status = cpu_to_be32(NFS4_OK);
> if (test_bit(RCA4_TYPE_MASK_RDATA_DLG, (const unsigned long *)
> @@ -657,23 +681,23 @@ __be32 nfs4_callback_recallany(struct cb_recallanyargs *args, void *dummy)
>
> if (flags)
> nfs_expire_all_delegation_types(clp, flags);
> -out_put:
> - nfs_put_client(clp);
> out:
> dprintk("%s: exit with status = %d\n", __func__, ntohl(status));
> return status;
> }
>
> /* Reduce the fore channel's max_slots to the target value */
> -__be32 nfs4_callback_recallslot(struct cb_recallslotargs *args, void *dummy)
> +__be32 nfs4_callback_recallslot(struct cb_recallslotargs *args, void *dummy,
> + struct cb_process_state *cps)
> {
> struct nfs_client *clp;
> struct nfs4_slot_table *fc_tbl;
> __be32 status;
>
> status = htonl(NFS4ERR_OP_NOT_IN_SESSION);
> - clp = nfs_find_client(args->crsa_addr, 4);
> - if (clp == NULL)
> + if (cps->session) /* set in cb_sequence */
> + clp = cps->session->clp;
> + else
> goto out;
>
> dprintk("NFS: CB_RECALL_SLOT request from %s target max slots %d\n",
> @@ -685,16 +709,14 @@ __be32 nfs4_callback_recallslot(struct cb_recallslotargs *args, void *dummy)
> status = htonl(NFS4ERR_BAD_HIGH_SLOT);
> if (args->crsa_target_max_slots > fc_tbl->max_slots ||
> args->crsa_target_max_slots < 1)
> - goto out_putclient;
> + goto out;
>
> status = htonl(NFS4_OK);
> if (args->crsa_target_max_slots == fc_tbl->max_slots)
> - goto out_putclient;
> + goto out;
>
> fc_tbl->target_max_slots = args->crsa_target_max_slots;
> nfs41_handle_recall_slot(clp);
> -out_putclient:
> - nfs_put_client(clp); /* balance nfs_find_client */
> out:
> dprintk("%s: exit with status = %d\n", __func__, ntohl(status));
> return status;
> diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
> index 63b17d0..1650ab0 100644
> --- a/fs/nfs/callback_xdr.c
> +++ b/fs/nfs/callback_xdr.c
> @@ -12,6 +12,7 @@
> #include <linux/slab.h>
> #include "nfs4_fs.h"
> #include "callback.h"
> +#include "internal.h"
>
> #define CB_OP_TAGLEN_MAXSZ (512)
> #define CB_OP_HDR_RES_MAXSZ (2 + CB_OP_TAGLEN_MAXSZ)
> @@ -34,7 +35,8 @@
> /* Internal error code */
> #define NFS4ERR_RESOURCE_HDR 11050
>
> -typedef __be32 (*callback_process_op_t)(void *, void *);
> +typedef __be32 (*callback_process_op_t)(void *, void *,
> + struct cb_process_state *);
> typedef __be32 (*callback_decode_arg_t)(struct svc_rqst *, struct xdr_stream *, void *);
> typedef __be32 (*callback_encode_res_t)(struct svc_rqst *, struct xdr_stream *, void *);
>
> @@ -676,7 +678,8 @@ preprocess_nfs4_op(unsigned int op_nr, struct callback_op **op)
> static __be32 process_op(uint32_t minorversion, int nop,
> struct svc_rqst *rqstp,
> struct xdr_stream *xdr_in, void *argp,
> - struct xdr_stream *xdr_out, void *resp, int* drc_status)
> + struct xdr_stream *xdr_out, void *resp,
> + struct cb_process_state *cps)
> {
> struct callback_op *op = &callback_ops[0];
> unsigned int op_nr;
> @@ -699,8 +702,8 @@ static __be32 process_op(uint32_t minorversion, int nop,
> if (status)
> goto encode_hdr;
>
> - if (*drc_status) {
> - status = *drc_status;
> + if (cps->drc_status) {
> + status = cps->drc_status;
> goto encode_hdr;
> }
>
> @@ -708,16 +711,10 @@ static __be32 process_op(uint32_t minorversion, int nop,
> if (maxlen > 0 && maxlen < PAGE_SIZE) {
> status = op->decode_args(rqstp, xdr_in, argp);
> if (likely(status == 0))
> - status = op->process_op(argp, resp);
> + status = op->process_op(argp, resp, cps);
> } else
> status = htonl(NFS4ERR_RESOURCE);
>
> - /* Only set by OP_CB_SEQUENCE processing */
> - if (status == htonl(NFS4ERR_RETRY_UNCACHED_REP)) {
> - *drc_status = status;
> - status = 0;
> - }
> -
> encode_hdr:
> res = encode_op_hdr(xdr_out, op_nr, status);
> if (unlikely(res))
> @@ -736,8 +733,10 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp, void *argp, void *r
> struct cb_compound_hdr_arg hdr_arg = { 0 };
> struct cb_compound_hdr_res hdr_res = { NULL };
> struct xdr_stream xdr_in, xdr_out;
> - __be32 *p;
> - __be32 status, drc_status = 0;
> + __be32 *p, status;
> + struct cb_process_state cps = {
> + .drc_status = 0,
> + };
> unsigned int nops = 0;
>
> dprintk("%s: start\n", __func__);
> @@ -758,7 +757,7 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp, void *argp, void *r
>
> while (status == 0 && nops != hdr_arg.nops) {
> status = process_op(hdr_arg.minorversion, nops, rqstp,
> - &xdr_in, argp, &xdr_out, resp, &drc_status);
> + &xdr_in, argp, &xdr_out, resp, &cps);
> nops++;
> }
>
> @@ -771,6 +770,8 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp, void *argp, void *r
>
> *hdr_res.status = status;
> *hdr_res.nops = htonl(nops);
> + if (cps.session) /* matched by cb_sequence find_client_with_session */
> + nfs_put_client(cps.session->clp);
> dprintk("%s: done, status = %u\n", __func__, ntohl(status));
> return rpc_success;
> }
next prev parent reply other threads:[~2010-11-10 13:37 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-11-04 15:22 [PATCH 00/18] rewrite of CB_LAYOUTRECALL and layoutstate code Fred Isaman
2010-11-04 15:22 ` [PATCH 01/18] NFSv4.1: Callback share session between ops Fred Isaman
2010-11-10 13:37 ` Benny Halevy [this message]
2010-11-10 13:41 ` [PATCH] SQUASHME: pnfs-submit: fixups for nfsv4.1 callbacks Benny Halevy
2010-11-10 14:53 ` Fred Isaman
2010-11-04 15:22 ` [PATCH 02/18] pnfs-submit: change pnfs_layout_segment refcounting from kref to atomic_t Fred Isaman
2010-11-04 15:22 ` [PATCH 03/18] pnfs-submit: remove _pnfs_can_return_lseg call from pnfs_clear_lseg_list Fred Isaman
2010-11-10 14:35 ` Benny Halevy
2010-11-10 14:46 ` Fred Isaman
2010-11-11 7:00 ` Benny Halevy
2010-11-11 13:52 ` Fred Isaman
2010-11-11 14:39 ` Benny Halevy
2010-11-04 15:22 ` [PATCH 04/18] pnfs-submit: change layout state seqlock to a spinlock Fred Isaman
2010-11-11 15:00 ` Benny Halevy
2010-11-11 15:09 ` Fred Isaman
2010-11-04 15:22 ` [PATCH 05/18] pnfs-submit: layoutreturn' rpc_call_op functions need to handle bulk returns Fred Isaman
2010-11-11 15:01 ` Benny Halevy
2010-11-04 15:22 ` [PATCH 06/18] pnfs_submit: nfs4_layoutreturn_release should not reference results Fred Isaman
2010-11-11 15:16 ` Benny Halevy
2010-11-04 15:22 ` [PATCH 07/18] pnfs-submit: reorganize struct cb_layoutrecallargs Fred Isaman
2010-11-04 15:22 ` [PATCH 08/18] pnfs-submit: rename lo->state to lo->plh_flags Fred Isaman
2010-11-04 15:22 ` [PATCH 09/18] pnfs-submit: change pnfs_layout_hdr refcount to atomic_t Fred Isaman
2010-11-04 15:22 ` [PATCH 10/18] pnfs-submit: argument to should_free_lseg changed from lseg to range Fred Isaman
2010-11-04 15:22 ` [PATCH 11/18] pnfs-submit: remove unnecessary field lgp->status Fred Isaman
2010-11-04 15:22 ` [PATCH 12/18] pnfs-submit: remove RPC_ASSASSINATED(task) checks Fred Isaman
2010-11-04 15:22 ` [PATCH 13/18] pnfs-submit: rewrite of layout state handling and cb_layoutrecall Fred Isaman
2010-11-04 15:22 ` [PATCH 14/18] pnfs-submit: increase number of outstanding CB_LAYOUTRECALLS we can handle Fred Isaman
2010-11-04 15:22 ` [PATCH 15/18] pnfs-submit: roc add layoutreturn op to close compound Fred Isaman
2010-11-04 15:22 ` [PATCH 16/18] pnfs-submit refactor layoutcommit xdr structures Fred Isaman
2010-11-04 15:22 ` [PATCH 17/18] pnfs-submit refactor pnfs_layoutcommit_setup Fred Isaman
2010-11-04 15:22 ` [PATCH 18/18] pnfs_submit: roc add layoutcommit op to close compound Fred Isaman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4CDAA02D.20204@panasas.com \
--to=bhalevy@panasas.com \
--cc=andros@netapp.com \
--cc=iisaman@netapp.com \
--cc=linux-nfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.