* [PATCH 0/2] nfsv4 compound status
@ 2008-03-31 14:35 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
0 siblings, 2 replies; 24+ messages in thread
From: Benny Halevy @ 2008-03-31 14:35 UTC (permalink / raw)
To: Trond Myklebust; +Cc: NFS list
Two patches in this patchset implement what we discussed last month:
http://linux-nfs.org/pipermail/nfsv4/2008-February/007945.html
[PATCH 1/2] nfs: return negative error value from nfs{,4}_stat_to_errno
All use sites for nfs{,4}_stat_to_errno negate their return value.
It's more efficient to return a negative error from the stat_to_errno convertors
rather than negating its return value everywhere. This also produces slightly
smaller code.
[PATCH 2/2] nfs: use compound hdr.status to override op status.
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
comound header instead.
This patch also fixes 3 call sites where we looked at the comound
hdr.status in the success case which is useless (yet benign).
These are nfs4_xdr_dec_{fsinfo,setclientid,setclientid_confirm}
Benny
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH 1/2] nfs: return negative error value from nfs{,4}_stat_to_errno 2008-03-31 14:35 [PATCH 0/2] nfsv4 compound status Benny Halevy @ 2008-03-31 14:39 ` Benny Halevy 2008-03-31 14:48 ` [PATCH 2/2] nfs: use compound hdr.status to override op status Benny Halevy 1 sibling, 0 replies; 24+ messages in thread From: Benny Halevy @ 2008-03-31 14:39 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, nfsv4, Benny Halevy All use sites for nfs{,4}_stat_to_errno negate their return value. It's more efficient to return a negative error from the stat_to_errno convertors rather than negating its return value everywhere. This also produces slightly smaller code. Signed-off-by: Benny Halevy <bhalevy@panasas.com> --- fs/nfs/nfs2xdr.c | 76 +++++++++++++++++++++++++------------------------- fs/nfs/nfs3xdr.c | 34 +++++++++++----------- fs/nfs/nfs4xdr.c | 80 +++++++++++++++++++++++++++--------------------------- 3 files changed, 95 insertions(+), 95 deletions(-) diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c index 1f7ea67..9930178 100644 --- a/fs/nfs/nfs2xdr.c +++ b/fs/nfs/nfs2xdr.c @@ -267,7 +267,7 @@ nfs_xdr_readres(struct rpc_rqst *req, __be32 *p, struct nfs_readres *res) int status; if ((status = ntohl(*p++))) - return -nfs_stat_to_errno(status); + return nfs_stat_to_errno(status); p = xdr_decode_fattr(p, res->fattr); count = ntohl(*p++); @@ -432,7 +432,7 @@ nfs_xdr_readdirres(struct rpc_rqst *req, __be32 *p, void *dummy) __be32 *end, *entry, *kaddr; if ((status = ntohl(*p++))) - return -nfs_stat_to_errno(status); + return nfs_stat_to_errno(status); hdrlen = (u8 *) p - (u8 *) iov->iov_base; if (iov->iov_len < hdrlen) { @@ -518,7 +518,7 @@ nfs_xdr_stat(struct rpc_rqst *req, __be32 *p, void *dummy) int status; if ((status = ntohl(*p++)) != 0) - status = -nfs_stat_to_errno(status); + status = nfs_stat_to_errno(status); return status; } @@ -532,7 +532,7 @@ nfs_xdr_attrstat(struct rpc_rqst *req, __be32 *p, struct nfs_fattr *fattr) int status; if ((status = ntohl(*p++))) - return -nfs_stat_to_errno(status); + return nfs_stat_to_errno(status); xdr_decode_fattr(p, fattr); return 0; } @@ -547,7 +547,7 @@ nfs_xdr_diropres(struct rpc_rqst *req, __be32 *p, struct nfs_diropok *res) int status; if ((status = ntohl(*p++))) - return -nfs_stat_to_errno(status); + return nfs_stat_to_errno(status); p = xdr_decode_fhandle(p, res->fh); xdr_decode_fattr(p, res->fattr); return 0; @@ -585,7 +585,7 @@ nfs_xdr_readlinkres(struct rpc_rqst *req, __be32 *p, void *dummy) int status; if ((status = ntohl(*p++))) - return -nfs_stat_to_errno(status); + return nfs_stat_to_errno(status); /* Convert length of symlink */ len = ntohl(*p++); if (len >= rcvbuf->page_len) { @@ -634,7 +634,7 @@ nfs_xdr_statfsres(struct rpc_rqst *req, __be32 *p, struct nfs2_fsstat *res) int status; if ((status = ntohl(*p++))) - return -nfs_stat_to_errno(status); + return nfs_stat_to_errno(status); res->tsize = ntohl(*p++); res->bsize = ntohl(*p++); @@ -653,39 +653,39 @@ static struct { int errno; } nfs_errtbl[] = { { NFS_OK, 0 }, - { NFSERR_PERM, EPERM }, - { NFSERR_NOENT, ENOENT }, - { NFSERR_IO, errno_NFSERR_IO }, - { NFSERR_NXIO, ENXIO }, -/* { NFSERR_EAGAIN, EAGAIN }, */ - { NFSERR_ACCES, EACCES }, - { NFSERR_EXIST, EEXIST }, - { NFSERR_XDEV, EXDEV }, - { NFSERR_NODEV, ENODEV }, - { NFSERR_NOTDIR, ENOTDIR }, - { NFSERR_ISDIR, EISDIR }, - { NFSERR_INVAL, EINVAL }, - { NFSERR_FBIG, EFBIG }, - { NFSERR_NOSPC, ENOSPC }, - { NFSERR_ROFS, EROFS }, - { NFSERR_MLINK, EMLINK }, - { NFSERR_NAMETOOLONG, ENAMETOOLONG }, - { NFSERR_NOTEMPTY, ENOTEMPTY }, - { NFSERR_DQUOT, EDQUOT }, - { NFSERR_STALE, ESTALE }, - { NFSERR_REMOTE, EREMOTE }, + { NFSERR_PERM, -EPERM }, + { NFSERR_NOENT, -ENOENT }, + { NFSERR_IO, -errno_NFSERR_IO}, + { NFSERR_NXIO, -ENXIO }, +/* { NFSERR_EAGAIN, -EAGAIN }, */ + { NFSERR_ACCES, -EACCES }, + { NFSERR_EXIST, -EEXIST }, + { NFSERR_XDEV, -EXDEV }, + { NFSERR_NODEV, -ENODEV }, + { NFSERR_NOTDIR, -ENOTDIR }, + { NFSERR_ISDIR, -EISDIR }, + { NFSERR_INVAL, -EINVAL }, + { NFSERR_FBIG, -EFBIG }, + { NFSERR_NOSPC, -ENOSPC }, + { NFSERR_ROFS, -EROFS }, + { NFSERR_MLINK, -EMLINK }, + { NFSERR_NAMETOOLONG, -ENAMETOOLONG }, + { NFSERR_NOTEMPTY, -ENOTEMPTY }, + { NFSERR_DQUOT, -EDQUOT }, + { NFSERR_STALE, -ESTALE }, + { NFSERR_REMOTE, -EREMOTE }, #ifdef EWFLUSH - { NFSERR_WFLUSH, EWFLUSH }, + { NFSERR_WFLUSH, -EWFLUSH }, #endif - { NFSERR_BADHANDLE, EBADHANDLE }, - { NFSERR_NOT_SYNC, ENOTSYNC }, - { NFSERR_BAD_COOKIE, EBADCOOKIE }, - { NFSERR_NOTSUPP, ENOTSUPP }, - { NFSERR_TOOSMALL, ETOOSMALL }, - { NFSERR_SERVERFAULT, ESERVERFAULT }, - { NFSERR_BADTYPE, EBADTYPE }, - { NFSERR_JUKEBOX, EJUKEBOX }, - { -1, EIO } + { NFSERR_BADHANDLE, -EBADHANDLE }, + { NFSERR_NOT_SYNC, -ENOTSYNC }, + { NFSERR_BAD_COOKIE, -EBADCOOKIE }, + { NFSERR_NOTSUPP, -ENOTSUPP }, + { NFSERR_TOOSMALL, -ETOOSMALL }, + { NFSERR_SERVERFAULT, -ESERVERFAULT }, + { NFSERR_BADTYPE, -EBADTYPE }, + { NFSERR_JUKEBOX, -EJUKEBOX }, + { -1, -EIO } }; /* diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c index 3917e2f..feeab62 100644 --- a/fs/nfs/nfs3xdr.c +++ b/fs/nfs/nfs3xdr.c @@ -515,7 +515,7 @@ nfs3_xdr_readdirres(struct rpc_rqst *req, __be32 *p, struct nfs3_readdirres *res /* Decode post_op_attrs */ p = xdr_decode_post_op_attr(p, res->dir_attr); if (status) - return -nfs_stat_to_errno(status); + return nfs_stat_to_errno(status); /* Decode verifier cookie */ if (res->verf) { res->verf[0] = *p++; @@ -732,7 +732,7 @@ nfs3_xdr_attrstat(struct rpc_rqst *req, __be32 *p, struct nfs_fattr *fattr) int status; if ((status = ntohl(*p++))) - return -nfs_stat_to_errno(status); + return nfs_stat_to_errno(status); xdr_decode_fattr(p, fattr); return 0; } @@ -747,7 +747,7 @@ nfs3_xdr_wccstat(struct rpc_rqst *req, __be32 *p, struct nfs_fattr *fattr) int status; if ((status = ntohl(*p++))) - status = -nfs_stat_to_errno(status); + status = nfs_stat_to_errno(status); xdr_decode_wcc_data(p, fattr); return status; } @@ -767,7 +767,7 @@ nfs3_xdr_lookupres(struct rpc_rqst *req, __be32 *p, struct nfs3_diropres *res) int status; if ((status = ntohl(*p++))) { - status = -nfs_stat_to_errno(status); + status = nfs_stat_to_errno(status); } else { if (!(p = xdr_decode_fhandle(p, res->fh))) return -errno_NFSERR_IO; @@ -787,7 +787,7 @@ nfs3_xdr_accessres(struct rpc_rqst *req, __be32 *p, struct nfs3_accessres *res) p = xdr_decode_post_op_attr(p, res->fattr); if (status) - return -nfs_stat_to_errno(status); + return nfs_stat_to_errno(status); res->access = ntohl(*p++); return 0; } @@ -824,7 +824,7 @@ nfs3_xdr_readlinkres(struct rpc_rqst *req, __be32 *p, struct nfs_fattr *fattr) p = xdr_decode_post_op_attr(p, fattr); if (status != 0) - return -nfs_stat_to_errno(status); + return nfs_stat_to_errno(status); /* Convert length of symlink */ len = ntohl(*p++); @@ -872,7 +872,7 @@ nfs3_xdr_readres(struct rpc_rqst *req, __be32 *p, struct nfs_readres *res) p = xdr_decode_post_op_attr(p, res->fattr); if (status != 0) - return -nfs_stat_to_errno(status); + return nfs_stat_to_errno(status); /* Decode reply count and EOF flag. NFSv3 is somewhat redundant * in that it puts the count both in the res struct and in the @@ -922,7 +922,7 @@ nfs3_xdr_writeres(struct rpc_rqst *req, __be32 *p, struct nfs_writeres *res) p = xdr_decode_wcc_data(p, res->fattr); if (status != 0) - return -nfs_stat_to_errno(status); + return nfs_stat_to_errno(status); res->count = ntohl(*p++); res->verf->committed = (enum nfs3_stable_how)ntohl(*p++); @@ -953,7 +953,7 @@ nfs3_xdr_createres(struct rpc_rqst *req, __be32 *p, struct nfs3_diropres *res) res->fattr->valid = 0; } } else { - status = -nfs_stat_to_errno(status); + status = nfs_stat_to_errno(status); } p = xdr_decode_wcc_data(p, res->dir_attr); return status; @@ -968,7 +968,7 @@ nfs3_xdr_renameres(struct rpc_rqst *req, __be32 *p, struct nfs3_renameres *res) int status; if ((status = ntohl(*p++)) != 0) - status = -nfs_stat_to_errno(status); + status = nfs_stat_to_errno(status); p = xdr_decode_wcc_data(p, res->fromattr); p = xdr_decode_wcc_data(p, res->toattr); return status; @@ -983,7 +983,7 @@ nfs3_xdr_linkres(struct rpc_rqst *req, __be32 *p, struct nfs3_linkres *res) int status; if ((status = ntohl(*p++)) != 0) - status = -nfs_stat_to_errno(status); + status = nfs_stat_to_errno(status); p = xdr_decode_post_op_attr(p, res->fattr); p = xdr_decode_wcc_data(p, res->dir_attr); return status; @@ -1001,7 +1001,7 @@ nfs3_xdr_fsstatres(struct rpc_rqst *req, __be32 *p, struct nfs_fsstat *res) p = xdr_decode_post_op_attr(p, res->fattr); if (status != 0) - return -nfs_stat_to_errno(status); + return nfs_stat_to_errno(status); p = xdr_decode_hyper(p, &res->tbytes); p = xdr_decode_hyper(p, &res->fbytes); @@ -1026,7 +1026,7 @@ nfs3_xdr_fsinfores(struct rpc_rqst *req, __be32 *p, struct nfs_fsinfo *res) p = xdr_decode_post_op_attr(p, res->fattr); if (status != 0) - return -nfs_stat_to_errno(status); + return nfs_stat_to_errno(status); res->rtmax = ntohl(*p++); res->rtpref = ntohl(*p++); @@ -1054,7 +1054,7 @@ nfs3_xdr_pathconfres(struct rpc_rqst *req, __be32 *p, struct nfs_pathconf *res) p = xdr_decode_post_op_attr(p, res->fattr); if (status != 0) - return -nfs_stat_to_errno(status); + return nfs_stat_to_errno(status); res->max_link = ntohl(*p++); res->max_namelen = ntohl(*p++); @@ -1073,7 +1073,7 @@ nfs3_xdr_commitres(struct rpc_rqst *req, __be32 *p, struct nfs_writeres *res) status = ntohl(*p++); p = xdr_decode_wcc_data(p, res->fattr); if (status != 0) - return -nfs_stat_to_errno(status); + return nfs_stat_to_errno(status); res->verf->verifier[0] = *p++; res->verf->verifier[1] = *p++; @@ -1095,7 +1095,7 @@ nfs3_xdr_getaclres(struct rpc_rqst *req, __be32 *p, int err, base; if (status != 0) - return -nfs_stat_to_errno(status); + return nfs_stat_to_errno(status); p = xdr_decode_post_op_attr(p, res->fattr); res->mask = ntohl(*p++); if (res->mask & ~(NFS_ACL|NFS_ACLCNT|NFS_DFACL|NFS_DFACLCNT)) @@ -1122,7 +1122,7 @@ nfs3_xdr_setaclres(struct rpc_rqst *req, __be32 *p, struct nfs_fattr *fattr) int status = ntohl(*p++); if (status) - return -nfs_stat_to_errno(status); + return nfs_stat_to_errno(status); xdr_decode_post_op_attr(p, fattr); return 0; } diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c index db1ed9c..bb95b7c 100644 --- a/fs/nfs/nfs4xdr.c +++ b/fs/nfs/nfs4xdr.c @@ -2241,7 +2241,7 @@ static int decode_op_hdr(struct xdr_stream *xdr, enum nfs_opnum4 expected) } READ32(nfserr); if (nfserr != NFS_OK) - return -nfs4_stat_to_errno(nfserr); + return nfs4_stat_to_errno(nfserr); return 0; } @@ -3727,7 +3727,7 @@ static int decode_setclientid(struct xdr_stream *xdr, struct nfs_client *clp) READ_BUF(len); return -NFSERR_CLID_INUSE; } else - return -nfs4_stat_to_errno(nfserr); + return nfs4_stat_to_errno(nfserr); return 0; } @@ -4389,7 +4389,7 @@ static int nfs4_xdr_dec_fsinfo(struct rpc_rqst *req, __be32 *p, struct nfs_fsinf if (!status) status = decode_fsinfo(&xdr, fsinfo); if (!status) - status = -nfs4_stat_to_errno(hdr.status); + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4479,7 +4479,7 @@ static int nfs4_xdr_dec_setclientid(struct rpc_rqst *req, __be32 *p, if (!status) status = decode_setclientid(&xdr, clp); if (!status) - status = -nfs4_stat_to_errno(hdr.status); + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4501,7 +4501,7 @@ static int nfs4_xdr_dec_setclientid_confirm(struct rpc_rqst *req, __be32 *p, str if (!status) status = decode_fsinfo(&xdr, fsinfo); if (!status) - status = -nfs4_stat_to_errno(hdr.status); + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4611,42 +4611,42 @@ static struct { int errno; } nfs_errtbl[] = { { NFS4_OK, 0 }, - { NFS4ERR_PERM, EPERM }, - { NFS4ERR_NOENT, ENOENT }, - { NFS4ERR_IO, errno_NFSERR_IO }, - { NFS4ERR_NXIO, ENXIO }, - { NFS4ERR_ACCESS, EACCES }, - { NFS4ERR_EXIST, EEXIST }, - { NFS4ERR_XDEV, EXDEV }, - { NFS4ERR_NOTDIR, ENOTDIR }, - { NFS4ERR_ISDIR, EISDIR }, - { NFS4ERR_INVAL, EINVAL }, - { NFS4ERR_FBIG, EFBIG }, - { NFS4ERR_NOSPC, ENOSPC }, - { NFS4ERR_ROFS, EROFS }, - { NFS4ERR_MLINK, EMLINK }, - { NFS4ERR_NAMETOOLONG, ENAMETOOLONG }, - { NFS4ERR_NOTEMPTY, ENOTEMPTY }, - { NFS4ERR_DQUOT, EDQUOT }, - { NFS4ERR_STALE, ESTALE }, - { NFS4ERR_BADHANDLE, EBADHANDLE }, - { NFS4ERR_BADOWNER, EINVAL }, - { NFS4ERR_BADNAME, EINVAL }, - { NFS4ERR_BAD_COOKIE, EBADCOOKIE }, - { NFS4ERR_NOTSUPP, ENOTSUPP }, - { NFS4ERR_TOOSMALL, ETOOSMALL }, - { NFS4ERR_SERVERFAULT, ESERVERFAULT }, - { NFS4ERR_BADTYPE, EBADTYPE }, - { NFS4ERR_LOCKED, EAGAIN }, - { NFS4ERR_RESOURCE, EREMOTEIO }, - { NFS4ERR_SYMLINK, ELOOP }, - { NFS4ERR_OP_ILLEGAL, EOPNOTSUPP }, - { NFS4ERR_DEADLOCK, EDEADLK }, - { NFS4ERR_WRONGSEC, EPERM }, /* FIXME: this needs + { NFS4ERR_PERM, -EPERM }, + { NFS4ERR_NOENT, -ENOENT }, + { NFS4ERR_IO, -errno_NFSERR_IO}, + { NFS4ERR_NXIO, -ENXIO }, + { NFS4ERR_ACCESS, -EACCES }, + { NFS4ERR_EXIST, -EEXIST }, + { NFS4ERR_XDEV, -EXDEV }, + { NFS4ERR_NOTDIR, -ENOTDIR }, + { NFS4ERR_ISDIR, -EISDIR }, + { NFS4ERR_INVAL, -EINVAL }, + { NFS4ERR_FBIG, -EFBIG }, + { NFS4ERR_NOSPC, -ENOSPC }, + { NFS4ERR_ROFS, -EROFS }, + { NFS4ERR_MLINK, -EMLINK }, + { NFS4ERR_NAMETOOLONG, -ENAMETOOLONG }, + { NFS4ERR_NOTEMPTY, -ENOTEMPTY }, + { NFS4ERR_DQUOT, -EDQUOT }, + { NFS4ERR_STALE, -ESTALE }, + { NFS4ERR_BADHANDLE, -EBADHANDLE }, + { NFS4ERR_BADOWNER, -EINVAL }, + { NFS4ERR_BADNAME, -EINVAL }, + { NFS4ERR_BAD_COOKIE, -EBADCOOKIE }, + { NFS4ERR_NOTSUPP, -ENOTSUPP }, + { NFS4ERR_TOOSMALL, -ETOOSMALL }, + { NFS4ERR_SERVERFAULT, -ESERVERFAULT }, + { NFS4ERR_BADTYPE, -EBADTYPE }, + { NFS4ERR_LOCKED, -EAGAIN }, + { NFS4ERR_RESOURCE, -EREMOTEIO }, + { NFS4ERR_SYMLINK, -ELOOP }, + { NFS4ERR_OP_ILLEGAL, -EOPNOTSUPP }, + { NFS4ERR_DEADLOCK, -EDEADLK }, + { NFS4ERR_WRONGSEC, -EPERM }, /* FIXME: this needs * to be handled by a * middle-layer. */ - { -1, EIO } + { -1, -EIO } }; /* @@ -4663,14 +4663,14 @@ nfs4_stat_to_errno(int stat) } if (stat <= 10000 || stat > 10100) { /* The server is looney tunes. */ - return ESERVERFAULT; + return -ESERVERFAULT; } /* If we cannot translate the error, the recovery routines should * handle it. * Note: remaining NFSv4 error codes have values > 10000, so should * not conflict with native Linux error codes. */ - return stat; + return -stat; } #define PROC(proc, argtype, restype) \ -- 1.5.3.3 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 2/2] nfs: use compound hdr.status to override op status. 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 ` Benny Halevy 2008-03-31 22:25 ` Trond Myklebust 1 sibling, 1 reply; 24+ messages in thread From: Benny Halevy @ 2008-03-31 14:48 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, nfsv4 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 comound header instead. This patch also fixes 3 call sites where we looked at the comound 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 | 74 +++++++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 68 insertions(+), 6 deletions(-) diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c index bb95b7c..edaa2fe 100644 --- a/fs/nfs/nfs4xdr.c +++ b/fs/nfs/nfs4xdr.c @@ -3779,6 +3779,8 @@ static int nfs4_xdr_dec_open_downgrade(struct rpc_rqst *rqstp, __be32 *p, struct goto out; decode_getfattr(&xdr, res->fattr, res->server); out: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -3806,6 +3808,8 @@ 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: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -3829,6 +3833,8 @@ 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: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -3849,6 +3855,8 @@ 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: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -3870,6 +3878,8 @@ 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: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -3900,6 +3910,8 @@ 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: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -3933,6 +3945,8 @@ 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: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -3962,6 +3976,8 @@ 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: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -3991,6 +4007,8 @@ 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: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4014,6 +4032,8 @@ nfs4_xdr_enc_setacl(struct rpc_rqst *req, __be32 *p, struct nfs_setaclargs *args goto out; status = encode_setacl(&xdr, args); out: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } /* @@ -4035,6 +4055,8 @@ nfs4_xdr_dec_setacl(struct rpc_rqst *rqstp, __be32 *p, void *res) goto out; status = decode_setattr(&xdr, res); out: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4058,6 +4080,8 @@ nfs4_xdr_dec_getacl(struct rpc_rqst *rqstp, __be32 *p, size_t *acl_len) status = decode_getacl(&xdr, rqstp, acl_len); out: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4088,6 +4112,8 @@ static int nfs4_xdr_dec_close(struct rpc_rqst *rqstp, __be32 *p, struct nfs_clos */ decode_getfattr(&xdr, res->fattr, res->server); out: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4121,6 +4147,8 @@ 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: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4142,6 +4170,8 @@ static int nfs4_xdr_dec_open_confirm(struct rpc_rqst *rqstp, __be32 *p, struct n goto out; status = decode_open_confirm(&xdr, res); out: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4166,6 +4196,8 @@ 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: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4190,8 +4222,10 @@ 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: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4213,6 +4247,8 @@ static int nfs4_xdr_dec_lock(struct rpc_rqst *rqstp, __be32 *p, struct nfs_lock_ goto out; status = decode_lock(&xdr, res); out: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4234,6 +4270,8 @@ static int nfs4_xdr_dec_lockt(struct rpc_rqst *rqstp, __be32 *p, struct nfs_lock goto out; status = decode_lockt(&xdr, res); out: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4255,6 +4293,8 @@ static int nfs4_xdr_dec_locku(struct rpc_rqst *rqstp, __be32 *p, struct nfs_lock goto out; status = decode_locku(&xdr, res); out: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4276,6 +4316,8 @@ static int nfs4_xdr_dec_readlink(struct rpc_rqst *rqstp, __be32 *p, void *res) goto out; status = decode_readlink(&xdr, rqstp); out: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4297,6 +4339,8 @@ static int nfs4_xdr_dec_readdir(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_r goto out; status = decode_readdir(&xdr, rqstp, res); out: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4318,8 +4362,10 @@ 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: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4344,8 +4390,10 @@ 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: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4370,6 +4418,8 @@ 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: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4388,7 +4438,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) + if (hdr.status) status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4408,6 +4458,8 @@ 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); + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4426,6 +4478,8 @@ 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); + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4445,6 +4499,8 @@ static int nfs4_xdr_dec_server_caps(struct rpc_rqst *req, __be32 *p, struct nfs4 goto out; status = decode_server_caps(&xdr, res); out: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4461,6 +4517,8 @@ 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); + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4478,7 +4536,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) + if (hdr.status) status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4500,7 +4558,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) + if (hdr.status) status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4524,6 +4582,8 @@ 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: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } @@ -4547,6 +4607,8 @@ 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: + if (hdr.status) + status = nfs4_stat_to_errno(hdr.status); return status; } -- 1.5.3.3 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] nfs: use compound hdr.status to override op status. 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> 0 siblings, 1 reply; 24+ messages in thread From: Trond Myklebust @ 2008-03-31 22:25 UTC (permalink / raw) To: Benny Halevy; +Cc: linux-nfs, nfsv4 On Mon, 2008-03-31 at 17:48 +0300, 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 > comound header instead. > > This patch also fixes 3 call sites where we looked at the comound > 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 | 74 +++++++++++++++++++++++++++++++++++++++++++++++++---- > 1 files changed, 68 insertions(+), 6 deletions(-) > > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c > index bb95b7c..edaa2fe 100644 > --- a/fs/nfs/nfs4xdr.c > +++ b/fs/nfs/nfs4xdr.c > @@ -3779,6 +3779,8 @@ static int nfs4_xdr_dec_open_downgrade(struct rpc_rqst *rqstp, __be32 *p, struct > goto out; > decode_getfattr(&xdr, res->fattr, res->server); > out: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); This changes the return value so that the outcome of the RPC call now depends on the success of a previously optional post-op GETATTR operation. For non-idempotent RPC calls, that's not acceptable. > return status; > } > > @@ -3806,6 +3808,8 @@ 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: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); ditto > return status; > } > > @@ -3829,6 +3833,8 @@ 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: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); > return status; > } > > @@ -3849,6 +3855,8 @@ 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: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); > return status; > } > > @@ -3870,6 +3878,8 @@ 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: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); ditto > return status; > } > > @@ -3900,6 +3910,8 @@ 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: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); ditto > return status; > } > > @@ -3933,6 +3945,8 @@ 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: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); ditto > return status; > } > > @@ -3962,6 +3976,8 @@ 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: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); ditto > return status; > } > > @@ -3991,6 +4007,8 @@ 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: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); > return status; > > } > @@ -4014,6 +4032,8 @@ nfs4_xdr_enc_setacl(struct rpc_rqst *req, __be32 *p, struct nfs_setaclargs *args > goto out; > status = encode_setacl(&xdr, args); > out: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); > return status; > } > /* > @@ -4035,6 +4055,8 @@ nfs4_xdr_dec_setacl(struct rpc_rqst *rqstp, __be32 *p, void *res) > goto out; > status = decode_setattr(&xdr, res); > out: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); > return status; > } > > @@ -4058,6 +4080,8 @@ nfs4_xdr_dec_getacl(struct rpc_rqst *rqstp, __be32 *p, size_t *acl_len) > status = decode_getacl(&xdr, rqstp, acl_len); > > out: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); > return status; > } > > @@ -4088,6 +4112,8 @@ static int nfs4_xdr_dec_close(struct rpc_rqst *rqstp, __be32 *p, struct nfs_clos > */ > decode_getfattr(&xdr, res->fattr, res->server); > out: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); ditto > return status; > } > > @@ -4121,6 +4147,8 @@ 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: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); ditto > return status; > } > > @@ -4142,6 +4170,8 @@ static int nfs4_xdr_dec_open_confirm(struct rpc_rqst *rqstp, __be32 *p, struct n > goto out; > status = decode_open_confirm(&xdr, res); > out: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); > return status; > } > > @@ -4166,6 +4196,8 @@ 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: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); ditto > return status; > } > > @@ -4190,8 +4222,10 @@ 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: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); > return status; > } > > @@ -4213,6 +4247,8 @@ static int nfs4_xdr_dec_lock(struct rpc_rqst *rqstp, __be32 *p, struct nfs_lock_ > goto out; > status = decode_lock(&xdr, res); > out: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); > return status; > } > > @@ -4234,6 +4270,8 @@ static int nfs4_xdr_dec_lockt(struct rpc_rqst *rqstp, __be32 *p, struct nfs_lock > goto out; > status = decode_lockt(&xdr, res); > out: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); > return status; > } > > @@ -4255,6 +4293,8 @@ static int nfs4_xdr_dec_locku(struct rpc_rqst *rqstp, __be32 *p, struct nfs_lock > goto out; > status = decode_locku(&xdr, res); > out: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); > return status; > } > > @@ -4276,6 +4316,8 @@ static int nfs4_xdr_dec_readlink(struct rpc_rqst *rqstp, __be32 *p, void *res) > goto out; > status = decode_readlink(&xdr, rqstp); > out: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); > return status; > } > > @@ -4297,6 +4339,8 @@ static int nfs4_xdr_dec_readdir(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_r > goto out; > status = decode_readdir(&xdr, rqstp, res); > out: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); > return status; > } > > @@ -4318,8 +4362,10 @@ 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: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); > return status; > } > > @@ -4344,8 +4390,10 @@ 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: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); > return status; > } > > @@ -4370,6 +4418,8 @@ 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: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); ditto > return status; > } > > @@ -4388,7 +4438,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) > + if (hdr.status) > status = nfs4_stat_to_errno(hdr.status); > return status; > } > @@ -4408,6 +4458,8 @@ 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); > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); > return status; > } > > @@ -4426,6 +4478,8 @@ 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); > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); > return status; > } > > @@ -4445,6 +4499,8 @@ static int nfs4_xdr_dec_server_caps(struct rpc_rqst *req, __be32 *p, struct nfs4 > goto out; > status = decode_server_caps(&xdr, res); > out: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); > return status; > } > > @@ -4461,6 +4517,8 @@ 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); > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); > return status; > } > > @@ -4478,7 +4536,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) > + if (hdr.status) > status = nfs4_stat_to_errno(hdr.status); > return status; > } > @@ -4500,7 +4558,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) > + if (hdr.status) > status = nfs4_stat_to_errno(hdr.status); > return status; > } > @@ -4524,6 +4582,8 @@ 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: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); ditto > return status; > } > > @@ -4547,6 +4607,8 @@ 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: > + if (hdr.status) > + status = nfs4_stat_to_errno(hdr.status); > return status; > } > ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <1207002349.15341.15.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org>]
* Re: [PATCH 2/2] nfs: use compound hdr.status to override op status. [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 0 siblings, 1 reply; 24+ messages in thread From: Benny Halevy @ 2008-04-01 9:37 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, nfsv4 On Apr. 01, 2008, 1:25 +0300, Trond Myklebust <trond.myklebust@fys.uio.no> wrote: > On Mon, 2008-03-31 at 17:48 +0300, 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 >> comound header instead. >> >> This patch also fixes 3 call sites where we looked at the comound >> 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 | 74 +++++++++++++++++++++++++++++++++++++++++++++++++---- >> 1 files changed, 68 insertions(+), 6 deletions(-) >> >> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c >> index bb95b7c..edaa2fe 100644 >> --- a/fs/nfs/nfs4xdr.c >> +++ b/fs/nfs/nfs4xdr.c >> @@ -3779,6 +3779,8 @@ static int nfs4_xdr_dec_open_downgrade(struct rpc_rqst *rqstp, __be32 *p, struct >> goto out; >> decode_getfattr(&xdr, res->fattr, res->server); >> out: >> + if (hdr.status) >> + status = nfs4_stat_to_errno(hdr.status); > > This changes the return value so that the outcome of the RPC call now > depends on the success of a previously optional post-op GETATTR > operation. For non-idempotent RPC calls, that's not acceptable. Point taken. Please see the patch in reply to this message. It takes a different approach which overrides the status only in the error case. For example: +#define nfs4_fixup_status(status, hdr_status) \ + ( likely(!status) ? 0 : nfs4_stat_to_errno(hdr_status) ) + /* * Decode OPEN_DOWNGRADE response */ @@ -3779,7 +3782,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); } /* Benny P.S. The reason I defined this macro rather than a static inline function is that it produced the smallest machine code (for x86_64, as reported by objdump -h). ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/2 v1] nfs: use compound hdr.status to override op status. 2008-04-01 9:37 ` Benny Halevy @ 2008-04-01 9:41 ` Benny Halevy 2008-05-09 22:21 ` [PATCH 1/1 v2] " Benny Halevy ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: Benny Halevy @ 2008-04-01 9:41 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, nfsv4 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 | 84 +++++++++++++++++++++++++---------------------------- 1 files changed, 40 insertions(+), 44 deletions(-) diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c index bb95b7c..85fae46 100644 --- a/fs/nfs/nfs4xdr.c +++ b/fs/nfs/nfs4xdr.c @@ -3758,6 +3758,9 @@ static int decode_delegreturn(struct xdr_stream *xdr) return decode_op_hdr(xdr, OP_DELEGRETURN); } +#define nfs4_fixup_status(status, hdr_status) \ + ( likely(!status) ? 0 : nfs4_stat_to_errno(hdr_status) ) + /* * Decode OPEN_DOWNGRADE response */ @@ -3779,7 +3782,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); } /* @@ -3806,7 +3809,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); } /* @@ -3829,7 +3832,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); } /* @@ -3849,7 +3852,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); } /* @@ -3870,7 +3873,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); } /* @@ -3900,7 +3903,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); } /* @@ -3933,7 +3936,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); } /* @@ -3962,7 +3965,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); } /* @@ -3991,8 +3994,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); } /* @@ -4014,7 +4016,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 nfs4_fixup_status(status, hdr.status); } /* * Decode SETACL response @@ -4035,7 +4037,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); } /* @@ -4058,7 +4060,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); } /* @@ -4088,7 +4090,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); } /* @@ -4121,7 +4123,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); } /* @@ -4142,7 +4144,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); } /* @@ -4166,7 +4168,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); } /* @@ -4190,9 +4192,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); } /* @@ -4213,7 +4215,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); } /* @@ -4234,7 +4236,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); } /* @@ -4255,7 +4257,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); } /* @@ -4276,7 +4278,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); } /* @@ -4297,7 +4299,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); } /* @@ -4318,9 +4320,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); } /* @@ -4344,9 +4346,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); } /* @@ -4370,7 +4372,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); } /* @@ -4388,9 +4390,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); } /* @@ -4408,7 +4408,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); } /* @@ -4426,7 +4426,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); } /* @@ -4445,7 +4445,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); } /* @@ -4461,7 +4461,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); } /* @@ -4478,9 +4478,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); } /* @@ -4500,9 +4498,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); } /* @@ -4524,7 +4520,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); } /* @@ -4547,7 +4543,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) -- 1.5.3.3 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 1/1 v2] nfs: use compound hdr.status to override op status. 2008-04-01 9:41 ` [PATCH 2/2 v1] " Benny Halevy @ 2008-05-09 22:21 ` Benny Halevy 2008-05-09 23:32 ` Benny Halevy 2008-05-09 23:38 ` [PATCH " Benny Halevy 2 siblings, 0 replies; 24+ messages in thread From: Benny Halevy @ 2008-05-09 22:21 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, nfsv4, Benny Halevy ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/1 v2] nfs: use compound hdr.status to override op status. 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 2 siblings, 1 reply; 24+ messages in thread From: Benny Halevy @ 2008-05-09 23:32 UTC (permalink / raw) To: Trond Myklebust; +Cc: nfsv4, linux-nfs, Benny Halevy ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1 v2] nfs: use compound hdr.status to override op status. 2008-05-09 23:32 ` Benny Halevy @ 2008-05-10 6:11 ` Benny Halevy 0 siblings, 0 replies; 24+ messages in thread From: Benny Halevy @ 2008-05-10 6:11 UTC (permalink / raw) To: Trond Myklebust; +Cc: nfsv4, linux-nfs Please ignore the set of empty messages. [problem with sendmail from inside the connectathon network...] Benny On May. 09, 2008, 16:32 -0700, Benny Halevy <bhalevy@panasas.com> wrote: > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2] nfs: use compound hdr.status to override op status. 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-09 23:38 ` Benny Halevy 2008-05-10 17:24 ` Trond Myklebust 2 siblings, 1 reply; 24+ messages in thread From: Benny Halevy @ 2008-05-09 23:38 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, nfsv4 >From 513dd2b4ebab280343db4bf6071b75c4820089fc Mon Sep 17 00:00:00 2001 From: Benny Halevy <bhalevy@panasas.com> Date: Tue, 1 Apr 2008 12:41:08 +0300 Subject: [PATCH 1/1 v2] nfs: use compound hdr.status to override op status. 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 | 84 +++++++++++++++++++++++++---------------------------- 1 files changed, 40 insertions(+), 44 deletions(-) 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..57b512e 100644 --- a/fs/nfs/nfs4xdr.c +++ b/fs/nfs/nfs4xdr.c @@ -3791,6 +3791,9 @@ static int decode_delegreturn(struct xdr_stream *xdr) return decode_op_hdr(xdr, OP_DELEGRETURN); } +#define nfs4_fixup_status(status, hdr_status) \ + (likely(!status) ? 0 : nfs4_stat_to_errno(hdr_status)) + /* * Decode OPEN_DOWNGRADE response */ @@ -3812,7 +3815,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); } /* @@ -3839,7 +3842,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 +3865,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 +3885,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 +3906,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 +3936,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 +3969,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 +3998,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 +4027,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 +4049,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 +4070,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 +4093,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 +4123,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 +4156,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 +4177,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 +4201,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 +4225,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 +4248,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 +4269,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 +4290,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 +4311,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 +4332,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 +4353,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 +4379,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 +4405,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 +4423,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 +4441,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 +4459,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 +4478,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 +4494,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 +4511,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 +4531,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 +4553,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 +4576,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) -- 1.5.3.3 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2] nfs: use compound hdr.status to override op status. 2008-05-09 23:38 ` [PATCH " Benny Halevy @ 2008-05-10 17:24 ` Trond Myklebust 2008-05-11 23:21 ` [PATCH v3] " Benny Halevy 0 siblings, 1 reply; 24+ messages in thread From: Trond Myklebust @ 2008-05-10 17:24 UTC (permalink / raw) To: Benny Halevy; +Cc: linux-nfs, nfsv4 On Fri, 2008-05-09 at 16:38 -0700, Benny Halevy wrote: > >From 513dd2b4ebab280343db4bf6071b75c4820089fc Mon Sep 17 00:00:00 2001 > From: Benny Halevy <bhalevy@panasas.com> > Date: Tue, 1 Apr 2008 12:41:08 +0300 > Subject: [PATCH 1/1 v2] nfs: use compound hdr.status to override op status. > > 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 | 84 +++++++++++++++++++++++++---------------------------- > 1 files changed, 40 insertions(+), 44 deletions(-) > > 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..57b512e 100644 > --- a/fs/nfs/nfs4xdr.c > +++ b/fs/nfs/nfs4xdr.c > @@ -3791,6 +3791,9 @@ static int decode_delegreturn(struct xdr_stream *xdr) > return decode_op_hdr(xdr, OP_DELEGRETURN); > } > > +#define nfs4_fixup_status(status, hdr_status) \ > + (likely(!status) ? 0 : nfs4_stat_to_errno(hdr_status)) > + static function please! Trond ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3] nfs: use compound hdr.status to override op status. 2008-05-10 17:24 ` Trond Myklebust @ 2008-05-11 23:21 ` Benny Halevy 2008-05-11 23:26 ` Trond Myklebust 2008-07-03 17:49 ` [PATCH 0/2 v4] nfs: return nfs4 compound header status on op header decoding error Benny Halevy 0 siblings, 2 replies; 24+ messages in thread From: Benny Halevy @ 2008-05-11 23:21 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, nfsv4 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); } /* @@ -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) -- 1.5.3.3 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v3] nfs: use compound hdr.status to override op status. 2008-05-11 23:21 ` [PATCH v3] " Benny Halevy @ 2008-05-11 23:26 ` Trond Myklebust 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 1 sibling, 1 reply; 24+ messages in thread From: Trond Myklebust @ 2008-05-11 23:26 UTC (permalink / raw) To: Benny Halevy; +Cc: linux-nfs, nfsv4 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) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3] nfs: use compound hdr.status to override op status. 2008-05-11 23:26 ` Trond Myklebust @ 2008-05-12 1:54 ` Benny Halevy 0 siblings, 0 replies; 24+ messages in thread From: Benny Halevy @ 2008-05-12 1:54 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, nfsv4 On May. 11, 2008, 16:26 -0700, Trond Myklebust <trond.myklebust@fys.uio.no> wrote: > 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. > Trond, sorry, but I don't see the problem. If all ops before the getattr succeeded then "status" will be zero regardless if getattr failed or not and nfs4_fixup_status will return success. If any other op before the getattr failed then getfattr will not get processed and hdr.status will contain the latest failure. Benny ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 0/2 v4] nfs: return nfs4 compound header status on op header decoding error 2008-05-11 23:21 ` [PATCH v3] " Benny Halevy 2008-05-11 23:26 ` Trond Myklebust @ 2008-07-03 17:49 ` Benny Halevy 2008-07-03 17:52 ` [PATCH 1/2] " Benny Halevy ` (2 more replies) 1 sibling, 3 replies; 24+ messages in thread From: Benny Halevy @ 2008-07-03 17:49 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, nfsv4 Trond, following our conversation during the Connectathon I reduced this patch as much as possible and restricted the use of the compound header status to cases where op_hdr decoding hit an error. This is needed for nfs41 for graceful fallback when trying to mount a 4.0 server with 4.1. In this case the server returns no ops and the hdr status is set to NFS4ERR_MINOR_VERS_MISMATCH. Please consider these patches: [PATCH 1/2] nfs: return nfs4 compound header status on op header decoding error [PATCH 2/2] nfs: remove incorrect usage of nfs4 compound response hdr.status Thanks, Benny ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/2] nfs: return nfs4 compound header status on op header decoding error 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 ` 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 2 siblings, 0 replies; 24+ messages in thread From: Benny Halevy @ 2008-07-03 17:52 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, nfsv4, Benny Halevy Signed-off-by: Benny Halevy <bhalevy@panasas.com> --- fs/nfs/nfs4xdr.c | 30 +++++++++++++++++++++++------- include/linux/sunrpc/xdr.h | 1 + 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c index b916297..2cf724d 100644 --- a/fs/nfs/nfs4xdr.c +++ b/fs/nfs/nfs4xdr.c @@ -2214,8 +2214,10 @@ static int decode_compound_hdr(struct xdr_stream *xdr, struct compound_hdr *hdr) { __be32 *p; + xdr->status = -EIO; READ_BUF(8); READ32(hdr->status); + xdr->status = hdr->status; READ32(hdr->taglen); READ_BUF(hdr->taglen + 4); @@ -2225,24 +2227,38 @@ static int decode_compound_hdr(struct xdr_stream *xdr, struct compound_hdr *hdr) return 0; } -static int decode_op_hdr(struct xdr_stream *xdr, enum nfs_opnum4 expected) +static int +decode_op_hdr(struct xdr_stream *xdr, enum nfs_opnum4 expected) { __be32 *p; uint32_t opnum; int32_t nfserr; - READ_BUF(8); + p = xdr_inline_decode(xdr, 8); + if (unlikely(!p)) { + dprintk("nfs: %s: prematurely hit end of receive" + " buffer\n", __func__); + dprintk("nfs: %s: xdr->p=%p, bytes=%u, xdr->end=%p\n", + __func__, xdr->p, 8, xdr->end); + goto err; + } READ32(opnum); - if (opnum != expected) { + if (unlikely(opnum != expected)) { dprintk("nfs: Server returned operation" " %d but we issued a request for %d\n", opnum, expected); - return -EIO; + goto err; } READ32(nfserr); - if (nfserr != NFS_OK) - return nfs4_stat_to_errno(nfserr); - return 0; + if (likely(nfserr == NFS_OK)) + return 0; + xdr->status = nfserr; +err: + if (xdr->status != NFS_OK) + return nfs4_stat_to_errno(xdr->status); + dprintk("nfs: %s: xdr status %d, returning %d\n", + __func__, xdr->status, -EIO); + return -EIO; } /* Dummy routine */ diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h index e4057d7..0c09ae5 100644 --- a/include/linux/sunrpc/xdr.h +++ b/include/linux/sunrpc/xdr.h @@ -208,6 +208,7 @@ struct xdr_stream { __be32 *end; /* end of available buffer space */ struct kvec *iov; /* pointer to the current kvec */ + int status; }; extern void xdr_init_encode(struct xdr_stream *xdr, struct xdr_buf *buf, __be32 *p); -- 1.5.6.GIT ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 2/2] nfs: remove incorrect usage of nfs4 compound response hdr.status 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 ` Benny Halevy 2008-07-15 21:57 ` [PATCH 0/2 v4] nfs: return nfs4 compound header status on op header decoding error Trond Myklebust 2 siblings, 0 replies; 24+ messages in thread From: Benny Halevy @ 2008-07-03 18:42 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, nfsv4, Benny Halevy 3 call sites look at hdr.status before returning success. hdr.status must be zero in this case so there's no point in this. Currently, hdr.status is correctly processed at decode_op_hdr time if the op status cannot be decoded. Signed-off-by: Benny Halevy <bhalevy@panasas.com> --- fs/nfs/nfs4xdr.c | 6 ------ 1 files changed, 0 insertions(+), 6 deletions(-) diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c index 2cf724d..22df89f 100644 --- a/fs/nfs/nfs4xdr.c +++ b/fs/nfs/nfs4xdr.c @@ -4437,8 +4437,6 @@ 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; } @@ -4527,8 +4525,6 @@ 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; } @@ -4549,8 +4545,6 @@ 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; } -- 1.5.6.GIT ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 0/2 v4] nfs: return nfs4 compound header status on op header decoding error 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 ` Trond Myklebust 2008-07-16 8:21 ` Benny Halevy 2008-07-21 16:59 ` [PATCH] nfs: return compound hdr.status when there are no op replies Benny Halevy 2 siblings, 2 replies; 24+ messages in thread From: Trond Myklebust @ 2008-07-15 21:57 UTC (permalink / raw) To: Benny Halevy; +Cc: linux-nfs, nfsv4 On Thu, 2008-07-03 at 20:49 +0300, Benny Halevy wrote: > Trond, following our conversation during the Connectathon > I reduced this patch as much as possible and restricted the > use of the compound header status to cases where op_hdr > decoding hit an error. > > This is needed for nfs41 for graceful fallback when trying > to mount a 4.0 server with 4.1. In this case the server > returns no ops and the hdr status is set to > NFS4ERR_MINOR_VERS_MISMATCH. > > Please consider these patches: > > [PATCH 1/2] nfs: return nfs4 compound header status on op header decoding error No, this patch doesn't look right either. If we overrun the end of the reply buffer, then xdr_inline_decode() will return a NULL pointer, so you should never hit the your (opnum != expected) case. So given that your concern is primarily the case where nops==0, why don't you just add that particular case to decode_compound_hdr? IOW: something like @@static int decode_compound_hdr( p += XDR_QUADLEN(hdr->taglen); READ32(hdr->nops); + if (hdr->nops < 1) + return nfs4_stat_to_errno(hdr->status); return 0; } ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/2 v4] nfs: return nfs4 compound header status on op header decoding error 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-21 16:59 ` [PATCH] nfs: return compound hdr.status when there are no op replies Benny Halevy 1 sibling, 1 reply; 24+ messages in thread From: Benny Halevy @ 2008-07-16 8:21 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, nfsv4 On Jul. 16, 2008, 0:57 +0300, Trond Myklebust <trond.myklebust@fys.uio.no> wrote: > On Thu, 2008-07-03 at 20:49 +0300, Benny Halevy wrote: >> Trond, following our conversation during the Connectathon >> I reduced this patch as much as possible and restricted the >> use of the compound header status to cases where op_hdr >> decoding hit an error. >> >> This is needed for nfs41 for graceful fallback when trying >> to mount a 4.0 server with 4.1. In this case the server >> returns no ops and the hdr status is set to >> NFS4ERR_MINOR_VERS_MISMATCH. >> >> Please consider these patches: >> >> [PATCH 1/2] nfs: return nfs4 compound header status on op header decoding error > > No, this patch doesn't look right either. If we overrun the end of the > reply buffer, then xdr_inline_decode() will return a NULL pointer, so > you should never hit the your (opnum != expected) case. In patch 1, this is supposed to be handled like this: decode_compound_hdr: + xdr->status = hdr->status; decode_op_hdr: + p = xdr_inline_decode(xdr, 8); + if (unlikely(!p)) { ... + goto err; + } +err: + if (xdr->status != NFS_OK) + return nfs4_stat_to_errno(xdr->status); > > So given that your concern is primarily the case where nops==0, why > don't you just add that particular case to decode_compound_hdr? > > IOW: something like > > > @@static int decode_compound_hdr( > p += XDR_QUADLEN(hdr->taglen); > READ32(hdr->nops); > + if (hdr->nops < 1) > + return nfs4_stat_to_errno(hdr->status); > return 0; > } > This certainly provides a shortcut for the nops==0 case. However, it doesn't solve the OP_ILLEGAL case all other cases where xdr_inline_decode failed or opnum != expected, we can easily handle the OP_ILLEGAL case explicitly in decode_op_hdr. Are you ok with the approach of carrying the hdr.status in xdr->status for decode_op_hdr use, or do you rather prefer to leave things as they are for the invalid cases and explicitly handle only the nops==0 and OP_ILLEGAL cases? Benny ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/2 v4] nfs: return nfs4 compound header status on op header decoding error 2008-07-16 8:21 ` Benny Halevy @ 2008-07-16 12:57 ` Trond Myklebust 2008-07-16 13:22 ` Benny Halevy 0 siblings, 1 reply; 24+ messages in thread From: Trond Myklebust @ 2008-07-16 12:57 UTC (permalink / raw) To: Benny Halevy; +Cc: linux-nfs, nfsv4 On Wed, 2008-07-16 at 11:21 +0300, Benny Halevy wrote: > On Jul. 16, 2008, 0:57 +0300, Trond Myklebust <trond.myklebust@fys.uio.no> wrote: > > IOW: something like > > > > > > @@static int decode_compound_hdr( > > p += XDR_QUADLEN(hdr->taglen); > > READ32(hdr->nops); > > + if (hdr->nops < 1) > > + return nfs4_stat_to_errno(hdr->status); > > return 0; > > } > > > > This certainly provides a shortcut for the nops==0 case. > However, it doesn't solve the OP_ILLEGAL case all other > cases where xdr_inline_decode failed or opnum != expected, > we can easily handle the OP_ILLEGAL case explicitly in > decode_op_hdr. Are you ok with the approach of > carrying the hdr.status in xdr->status for decode_op_hdr > use, or do you rather prefer to leave things as they are > for the invalid cases and explicitly handle only the > nops==0 and OP_ILLEGAL cases? Why do we need to handle OP_ILLEGAL in the first place? This is the client; it isn't supposed to send illegal operations... Trond ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/2 v4] nfs: return nfs4 compound header status on op header decoding error 2008-07-16 12:57 ` Trond Myklebust @ 2008-07-16 13:22 ` Benny Halevy 2008-07-17 12:09 ` Trond Myklebust 0 siblings, 1 reply; 24+ messages in thread From: Benny Halevy @ 2008-07-16 13:22 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, nfsv4 On Jul. 16, 2008, 15:57 +0300, Trond Myklebust <trond.myklebust@fys.uio.no> wrote: > On Wed, 2008-07-16 at 11:21 +0300, Benny Halevy wrote: >> On Jul. 16, 2008, 0:57 +0300, Trond Myklebust <trond.myklebust@fys.uio.no> wrote: >>> IOW: something like >>> >>> >>> @@static int decode_compound_hdr( >>> p += XDR_QUADLEN(hdr->taglen); >>> READ32(hdr->nops); >>> + if (hdr->nops < 1) >>> + return nfs4_stat_to_errno(hdr->status); >>> return 0; >>> } >>> >> This certainly provides a shortcut for the nops==0 case. >> However, it doesn't solve the OP_ILLEGAL case all other >> cases where xdr_inline_decode failed or opnum != expected, >> we can easily handle the OP_ILLEGAL case explicitly in >> decode_op_hdr. Are you ok with the approach of >> carrying the hdr.status in xdr->status for decode_op_hdr >> use, or do you rather prefer to leave things as they are >> for the invalid cases and explicitly handle only the >> nops==0 and OP_ILLEGAL cases? > > Why do we need to handle OP_ILLEGAL in the first place? This is the > client; it isn't supposed to send illegal operations... Right, but it helps in the development process when dealing with a broken version of the server or the client to pass a less generic error (-EOPNOTSUPP) up the stack rather than -EIO. Benny > > Trond > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/2 v4] nfs: return nfs4 compound header status on op header decoding error 2008-07-16 13:22 ` Benny Halevy @ 2008-07-17 12:09 ` Trond Myklebust 2008-07-17 13:20 ` Benny Halevy 0 siblings, 1 reply; 24+ messages in thread From: Trond Myklebust @ 2008-07-17 12:09 UTC (permalink / raw) To: Benny Halevy; +Cc: linux-nfs, nfsv4 On Wed, 2008-07-16 at 16:22 +0300, Benny Halevy wrote: > On Jul. 16, 2008, 15:57 +0300, Trond Myklebust <trond.myklebust@fys.uio.no> wrote: > > Why do we need to handle OP_ILLEGAL in the first place? This is the > > client; it isn't supposed to send illegal operations... > > Right, but it helps in the development process when dealing with > a broken version of the server or the client to pass a less > generic error (-EOPNOTSUPP) up the stack rather than -EIO. NFS4ERR_OP_ILLEGAL literally means "this operation isn't even listed in the 4.0/4.1 RFC". That's out in EYOUUTTERLYINSANECLIENT territory, and so the current mapping to EOPNOTSUPP is just wrong. NFS4ERR_NOTSUPP is the correct return value if a server doesn't (yet) support an operation. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/2 v4] nfs: return nfs4 compound header status on op header decoding error 2008-07-17 12:09 ` Trond Myklebust @ 2008-07-17 13:20 ` Benny Halevy 0 siblings, 0 replies; 24+ messages in thread From: Benny Halevy @ 2008-07-17 13:20 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, nfsv4 On Jul. 17, 2008, 15:09 +0300, Trond Myklebust <trond.myklebust@fys.uio.no> wrote: > On Wed, 2008-07-16 at 16:22 +0300, Benny Halevy wrote: >> On Jul. 16, 2008, 15:57 +0300, Trond Myklebust <trond.myklebust@fys.uio.no> wrote: >>> Why do we need to handle OP_ILLEGAL in the first place? This is the >>> client; it isn't supposed to send illegal operations... >> Right, but it helps in the development process when dealing with >> a broken version of the server or the client to pass a less >> generic error (-EOPNOTSUPP) up the stack rather than -EIO. > > NFS4ERR_OP_ILLEGAL literally means "this operation isn't even listed in > the 4.0/4.1 RFC". That's out in EYOUUTTERLYINSANECLIENT territory, and > so the current mapping to EOPNOTSUPP is just wrong. FWIW, we currently map NFS4ERR_NOTSUPP to -ENOTSUPP and NFS4ERR_OP_ILLEGAL to -EOPNOTSUPP so one can distinct between the two cases, even if the latter mapping is wrong. I'm not sure if there's an available error code that would describe this NFS4ERR_OP_ILLEGAL perfectly, -EINVAL might be reasonable but I it's not very distinctive. Maybe we should add one to include/linux/errno.h? > > NFS4ERR_NOTSUPP is the correct return value if a server doesn't (yet) > support an operation. > > > Agreed. Just that we had a bug in the server that caused us to return NFS4ERR_OP_ILLEGAL for unimplemented ops rather than NFS4ERR_NOTSUPP. That's a condition I really want to be aware of while developing the server. Benny ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH] nfs: return compound hdr.status when there are no op replies 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-21 16:59 ` Benny Halevy 1 sibling, 0 replies; 24+ messages in thread From: Benny Halevy @ 2008-07-21 16:59 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, nfsv4 When there are no op replies encoded in the comound reply hdr.status still contains the overall status of the compound rpc. This can happen, e.g., when the server returns an NFS4ERR_MINOR_VERS_MISMATCH error. Signed-off-by: Benny Halevy <bhalevy@panasas.com> --- fs/nfs/nfs4xdr.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c index b916297..ac2386b 100644 --- a/fs/nfs/nfs4xdr.c +++ b/fs/nfs/nfs4xdr.c @@ -2222,6 +2222,8 @@ static int decode_compound_hdr(struct xdr_stream *xdr, struct compound_hdr *hdr) hdr->tag = (char *)p; p += XDR_QUADLEN(hdr->taglen); READ32(hdr->nops); + if (unlikely(hdr->nops < 1)) + return nfs4_stat_to_errno(hdr->status); return 0; } -- 1.5.6.4 ^ permalink raw reply related [flat|nested] 24+ messages in thread
end of thread, other threads:[~2008-07-21 16:59 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox