NFSD: Added fault injection

PATCH In review [email protected] 2 participants 8 messages Oct 24, 2011 – Nov 1, 2011

Subsystems (3)

Documentation NFS client NFS server

lore

Share & tools

lore

[email protected]

author

2011-10-24 11:21 UTC #0 lore raw

PATCH 1/3 NFSD: Added fault injection

From: Bryan Schumaker <[email protected]>

Fault injection on the NFS server makes it easier to test the client's state manager and recovery threads. Simulating errors on the server is easier than finding the right conditions that cause them naturally.

This patch uses debugfs to add a simple framework for fault injection to the server. This framework is a config option, and can be enabled through CONFIG_NFSD_FAULT_INJECTION. Assuming you have debugfs mounted to /sys/debug, a set of files will be created in /sys/debug/nfsd/. Writing to any of these files will cause the corresponding action and write a log entry to dmesg.

Changes in v5:
- Fill out inject_ops[] array without a macro
- Simplify debug messages and file creation

Changes in v4:
- Move fault injection function declarations to a new .h file
- Use the Makefile to selectively compile fault_injection.c
- Add copyright notices to top of files

Changes in v3:
- Code cleanup and better use of generic functions
- Allow the user to input the number of state objects to delete
- Remove "forget_everything()" since forgetting a client is basically the same thing

Changes in v2:
- Replaced "forget all state owners" with "forget all open owners"
- Include fs/nfsd/fault_inject.c in the patch

Signed-off-by Bryan Schumaker <[email protected]>

---
 fs/nfsd/Kconfig        |   10 ++++
 fs/nfsd/Makefile       |    1 +
 fs/nfsd/fault_inject.c |   90 +++++++++++++++++++++++++++++++++++++
 fs/nfsd/fault_inject.h |   28 ++++++++++++
 fs/nfsd/nfs4state.c    |  115 ++++++++++++++++++++++++++++++++++++++++++++++++
 fs/nfsd/nfsctl.c       |    7 +++
 6 files changed, 251 insertions(+), 0 deletions(-)
 create mode 100644 fs/nfsd/fault_inject.c
 create mode 100644 fs/nfsd/fault_inject.h

fs/nfsd/Kconfig +10 -0

index 10e6366..8df1ea4 100644
@@ -80,3 +80,13 @@ config NFSD_V4
 	  available from http://linux-nfs.org/.
 
 	  If unsure, say N.
+
+config NFSD_FAULT_INJECTION
+	bool "NFS server manual fault injection"
+	depends on NFSD_V4 && DEBUG_KERNEL
+	help
+	  This option enables support for manually injecting faults
+	  into the NFS server.  This is intended to be used for
+	  testing error recovery on the NFS client.
+
+	  If unsure, say N.

fs/nfsd/Makefile +1 -0

index 9b118ee..af32ef0 100644
@@ -6,6 +6,7 @@ obj-$(CONFIG_NFSD)	+= nfsd.o
 
 nfsd-y 			:= nfssvc.o nfsctl.o nfsproc.o nfsfh.o vfs.o \
 			   export.o auth.o lockd.o nfscache.o nfsxdr.o stats.o
+nfsd-$(CONFIG_NFSD_FAULT_INJECTION) += fault_inject.o
 nfsd-$(CONFIG_NFSD_V2_ACL) += nfs2acl.o
 nfsd-$(CONFIG_NFSD_V3)	+= nfs3proc.o nfs3xdr.o
 nfsd-$(CONFIG_NFSD_V3_ACL) += nfs3acl.o

fs/nfsd/fault_inject.c new file +90 -0

90 lines not on this page · raw message

fs/nfsd/fault_inject.h new file +28 -0

new file mode 100644
index 0000000..90bd057
@@ -0,0 +1,28 @@
+/*
+ * Copyright (c) 2011 Bryan Schumaker <[email protected]>
+ *
+ * Function definitions for fault injection
+ */
+
+#ifndef LINUX_NFSD_FAULT_INJECT_H
+#define LINUX_NFSD_FAULT_INJECT_H
+
+#ifdef CONFIG_NFSD_FAULT_INJECTION
+int nfsd_fault_inject_init(void);
+void nfsd_fault_inject_cleanup(void);
+void nfsd_forget_clients(u64);
+void nfsd_forget_locks(u64);
+void nfsd_forget_openowners(u64);
+void nfsd_forget_delegations(u64);
+void nfsd_recall_delegations(u64);
+#else /* CONFIG_NFSD_FAULT_INJECTION */
+static inline int nfsd_fault_inject_init(void) { return 0; }
+static inline void nfsd_fault_inject_cleanup(void) {}
+static inline void nfsd_forget_clients(u64 num) {}
+static inline void nfsd_forget_locks(u64 num) {}
+static inline void nfsd_forget_openowners(u64 num) {}
+static inline void nfsd_forget_delegations(u64 num) {}
+static inline void nfsd_recall_delegations(u64 num) {}
+#endif /* CONFIG_NFSD_FAULT_INJECTION */
+
+#endif /* LINUX_NFSD_FAULT_INJECT_H */

