2008-03-31 14:35:37

by Benny Halevy

[permalink] [raw]
Subject: [PATCH 0/2] nfsv4 compound status

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


2008-03-31 14:39:30

by Benny Halevy

[permalink] [raw]
Subject: [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.

Signed-off-by: Benny Halevy <[email protected]>
---
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


2008-03-31 14:48:20

by Benny Halevy

[permalink] [raw]
Subject: [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}

Signed-off-by: Benny Halevy <[email protected]>
---
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

2008-03-31 22:25:49

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH 2/2] nfs: use compound hdr.status to override op status.


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 <[email protected]>
> ---
> 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;
> }
>

2008-04-01 09:37:50

by Benny Halevy

[permalink] [raw]
Subject: Re: [PATCH 2/2] nfs: use compound hdr.status to override op status.

On Apr. 01, 2008, 1:25 +0300, Trond Myklebust <[email protected]> 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 <[email protected]>
>> ---
>> 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).

2008-04-01 09:41:27

by Benny Halevy

[permalink] [raw]
Subject: [PATCH 2/2 v1] 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 <[email protected]>
---
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



2008-05-09 23:38:59

by Benny Halevy

[permalink] [raw]
Subject: [PATCH v2] nfs: use compound hdr.status to override op status.

>From 513dd2b4ebab280343db4bf6071b75c4820089fc Mon Sep 17 00:00:00 2001
From: Benny Halevy <[email protected]>
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 <[email protected]>
---
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



2008-05-10 06:12:13

by Benny Halevy

[permalink] [raw]
Subject: Re: [PATCH 1/1 v2] nfs: use compound hdr.status to override op status.

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 <[email protected]> wrote:
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html


2008-05-10 17:24:36

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH v2] nfs: use compound hdr.status to override op status.

On Fri, 2008-05-09 at 16:38 -0700, Benny Halevy wrote:
> >From 513dd2b4ebab280343db4bf6071b75c4820089fc Mon Sep 17 00:00:00 2001
> From: Benny Halevy <[email protected]>
> 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 <[email protected]>
> ---
> 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

2008-05-11 23:21:31

by Benny Halevy

[permalink] [raw]
Subject: [PATCH v3] 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 <[email protected]>
---
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

2008-05-11 23:26:12

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH v3] nfs: use compound hdr.status to override op status.

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 <[email protected]>
> ---
> 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)


2008-05-12 01:55:11

by Benny Halevy

[permalink] [raw]
Subject: Re: [PATCH v3] nfs: use compound hdr.status to override op status.

On May. 11, 2008, 16:26 -0700, Trond Myklebust <[email protected]> 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 <[email protected]>
>> ---
>> 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

2008-07-21 16:59:36

by Benny Halevy

[permalink] [raw]
Subject: [PATCH] nfs: return compound hdr.status when there are no op replies

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 <[email protected]>
---
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

2008-07-15 21:57:20

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH 0/2 v4] nfs: return nfs4 compound header status on op header decoding error

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;
}


2008-07-16 08:21:25

by Benny Halevy

[permalink] [raw]
Subject: Re: [PATCH 0/2 v4] nfs: return nfs4 compound header status on op header decoding error

On Jul. 16, 2008, 0:57 +0300, Trond Myklebust <[email protected]> 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

2008-07-16 12:57:43

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH 0/2 v4] nfs: return nfs4 compound header status on op header decoding error

On Wed, 2008-07-16 at 11:21 +0300, Benny Halevy wrote:
> On Jul. 16, 2008, 0:57 +0300, Trond Myklebust <[email protected]> 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


2008-07-16 13:22:52

by Benny Halevy

[permalink] [raw]
Subject: Re: [PATCH 0/2 v4] nfs: return nfs4 compound header status on op header decoding error

On Jul. 16, 2008, 15:57 +0300, Trond Myklebust <[email protected]> wrote:
> On Wed, 2008-07-16 at 11:21 +0300, Benny Halevy wrote:
>> On Jul. 16, 2008, 0:57 +0300, Trond Myklebust <[email protected]> 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
>

2008-07-17 12:09:02

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH 0/2 v4] nfs: return nfs4 compound header status on op header decoding error

On Wed, 2008-07-16 at 16:22 +0300, Benny Halevy wrote:
> On Jul. 16, 2008, 15:57 +0300, Trond Myklebust <[email protected]> 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.

2008-07-17 13:20:53

by Benny Halevy

[permalink] [raw]
Subject: Re: [PATCH 0/2 v4] nfs: return nfs4 compound header status on op header decoding error

On Jul. 17, 2008, 15:09 +0300, Trond Myklebust <[email protected]> wrote:
> On Wed, 2008-07-16 at 16:22 +0300, Benny Halevy wrote:
>> On Jul. 16, 2008, 15:57 +0300, Trond Myklebust <[email protected]> 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

2008-07-03 17:49:53

by Benny Halevy

[permalink] [raw]
Subject: [PATCH 0/2 v4] nfs: return nfs4 compound header status on op header decoding error

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

2008-07-03 17:52:40

by Benny Halevy

[permalink] [raw]
Subject: [PATCH 1/2] nfs: return nfs4 compound header status on op header decoding error

Signed-off-by: Benny Halevy <[email protected]>
---
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


2008-07-03 18:42:21

by Benny Halevy

[permalink] [raw]
Subject: [PATCH 2/2] nfs: remove incorrect usage of nfs4 compound response hdr.status

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 <[email protected]>
---
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