Hi-
I'm sure a hunk here or there is no longer necessary. This is the set
of minor clean ups that have been hanging around in my tree for about
a year, as part of a patch series to support NFSv4.0 migration.
Please consider for inclusion in 3.4.
---
Chuck Lever (13):
NFS: Request fh_expire_type attribute in "server caps" operation
NFS: Introduce NFS_ATTR_FATTR_V4_LOCATIONS
NFS: Simplify arguments of encode_renew()
NFS: Save root file handle in nfs_server
NFS: Add a client-side function to display NFS file handles
SUNRPC: Add API to acquire source address
NFS: Reduce debugging noise from encode_compound_hdr
NFS: Add debugging messages to NFSv4's CLOSE procedure
NFS: Clean up debugging in decode_pathname()
nfs: Clean up debugging in nfs_follow_mountpoint()
SUNRPC: Use KERN_DEFAULT for debugging printk's
NFS: Fix comparison signage warnings with slot ID computations
NFS: Make nfs_cache_array.size a signed integer
fs/nfs/client.c | 1
fs/nfs/dir.c | 2 -
fs/nfs/getroot.c | 5 +
fs/nfs/inode.c | 45 +++++++++++++
fs/nfs/namespace.c | 5 +
fs/nfs/nfs4proc.c | 24 ++++---
fs/nfs/nfs4xdr.c | 54 ++++++++++++---
include/linux/nfs_fs.h | 14 ++++
include/linux/nfs_fs_sb.h | 4 +
include/linux/nfs_xdr.h | 8 +-
include/linux/sunrpc/clnt.h | 1
include/linux/sunrpc/debug.h | 6 +-
net/sunrpc/clnt.c | 149 ++++++++++++++++++++++++++++++++++++++++++
13 files changed, 290 insertions(+), 28 deletions(-)
--
Chuck Lever
CLOSE is new with NFSv4. Sometimes it's important to know the timing
of this operation compared to things like lease renewal.
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfs/nfs4proc.c | 11 ++++++++---
1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 5ff9c5a..5629858 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1947,6 +1947,7 @@ static void nfs4_close_done(struct rpc_task *task, void *data)
struct nfs4_state *state = calldata->state;
struct nfs_server *server = NFS_SERVER(calldata->inode);
+ dprintk("%s: begin!\n", __func__);
if (!nfs4_sequence_done(task, &calldata->res.seq_res))
return;
/* hmm. we are done with the inode, and in the process of freeing
@@ -1974,6 +1975,7 @@ static void nfs4_close_done(struct rpc_task *task, void *data)
}
nfs_release_seqid(calldata->arg.seqid);
nfs_refresh_inode(calldata->inode, calldata->res.fattr);
+ dprintk("%s: done, ret = %d!\n", __func__, task->tk_status);
}
static void nfs4_close_prepare(struct rpc_task *task, void *data)
@@ -1982,6 +1984,7 @@ static void nfs4_close_prepare(struct rpc_task *task, void *data)
struct nfs4_state *state = calldata->state;
int call_close = 0;
+ dprintk("%s: begin!\n", __func__);
if (nfs_wait_on_sequence(calldata->arg.seqid, task) != 0)
return;
@@ -2006,7 +2009,7 @@ static void nfs4_close_prepare(struct rpc_task *task, void *data)
if (!call_close) {
/* Note: exit _without_ calling nfs4_close_done */
task->tk_action = NULL;
- return;
+ goto out;
}
if (calldata->arg.fmode == 0) {
@@ -2015,7 +2018,7 @@ static void nfs4_close_prepare(struct rpc_task *task, void *data)
pnfs_roc_drain(calldata->inode, &calldata->roc_barrier)) {
rpc_sleep_on(&NFS_SERVER(calldata->inode)->roc_rpcwaitq,
task, NULL);
- return;
+ goto out;
}
}
@@ -2024,8 +2027,10 @@ static void nfs4_close_prepare(struct rpc_task *task, void *data)
if (nfs4_setup_sequence(NFS_SERVER(calldata->inode),
&calldata->arg.seq_args, &calldata->res.seq_res,
1, task))
- return;
+ goto out;
rpc_call_start(task);
+out:
+ dprintk("%s: done!\n", __func__);
}
static const struct rpc_call_ops nfs4_close_ops = {
To make nfs4proc.c compile cleanly, address the following compiler
warnings:
fs/nfs/nfs4proc.c: In function ‘nfs4_free_slot’:
fs/nfs/nfs4proc.c:371:2: warning: comparison between signed and
unsigned integer expressions [-Wsign-compare]
fs/nfs/nfs4proc.c: In function ‘nfs4_reset_slot_table’:
fs/nfs/nfs4proc.c:5000:15: warning: comparison between signed and
unsigned integer expressions [-Wsign-compare]
fs/nfs/nfs4proc.c: In function ‘nfs4_init_slot_table’:
fs/nfs/nfs4proc.c:5064:2: warning: comparison between signed and
unsigned integer expressions [-Wsign-compare]
Why is max_reqs always u32, but nfs4_slot_table.max_slots a signed
integer?
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfs/nfs4proc.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index d9f4d78..5ff9c5a 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -368,7 +368,7 @@ nfs4_free_slot(struct nfs4_slot_table *tbl, struct nfs4_slot *free_slot)
int free_slotid = free_slot - tbl->slots;
int slotid = free_slotid;
- BUG_ON(slotid < 0 || slotid >= NFS4_MAX_SLOT_TABLE);
+ BUG_ON(slotid < 0 || slotid >= (int)NFS4_MAX_SLOT_TABLE);
/* clear used bit in bitmap */
__clear_bit(slotid, tbl->used_slots);
@@ -4997,7 +4997,7 @@ static int nfs4_reset_slot_table(struct nfs4_slot_table *tbl, u32 max_reqs,
max_reqs, tbl->max_slots);
/* Does the newly negotiated max_reqs match the existing slot table? */
- if (max_reqs != tbl->max_slots) {
+ if (max_reqs != (u32)tbl->max_slots) {
ret = -ENOMEM;
new = kmalloc(max_reqs * sizeof(struct nfs4_slot),
GFP_NOFS);
@@ -5056,7 +5056,7 @@ static void nfs4_destroy_slot_tables(struct nfs4_session *session)
* Initialize slot table
*/
static int nfs4_init_slot_table(struct nfs4_slot_table *tbl,
- int max_slots, int ivalue)
+ u32 max_slots, int ivalue)
{
struct nfs4_slot *slot;
int ret = -ENOMEM;
NFSv4.0 clients must send endpoint information for their callback
service to NFSv4.0 servers during their first contact with a server.
Traditionally on Linux, user space provides the callback endpoint IP
address via the "clientaddr=" mount option.
During an NFSv4 migration event, it is possible that an FSID may be
migrated to a destination server that is accessible via a different
source IP address than the source server was. The client must update
callback endpoint information on the destination server so that it can
maintain leases and allow delegation.
Without a new "clientaddr=" option from user space, however, the
kernel itself must construct an appropriate IP address for the
callback update. Provide an API in the RPC client for upper layer
RPC consumers to acquire a source address for a remote.
The mechanism used by the mount.nfs command is copied: set up a
connected UDP socket to the designated remote, then scrape the source
address off the socket. We are careful to select the correct network
namespace when setting up the temporary UDP socket.
Signed-off-by: Chuck Lever <[email protected]>
---
include/linux/sunrpc/clnt.h | 1
net/sunrpc/clnt.c | 149 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 150 insertions(+), 0 deletions(-)
diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index 3d8f9c4..6a869b4 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -160,6 +160,7 @@ size_t rpc_max_payload(struct rpc_clnt *);
void rpc_force_rebind(struct rpc_clnt *);
size_t rpc_peeraddr(struct rpc_clnt *, struct sockaddr *, size_t);
const char *rpc_peeraddr2str(struct rpc_clnt *, enum rpc_display_format_t);
+int rpc_localaddr(struct rpc_clnt *, struct sockaddr *, size_t);
size_t rpc_ntop(const struct sockaddr *, char *, const size_t);
size_t rpc_pton(const char *, const size_t,
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index f0268ea..ebd2192 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -806,6 +806,155 @@ const char *rpc_peeraddr2str(struct rpc_clnt *clnt,
}
EXPORT_SYMBOL_GPL(rpc_peeraddr2str);
+static const struct sockaddr_in rpc_inaddr_loopback = {
+ .sin_family = AF_INET,
+ .sin_addr.s_addr = htonl(INADDR_ANY),
+};
+
+static const struct sockaddr_in6 rpc_in6addr_loopback = {
+ .sin6_family = AF_INET6,
+ .sin6_addr = IN6ADDR_ANY_INIT,
+};
+
+/*
+ * Try a getsockname() on a connected datagram socket. Using a
+ * connected datagram socket prevents leaving a socket in TIME_WAIT.
+ * This conserves the ephemeral port number space.
+ *
+ * Returns zero and fills in "buf" if successful; otherwise, a
+ * negative errno is returned.
+ */
+static int rpc_sockname(struct net *net, struct sockaddr *sap, size_t salen,
+ struct sockaddr *buf, int buflen)
+{
+ struct socket *sock;
+ int err;
+
+ err = __sock_create(net, sap->sa_family,
+ SOCK_DGRAM, IPPROTO_UDP, &sock, 1);
+ if (err < 0) {
+ dprintk("RPC: can't create UDP socket (%d)\n", err);
+ goto out;
+ }
+
+ switch (sap->sa_family) {
+ case AF_INET:
+ err = kernel_bind(sock,
+ (struct sockaddr *)&rpc_inaddr_loopback,
+ sizeof(rpc_inaddr_loopback));
+ break;
+ case AF_INET6:
+ err = kernel_bind(sock,
+ (struct sockaddr *)&rpc_in6addr_loopback,
+ sizeof(rpc_in6addr_loopback));
+ break;
+ default:
+ err = -EAFNOSUPPORT;
+ goto out;
+ }
+ if (err < 0) {
+ dprintk("RPC: can't bind UDP socket (%d)\n", err);
+ goto out_release;
+ }
+
+ err = kernel_connect(sock, sap, salen, 0);
+ if (err < 0) {
+ dprintk("RPC: can't connect UDP socket (%d)\n", err);
+ goto out_release;
+ }
+
+ err = kernel_getsockname(sock, buf, &buflen);
+ if (err < 0) {
+ dprintk("RPC: getsockname failed (%d)\n", err);
+ goto out_release;
+ }
+
+ err = 0;
+ if (buf->sa_family == AF_INET6) {
+ struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)buf;
+ sin6->sin6_scope_id = 0;
+ }
+ dprintk("RPC: %s succeeded\n", __func__);
+
+out_release:
+ sock_release(sock);
+out:
+ return err;
+}
+
+/*
+ * Scraping a connected socket failed, so we don't have a useable
+ * local address. Fallback: generate an address that will prevent
+ * the server from calling us back.
+ *
+ * Returns zero and fills in "buf" if successful; otherwise, a
+ * negative errno is returned.
+ */
+static int rpc_anyaddr(int family, struct sockaddr *buf, size_t buflen)
+{
+ switch (family) {
+ case AF_INET:
+ if (buflen < sizeof(rpc_inaddr_loopback))
+ return -EINVAL;
+ memcpy(buf, &rpc_inaddr_loopback,
+ sizeof(rpc_inaddr_loopback));
+ break;
+ case AF_INET6:
+ if (buflen < sizeof(rpc_in6addr_loopback))
+ return -EINVAL;
+ memcpy(buf, &rpc_in6addr_loopback,
+ sizeof(rpc_in6addr_loopback));
+ default:
+ dprintk("RPC: %s: address family not supported\n",
+ __func__);
+ return -EAFNOSUPPORT;
+ }
+ dprintk("RPC: %s: succeeded\n", __func__);
+ return 0;
+}
+
+/**
+ * rpc_localaddr - discover local endpoint address for an RPC client
+ * @clnt: RPC client structure
+ * @buf: target buffer
+ * @buflen: size of target buffer, in bytes
+ *
+ * Returns zero and fills in "buf" and "buflen" if successful;
+ * otherwise, a negative errno is returned.
+ *
+ * This works even if the underlying transport is not currently connected,
+ * or if the upper layer never previously provided a source address.
+ *
+ * The result of this function call is transient: multiple calls in
+ * succession may give different results, depending on how local
+ * networking configuration changes over time.
+ */
+int rpc_localaddr(struct rpc_clnt *clnt, struct sockaddr *buf, size_t buflen)
+{
+ struct sockaddr_storage address;
+ struct sockaddr *sap = (struct sockaddr *)&address;
+ struct rpc_xprt *xprt;
+ struct net *net;
+ size_t salen;
+ int err;
+
+ rcu_read_lock();
+ xprt = rcu_dereference(clnt->cl_xprt);
+ salen = xprt->addrlen;
+ memcpy(sap, &xprt->addr, salen);
+ net = get_net(xprt->xprt_net);
+ rcu_read_unlock();
+
+ rpc_set_port(sap, 0);
+ err = rpc_sockname(net, sap, salen, buf, buflen);
+ put_net(net);
+ if (err != 0)
+ /* Couldn't discover local address, return ANYADDR */
+ return rpc_anyaddr(sap->sa_family, buf, buflen);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(rpc_localaddr);
+
void
rpc_setbufsize(struct rpc_clnt *clnt, unsigned int sndsize, unsigned int rcvsize)
{
On Feb 16, 2012, at 3:07 PM, Myklebust, Trond wrote:
> On Wed, 2012-02-15 at 16:35 -0500, Chuck Lever wrote:
>> NFSv4.0 clients must send endpoint information for their callback
>> service to NFSv4.0 servers during their first contact with a server.
>> Traditionally on Linux, user space provides the callback endpoint IP
>> address via the "clientaddr=" mount option.
>>
>> During an NFSv4 migration event, it is possible that an FSID may be
>> migrated to a destination server that is accessible via a different
>> source IP address than the source server was. The client must update
>> callback endpoint information on the destination server so that it can
>> maintain leases and allow delegation.
>>
>> Without a new "clientaddr=" option from user space, however, the
>> kernel itself must construct an appropriate IP address for the
>> callback update. Provide an API in the RPC client for upper layer
>> RPC consumers to acquire a source address for a remote.
>>
>> The mechanism used by the mount.nfs command is copied: set up a
>> connected UDP socket to the designated remote, then scrape the source
>> address off the socket. We are careful to select the correct network
>> namespace when setting up the temporary UDP socket.
>>
>> Signed-off-by: Chuck Lever <[email protected]>
>> ---
>
> This doesn't appear to apply (nor do several of the debugging patches).
> Was it written against the nfs-for-next branch?
No, against 3.2. These were intended for review only, since I wasn't sure which branch to port them to. Now I know.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
T24gV2VkLCAyMDEyLTAyLTE1IGF0IDE2OjM1IC0wNTAwLCBDaHVjayBMZXZlciB3cm90ZToNCj4g
VG8gbWFrZSBuZnM0cHJvYy5jIGNvbXBpbGUgY2xlYW5seSwgYWRkcmVzcyB0aGUgZm9sbG93aW5n
IGNvbXBpbGVyDQo+IHdhcm5pbmdzOg0KPiANCj4gZnMvbmZzL25mczRwcm9jLmM6IEluIGZ1bmN0
aW9uIOKAmG5mczRfZnJlZV9zbG904oCZOg0KPiBmcy9uZnMvbmZzNHByb2MuYzozNzE6Mjogd2Fy
bmluZzogY29tcGFyaXNvbiBiZXR3ZWVuIHNpZ25lZCBhbmQNCj4gCQl1bnNpZ25lZCBpbnRlZ2Vy
IGV4cHJlc3Npb25zIFstV3NpZ24tY29tcGFyZV0NCj4gZnMvbmZzL25mczRwcm9jLmM6IEluIGZ1
bmN0aW9uIOKAmG5mczRfcmVzZXRfc2xvdF90YWJsZeKAmToNCj4gZnMvbmZzL25mczRwcm9jLmM6
NTAwMDoxNTogd2FybmluZzogY29tcGFyaXNvbiBiZXR3ZWVuIHNpZ25lZCBhbmQNCj4gCQl1bnNp
Z25lZCBpbnRlZ2VyIGV4cHJlc3Npb25zIFstV3NpZ24tY29tcGFyZV0NCj4gZnMvbmZzL25mczRw
cm9jLmM6IEluIGZ1bmN0aW9uIOKAmG5mczRfaW5pdF9zbG90X3RhYmxl4oCZOg0KPiBmcy9uZnMv
bmZzNHByb2MuYzo1MDY0OjI6IHdhcm5pbmc6IGNvbXBhcmlzb24gYmV0d2VlbiBzaWduZWQgYW5k
DQo+IAkJdW5zaWduZWQgaW50ZWdlciBleHByZXNzaW9ucyBbLVdzaWduLWNvbXBhcmVdDQo+IA0K
PiBXaHkgaXMgbWF4X3JlcXMgYWx3YXlzIHUzMiwgYnV0IG5mczRfc2xvdF90YWJsZS5tYXhfc2xv
dHMgYSBzaWduZWQNCj4gaW50ZWdlcj8NCj4gDQoNClRoZXJlIGlzIGFscmVhZHkgYSBmaXggZm9y
IHRoaXMgaW4gdGhlIG5mcy1mb3ItbmV4dCBicmFuY2guDQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0
DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXINCg0KTmV0QXBwDQpUcm9uZC5NeWtsZWJ1c3RA
bmV0YXBwLmNvbQ0Kd3d3Lm5ldGFwcC5jb20NCg0K
Clean up: Fix a debugging message which had an obsolete function name
in it (nfs_follow_mountpoint).
Introduced by commit 36d43a43 "NFS: Use d_automount() rather than
abusing follow_link()" (January 14, 2011)
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfs/namespace.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
index 8102391..1807866 100644
--- a/fs/nfs/namespace.c
+++ b/fs/nfs/namespace.c
@@ -276,7 +276,10 @@ out:
nfs_free_fattr(fattr);
nfs_free_fhandle(fh);
out_nofree:
- dprintk("<-- nfs_follow_mountpoint() = %p\n", mnt);
+ if (IS_ERR(mnt))
+ dprintk("<-- %s(): error %ld\n", __func__, PTR_ERR(mnt));
+ else
+ dprintk("<-- %s() = %p\n", __func__, mnt);
return mnt;
}
Chuck Lever [[email protected]] wrote:
> + rcu_read_lock();
> + xprt = rcu_dereference(clnt->cl_xprt);
> + salen = xprt->addrlen;
> + memcpy(sap, &xprt->addr, salen);
> + net = get_net(xprt->xprt_net);
> + rcu_read_unlock();
Did I miss a patch that made cl_xprt go away unless accessed under RCU?
Thanks, Malahal.
T24gV2VkLCAyMDEyLTAyLTE1IGF0IDE2OjM1IC0wNTAwLCBDaHVjayBMZXZlciB3cm90ZToNCj4g
TkZTdjQuMCBjbGllbnRzIG11c3Qgc2VuZCBlbmRwb2ludCBpbmZvcm1hdGlvbiBmb3IgdGhlaXIg
Y2FsbGJhY2sNCj4gc2VydmljZSB0byBORlN2NC4wIHNlcnZlcnMgZHVyaW5nIHRoZWlyIGZpcnN0
IGNvbnRhY3Qgd2l0aCBhIHNlcnZlci4NCj4gVHJhZGl0aW9uYWxseSBvbiBMaW51eCwgdXNlciBz
cGFjZSBwcm92aWRlcyB0aGUgY2FsbGJhY2sgZW5kcG9pbnQgSVANCj4gYWRkcmVzcyB2aWEgdGhl
ICJjbGllbnRhZGRyPSIgbW91bnQgb3B0aW9uLg0KPiANCj4gRHVyaW5nIGFuIE5GU3Y0IG1pZ3Jh
dGlvbiBldmVudCwgaXQgaXMgcG9zc2libGUgdGhhdCBhbiBGU0lEIG1heSBiZQ0KPiBtaWdyYXRl
ZCB0byBhIGRlc3RpbmF0aW9uIHNlcnZlciB0aGF0IGlzIGFjY2Vzc2libGUgdmlhIGEgZGlmZmVy
ZW50DQo+IHNvdXJjZSBJUCBhZGRyZXNzIHRoYW4gdGhlIHNvdXJjZSBzZXJ2ZXIgd2FzLiAgVGhl
IGNsaWVudCBtdXN0IHVwZGF0ZQ0KPiBjYWxsYmFjayBlbmRwb2ludCBpbmZvcm1hdGlvbiBvbiB0
aGUgZGVzdGluYXRpb24gc2VydmVyIHNvIHRoYXQgaXQgY2FuDQo+IG1haW50YWluIGxlYXNlcyBh
bmQgYWxsb3cgZGVsZWdhdGlvbi4NCj4gDQo+IFdpdGhvdXQgYSBuZXcgImNsaWVudGFkZHI9IiBv
cHRpb24gZnJvbSB1c2VyIHNwYWNlLCBob3dldmVyLCB0aGUNCj4ga2VybmVsIGl0c2VsZiBtdXN0
IGNvbnN0cnVjdCBhbiBhcHByb3ByaWF0ZSBJUCBhZGRyZXNzIGZvciB0aGUNCj4gY2FsbGJhY2sg
dXBkYXRlLiAgUHJvdmlkZSBhbiBBUEkgaW4gdGhlIFJQQyBjbGllbnQgZm9yIHVwcGVyIGxheWVy
DQo+IFJQQyBjb25zdW1lcnMgdG8gYWNxdWlyZSBhIHNvdXJjZSBhZGRyZXNzIGZvciBhIHJlbW90
ZS4NCj4gDQo+IFRoZSBtZWNoYW5pc20gdXNlZCBieSB0aGUgbW91bnQubmZzIGNvbW1hbmQgaXMg
Y29waWVkOiBzZXQgdXAgYQ0KPiBjb25uZWN0ZWQgVURQIHNvY2tldCB0byB0aGUgZGVzaWduYXRl
ZCByZW1vdGUsIHRoZW4gc2NyYXBlIHRoZSBzb3VyY2UNCj4gYWRkcmVzcyBvZmYgdGhlIHNvY2tl
dC4gIFdlIGFyZSBjYXJlZnVsIHRvIHNlbGVjdCB0aGUgY29ycmVjdCBuZXR3b3JrDQo+IG5hbWVz
cGFjZSB3aGVuIHNldHRpbmcgdXAgdGhlIHRlbXBvcmFyeSBVRFAgc29ja2V0Lg0KPiANCj4gU2ln
bmVkLW9mZi1ieTogQ2h1Y2sgTGV2ZXIgPGNodWNrLmxldmVyQG9yYWNsZS5jb20+DQo+IC0tLQ0K
DQpUaGlzIGRvZXNuJ3QgYXBwZWFyIHRvIGFwcGx5IChub3IgZG8gc2V2ZXJhbCBvZiB0aGUgZGVi
dWdnaW5nIHBhdGNoZXMpLg0KV2FzIGl0IHdyaXR0ZW4gYWdhaW5zdCB0aGUgbmZzLWZvci1uZXh0
IGJyYW5jaD8NCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRh
aW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cubmV0YXBwLmNv
bQ0KDQo=
When NFSDBG_XDR debugging is enabled, I see a lot of
encode_compound: tag=
on the console. This is noise if the tag is empty. Some might argue
that it is noise even if the tag isn't empty...
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfs/nfs4xdr.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 606852a..2fb129d 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -889,7 +889,9 @@ static void encode_compound_hdr(struct xdr_stream *xdr,
* but this is not required as a MUST for the server to do so. */
hdr->replen = RPC_REPHDRSIZE + auth->au_rslack + 3 + hdr->taglen;
- dprintk("encode_compound: tag=%.*s\n", (int)hdr->taglen, hdr->tag);
+ if (unlikely(hdr->taglen != 0))
+ dprintk("%s: tag=%.*s\n", __func__,
+ (int)hdr->taglen, hdr->tag);
BUG_ON(hdr->taglen > NFS4_MAXTAGLEN);
p = reserve_space(xdr, 4 + hdr->taglen + 8);
p = xdr_encode_opaque(p, hdr->tag, hdr->taglen);
On Feb 16, 2012, at 2:52 PM, Myklebust, Trond wrote:
> On Wed, 2012-02-15 at 16:35 -0500, Chuck Lever wrote:
>> To make nfs4proc.c compile cleanly, address the following compiler
>> warnings:
>>
>> fs/nfs/nfs4proc.c: In function ?nfs4_free_slot?:
>> fs/nfs/nfs4proc.c:371:2: warning: comparison between signed and
>> unsigned integer expressions [-Wsign-compare]
>> fs/nfs/nfs4proc.c: In function ?nfs4_reset_slot_table?:
>> fs/nfs/nfs4proc.c:5000:15: warning: comparison between signed and
>> unsigned integer expressions [-Wsign-compare]
>> fs/nfs/nfs4proc.c: In function ?nfs4_init_slot_table?:
>> fs/nfs/nfs4proc.c:5064:2: warning: comparison between signed and
>> unsigned integer expressions [-Wsign-compare]
>>
>> Why is max_reqs always u32, but nfs4_slot_table.max_slots a signed
>> integer?
>>
>
> There is already a fix for this in the nfs-for-next branch.
I'll drop mine.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Feb 16, 2012, at 3:04 PM, Myklebust, Trond wrote:
> On Wed, 2012-02-15 at 16:35 -0500, Chuck Lever wrote:
>> When NFSDBG_XDR debugging is enabled, I see a lot of
>>
>> encode_compound: tag=
>>
>> on the console. This is noise if the tag is empty. Some might argue
>> that it is noise even if the tag isn't empty...
>>
>> Signed-off-by: Chuck Lever <[email protected]>
>> ---
>>
>> fs/nfs/nfs4xdr.c | 4 +++-
>> 1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>> index 606852a..2fb129d 100644
>> --- a/fs/nfs/nfs4xdr.c
>> +++ b/fs/nfs/nfs4xdr.c
>> @@ -889,7 +889,9 @@ static void encode_compound_hdr(struct xdr_stream *xdr,
>> * but this is not required as a MUST for the server to do so. */
>> hdr->replen = RPC_REPHDRSIZE + auth->au_rslack + 3 + hdr->taglen;
>>
>> - dprintk("encode_compound: tag=%.*s\n", (int)hdr->taglen, hdr->tag);
>> + if (unlikely(hdr->taglen != 0))
>> + dprintk("%s: tag=%.*s\n", __func__,
>> + (int)hdr->taglen, hdr->tag);
>> BUG_ON(hdr->taglen > NFS4_MAXTAGLEN);
>> p = reserve_space(xdr, 4 + hdr->taglen + 8);
>> p = xdr_encode_opaque(p, hdr->tag, hdr->taglen);
>>
>
> Let's just comment out the tag dprintk for now. We don't use the tag
> field at all...
OK.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
Our dprintk() debugging facility doesn't specify any verbosity level
for it's printk() calls, but it should.
The default verbosity for printk's is KERN_DEFAULT. You might argue
that these are debugging printk's and thus the verbosity should be
KERN_DEBUG. That would mean that to see NFS and SUNRPC debugging
output an admin would also have to boost the syslog verbosity, which
would be insufferably noisy.
Signed-off-by: Chuck Lever <[email protected]>
---
include/linux/sunrpc/debug.h | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/include/linux/sunrpc/debug.h b/include/linux/sunrpc/debug.h
index c2786f2..e0927c2 100644
--- a/include/linux/sunrpc/debug.h
+++ b/include/linux/sunrpc/debug.h
@@ -51,7 +51,11 @@ extern unsigned int nlm_debug;
#undef ifdebug
#ifdef RPC_DEBUG
# define ifdebug(fac) if (unlikely(rpc_debug & RPCDBG_##fac))
-# define dfprintk(fac, args...) do { ifdebug(fac) printk(args); } while(0)
+# define dfprintk(fac, args...) \
+ do { \
+ ifdebug(fac) \
+ printk(KERN_DEFAULT args); \
+ } while (0)
# define RPC_IFDEBUG(x) x
#else
# define ifdebug(fac) if (0)
I noticed recently that decode_attr_fs_locations() is not generating
very pretty debugging output. The pathname components each appear on
a separate line of output, though that does not appear to be the
intended display behavior. The preferred way to generate continued
lines of output on the console is to use pr_cont().
Note that incoming pathname4 components contain a string that is not
necessarily NUL-terminated. I did actually see some trailing garbage
on the console. In addition to correcting the line continuation
problem, add a string precision format specifier to ensure that each
component string is displayed properly, and that vsnprintf() does
not Oops.
Someone pointed out that allowing incoming network data to possibly
generate a console line of unbounded length may not be such a good
idea. Since this output will rarely be enabled, and there is a hard
upper bound (NFS4_PATHNAME_MAXCOMPONENTS) in our implementation, this
is probably not a major concern.
It might be useful to additionally sanity-check the length of each
incoming component, however. RFC 3530bis15 does not suggest a maximum
number of UTF-8 characters per component for either the pathname4 or
component4 types. However, we could invent one that is appropriate
for our implementation.
Another possibility is to scrap all of this and print these pathnames
in upper layers after a reasonable amount of sanity checking in the
XDR layer. This would give us an opportunity to allocate a full
buffer so that the whole pathname would be output via a single
dprintk.
Introduced by commit 7aaa0b3b: "NFSv4: convert fs-locations-components
to conform to RFC3530," (June 9, 2006).
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfs/nfs4xdr.c | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 51ef966..606852a 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -3512,16 +3512,17 @@ static int decode_pathname(struct xdr_stream *xdr, struct nfs4_pathname *path)
n = be32_to_cpup(p);
if (n == 0)
goto root_path;
- dprintk("path ");
+ dprintk("pathname4: ");
path->ncomponents = 0;
while (path->ncomponents < n) {
struct nfs4_string *component = &path->components[path->ncomponents];
status = decode_opaque_inline(xdr, &component->len, &component->data);
if (unlikely(status != 0))
goto out_eio;
- if (path->ncomponents != n)
- dprintk("/");
- dprintk("%s", component->data);
+ if (unlikely(nfs_debug & NFSDBG_XDR))
+ pr_cont("%s%.*s ",
+ (path->ncomponents != n ? "/ " : ""),
+ component->len, component->data);
if (path->ncomponents < NFS4_PATHNAME_MAXCOMPONENTS)
path->ncomponents++;
else {
@@ -3530,14 +3531,13 @@ static int decode_pathname(struct xdr_stream *xdr, struct nfs4_pathname *path)
}
}
out:
- dprintk("\n");
return status;
root_path:
/* a root pathname is sent as a zero component4 */
path->ncomponents = 1;
path->components[0].len=0;
path->components[0].data=NULL;
- dprintk("path /\n");
+ dprintk("pathname4: /\n");
goto out;
out_eio:
dprintk(" status %d", status);
@@ -3559,7 +3559,7 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
status = 0;
if (unlikely(!(bitmap[0] & FATTR4_WORD0_FS_LOCATIONS)))
goto out;
- dprintk("%s: fsroot ", __func__);
+ dprintk("%s: fsroot:\n", __func__);
status = decode_pathname(xdr, &res->fs_path);
if (unlikely(status != 0))
goto out;
@@ -3580,7 +3580,7 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
m = be32_to_cpup(p);
loc->nservers = 0;
- dprintk("%s: servers ", __func__);
+ dprintk("%s: servers:\n", __func__);
while (loc->nservers < m) {
struct nfs4_string *server = &loc->servers[loc->nservers];
status = decode_opaque_inline(xdr, &server->len, &server->data);
On Feb 15, 2012, at 5:12 PM, Malahal Naineni wrote:
> Chuck Lever [[email protected]] wrote:
>> + rcu_read_lock();
>> + xprt = rcu_dereference(clnt->cl_xprt);
>> + salen = xprt->addrlen;
>> + memcpy(sap, &xprt->addr, salen);
>> + net = get_net(xprt->xprt_net);
>> + rcu_read_unlock();
>
> Did I miss a patch that made cl_xprt go away unless accessed under RCU?
Malahal, I re-ordered these so the series includes only clean ups. The RCU patch is now later.
Trond, would you like me to include the cl_xprt RCU patches in this "for 3.4" series too? If not, I can rework this hunk.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
Eliminate a number of implicit type casts in comparisons, and these
compiler warnings:
fs/nfs/dir.c: In function ‘nfs_readdir_clear_array’:
fs/nfs/dir.c:264:16: warning: comparison between signed and unsigned
integer expressions [-Wsign-compare]
fs/nfs/dir.c: In function ‘nfs_readdir_search_for_cookie’:
fs/nfs/dir.c:352:16: warning: comparison between signed and unsigned
integer expressions [-Wsign-compare]
fs/nfs/dir.c: In function ‘nfs_do_filldir’:
fs/nfs/dir.c:769:38: warning: comparison between signed and unsigned
integer expressions [-Wsign-compare]
fs/nfs/dir.c:780:9: warning: comparison between signed and unsigned
integer expressions [-Wsign-compare]
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfs/dir.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index ac28990..8661e13 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -207,7 +207,7 @@ struct nfs_cache_array_entry {
};
struct nfs_cache_array {
- unsigned int size;
+ int size;
int eof_index;
u64 last_cookie;
struct nfs_cache_array_entry array[0];