fs/nfsd/nfs4state.c +115 -0

121 lines not on this page · raw message

fs/nfsd/nfsctl.c +7 -0

index db34a58..e67f30c 100644
@@ -17,6 +17,7 @@
 #include "idmap.h"
 #include "nfsd.h"
 #include "cache.h"
+#include "fault_inject.h"
 
 /*
  *	We have a single directory with several nodes in it.

@@ -1130,6 +1131,9 @@ static int __init init_nfsd(void)
 	retval = nfs4_state_init(); /* nfs4 locking state */
 	if (retval)
 		return retval;
+	retval = nfsd_fault_inject_init(); /* nfsd fault injection controls */
+	if (retval)
+		goto out_cleanup_fault_injection;
 	nfsd_stat_init();	/* Statistics */
 	retval = nfsd_reply_cache_init();
 	if (retval)

@@ -1161,6 +1165,8 @@ out_free_cache:
 out_free_stat:
 	nfsd_stat_shutdown();
 	nfsd4_free_slabs();
+out_cleanup_fault_injection:
+	nfsd_fault_inject_cleanup();
 	return retval;
 }

@@ -1174,6 +1180,7 @@ static void __exit exit_nfsd(void)
 	nfsd_lockd_shutdown();
 	nfsd_idmap_shutdown();
 	nfsd4_free_slabs();
+	nfsd_fault_inject_cleanup();
 	unregister_filesystem(&nfsd_fs_type);
 }

Signature · 1 line

1.7.7

[email protected]

author

2011-10-24 11:21 UTC #1 lore raw

PATCH 2/3 NFSD: Added fault injection script

From: Bryan Schumaker <[email protected]>

This script provides a convenient way to use the NFSD fault injection framework. Fault injection writes to dmesg using the KERN_INFO flag, so this script will compare the before and after output of dmesg to show the user what happened

Signed-off-by Bryan Schumaker <[email protected]>

---
 tools/nfsd/inject_fault.sh |   49 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 49 insertions(+), 0 deletions(-)
 create mode 100755 tools/nfsd/inject_fault.sh

tools/nfsd/inject_fault.sh new file +49 -0

