From: Trond Myklebust <trond.myklebust@fys.uio.no>
To: Benny Halevy <bhalevy@panasas.com>
Cc: linux-nfs@vger.kernel.org, nfsv4@linux-nfs.org
Subject: Re: [PATCH v3] nfs: use compound hdr.status to override op status.
Date: Sun, 11 May 2008 16:26:05 -0700 [thread overview]
Message-ID: <1210548365.7796.2.camel@localhost> (raw)
In-Reply-To: <48277F7B.2060307@panasas.com>
On Sun, 2008-05-11 at 16:21 -0700, Benny Halevy wrote:
> The compound header status must be equivalent to the
> status of the last operation in the compound results.
> In certain cases like lack of resources or xdr decoding error,
> the nfs server may return a non-zero status in the compound header
> which is not returned by any operation. In this case we would
> notice that today when looking for the respective operations
> code in the results and we return -EIO when we cannot find it.
> This patch fixes that by returning the status available in the
> compound header instead.
>
> This patch also fixes 3 call sites where we looked at the compound
> hdr.status in the success case which is useless (yet benign).
> These are nfs4_xdr_dec_{fsinfo,setclientid,setclientid_confirm}
>
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
> fs/nfs/nfs4xdr.c | 88 +++++++++++++++++++++++++++---------------------------
> 1 files changed, 44 insertions(+), 44 deletions(-)
>
> Changes in PATCH v3:
> * nfs4_fixup_status made static inline (with very minor increase
> in code size over macro)
>
> Changes in PATCH v2:
> * rebased onto 2.6.26 (off of linux-2.6 28a4acb4)
> * fixed checkpatch.pl nits
> * do not fixup status in nfs4_xdr_enc_setacl
>
>
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index 5a2d649..e6f7f0b 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -3791,6 +3791,13 @@ static int decode_delegreturn(struct xdr_stream *xdr)
> return decode_op_hdr(xdr, OP_DELEGRETURN);
> }
>
> +static inline int nfs4_fixup_status(int status, int hdr_status)
> +{
> + if (likely(!status))
> + return 0;
> + return nfs4_stat_to_errno(hdr_status);
> +}
> +
> /*
> * Decode OPEN_DOWNGRADE response
> */
> @@ -3812,7 +3819,7 @@ static int nfs4_xdr_dec_open_downgrade(struct rpc_rqst *rqstp, __be32 *p, struct
> goto out;
> decode_getfattr(&xdr, res->fattr, res->server);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
NACK.
This is still screwed up: if the getattr above fails, then your change
will propagate that error despite the fact that we don't care. Please
see the same comments on earlier drafts of this patch.
>
> /*
> @@ -3839,7 +3846,7 @@ static int nfs4_xdr_dec_access(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_ac
> goto out;
> decode_getfattr(&xdr, res->fattr, res->server);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -3862,7 +3869,7 @@ static int nfs4_xdr_dec_lookup(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_lo
> goto out;
> status = decode_getfattr(&xdr, res->fattr, res->server);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -3882,7 +3889,7 @@ static int nfs4_xdr_dec_lookup_root(struct rpc_rqst *rqstp, __be32 *p, struct nf
> if ((status = decode_getfh(&xdr, res->fh)) == 0)
> status = decode_getfattr(&xdr, res->fattr, res->server);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -3903,7 +3910,7 @@ static int nfs4_xdr_dec_remove(struct rpc_rqst *rqstp, __be32 *p, struct nfs_rem
> goto out;
> decode_getfattr(&xdr, &res->dir_attr, res->server);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -3933,7 +3940,7 @@ static int nfs4_xdr_dec_rename(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_re
> goto out;
> decode_getfattr(&xdr, res->old_fattr, res->server);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -3966,7 +3973,7 @@ static int nfs4_xdr_dec_link(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_link
> goto out;
> decode_getfattr(&xdr, res->fattr, res->server);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -3995,7 +4002,7 @@ static int nfs4_xdr_dec_create(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_cr
> goto out;
> decode_getfattr(&xdr, res->dir_fattr, res->server);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4024,8 +4031,7 @@ static int nfs4_xdr_dec_getattr(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_g
> goto out;
> status = decode_getfattr(&xdr, res->fattr, res->server);
> out:
> - return status;
> -
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4047,7 +4053,7 @@ nfs4_xdr_enc_setacl(struct rpc_rqst *req, __be32 *p, struct nfs_setaclargs *args
> goto out;
> status = encode_setacl(&xdr, args);
> out:
> - return status;
> + return status;
> }
> /*
> * Decode SETACL response
> @@ -4068,7 +4074,7 @@ nfs4_xdr_dec_setacl(struct rpc_rqst *rqstp, __be32 *p, void *res)
> goto out;
> status = decode_setattr(&xdr, res);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4091,7 +4097,7 @@ nfs4_xdr_dec_getacl(struct rpc_rqst *rqstp, __be32 *p, size_t *acl_len)
> status = decode_getacl(&xdr, rqstp, acl_len);
>
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4121,7 +4127,7 @@ static int nfs4_xdr_dec_close(struct rpc_rqst *rqstp, __be32 *p, struct nfs_clos
> */
> decode_getfattr(&xdr, res->fattr, res->server);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4154,7 +4160,7 @@ static int nfs4_xdr_dec_open(struct rpc_rqst *rqstp, __be32 *p, struct nfs_openr
> goto out;
> decode_getfattr(&xdr, res->dir_attr, res->server);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4175,7 +4181,7 @@ static int nfs4_xdr_dec_open_confirm(struct rpc_rqst *rqstp, __be32 *p, struct n
> goto out;
> status = decode_open_confirm(&xdr, res);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4199,7 +4205,7 @@ static int nfs4_xdr_dec_open_noattr(struct rpc_rqst *rqstp, __be32 *p, struct nf
> goto out;
> decode_getfattr(&xdr, res->f_attr, res->server);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4223,9 +4229,9 @@ static int nfs4_xdr_dec_setattr(struct rpc_rqst *rqstp, __be32 *p, struct nfs_se
> goto out;
> status = decode_getfattr(&xdr, res->fattr, res->server);
> if (status == NFS4ERR_DELAY)
> - status = 0;
> + return 0;
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4246,7 +4252,7 @@ static int nfs4_xdr_dec_lock(struct rpc_rqst *rqstp, __be32 *p, struct nfs_lock_
> goto out;
> status = decode_lock(&xdr, res);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4267,7 +4273,7 @@ static int nfs4_xdr_dec_lockt(struct rpc_rqst *rqstp, __be32 *p, struct nfs_lock
> goto out;
> status = decode_lockt(&xdr, res);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4288,7 +4294,7 @@ static int nfs4_xdr_dec_locku(struct rpc_rqst *rqstp, __be32 *p, struct nfs_lock
> goto out;
> status = decode_locku(&xdr, res);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4309,7 +4315,7 @@ static int nfs4_xdr_dec_readlink(struct rpc_rqst *rqstp, __be32 *p, void *res)
> goto out;
> status = decode_readlink(&xdr, rqstp);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4330,7 +4336,7 @@ static int nfs4_xdr_dec_readdir(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_r
> goto out;
> status = decode_readdir(&xdr, rqstp, res);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4351,9 +4357,9 @@ static int nfs4_xdr_dec_read(struct rpc_rqst *rqstp, __be32 *p, struct nfs_readr
> goto out;
> status = decode_read(&xdr, rqstp, res);
> if (!status)
> - status = res->count;
> + return res->count;
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4377,9 +4383,9 @@ static int nfs4_xdr_dec_write(struct rpc_rqst *rqstp, __be32 *p, struct nfs_writ
> goto out;
> decode_getfattr(&xdr, res->fattr, res->server);
> if (!status)
> - status = res->count;
> + return res->count;
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4403,7 +4409,7 @@ static int nfs4_xdr_dec_commit(struct rpc_rqst *rqstp, __be32 *p, struct nfs_wri
> goto out;
> decode_getfattr(&xdr, res->fattr, res->server);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4421,9 +4427,7 @@ static int nfs4_xdr_dec_fsinfo(struct rpc_rqst *req, __be32 *p, struct nfs_fsinf
> status = decode_putfh(&xdr);
> if (!status)
> status = decode_fsinfo(&xdr, fsinfo);
> - if (!status)
> - status = nfs4_stat_to_errno(hdr.status);
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4441,7 +4445,7 @@ static int nfs4_xdr_dec_pathconf(struct rpc_rqst *req, __be32 *p, struct nfs_pat
> status = decode_putfh(&xdr);
> if (!status)
> status = decode_pathconf(&xdr, pathconf);
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4459,7 +4463,7 @@ static int nfs4_xdr_dec_statfs(struct rpc_rqst *req, __be32 *p, struct nfs_fssta
> status = decode_putfh(&xdr);
> if (!status)
> status = decode_statfs(&xdr, fsstat);
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4478,7 +4482,7 @@ static int nfs4_xdr_dec_server_caps(struct rpc_rqst *req, __be32 *p, struct nfs4
> goto out;
> status = decode_server_caps(&xdr, res);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4494,7 +4498,7 @@ static int nfs4_xdr_dec_renew(struct rpc_rqst *rqstp, __be32 *p, void *dummy)
> status = decode_compound_hdr(&xdr, &hdr);
> if (!status)
> status = decode_renew(&xdr);
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4511,9 +4515,7 @@ static int nfs4_xdr_dec_setclientid(struct rpc_rqst *req, __be32 *p,
> status = decode_compound_hdr(&xdr, &hdr);
> if (!status)
> status = decode_setclientid(&xdr, clp);
> - if (!status)
> - status = nfs4_stat_to_errno(hdr.status);
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4533,9 +4535,7 @@ static int nfs4_xdr_dec_setclientid_confirm(struct rpc_rqst *req, __be32 *p, str
> status = decode_putrootfh(&xdr);
> if (!status)
> status = decode_fsinfo(&xdr, fsinfo);
> - if (!status)
> - status = nfs4_stat_to_errno(hdr.status);
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4557,7 +4557,7 @@ static int nfs4_xdr_dec_delegreturn(struct rpc_rqst *rqstp, __be32 *p, struct nf
> status = decode_delegreturn(&xdr);
> decode_getfattr(&xdr, res->fattr, res->server);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> /*
> @@ -4580,7 +4580,7 @@ static int nfs4_xdr_dec_fs_locations(struct rpc_rqst *req, __be32 *p, struct nfs
> xdr_enter_page(&xdr, PAGE_SIZE);
> status = decode_getfattr(&xdr, &res->fattr, res->server);
> out:
> - return status;
> + return nfs4_fixup_status(status, hdr.status);
> }
>
> __be32 *nfs4_decode_dirent(__be32 *p, struct nfs_entry *entry, int plus)
next prev parent reply other threads:[~2008-05-11 23:26 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-03-31 14:35 [PATCH 0/2] nfsv4 compound status Benny Halevy
2008-03-31 14:39 ` [PATCH 1/2] nfs: return negative error value from nfs{,4}_stat_to_errno Benny Halevy
2008-03-31 14:48 ` [PATCH 2/2] nfs: use compound hdr.status to override op status Benny Halevy
2008-03-31 22:25 ` Trond Myklebust
[not found] ` <1207002349.15341.15.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org>
2008-04-01 9:37 ` Benny Halevy
2008-04-01 9:41 ` [PATCH 2/2 v1] " Benny Halevy
2008-05-09 22:21 ` [PATCH 1/1 v2] " Benny Halevy
2008-05-09 23:32 ` Benny Halevy
2008-05-10 6:11 ` Benny Halevy
2008-05-09 23:38 ` [PATCH " Benny Halevy
2008-05-10 17:24 ` Trond Myklebust
2008-05-11 23:21 ` [PATCH v3] " Benny Halevy
2008-05-11 23:26 ` Trond Myklebust [this message]
2008-05-12 1:54 ` Benny Halevy
2008-07-03 17:49 ` [PATCH 0/2 v4] nfs: return nfs4 compound header status on op header decoding error Benny Halevy
2008-07-03 17:52 ` [PATCH 1/2] " Benny Halevy
2008-07-03 18:42 ` [PATCH 2/2] nfs: remove incorrect usage of nfs4 compound response hdr.status Benny Halevy
2008-07-15 21:57 ` [PATCH 0/2 v4] nfs: return nfs4 compound header status on op header decoding error Trond Myklebust
2008-07-16 8:21 ` Benny Halevy
2008-07-16 12:57 ` Trond Myklebust
2008-07-16 13:22 ` Benny Halevy
2008-07-17 12:09 ` Trond Myklebust
2008-07-17 13:20 ` Benny Halevy
2008-07-21 16:59 ` [PATCH] nfs: return compound hdr.status when there are no op replies Benny Halevy
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=1210548365.7796.2.camel@localhost \
--to=trond.myklebust@fys.uio.no \
--cc=bhalevy@panasas.com \
--cc=linux-nfs@vger.kernel.org \
--cc=nfsv4@linux-nfs.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.