* Empty core dumps on NFSv4 mounts
@ 2010-07-02 8:01 Arnaud Giersch
2010-07-02 13:03 ` Trond Myklebust
0 siblings, 1 reply; 9+ messages in thread
From: Arnaud Giersch @ 2010-07-02 8:01 UTC (permalink / raw)
To: linux-nfs
Hi,
On NFSv4 mounts, many core dumps are empty, although ulimit -c is
unlimited. An ls command shortly after the core dump often shows
4294967294 (2^32-2) as UID and GID for the "core" file.
This only happens when there was no "core" file before the dump. If a
"core" file owned by the current user is already present, it is
correctly filled.
After having done a git bisect, it seems that the problem was
introduced by commit 80e52aced138bb41b045a8595a87510f27d8d8c5
(NFSv4: Don't do idmapper upcalls for asynchronous RPC calls).
If I understand correctly what happens, do_coredump() [fs/exec.c] fails
because (inode->i_uid != current_fsuid()). In fact inode->i_uid equals
-2, because decode_attr_owner() [fs/nfs/nfs4xdr.c], which is called from
nfs4_xdr_dec_open() via decode_getfattr(), returns without calling
nfs_map_to_uid(), since its may_sleep parameter is false.
I however do not clearly understand what the aforementioned commit is
supposed to fix. I read the linux-nfs mailing list archive, and tried
some google search, but I didn't find anything.
Regards,
Arnaud Giersch
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Empty core dumps on NFSv4 mounts
2010-07-02 8:01 Empty core dumps on NFSv4 mounts Arnaud Giersch
@ 2010-07-02 13:03 ` Trond Myklebust
2010-07-05 13:28 ` [PATCH/RFC 2.6.35-rc4] NFSv4: Don't do idmapper upcalls during XDR decode Arnaud Giersch
0 siblings, 1 reply; 9+ messages in thread
From: Trond Myklebust @ 2010-07-02 13:03 UTC (permalink / raw)
To: Arnaud Giersch; +Cc: linux-nfs
On Fri, 2010-07-02 at 10:01 +0200, Arnaud Giersch wrote:
> Hi,
>
> On NFSv4 mounts, many core dumps are empty, although ulimit -c is
> unlimited. An ls command shortly after the core dump often shows
> 4294967294 (2^32-2) as UID and GID for the "core" file.
>
> This only happens when there was no "core" file before the dump. If a
> "core" file owned by the current user is already present, it is
> correctly filled.
>
> After having done a git bisect, it seems that the problem was
> introduced by commit 80e52aced138bb41b045a8595a87510f27d8d8c5
> (NFSv4: Don't do idmapper upcalls for asynchronous RPC calls).
>
> If I understand correctly what happens, do_coredump() [fs/exec.c] fails
> because (inode->i_uid != current_fsuid()). In fact inode->i_uid equals
> -2, because decode_attr_owner() [fs/nfs/nfs4xdr.c], which is called from
> nfs4_xdr_dec_open() via decode_getfattr(), returns without calling
> nfs_map_to_uid(), since its may_sleep parameter is false.
>
> I however do not clearly understand what the aforementioned commit is
> supposed to fix. I read the linux-nfs mailing list archive, and tried
> some google search, but I didn't find anything.
rpciod threads should never do anything that can cause them to sleep.
Doing an upcall as part of an XDR decode is therefore verboten.
We should ideally rather be saving the owner/group strings and doing the
upcall after the XDR is done. While that wasn't feasible when 'fattr'
structs were being allocated on the stack, now that they are dynamically
allocated, maybe we can rethink that...
Cheers
Trond
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH/RFC 2.6.35-rc4] NFSv4: Don't do idmapper upcalls during XDR decode
2010-07-02 13:03 ` Trond Myklebust
@ 2010-07-05 13:28 ` Arnaud Giersch
2010-08-12 10:15 ` [PATCH 2.6.35] " Arnaud Giersch
0 siblings, 1 reply; 9+ messages in thread
From: Arnaud Giersch @ 2010-07-05 13:28 UTC (permalink / raw)
To: Trond Myklebust; +Cc: linux-nfs
From: Arnaud Giersch <arnaud.giersch@free.fr>
Move the name to id mapping out of the XDR decode functions.
Add two new fields to struct nfs_fattr: user_name and group_name, that
are defined iff a name to id mapping should be done.
A new function, nfs_fattr_name_to_id(), is defined to call the
idmapper if needed.
This also fixes the empty core dumps that may be produced since commit
80e52aced138bb41b045a8595a87510f27d8d8c5.
Signed-off-by: Arnaud Giersch <arnaud.giersch@free.fr>
---
Trond, did you think about something like this when you wrote "saving
the owner/group strings and doing the upcall after the XDR is done"?
I tagged the patch RFC mainly because I am not sure of the following:
* if the kmalloc() call if OK inside decode_attr_owner() and
decode_attr_group();
* the calls to nfs_fattr_name_to_id() are done at the proper places.
Regards,
Arnaud
fs/nfs/inode.c | 35 ++++++++++++
fs/nfs/nfs4xdr.c | 109 ++++++++++++++-----------------------
include/linux/nfs_fs.h | 2 +
include/linux/nfs_xdr.h | 7 +++
4 files changed, 86 insertions(+), 67 deletions(-)
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 099b351..6df59ad 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -227,6 +227,35 @@ nfs_init_locked(struct inode *inode, void *opaque)
return 0;
}
+static void
+nfs_fattr_name_to_id(struct nfs_client *clp, struct nfs_fattr *fattr)
+{
+ if (fattr->user_name) {
+ struct nfs_name *name = fattr->user_name;
+ if (nfs_map_name_to_uid(clp, name->buf, name->len,
+ &fattr->uid) == 0)
+ fattr->valid |= NFS_ATTR_FATTR_OWNER;
+ else
+ dprintk("%s: nfs_map_name_to_uid failed!\n",
+ __func__);
+ kfree(fattr->user_name);
+ fattr->user_name = NULL;
+ dprintk("%s: uid=%d\n", __func__, (int)fattr->uid);
+ }
+ if (fattr->group_name) {
+ struct nfs_name *name = fattr->group_name;
+ if (nfs_map_group_to_gid(clp, name->buf, name->len,
+ &fattr->gid) == 0)
+ fattr->valid |= NFS_ATTR_FATTR_GROUP;
+ else
+ dprintk("%s: nfs_map_group_to_gid failed!\n",
+ __func__);
+ kfree(fattr->group_name);
+ fattr->group_name = NULL;
+ dprintk("%s: gid=%d\n", __func__, (int)fattr->gid);
+ }
+}
+
/* Don't use READDIRPLUS on directories that we believe are too large */
#define NFS_LIMIT_READDIRPLUS (8*PAGE_SIZE)
@@ -256,6 +285,7 @@ nfs_fhget(struct super_block *sb, struct nfs_fh *fh, struct nfs_fattr *fattr)
inode = ERR_PTR(-ENOMEM);
goto out_no_inode;
}
+ nfs_fattr_name_to_id(NFS_SERVER(inode)->nfs_client, fattr);
if (inode->i_state & I_NEW) {
struct nfs_inode *nfsi = NFS_I(inode);
@@ -930,6 +960,8 @@ unsigned long nfs_inc_attr_generation_counter(void)
void nfs_fattr_init(struct nfs_fattr *fattr)
{
fattr->valid = 0;
+ fattr->user_name = NULL;
+ fattr->group_name = NULL;
fattr->time_start = jiffies;
fattr->gencount = nfs_inc_attr_generation_counter();
}
@@ -1004,6 +1036,7 @@ int nfs_refresh_inode(struct inode *inode, struct nfs_fattr *fattr)
{
int status;
+ nfs_fattr_name_to_id(NFS_SERVER(inode)->nfs_client, fattr);
if ((fattr->valid & NFS_ATTR_FATTR) == 0)
return 0;
spin_lock(&inode->i_lock);
@@ -1043,6 +1076,7 @@ int nfs_post_op_update_inode(struct inode *inode, struct nfs_fattr *fattr)
{
int status;
+ nfs_fattr_name_to_id(NFS_SERVER(inode)->nfs_client, fattr);
spin_lock(&inode->i_lock);
status = nfs_post_op_update_inode_locked(inode, fattr);
spin_unlock(&inode->i_lock);
@@ -1064,6 +1098,7 @@ int nfs_post_op_update_inode_force_wcc(struct inode *inode, struct nfs_fattr *fa
{
int status;
+ nfs_fattr_name_to_id(NFS_SERVER(inode)->nfs_client, fattr);
spin_lock(&inode->i_lock);
/* Don't do a WCC update if these attributes are already stale */
if ((fattr->valid & NFS_ATTR_FATTR) == 0 ||
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 65c8dae..b4c2735 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -3227,13 +3227,13 @@ out_overflow:
}
static int decode_attr_owner(struct xdr_stream *xdr, uint32_t *bitmap,
- struct nfs_client *clp, uint32_t *uid, int may_sleep)
+ struct nfs_client *clp, struct nfs_name **owner)
{
uint32_t len;
__be32 *p;
int ret = 0;
- *uid = -2;
+ BUG_ON(*owner);
if (unlikely(bitmap[1] & (FATTR4_WORD1_OWNER - 1U)))
return -EIO;
if (likely(bitmap[1] & FATTR4_WORD1_OWNER)) {
@@ -3244,20 +3244,18 @@ static int decode_attr_owner(struct xdr_stream *xdr, uint32_t *bitmap,
p = xdr_inline_decode(xdr, len);
if (unlikely(!p))
goto out_overflow;
- if (!may_sleep) {
- /* do nothing */
- } else if (len < XDR_MAX_NETOBJ) {
- if (nfs_map_name_to_uid(clp, (char *)p, len, uid) == 0)
- ret = NFS_ATTR_FATTR_OWNER;
- else
- dprintk("%s: nfs_map_name_to_uid failed!\n",
- __func__);
+ if (len < XDR_MAX_NETOBJ) {
+ *owner = kmalloc((sizeof **owner) + len, GFP_NOFS);
+ if (*owner) {
+ (*owner)->len = len;
+ memcpy((*owner)->buf, p, len);
+ } else
+ dprintk("%s: kmalloc failed!\n", __func__);
} else
dprintk("%s: name too long (%u)!\n",
__func__, len);
bitmap[1] &= ~FATTR4_WORD1_OWNER;
}
- dprintk("%s: uid=%d\n", __func__, (int)*uid);
return ret;
out_overflow:
print_overflow_msg(__func__, xdr);
@@ -3265,13 +3263,13 @@ out_overflow:
}
static int decode_attr_group(struct xdr_stream *xdr, uint32_t *bitmap,
- struct nfs_client *clp, uint32_t *gid, int may_sleep)
+ struct nfs_client *clp, struct nfs_name **group)
{
uint32_t len;
__be32 *p;
int ret = 0;
- *gid = -2;
+ BUG_ON(*group);
if (unlikely(bitmap[1] & (FATTR4_WORD1_OWNER_GROUP - 1U)))
return -EIO;
if (likely(bitmap[1] & FATTR4_WORD1_OWNER_GROUP)) {
@@ -3282,20 +3280,18 @@ static int decode_attr_group(struct xdr_stream *xdr, uint32_t *bitmap,
p = xdr_inline_decode(xdr, len);
if (unlikely(!p))
goto out_overflow;
- if (!may_sleep) {
- /* do nothing */
- } else if (len < XDR_MAX_NETOBJ) {
- if (nfs_map_group_to_gid(clp, (char *)p, len, gid) == 0)
- ret = NFS_ATTR_FATTR_GROUP;
- else
- dprintk("%s: nfs_map_group_to_gid failed!\n",
- __func__);
+ if (len < XDR_MAX_NETOBJ) {
+ *group = kmalloc((sizeof **group) + len, GFP_NOFS);
+ if (*group) {
+ (*group)->len = len;
+ memcpy((*group)->buf, p, len);
+ } else
+ dprintk("%s: kmalloc failed!\n", __func__);
} else
dprintk("%s: name too long (%u)!\n",
__func__, len);
bitmap[1] &= ~FATTR4_WORD1_OWNER_GROUP;
}
- dprintk("%s: gid=%d\n", __func__, (int)*gid);
return ret;
out_overflow:
print_overflow_msg(__func__, xdr);
@@ -3701,7 +3697,7 @@ xdr_error:
}
static int decode_getfattr(struct xdr_stream *xdr, struct nfs_fattr *fattr,
- const struct nfs_server *server, int may_sleep)
+ const struct nfs_server *server)
{
__be32 *savep;
uint32_t attrlen,
@@ -3774,13 +3770,13 @@ static int decode_getfattr(struct xdr_stream *xdr, struct nfs_fattr *fattr,
fattr->valid |= status;
status = decode_attr_owner(xdr, bitmap, server->nfs_client,
- &fattr->uid, may_sleep);
+ &fattr->user_name);
if (status < 0)
goto xdr_error;
fattr->valid |= status;
status = decode_attr_group(xdr, bitmap, server->nfs_client,
- &fattr->gid, may_sleep);
+ &fattr->group_name);
if (status < 0)
goto xdr_error;
fattr->valid |= status;
@@ -4708,8 +4704,7 @@ static int nfs4_xdr_dec_open_downgrade(struct rpc_rqst *rqstp, __be32 *p, struct
status = decode_open_downgrade(&xdr, res);
if (status != 0)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -4736,8 +4731,7 @@ static int nfs4_xdr_dec_access(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_ac
status = decode_access(&xdr, res);
if (status != 0)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -4764,8 +4758,7 @@ static int nfs4_xdr_dec_lookup(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_lo
goto out;
if ((status = decode_getfh(&xdr, res->fh)) != 0)
goto out;
- status = decode_getfattr(&xdr, res->fattr, res->server
- ,!RPC_IS_ASYNC(rqstp->rq_task));
+ status = decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -4789,8 +4782,7 @@ static int nfs4_xdr_dec_lookup_root(struct rpc_rqst *rqstp, __be32 *p, struct nf
if ((status = decode_putrootfh(&xdr)) != 0)
goto out;
if ((status = decode_getfh(&xdr, res->fh)) == 0)
- status = decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ status = decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -4815,8 +4807,7 @@ static int nfs4_xdr_dec_remove(struct rpc_rqst *rqstp, __be32 *p, struct nfs_rem
goto out;
if ((status = decode_remove(&xdr, &res->cinfo)) != 0)
goto out;
- decode_getfattr(&xdr, res->dir_attr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->dir_attr, res->server);
out:
return status;
}
@@ -4846,13 +4837,11 @@ static int nfs4_xdr_dec_rename(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_re
if ((status = decode_rename(&xdr, &res->old_cinfo, &res->new_cinfo)) != 0)
goto out;
/* Current FH is target directory */
- if (decode_getfattr(&xdr, res->new_fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task)) != 0)
+ if (decode_getfattr(&xdr, res->new_fattr, res->server) != 0)
goto out;
if ((status = decode_restorefh(&xdr)) != 0)
goto out;
- decode_getfattr(&xdr, res->old_fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->old_fattr, res->server);
out:
return status;
}
@@ -4885,13 +4874,11 @@ static int nfs4_xdr_dec_link(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_link
* Note order: OP_LINK leaves the directory as the current
* filehandle.
*/
- if (decode_getfattr(&xdr, res->dir_attr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task)) != 0)
+ if (decode_getfattr(&xdr, res->dir_attr, res->server) != 0)
goto out;
if ((status = decode_restorefh(&xdr)) != 0)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -4920,13 +4907,11 @@ static int nfs4_xdr_dec_create(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_cr
goto out;
if ((status = decode_getfh(&xdr, res->fh)) != 0)
goto out;
- if (decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task)) != 0)
+ if (decode_getfattr(&xdr, res->fattr, res->server) != 0)
goto out;
if ((status = decode_restorefh(&xdr)) != 0)
goto out;
- decode_getfattr(&xdr, res->dir_fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->dir_fattr, res->server);
out:
return status;
}
@@ -4958,8 +4943,7 @@ static int nfs4_xdr_dec_getattr(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_g
status = decode_putfh(&xdr);
if (status)
goto out;
- status = decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ status = decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -5066,8 +5050,7 @@ static int nfs4_xdr_dec_close(struct rpc_rqst *rqstp, __be32 *p, struct nfs_clos
* an ESTALE error. Shouldn't be a problem,
* though, since fattr->valid will remain unset.
*/
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -5099,13 +5082,11 @@ static int nfs4_xdr_dec_open(struct rpc_rqst *rqstp, __be32 *p, struct nfs_openr
goto out;
if (decode_getfh(&xdr, &res->fh) != 0)
goto out;
- if (decode_getfattr(&xdr, res->f_attr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task)) != 0)
+ if (decode_getfattr(&xdr, res->f_attr, res->server) != 0)
goto out;
if (decode_restorefh(&xdr) != 0)
goto out;
- decode_getfattr(&xdr, res->dir_attr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->dir_attr, res->server);
out:
return status;
}
@@ -5153,8 +5134,7 @@ static int nfs4_xdr_dec_open_noattr(struct rpc_rqst *rqstp, __be32 *p, struct nf
status = decode_open(&xdr, res);
if (status)
goto out;
- decode_getfattr(&xdr, res->f_attr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->f_attr, res->server);
out:
return status;
}
@@ -5181,8 +5161,7 @@ static int nfs4_xdr_dec_setattr(struct rpc_rqst *rqstp, __be32 *p, struct nfs_se
status = decode_setattr(&xdr);
if (status)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -5356,8 +5335,7 @@ static int nfs4_xdr_dec_write(struct rpc_rqst *rqstp, __be32 *p, struct nfs_writ
status = decode_write(&xdr, res);
if (status)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
if (!status)
status = res->count;
out:
@@ -5386,8 +5364,7 @@ static int nfs4_xdr_dec_commit(struct rpc_rqst *rqstp, __be32 *p, struct nfs_wri
status = decode_commit(&xdr, res);
if (status)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -5553,8 +5530,7 @@ static int nfs4_xdr_dec_delegreturn(struct rpc_rqst *rqstp, __be32 *p, struct nf
status = decode_delegreturn(&xdr);
if (status != 0)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -5582,8 +5558,7 @@ static int nfs4_xdr_dec_fs_locations(struct rpc_rqst *req, __be32 *p,
goto out;
xdr_enter_page(&xdr, PAGE_SIZE);
status = decode_getfattr(&xdr, &res->fs_locations->fattr,
- res->fs_locations->server,
- !RPC_IS_ASYNC(req->rq_task));
+ res->fs_locations->server);
out:
return status;
}
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 77c2ae5..fd4cea0 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -360,6 +360,8 @@ extern struct nfs_fattr *nfs_alloc_fattr(void);
static inline void nfs_free_fattr(const struct nfs_fattr *fattr)
{
+ kfree(fattr->user_name);
+ kfree(fattr->group_name);
kfree(fattr);
}
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index 51914d7..074c3db 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -27,10 +27,17 @@ static inline int nfs_fsid_equal(const struct nfs_fsid *a, const struct nfs_fsid
return a->major == b->major && a->minor == b->minor;
}
+struct nfs_name {
+ size_t len;
+ char buf[];
+};
+
struct nfs_fattr {
unsigned int valid; /* which fields are valid */
umode_t mode;
__u32 nlink;
+ struct nfs_name * user_name;
+ struct nfs_name * group_name;
__u32 uid;
__u32 gid;
dev_t rdev;
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2.6.35] NFSv4: Don't do idmapper upcalls during XDR decode
2010-07-05 13:28 ` [PATCH/RFC 2.6.35-rc4] NFSv4: Don't do idmapper upcalls during XDR decode Arnaud Giersch
@ 2010-08-12 10:15 ` Arnaud Giersch
2010-09-21 11:17 ` [PATCH resend] " Arnaud Giersch
0 siblings, 1 reply; 9+ messages in thread
From: Arnaud Giersch @ 2010-08-12 10:15 UTC (permalink / raw)
To: linux-nfs, linux-kernel; +Cc: Trond Myklebust
From: Arnaud Giersch <arnaud.giersch@free.fr>
Move the name to id mapping out of the XDR decode functions.
Add two new fields to struct nfs_fattr: user_name and group_name, that
are defined iff a name to id mapping should be done.
A new function, nfs_fattr_name_to_id(), is defined to call the
idmapper if needed.
This also fixes the empty core dumps that may be produced on NFSv4
mounts since commit 80e52aced138bb41b045a8595a87510f27d8d8c5.
Signed-off-by: Arnaud Giersch <arnaud.giersch@free.fr>
---
Hi,
The purpose of this patch is to avoid empty core dumps on NFS4 mounts,
as reported in http://article.gmane.org/gmane.linux.kernel/998617, or
http://article.gmane.org/gmane.linux.nfs/33390.
This is my attempt to implement the suggestion made by Trond Myklebust
in http://article.gmane.org/gmane.linux.nfs/33391.
A previous submission (http://article.gmane.org/gmane.linux.nfs/33422)
did not get any feedback. The only difference with this first patch is
the test in nfs_free_fattr(), as the fattr parameter may be NULL.
I am currently running a patched kernel on my workstation, and did not
notice any problem.
Regards,
Arnaud Giersch
fs/nfs/inode.c | 35 ++++++++++++
fs/nfs/nfs4xdr.c | 109 ++++++++++++++-----------------------
include/linux/nfs_fs.h | 4 ++
include/linux/nfs_xdr.h | 7 +++
4 files changed, 88 insertions(+), 67 deletions(-)
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 7d2d6c7..b1f7799 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -234,6 +234,35 @@ nfs_init_locked(struct inode *inode, void *opaque)
return 0;
}
+static void
+nfs_fattr_name_to_id(struct nfs_client *clp, struct nfs_fattr *fattr)
+{
+ if (fattr->user_name) {
+ struct nfs_name *name = fattr->user_name;
+ if (nfs_map_name_to_uid(clp, name->buf, name->len,
+ &fattr->uid) == 0)
+ fattr->valid |= NFS_ATTR_FATTR_OWNER;
+ else
+ dprintk("%s: nfs_map_name_to_uid failed!\n",
+ __func__);
+ kfree(fattr->user_name);
+ fattr->user_name = NULL;
+ dprintk("%s: uid=%d\n", __func__, (int)fattr->uid);
+ }
+ if (fattr->group_name) {
+ struct nfs_name *name = fattr->group_name;
+ if (nfs_map_group_to_gid(clp, name->buf, name->len,
+ &fattr->gid) == 0)
+ fattr->valid |= NFS_ATTR_FATTR_GROUP;
+ else
+ dprintk("%s: nfs_map_group_to_gid failed!\n",
+ __func__);
+ kfree(fattr->group_name);
+ fattr->group_name = NULL;
+ dprintk("%s: gid=%d\n", __func__, (int)fattr->gid);
+ }
+}
+
/* Don't use READDIRPLUS on directories that we believe are too large */
#define NFS_LIMIT_READDIRPLUS (8*PAGE_SIZE)
@@ -263,6 +292,7 @@ nfs_fhget(struct super_block *sb, struct nfs_fh *fh, struct nfs_fattr *fattr)
inode = ERR_PTR(-ENOMEM);
goto out_no_inode;
}
+ nfs_fattr_name_to_id(NFS_SERVER(inode)->nfs_client, fattr);
if (inode->i_state & I_NEW) {
struct nfs_inode *nfsi = NFS_I(inode);
@@ -997,6 +1027,8 @@ unsigned long nfs_inc_attr_generation_counter(void)
void nfs_fattr_init(struct nfs_fattr *fattr)
{
fattr->valid = 0;
+ fattr->user_name = NULL;
+ fattr->group_name = NULL;
fattr->time_start = jiffies;
fattr->gencount = nfs_inc_attr_generation_counter();
}
@@ -1071,6 +1103,7 @@ int nfs_refresh_inode(struct inode *inode, struct nfs_fattr *fattr)
{
int status;
+ nfs_fattr_name_to_id(NFS_SERVER(inode)->nfs_client, fattr);
if ((fattr->valid & NFS_ATTR_FATTR) == 0)
return 0;
spin_lock(&inode->i_lock);
@@ -1110,6 +1143,7 @@ int nfs_post_op_update_inode(struct inode *inode, struct nfs_fattr *fattr)
{
int status;
+ nfs_fattr_name_to_id(NFS_SERVER(inode)->nfs_client, fattr);
spin_lock(&inode->i_lock);
status = nfs_post_op_update_inode_locked(inode, fattr);
spin_unlock(&inode->i_lock);
@@ -1131,6 +1165,7 @@ int nfs_post_op_update_inode_force_wcc(struct inode *inode, struct nfs_fattr *fa
{
int status;
+ nfs_fattr_name_to_id(NFS_SERVER(inode)->nfs_client, fattr);
spin_lock(&inode->i_lock);
/* Don't do a WCC update if these attributes are already stale */
if ((fattr->valid & NFS_ATTR_FATTR) == 0 ||
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 08ef912..c6e7197 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -3271,13 +3271,13 @@ out_overflow:
}
static int decode_attr_owner(struct xdr_stream *xdr, uint32_t *bitmap,
- struct nfs_client *clp, uint32_t *uid, int may_sleep)
+ struct nfs_client *clp, struct nfs_name **owner)
{
uint32_t len;
__be32 *p;
int ret = 0;
- *uid = -2;
+ BUG_ON(*owner);
if (unlikely(bitmap[1] & (FATTR4_WORD1_OWNER - 1U)))
return -EIO;
if (likely(bitmap[1] & FATTR4_WORD1_OWNER)) {
@@ -3288,20 +3288,18 @@ static int decode_attr_owner(struct xdr_stream *xdr, uint32_t *bitmap,
p = xdr_inline_decode(xdr, len);
if (unlikely(!p))
goto out_overflow;
- if (!may_sleep) {
- /* do nothing */
- } else if (len < XDR_MAX_NETOBJ) {
- if (nfs_map_name_to_uid(clp, (char *)p, len, uid) == 0)
- ret = NFS_ATTR_FATTR_OWNER;
- else
- dprintk("%s: nfs_map_name_to_uid failed!\n",
- __func__);
+ if (len < XDR_MAX_NETOBJ) {
+ *owner = kmalloc((sizeof **owner) + len, GFP_NOFS);
+ if (*owner) {
+ (*owner)->len = len;
+ memcpy((*owner)->buf, p, len);
+ } else
+ dprintk("%s: kmalloc failed!\n", __func__);
} else
dprintk("%s: name too long (%u)!\n",
__func__, len);
bitmap[1] &= ~FATTR4_WORD1_OWNER;
}
- dprintk("%s: uid=%d\n", __func__, (int)*uid);
return ret;
out_overflow:
print_overflow_msg(__func__, xdr);
@@ -3309,13 +3307,13 @@ out_overflow:
}
static int decode_attr_group(struct xdr_stream *xdr, uint32_t *bitmap,
- struct nfs_client *clp, uint32_t *gid, int may_sleep)
+ struct nfs_client *clp, struct nfs_name **group)
{
uint32_t len;
__be32 *p;
int ret = 0;
- *gid = -2;
+ BUG_ON(*group);
if (unlikely(bitmap[1] & (FATTR4_WORD1_OWNER_GROUP - 1U)))
return -EIO;
if (likely(bitmap[1] & FATTR4_WORD1_OWNER_GROUP)) {
@@ -3326,20 +3324,18 @@ static int decode_attr_group(struct xdr_stream *xdr, uint32_t *bitmap,
p = xdr_inline_decode(xdr, len);
if (unlikely(!p))
goto out_overflow;
- if (!may_sleep) {
- /* do nothing */
- } else if (len < XDR_MAX_NETOBJ) {
- if (nfs_map_group_to_gid(clp, (char *)p, len, gid) == 0)
- ret = NFS_ATTR_FATTR_GROUP;
- else
- dprintk("%s: nfs_map_group_to_gid failed!\n",
- __func__);
+ if (len < XDR_MAX_NETOBJ) {
+ *group = kmalloc((sizeof **group) + len, GFP_NOFS);
+ if (*group) {
+ (*group)->len = len;
+ memcpy((*group)->buf, p, len);
+ } else
+ dprintk("%s: kmalloc failed!\n", __func__);
} else
dprintk("%s: name too long (%u)!\n",
__func__, len);
bitmap[1] &= ~FATTR4_WORD1_OWNER_GROUP;
}
- dprintk("%s: gid=%d\n", __func__, (int)*gid);
return ret;
out_overflow:
print_overflow_msg(__func__, xdr);
@@ -3745,7 +3741,7 @@ xdr_error:
}
static int decode_getfattr(struct xdr_stream *xdr, struct nfs_fattr *fattr,
- const struct nfs_server *server, int may_sleep)
+ const struct nfs_server *server)
{
__be32 *savep;
uint32_t attrlen,
@@ -3818,13 +3814,13 @@ static int decode_getfattr(struct xdr_stream *xdr, struct nfs_fattr *fattr,
fattr->valid |= status;
status = decode_attr_owner(xdr, bitmap, server->nfs_client,
- &fattr->uid, may_sleep);
+ &fattr->user_name);
if (status < 0)
goto xdr_error;
fattr->valid |= status;
status = decode_attr_group(xdr, bitmap, server->nfs_client,
- &fattr->gid, may_sleep);
+ &fattr->group_name);
if (status < 0)
goto xdr_error;
fattr->valid |= status;
@@ -4757,8 +4753,7 @@ static int nfs4_xdr_dec_open_downgrade(struct rpc_rqst *rqstp, __be32 *p, struct
status = decode_open_downgrade(&xdr, res);
if (status != 0)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -4785,8 +4780,7 @@ static int nfs4_xdr_dec_access(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_ac
status = decode_access(&xdr, res);
if (status != 0)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -4813,8 +4807,7 @@ static int nfs4_xdr_dec_lookup(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_lo
goto out;
if ((status = decode_getfh(&xdr, res->fh)) != 0)
goto out;
- status = decode_getfattr(&xdr, res->fattr, res->server
- ,!RPC_IS_ASYNC(rqstp->rq_task));
+ status = decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -4838,8 +4831,7 @@ static int nfs4_xdr_dec_lookup_root(struct rpc_rqst *rqstp, __be32 *p, struct nf
if ((status = decode_putrootfh(&xdr)) != 0)
goto out;
if ((status = decode_getfh(&xdr, res->fh)) == 0)
- status = decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ status = decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -4864,8 +4856,7 @@ static int nfs4_xdr_dec_remove(struct rpc_rqst *rqstp, __be32 *p, struct nfs_rem
goto out;
if ((status = decode_remove(&xdr, &res->cinfo)) != 0)
goto out;
- decode_getfattr(&xdr, res->dir_attr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->dir_attr, res->server);
out:
return status;
}
@@ -4895,13 +4886,11 @@ static int nfs4_xdr_dec_rename(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_re
if ((status = decode_rename(&xdr, &res->old_cinfo, &res->new_cinfo)) != 0)
goto out;
/* Current FH is target directory */
- if (decode_getfattr(&xdr, res->new_fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task)) != 0)
+ if (decode_getfattr(&xdr, res->new_fattr, res->server) != 0)
goto out;
if ((status = decode_restorefh(&xdr)) != 0)
goto out;
- decode_getfattr(&xdr, res->old_fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->old_fattr, res->server);
out:
return status;
}
@@ -4934,13 +4923,11 @@ static int nfs4_xdr_dec_link(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_link
* Note order: OP_LINK leaves the directory as the current
* filehandle.
*/
- if (decode_getfattr(&xdr, res->dir_attr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task)) != 0)
+ if (decode_getfattr(&xdr, res->dir_attr, res->server) != 0)
goto out;
if ((status = decode_restorefh(&xdr)) != 0)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -4969,13 +4956,11 @@ static int nfs4_xdr_dec_create(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_cr
goto out;
if ((status = decode_getfh(&xdr, res->fh)) != 0)
goto out;
- if (decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task)) != 0)
+ if (decode_getfattr(&xdr, res->fattr, res->server) != 0)
goto out;
if ((status = decode_restorefh(&xdr)) != 0)
goto out;
- decode_getfattr(&xdr, res->dir_fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->dir_fattr, res->server);
out:
return status;
}
@@ -5007,8 +4992,7 @@ static int nfs4_xdr_dec_getattr(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_g
status = decode_putfh(&xdr);
if (status)
goto out;
- status = decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ status = decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -5115,8 +5099,7 @@ static int nfs4_xdr_dec_close(struct rpc_rqst *rqstp, __be32 *p, struct nfs_clos
* an ESTALE error. Shouldn't be a problem,
* though, since fattr->valid will remain unset.
*/
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -5148,13 +5131,11 @@ static int nfs4_xdr_dec_open(struct rpc_rqst *rqstp, __be32 *p, struct nfs_openr
goto out;
if (decode_getfh(&xdr, &res->fh) != 0)
goto out;
- if (decode_getfattr(&xdr, res->f_attr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task)) != 0)
+ if (decode_getfattr(&xdr, res->f_attr, res->server) != 0)
goto out;
if (decode_restorefh(&xdr) != 0)
goto out;
- decode_getfattr(&xdr, res->dir_attr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->dir_attr, res->server);
out:
return status;
}
@@ -5202,8 +5183,7 @@ static int nfs4_xdr_dec_open_noattr(struct rpc_rqst *rqstp, __be32 *p, struct nf
status = decode_open(&xdr, res);
if (status)
goto out;
- decode_getfattr(&xdr, res->f_attr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->f_attr, res->server);
out:
return status;
}
@@ -5230,8 +5210,7 @@ static int nfs4_xdr_dec_setattr(struct rpc_rqst *rqstp, __be32 *p, struct nfs_se
status = decode_setattr(&xdr);
if (status)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -5418,8 +5397,7 @@ static int nfs4_xdr_dec_write(struct rpc_rqst *rqstp, __be32 *p, struct nfs_writ
status = decode_write(&xdr, res);
if (status)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
if (!status)
status = res->count;
out:
@@ -5448,8 +5426,7 @@ static int nfs4_xdr_dec_commit(struct rpc_rqst *rqstp, __be32 *p, struct nfs_wri
status = decode_commit(&xdr, res);
if (status)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -5615,8 +5592,7 @@ static int nfs4_xdr_dec_delegreturn(struct rpc_rqst *rqstp, __be32 *p, struct nf
status = decode_delegreturn(&xdr);
if (status != 0)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -5644,8 +5620,7 @@ static int nfs4_xdr_dec_fs_locations(struct rpc_rqst *req, __be32 *p,
goto out;
xdr_enter_page(&xdr, PAGE_SIZE);
status = decode_getfattr(&xdr, &res->fs_locations->fattr,
- res->fs_locations->server,
- !RPC_IS_ASYNC(req->rq_task));
+ res->fs_locations->server);
out:
return status;
}
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 508f8cf..6ce7a08 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -369,6 +369,10 @@ extern struct nfs_fattr *nfs_alloc_fattr(void);
static inline void nfs_free_fattr(const struct nfs_fattr *fattr)
{
+ if (!fattr)
+ return;
+ kfree(fattr->user_name);
+ kfree(fattr->group_name);
kfree(fattr);
}
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index fc46192..fc58d5d 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -27,10 +27,17 @@ static inline int nfs_fsid_equal(const struct nfs_fsid *a, const struct nfs_fsid
return a->major == b->major && a->minor == b->minor;
}
+struct nfs_name {
+ size_t len;
+ char buf[];
+};
+
struct nfs_fattr {
unsigned int valid; /* which fields are valid */
umode_t mode;
__u32 nlink;
+ struct nfs_name *user_name;
+ struct nfs_name *group_name;
__u32 uid;
__u32 gid;
dev_t rdev;
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH resend] NFSv4: Don't do idmapper upcalls during XDR decode
2010-08-12 10:15 ` [PATCH 2.6.35] " Arnaud Giersch
@ 2010-09-21 11:17 ` Arnaud Giersch
0 siblings, 0 replies; 9+ messages in thread
From: Arnaud Giersch @ 2010-09-21 11:17 UTC (permalink / raw)
To: linux-nfs; +Cc: linux-kernel, Trond Myklebust
From: Arnaud Giersch <arnaud.giersch@free.fr>
Move the name to id mapping out of the XDR decode functions.
Add two new fields to struct nfs_fattr: user_name and group_name, that
are defined iff a name to id mapping should be done.
A new function, nfs_fattr_name_to_id(), is defined to call the
idmapper if needed.
This also fixes the empty core dumps that may be produced on NFSv4
mounts since commit 80e52aced138bb41b045a8595a87510f27d8d8c5.
Signed-off-by: Arnaud Giersch <arnaud.giersch@free.fr>
---
Hi,
[ Resend, since a previous sending on August 12 did not get any answer. ]
The purpose of this patch is to avoid empty core dumps on NFS4 mounts,
as reported in http://article.gmane.org/gmane.linux.kernel/998617, or
http://article.gmane.org/gmane.linux.nfs/33390.
This is my attempt to implement the suggestion made by Trond Myklebust
in http://article.gmane.org/gmane.linux.nfs/33391.
A previous submission (http://article.gmane.org/gmane.linux.nfs/33422)
did not get any feedback. The only difference with this first patch is
the test in nfs_free_fattr(), as the fattr parameter may be NULL.
I am currently running a patched kernel on my workstation, and did not
notice any problem.
Regards,
Arnaud Giersch
fs/nfs/inode.c | 35 ++++++++++++
fs/nfs/nfs4xdr.c | 109 ++++++++++++++-----------------------
include/linux/nfs_fs.h | 4 ++
include/linux/nfs_xdr.h | 7 +++
4 files changed, 88 insertions(+), 67 deletions(-)
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 7d2d6c7..b1f7799 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -234,6 +234,35 @@ nfs_init_locked(struct inode *inode, void *opaque)
return 0;
}
+static void
+nfs_fattr_name_to_id(struct nfs_client *clp, struct nfs_fattr *fattr)
+{
+ if (fattr->user_name) {
+ struct nfs_name *name = fattr->user_name;
+ if (nfs_map_name_to_uid(clp, name->buf, name->len,
+ &fattr->uid) == 0)
+ fattr->valid |= NFS_ATTR_FATTR_OWNER;
+ else
+ dprintk("%s: nfs_map_name_to_uid failed!\n",
+ __func__);
+ kfree(fattr->user_name);
+ fattr->user_name = NULL;
+ dprintk("%s: uid=%d\n", __func__, (int)fattr->uid);
+ }
+ if (fattr->group_name) {
+ struct nfs_name *name = fattr->group_name;
+ if (nfs_map_group_to_gid(clp, name->buf, name->len,
+ &fattr->gid) == 0)
+ fattr->valid |= NFS_ATTR_FATTR_GROUP;
+ else
+ dprintk("%s: nfs_map_group_to_gid failed!\n",
+ __func__);
+ kfree(fattr->group_name);
+ fattr->group_name = NULL;
+ dprintk("%s: gid=%d\n", __func__, (int)fattr->gid);
+ }
+}
+
/* Don't use READDIRPLUS on directories that we believe are too large */
#define NFS_LIMIT_READDIRPLUS (8*PAGE_SIZE)
@@ -263,6 +292,7 @@ nfs_fhget(struct super_block *sb, struct nfs_fh *fh, struct nfs_fattr *fattr)
inode = ERR_PTR(-ENOMEM);
goto out_no_inode;
}
+ nfs_fattr_name_to_id(NFS_SERVER(inode)->nfs_client, fattr);
if (inode->i_state & I_NEW) {
struct nfs_inode *nfsi = NFS_I(inode);
@@ -997,6 +1027,8 @@ unsigned long nfs_inc_attr_generation_counter(void)
void nfs_fattr_init(struct nfs_fattr *fattr)
{
fattr->valid = 0;
+ fattr->user_name = NULL;
+ fattr->group_name = NULL;
fattr->time_start = jiffies;
fattr->gencount = nfs_inc_attr_generation_counter();
}
@@ -1071,6 +1103,7 @@ int nfs_refresh_inode(struct inode *inode, struct nfs_fattr *fattr)
{
int status;
+ nfs_fattr_name_to_id(NFS_SERVER(inode)->nfs_client, fattr);
if ((fattr->valid & NFS_ATTR_FATTR) == 0)
return 0;
spin_lock(&inode->i_lock);
@@ -1110,6 +1143,7 @@ int nfs_post_op_update_inode(struct inode *inode, struct nfs_fattr *fattr)
{
int status;
+ nfs_fattr_name_to_id(NFS_SERVER(inode)->nfs_client, fattr);
spin_lock(&inode->i_lock);
status = nfs_post_op_update_inode_locked(inode, fattr);
spin_unlock(&inode->i_lock);
@@ -1131,6 +1165,7 @@ int nfs_post_op_update_inode_force_wcc(struct inode *inode, struct nfs_fattr *fa
{
int status;
+ nfs_fattr_name_to_id(NFS_SERVER(inode)->nfs_client, fattr);
spin_lock(&inode->i_lock);
/* Don't do a WCC update if these attributes are already stale */
if ((fattr->valid & NFS_ATTR_FATTR) == 0 ||
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 08ef912..c6e7197 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -3271,13 +3271,13 @@ out_overflow:
}
static int decode_attr_owner(struct xdr_stream *xdr, uint32_t *bitmap,
- struct nfs_client *clp, uint32_t *uid, int may_sleep)
+ struct nfs_client *clp, struct nfs_name **owner)
{
uint32_t len;
__be32 *p;
int ret = 0;
- *uid = -2;
+ BUG_ON(*owner);
if (unlikely(bitmap[1] & (FATTR4_WORD1_OWNER - 1U)))
return -EIO;
if (likely(bitmap[1] & FATTR4_WORD1_OWNER)) {
@@ -3288,20 +3288,18 @@ static int decode_attr_owner(struct xdr_stream *xdr, uint32_t *bitmap,
p = xdr_inline_decode(xdr, len);
if (unlikely(!p))
goto out_overflow;
- if (!may_sleep) {
- /* do nothing */
- } else if (len < XDR_MAX_NETOBJ) {
- if (nfs_map_name_to_uid(clp, (char *)p, len, uid) == 0)
- ret = NFS_ATTR_FATTR_OWNER;
- else
- dprintk("%s: nfs_map_name_to_uid failed!\n",
- __func__);
+ if (len < XDR_MAX_NETOBJ) {
+ *owner = kmalloc((sizeof **owner) + len, GFP_NOFS);
+ if (*owner) {
+ (*owner)->len = len;
+ memcpy((*owner)->buf, p, len);
+ } else
+ dprintk("%s: kmalloc failed!\n", __func__);
} else
dprintk("%s: name too long (%u)!\n",
__func__, len);
bitmap[1] &= ~FATTR4_WORD1_OWNER;
}
- dprintk("%s: uid=%d\n", __func__, (int)*uid);
return ret;
out_overflow:
print_overflow_msg(__func__, xdr);
@@ -3309,13 +3307,13 @@ out_overflow:
}
static int decode_attr_group(struct xdr_stream *xdr, uint32_t *bitmap,
- struct nfs_client *clp, uint32_t *gid, int may_sleep)
+ struct nfs_client *clp, struct nfs_name **group)
{
uint32_t len;
__be32 *p;
int ret = 0;
- *gid = -2;
+ BUG_ON(*group);
if (unlikely(bitmap[1] & (FATTR4_WORD1_OWNER_GROUP - 1U)))
return -EIO;
if (likely(bitmap[1] & FATTR4_WORD1_OWNER_GROUP)) {
@@ -3326,20 +3324,18 @@ static int decode_attr_group(struct xdr_stream *xdr, uint32_t *bitmap,
p = xdr_inline_decode(xdr, len);
if (unlikely(!p))
goto out_overflow;
- if (!may_sleep) {
- /* do nothing */
- } else if (len < XDR_MAX_NETOBJ) {
- if (nfs_map_group_to_gid(clp, (char *)p, len, gid) == 0)
- ret = NFS_ATTR_FATTR_GROUP;
- else
- dprintk("%s: nfs_map_group_to_gid failed!\n",
- __func__);
+ if (len < XDR_MAX_NETOBJ) {
+ *group = kmalloc((sizeof **group) + len, GFP_NOFS);
+ if (*group) {
+ (*group)->len = len;
+ memcpy((*group)->buf, p, len);
+ } else
+ dprintk("%s: kmalloc failed!\n", __func__);
} else
dprintk("%s: name too long (%u)!\n",
__func__, len);
bitmap[1] &= ~FATTR4_WORD1_OWNER_GROUP;
}
- dprintk("%s: gid=%d\n", __func__, (int)*gid);
return ret;
out_overflow:
print_overflow_msg(__func__, xdr);
@@ -3745,7 +3741,7 @@ xdr_error:
}
static int decode_getfattr(struct xdr_stream *xdr, struct nfs_fattr *fattr,
- const struct nfs_server *server, int may_sleep)
+ const struct nfs_server *server)
{
__be32 *savep;
uint32_t attrlen,
@@ -3818,13 +3814,13 @@ static int decode_getfattr(struct xdr_stream *xdr, struct nfs_fattr *fattr,
fattr->valid |= status;
status = decode_attr_owner(xdr, bitmap, server->nfs_client,
- &fattr->uid, may_sleep);
+ &fattr->user_name);
if (status < 0)
goto xdr_error;
fattr->valid |= status;
status = decode_attr_group(xdr, bitmap, server->nfs_client,
- &fattr->gid, may_sleep);
+ &fattr->group_name);
if (status < 0)
goto xdr_error;
fattr->valid |= status;
@@ -4757,8 +4753,7 @@ static int nfs4_xdr_dec_open_downgrade(struct rpc_rqst *rqstp, __be32 *p, struct
status = decode_open_downgrade(&xdr, res);
if (status != 0)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -4785,8 +4780,7 @@ static int nfs4_xdr_dec_access(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_ac
status = decode_access(&xdr, res);
if (status != 0)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -4813,8 +4807,7 @@ static int nfs4_xdr_dec_lookup(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_lo
goto out;
if ((status = decode_getfh(&xdr, res->fh)) != 0)
goto out;
- status = decode_getfattr(&xdr, res->fattr, res->server
- ,!RPC_IS_ASYNC(rqstp->rq_task));
+ status = decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -4838,8 +4831,7 @@ static int nfs4_xdr_dec_lookup_root(struct rpc_rqst *rqstp, __be32 *p, struct nf
if ((status = decode_putrootfh(&xdr)) != 0)
goto out;
if ((status = decode_getfh(&xdr, res->fh)) == 0)
- status = decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ status = decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -4864,8 +4856,7 @@ static int nfs4_xdr_dec_remove(struct rpc_rqst *rqstp, __be32 *p, struct nfs_rem
goto out;
if ((status = decode_remove(&xdr, &res->cinfo)) != 0)
goto out;
- decode_getfattr(&xdr, res->dir_attr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->dir_attr, res->server);
out:
return status;
}
@@ -4895,13 +4886,11 @@ static int nfs4_xdr_dec_rename(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_re
if ((status = decode_rename(&xdr, &res->old_cinfo, &res->new_cinfo)) != 0)
goto out;
/* Current FH is target directory */
- if (decode_getfattr(&xdr, res->new_fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task)) != 0)
+ if (decode_getfattr(&xdr, res->new_fattr, res->server) != 0)
goto out;
if ((status = decode_restorefh(&xdr)) != 0)
goto out;
- decode_getfattr(&xdr, res->old_fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->old_fattr, res->server);
out:
return status;
}
@@ -4934,13 +4923,11 @@ static int nfs4_xdr_dec_link(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_link
* Note order: OP_LINK leaves the directory as the current
* filehandle.
*/
- if (decode_getfattr(&xdr, res->dir_attr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task)) != 0)
+ if (decode_getfattr(&xdr, res->dir_attr, res->server) != 0)
goto out;
if ((status = decode_restorefh(&xdr)) != 0)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -4969,13 +4956,11 @@ static int nfs4_xdr_dec_create(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_cr
goto out;
if ((status = decode_getfh(&xdr, res->fh)) != 0)
goto out;
- if (decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task)) != 0)
+ if (decode_getfattr(&xdr, res->fattr, res->server) != 0)
goto out;
if ((status = decode_restorefh(&xdr)) != 0)
goto out;
- decode_getfattr(&xdr, res->dir_fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->dir_fattr, res->server);
out:
return status;
}
@@ -5007,8 +4992,7 @@ static int nfs4_xdr_dec_getattr(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_g
status = decode_putfh(&xdr);
if (status)
goto out;
- status = decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ status = decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -5115,8 +5099,7 @@ static int nfs4_xdr_dec_close(struct rpc_rqst *rqstp, __be32 *p, struct nfs_clos
* an ESTALE error. Shouldn't be a problem,
* though, since fattr->valid will remain unset.
*/
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -5148,13 +5131,11 @@ static int nfs4_xdr_dec_open(struct rpc_rqst *rqstp, __be32 *p, struct nfs_openr
goto out;
if (decode_getfh(&xdr, &res->fh) != 0)
goto out;
- if (decode_getfattr(&xdr, res->f_attr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task)) != 0)
+ if (decode_getfattr(&xdr, res->f_attr, res->server) != 0)
goto out;
if (decode_restorefh(&xdr) != 0)
goto out;
- decode_getfattr(&xdr, res->dir_attr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->dir_attr, res->server);
out:
return status;
}
@@ -5202,8 +5183,7 @@ static int nfs4_xdr_dec_open_noattr(struct rpc_rqst *rqstp, __be32 *p, struct nf
status = decode_open(&xdr, res);
if (status)
goto out;
- decode_getfattr(&xdr, res->f_attr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->f_attr, res->server);
out:
return status;
}
@@ -5230,8 +5210,7 @@ static int nfs4_xdr_dec_setattr(struct rpc_rqst *rqstp, __be32 *p, struct nfs_se
status = decode_setattr(&xdr);
if (status)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -5418,8 +5397,7 @@ static int nfs4_xdr_dec_write(struct rpc_rqst *rqstp, __be32 *p, struct nfs_writ
status = decode_write(&xdr, res);
if (status)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
if (!status)
status = res->count;
out:
@@ -5448,8 +5426,7 @@ static int nfs4_xdr_dec_commit(struct rpc_rqst *rqstp, __be32 *p, struct nfs_wri
status = decode_commit(&xdr, res);
if (status)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -5615,8 +5592,7 @@ static int nfs4_xdr_dec_delegreturn(struct rpc_rqst *rqstp, __be32 *p, struct nf
status = decode_delegreturn(&xdr);
if (status != 0)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -5644,8 +5620,7 @@ static int nfs4_xdr_dec_fs_locations(struct rpc_rqst *req, __be32 *p,
goto out;
xdr_enter_page(&xdr, PAGE_SIZE);
status = decode_getfattr(&xdr, &res->fs_locations->fattr,
- res->fs_locations->server,
- !RPC_IS_ASYNC(req->rq_task));
+ res->fs_locations->server);
out:
return status;
}
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 508f8cf..6ce7a08 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -369,6 +369,10 @@ extern struct nfs_fattr *nfs_alloc_fattr(void);
static inline void nfs_free_fattr(const struct nfs_fattr *fattr)
{
+ if (!fattr)
+ return;
+ kfree(fattr->user_name);
+ kfree(fattr->group_name);
kfree(fattr);
}
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index fc46192..fc58d5d 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -27,10 +27,17 @@ static inline int nfs_fsid_equal(const struct nfs_fsid *a, const struct nfs_fsid
return a->major == b->major && a->minor == b->minor;
}
+struct nfs_name {
+ size_t len;
+ char buf[];
+};
+
struct nfs_fattr {
unsigned int valid; /* which fields are valid */
umode_t mode;
__u32 nlink;
+ struct nfs_name *user_name;
+ struct nfs_name *group_name;
__u32 uid;
__u32 gid;
dev_t rdev;
^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: Empty core dumps on NFSv4 mounts
2011-12-05 20:19 ` Chuck Lever
@ 2011-12-05 20:21 ` Myklebust, Trond
0 siblings, 0 replies; 9+ messages in thread
From: Myklebust, Trond @ 2011-12-05 20:21 UTC (permalink / raw)
To: Chuck Lever; +Cc: Linux NFS Mailing List
> -----Original Message-----
> From: Chuck Lever [mailto:chuck.lever@oracle.com]
> Sent: Monday, December 05, 2011 3:19 PM
> To: Myklebust, Trond
> Cc: Linux NFS Mailing List
> Subject: Re: Empty core dumps on NFSv4 mounts
>
>
> On Dec 5, 2011, at 3:14 PM, Myklebust, Trond wrote:
>
> > No. There have been no kernel idmapper changes since then.
> >
> > The correct thing to do here is to cache the string name in struct
> > nfs_fattr, and then do the upcall from the caller of OPEN. I believe
> > that Arnaud wrote a patch to do this, however the whole pNFS merge
got
> > in my way of reviewing this. Feel free to take a look at what he
did,
> > and post comments...
>
> We're looking at his patch, from July 2010, now. It calls kmalloc()
in the XDR
> decoder. Is there a newer version?
Not as far as I know, and I agree that calling kmalloc in the XDR is a
problem... If, however, we can accept that it will occasionally fail,
then perhaps kmalloc(GFP_NOWAIT) might be acceptable?
Cheers
Trond
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Empty core dumps on NFSv4 mounts
2011-12-05 20:14 ` Myklebust, Trond
@ 2011-12-05 20:19 ` Chuck Lever
2011-12-05 20:21 ` Myklebust, Trond
0 siblings, 1 reply; 9+ messages in thread
From: Chuck Lever @ 2011-12-05 20:19 UTC (permalink / raw)
To: Myklebust, Trond; +Cc: Linux NFS Mailing List
On Dec 5, 2011, at 3:14 PM, Myklebust, Trond wrote:
> No. There have been no kernel idmapper changes since then.
>
> The correct thing to do here is to cache the string name in struct
> nfs_fattr, and then do the upcall from the caller of OPEN. I believe
> that Arnaud wrote a patch to do this, however the whole pNFS merge got
> in my way of reviewing this. Feel free to take a look at what he did,
> and post comments...
We're looking at his patch, from July 2010, now. It calls kmalloc() in the XDR decoder. Is there a newer version?
> Cheers
> Trond
>
>> -----Original Message-----
>> From: Chuck Lever [mailto:chuck.lever@oracle.com]
>> Sent: Monday, December 05, 2011 3:10 PM
>> To: Myklebust, Trond
>> Cc: Linux NFS Mailing List
>> Subject: Empty core dumps on NFSv4 mounts
>>
>> Bump.
>>
>> http://comments.gmane.org/gmane.linux.nfs/33390
>>
>> We have evidence that this is still occurring as late as 3.0. Have
> recent
>> idmapper changes addressed this issue?
>>
>> --
>> Chuck Lever
>> chuck[dot]lever[at]oracle[dot]com
>>
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: Empty core dumps on NFSv4 mounts
2011-12-05 20:09 Empty core dumps on NFSv4 mounts Chuck Lever
@ 2011-12-05 20:14 ` Myklebust, Trond
2011-12-05 20:19 ` Chuck Lever
0 siblings, 1 reply; 9+ messages in thread
From: Myklebust, Trond @ 2011-12-05 20:14 UTC (permalink / raw)
To: Chuck Lever; +Cc: Linux NFS Mailing List
No. There have been no kernel idmapper changes since then.
The correct thing to do here is to cache the string name in struct
nfs_fattr, and then do the upcall from the caller of OPEN. I believe
that Arnaud wrote a patch to do this, however the whole pNFS merge got
in my way of reviewing this. Feel free to take a look at what he did,
and post comments...
Cheers
Trond
> -----Original Message-----
> From: Chuck Lever [mailto:chuck.lever@oracle.com]
> Sent: Monday, December 05, 2011 3:10 PM
> To: Myklebust, Trond
> Cc: Linux NFS Mailing List
> Subject: Empty core dumps on NFSv4 mounts
>
> Bump.
>
> http://comments.gmane.org/gmane.linux.nfs/33390
>
> We have evidence that this is still occurring as late as 3.0. Have
recent
> idmapper changes addressed this issue?
>
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Empty core dumps on NFSv4 mounts
@ 2011-12-05 20:09 Chuck Lever
2011-12-05 20:14 ` Myklebust, Trond
0 siblings, 1 reply; 9+ messages in thread
From: Chuck Lever @ 2011-12-05 20:09 UTC (permalink / raw)
To: Trond Myklebust; +Cc: Linux NFS Mailing List
Bump.
http://comments.gmane.org/gmane.linux.nfs/33390
We have evidence that this is still occurring as late as 3.0. Have recent idmapper changes addressed this issue?
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-12-05 20:21 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-07-02 8:01 Empty core dumps on NFSv4 mounts Arnaud Giersch
2010-07-02 13:03 ` Trond Myklebust
2010-07-05 13:28 ` [PATCH/RFC 2.6.35-rc4] NFSv4: Don't do idmapper upcalls during XDR decode Arnaud Giersch
2010-08-12 10:15 ` [PATCH 2.6.35] " Arnaud Giersch
2010-09-21 11:17 ` [PATCH resend] " Arnaud Giersch
2011-12-05 20:09 Empty core dumps on NFSv4 mounts Chuck Lever
2011-12-05 20:14 ` Myklebust, Trond
2011-12-05 20:19 ` Chuck Lever
2011-12-05 20:21 ` Myklebust, Trond
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.