new file mode 100755
index 0000000..06a399a
@@ -0,0 +1,49 @@
+#!/bin/bash
+#
+# Copyright (c) 2011 Bryan Schumaker <[email protected]>
+#
+# Script for easier NFSD fault injection
+
+# Check that debugfs has been mounted
+DEBUGFS=`cat /proc/mounts | grep debugfs`
+if [ "$DEBUGFS" == "" ]; then
+	echo "debugfs does not appear to be mounted!"
+	echo "Please mount debugfs and try again"
+	exit 1
+fi
+
+# Check that the fault injection directory exists
+DEBUGDIR=`echo $DEBUGFS | awk '{print $2}'`/nfsd
+if [ ! -d "$DEBUGDIR" ]; then
+	echo "$DEBUGDIR does not exist"
+	echo "Check that your .config selects CONFIG_NFSD_FAULT_INJECTION"
+	exit 1
+fi
+
+function help()
+{
+	echo "Usage $0 injection_type [count]"
+	echo ""
+	echo "Injection types are:"
+	ls $DEBUGDIR
+	exit 1
+}
+
+if [ $# == 0 ]; then
+	help
+elif [ ! -f $DEBUGDIR/$1 ]; then
+	help
+elif [ $# != 2 ]; then
+	COUNT=0
+else
+	COUNT=$2
+fi
+
+BEFORE=`mktemp`
+AFTER=`mktemp`
+dmesg > $BEFORE
+echo $COUNT > $DEBUGDIR/$1
+dmesg > $AFTER
+# Capture lines that only exist in the $AFTER file
+diff $BEFORE $AFTER | grep ">"
+rm -f $BEFORE $AFTER

Signature · 1 line

1.7.7

[email protected]

author

2011-10-24 11:21 UTC #2 lore raw

PATCH 3/3 NFSD: Added fault injection documentation

In reply to [email protected]

From: Bryan Schumaker <[email protected]>

Signed-off-by Bryan Schumaker <[email protected]>

---
 Documentation/filesystems/nfs/00-INDEX            |    2 +
 Documentation/filesystems/nfs/fault_injection.txt |   69 +++++++++++++++++++++
 2 files changed, 71 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/filesystems/nfs/fault_injection.txt

Documentation/filesystems/nfs/00-INDEX +2 -0

index a57e124..1716874 100644
@@ -2,6 +2,8 @@
 	- this file (nfs-related documentation).
 Exporting
 	- explanation of how to make filesystems exportable.
+fault_injection.txt
+	- information for using fault injection on the server
 knfsd-stats.txt
 	- statistics which the NFS server makes available to user space.
 nfs.txt

Documentation/filesystems/nfs/fault_injection.txt new file +69 -0

69 lines not on this page · raw message

Signature · 1 line

1.7.7

J. Bruce Fields

2011-10-28 21:24 UTC #3 lore raw

In reply to [email protected]

On Mon, Oct 24, 2011 at 07:20:57AM -0400, [email protected] wrote:
...

Quoted 6 lines

fs/nfsd/fault_inject.c new file +0 -0
new file mode 100644
index 0000000..1ac4134
@@ -0,0 +1,90 @@

...

fs/nfsd/fault_inject.c line 83

Quoted 13 lines

fs/nfsd/fault_inject.c new file +13 -0

+int nfsd_fault_inject_init(void)
+{
+	unsigned int i;
+	struct nfsd_fault_inject_op *op;
+	mode_t mode = S_IFREG | S_IRUSR | S_IWUSR;
+
+	debug_dir = debugfs_create_dir("nfsd", NULL);
+	if (!debug_dir)
+		goto fail;
+
+	for (i = 0; i < NUM_INJECT_OPS; i++) {
+		op = &inject_ops[i];
+		debugfs_create_file(op->file, mode, debug_dir, op, &fops_nfsd);

I think you need to check the return value and "goto fail" on NULL.

(Do you also need to put the returned dentry on success? Checking other callers.... No, I guess not, OK.)

Quoted 7 lines

fs/nfsd/fault_inject.c new file +7 -0

+	}
+	return 0;
+
+fail:
+	nfsd_fault_inject_cleanup();
+	return -ENOMEM;
+}

...

Quoted 4 lines

fs/nfsd/nfsctl.c +0 -0
index db34a58..e67f30c 100644

...

Quoted 11 lines

fs/nfsd/nfsctl.c +3 -0

@@ -1130,6 +1131,9 @@ static int __init init_nfsd(void)
 	retval = nfs4_state_init(); /* nfs4 locking state */
 	if (retval)
 		return retval;
+	retval = nfsd_fault_inject_init(); /* nfsd fault injection controls */
+	if (retval)
+		goto out_cleanup_fault_injection;
 	nfsd_stat_init();	/* Statistics */
 	retval = nfsd_reply_cache_init();
 	if (retval)

fs/nfsd/nfsctl.c out_free_cache: · line 1169

Quoted 5 lines

fs/nfsd/nfsctl.c +2 -0

@@ -1161,6 +1165,8 @@ out_free_cache:
 out_free_stat:
 	nfsd_stat_shutdown();
 	nfsd4_free_slabs();
+out_cleanup_fault_injection:
+	nfsd_fault_inject_cleanup();

Check the cleanup here again.... The slabs are allocated in state_init, so you need to do that on nfsd_fault_inject_init, but you *don't* need the fault_inject_cleanup in that case, since fault_inject_init cleanup cleans up after itself--that's needed only on later failures.

--b.

Quoted 14 lines

fs/nfsd/nfsctl.c +1 -0

 	return retval;
 }

@@ -1174,6 +1180,7 @@ static void __exit exit_nfsd(void)
 	nfsd_lockd_shutdown();
 	nfsd_idmap_shutdown();
 	nfsd4_free_slabs();
+	nfsd_fault_inject_cleanup();
 	unregister_filesystem(&nfsd_fs_type);
 }

Signature · 1 line

1.7.7

Bryan Schumaker

author

2011-11-01 14:08 UTC #4 lore raw

Quoted 9 lines · J. Bruce Fields

On Mon, Oct 24, 2011 at 07:20:57AM -0400, [email protected] wrote:
...
Quoted 6 lines
fs/nfsd/fault_inject.c new file +0 -0
new file mode 100644
index 0000000..1ac4134
@@ -0,0 +1,90 @@
...

Quoted 15 lines

fs/nfsd/fault_inject.c line 83

Quoted 13 lines

fs/nfsd/fault_inject.c new file +13 -0

+int nfsd_fault_inject_init(void)
+{
+	unsigned int i;
+	struct nfsd_fault_inject_op *op;
+	mode_t mode = S_IFREG | S_IRUSR | S_IWUSR;
+
+	debug_dir = debugfs_create_dir("nfsd", NULL);
+	if (!debug_dir)
+		goto fail;
+
+	for (i = 0; i < NUM_INJECT_OPS; i++) {
+		op = &inject_ops[i];
+		debugfs_create_file(op->file, mode, debug_dir, op, &fops_nfsd);

I think you need to check the return value and "goto fail" on NULL.

Makes sense. Thanks for catching that!

Quoted 30 lines

(Do you also need to put the returned dentry on success? Checking other callers.... No, I guess not, OK.)

Quoted 7 lines

fs/nfsd/fault_inject.c new file +7 -0

+	}
+	return 0;
+
+fail:
+	nfsd_fault_inject_cleanup();
+	return -ENOMEM;
+}

...

Quoted 4 lines

fs/nfsd/nfsctl.c +0 -0
index db34a58..e67f30c 100644

...

Quoted 13 lines

fs/nfsd/nfsctl.c +3 -0

@@ -1130,6 +1131,9 @@ static int __init init_nfsd(void)
 	retval = nfs4_state_init(); /* nfs4 locking state */
 	if (retval)
 		return retval;
+	retval = nfsd_fault_inject_init(); /* nfsd fault injection controls */
+	if (retval)
+		goto out_cleanup_fault_injection;
 	nfsd_stat_init();	/* Statistics */
 	retval = nfsd_reply_cache_init();
 	if (retval)

@@ -1161,6 +1165,8 @@ out_free_cache:
 out_free_stat:
 	nfsd_stat_shutdown();

Quoted 8 lines

fs/nfsd/nfsctl.c out_free_cache: · line 1169
Quoted 3 lines
fs/nfsd/nfsctl.c +2 -0
 	nfsd4_free_slabs();
+out_cleanup_fault_injection:
+	nfsd_fault_inject_cleanup();
Check the cleanup here again.... The slabs are allocated in state_init, so you need to do that on nfsd_fault_inject_init, but you *don't* need the fault_inject_cleanup in that case, since fault_inject_init cleanup cleans up after itself--that's needed only on later failures.

Would it be better to change the fault_inject_init() function so it doesn't clean up after itself? This would keep it consistent with the other nfsd init functions. I didn't realize that slabs were initialized in the state_init() function, I'll fix that.

Thanks for the review!

- Bryan

Quoted 21 lines

--b.
Quoted 14 lines
fs/nfsd/nfsctl.c +1 -0
 	return retval;
 }
 
@@ -1174,6 +1180,7 @@ static void __exit exit_nfsd(void)
 	nfsd_lockd_shutdown();
 	nfsd_idmap_shutdown();
 	nfsd4_free_slabs();
+	nfsd_fault_inject_cleanup();
 	unregister_filesystem(&nfsd_fs_type);
 }
 
Signature · 1 line
1.7.7
--
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

J. Bruce Fields

2011-11-01 14:17 UTC #5 lore raw

Quoted 30 lines · Bryan Schumaker

Quoted 20 lines · J. Bruce Fields
On Mon, Oct 24, 2011 at 07:20:57AM -0400, [email protected] wrote:
...
Quoted 4 lines
fs/nfsd/nfsctl.c +0 -0
index db34a58..e67f30c 100644
...
Quoted 13 lines
fs/nfsd/nfsctl.c +3 -0
@@ -1130,6 +1131,9 @@ static int __init init_nfsd(void)
 	retval = nfs4_state_init(); /* nfs4 locking state */
 	if (retval)
 		return retval;
+	retval = nfsd_fault_inject_init(); /* nfsd fault injection controls */
+	if (retval)
+		goto out_cleanup_fault_injection;
 	nfsd_stat_init();	/* Statistics */
 	retval = nfsd_reply_cache_init();
 	if (retval)
@@ -1161,6 +1165,8 @@ out_free_cache:
 out_free_stat:
 	nfsd_stat_shutdown();
