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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox