Re: [PATCH 1/3] NFSD: Added fault injection

["J. Bruce Fields" <bfields@fieldses.org>]

On Mon, Oct 17, 2011 at 06:57:18PM -0400, Bryan Schumaker wrote:
> On 10/17/2011 06:18 PM, J. Bruce Fields wrote:
> > On Fri, Oct 07, 2011 at 01:44:26PM -0400, bjschuma@netapp.com wrote:
> >> +#define INJECTION_OP(op_action, op_item, op_func)	\
> >> +{							\
> >> +	.action = op_action,				\
> >> +	.item   = op_item,				\
> >> +	.file   = op_action"_"op_item,			\
> >> +	.func   = op_func,				\
> >> +}
> >> +
> >> +static struct nfsd_fault_inject_op inject_ops[] = {
> >> +	INJECTION_OP("forget", "clients",     nfsd_forget_clients),
> >> +	INJECTION_OP("forget", "locks",       nfsd_forget_locks),
> >> +	INJECTION_OP("forget", "openowners",  nfsd_forget_openowners),
> >> +	INJECTION_OP("forget", "delegations", nfsd_forget_delegations),
> >> +	INJECTION_OP("recall", "delegations", nfsd_recall_delegations),
> > 
> > This is a little clever for my taste....  Could we just do
> > 
> > 	static struct nfsd_fault_inject_op inject_ops[] = {
> > 		{
> > 			.file = "forget_client",
> > 			.op = nfsd_forget_clients,
> > 		},
> > 		...
> > 	}
> > 
> > and do away with the separate item and action fields?
> > 
> > I'd rather be sort of obvious and boring even if it's slightly less
> > compact.
> > 
> I was going for compact when I initially wrote this, but I can change it.  I have them as separate fields so I can print out slightly different messages based on what is going on.  Such as:  "NFSD: Server forgetting all clients" or "NFSD: Server recalling at most 4 delegations".

Even

	{ .file = "forget_client", .op=nfsd_forget_clients },
	{ ... }

would be fine by me and still pretty compact.

And log messages are probably a good idea but I don't think they have to
be beautiful--"NFSD: recall_delegations(4)" would do fine.

--b.


> 
> >> +};
> >> +
> >> +static long int NUM_INJECT_OPS = sizeof(inject_ops) / sizeof(struct nfsd_fault_inject_op);
> >> +static struct dentry *debug_dir;
> >> +
> >> +static int nfsd_inject_set(void *data, u64 val)
> >> +{
> >> +	int i;
> >> +	struct nfsd_fault_inject_op *op;
> >> +
> >> +	for (i = 0; i < NUM_INJECT_OPS; i++) {
> >> +		op = &inject_ops[i];
> >> +		if (&op->file_data == data) {
> > 
> > Huh, OK, so if I understand right, the contents of file_data doesn't
> > matter, you're just using a pointer to that field as a way to identify
> > the op array.
> > 
> > But then couldn't you just pass in a pointer to the op itself:
> > 
> >> +	for (i = 0; i < NUM_INJECT_OPS; i++) {
> >> +		op = &inject_ops[i];
> >> +		debugfs_create_file(op->file, mode, debug_dir, &op->file_data, &fops_nfsd);
> > 
> > like:
> > 
> > 		debugfs_create_file(op->file, mode, debug_dir, op, &fops_nfsd);
> > 
> > and eliminate the file_data field?
> 
> I've never thought about trying it that way, but it seems fairly straightforward.  I'll try it that way and see if it works!
> > 
> > Patches look OK otherwise on a quick skim, thanks.
> > 
> > --b.
> > 
> > 
> >> +	}
> >> +	return 0;
> 
> --
> 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