Quoted 8 lines
fs/nfsd/nfsctl.c out_free_cache: · line 1169
Quoted 3 lines
fs/nfsd/nfsctl.c +2 -0
 	nfsd4_free_slabs();
+out_cleanup_fault_injection:
+	nfsd_fault_inject_cleanup();
Check the cleanup here again.... The slabs are allocated in state_init, so you need to do that on nfsd_fault_inject_init, but you *don't* need the fault_inject_cleanup in that case, since fault_inject_init cleanup cleans up after itself--that's needed only on later failures.

Quoted 3 lines

Would it be better to change the fault_inject_init() function so it doesn't clean up after itself? This would keep it consistent with the other nfsd init functions.

I don't think so. The pattern here is pretty common throughout the kernel; it's approximately:

	do_thing1();
	err = do_thing2();
	if (err)
		goto undo_thing1;
	err = do_thing3();
	if (err)
		goto undo_thing2;
	...

Functions are no-ops on failure, and the caller's just responsible for cleaning up previous stuff.

Quoted 2 lines

I didn't realize that slabs were initialized in the state_init() function, I'll fix that.

I'll admit that's confusing. Any suggestion to make it clearer welcome.

--b.

Bryan Schumaker

author

2011-11-01 14:22 UTC #6 lore raw

Quoted 47 lines · J. Bruce Fields

Quoted 30 lines · Bryan Schumaker
Quoted 20 lines · J. Bruce Fields
On Mon, Oct 24, 2011 at 07:20:57AM -0400, [email protected] wrote:
...
Quoted 4 lines
fs/nfsd/nfsctl.c +0 -0
index db34a58..e67f30c 100644
...
Quoted 13 lines
fs/nfsd/nfsctl.c +3 -0
@@ -1130,6 +1131,9 @@ static int __init init_nfsd(void)
 	retval = nfs4_state_init(); /* nfs4 locking state */
 	if (retval)
 		return retval;
+	retval = nfsd_fault_inject_init(); /* nfsd fault injection controls */
+	if (retval)
+		goto out_cleanup_fault_injection;
 	nfsd_stat_init();	/* Statistics */
 	retval = nfsd_reply_cache_init();
 	if (retval)
@@ -1161,6 +1165,8 @@ out_free_cache:
 out_free_stat:
 	nfsd_stat_shutdown();
Quoted 8 lines
fs/nfsd/nfsctl.c out_free_cache: · line 1169
Quoted 3 lines
fs/nfsd/nfsctl.c +2 -0
 	nfsd4_free_slabs();
+out_cleanup_fault_injection:
+	nfsd_fault_inject_cleanup();
Check the cleanup here again.... The slabs are allocated in state_init, so you need to do that on nfsd_fault_inject_init, but you *don't* need the fault_inject_cleanup in that case, since fault_inject_init cleanup cleans up after itself--that's needed only on later failures.
Quoted 3 lines
Would it be better to change the fault_inject_init() function so it doesn't clean up after itself? This would keep it consistent with the other nfsd init functions.
I don't think so. The pattern here is pretty common throughout the kernel; it's approximately:
	do_thing1();
	err = do_thing2();
	if (err)
		goto undo_thing1;
	err = do_thing3();
	if (err)
		goto undo_thing2;
	...

Quoted 2 lines

Functions are no-ops on failure, and the caller's just responsible for cleaning up previous stuff.

Ok, I won't change that.

Quoted 5 lines

Quoted 3 lines
I didn't realize that slabs were initialized in the state_init() function, I'll fix that.
I'll admit that's confusing. Any suggestion to make it clearer welcome.

Sure. The comment after the call to nfs4_state_init() is "nfs4 locking state". Maybe this could be changed to "nfs4 locking state and slabs"?
I can make this change and send it as a separate patch if you would
like.

Quoted 6 lines

--b.
--
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

J. Bruce Fields

2011-11-01 14:35 UTC #7 lore raw

Quoted 10 lines · Bryan Schumaker

Quoted 4 lines · J. Bruce Fields
Quoted 2 lines
I didn't realize that slabs were initialized in the state_init() function, I'll fix that.
I'll admit that's confusing. Any suggestion to make it clearer welcome.
Sure. The comment after the call to nfs4_state_init() is "nfs4 locking state". Maybe this could be changed to "nfs4 locking state and slabs"?
I can make this change and send it as a separate patch if you would
like.

If there was a way to fix the code itself to make it clearer, that's usually better--a comment is a workaround.

--b.