Re: [PATCH v2] reduce NFS stack usage

[Steve Dickson <SteveD@redhat.com>]

Hey Jeff,

Question:
Why are only nfs_lookup_revalidate() and nfs_readdir()
a problem and not the other 4 ops (like nfs_lookup())?
Is the case only those two showed up in the stack overflow
oops trace?

Also, not like there much choice in matter, but I wonder what
type of performance hit (if any) there will be by making
these routines call kmalloc()... lookups and readdirs are
pretty popular ops...

SteveD.

Jeff Garzik wrote:
> David Mansfield spotted a bug in a memset() call, in my initial patch.
> 
> Attached is an updated version.  The only change is 
> s/sizeof(my_entry)/sizeof(*my_entry)/ in the memset.
> 
>     Jeff
> 
> 
> 
> ------------------------------------------------------------------------
> 
> 
> ===== fs/nfs/dir.c 1.12 vs edited =====
> --- 1.12/fs/nfs/dir.c	Tue Oct 15 00:59:27 2002
> +++ edited/fs/nfs/dir.c	Mon Sep 22 15:08:27 2003
> @@ -349,14 +349,25 @@
>  {
>  	struct dentry	*dentry = filp->f_dentry;
>  	struct inode	*inode = dentry->d_inode;
> -	nfs_readdir_descriptor_t my_desc,
> -			*desc = &my_desc;
> -	struct nfs_entry my_entry;
> +	nfs_readdir_descriptor_t *desc;
> +	struct nfs_entry *my_entry;
>  	long		res;
> +	int		rc = -EINVAL;
> +	void		*mem;
> +
> +	mem = kmalloc(sizeof(struct nfs_entry) +
> +		      sizeof(nfs_readdir_descriptor_t), GFP_KERNEL);
> +	if (!mem)
> +		return -ENOMEM;
> +
> +	my_entry = mem;
> +	desc = mem + sizeof(struct nfs_entry);
>  
>  	res = nfs_revalidate(dentry);
> -	if (res < 0)
> -		return res;
> +	if (res < 0) {
> +		rc = (int) res;
> +		goto out;
> +	}
>  
>  	/*
>  	 * filp->f_pos points to the file offset in the page cache.
> @@ -365,11 +376,11 @@
>  	 * itself.
>  	 */
>  	memset(desc, 0, sizeof(*desc));
> -	memset(&my_entry, 0, sizeof(my_entry));
> +	memset(my_entry, 0, sizeof(*my_entry));
>  
>  	desc->file = filp;
>  	desc->target = filp->f_pos;
> -	desc->entry = &my_entry;
> +	desc->entry = my_entry;
>  	desc->decode = NFS_PROTO(inode)->decode_dirent;
>  
>  	while(!desc->entry->eof) {
> @@ -393,11 +404,16 @@
>  			break;
>  		}
>  	}
> +
>  	if (desc->error < 0)
> -		return desc->error;
> -	if (res < 0)
> -		return res;
> -	return 0;
> +		rc = desc->error;
> +	else if (res < 0)
> +		rc = res;
> +	/* fall through */
> +
> +out:
> +	kfree(mem);
> +	return rc;
>  }
>  
>  /*
> @@ -476,13 +492,22 @@
>  	struct inode *dir;
>  	struct inode *inode;
>  	int error;
> -	struct nfs_fh fhandle;
> -	struct nfs_fattr fattr;
> +	struct nfs_fh *fhandle;
> +	struct nfs_fattr *fattr;
> +	void *mem;
> +	int rc = 0;
>  
>  	lock_kernel();
>  	dir = dentry->d_parent->d_inode;
>  	inode = dentry->d_inode;
>  
> +	mem = kmalloc(sizeof(struct nfs_fh) + sizeof(struct nfs_fattr),
> +		      GFP_KERNEL);
> +	if (!mem)
> +		goto out_bad;
> +	fhandle = mem;
> +	fattr = mem + sizeof(struct nfs_fh);
> +
>  	if (!inode) {
>  		if (nfs_neg_need_reval(dir, dentry))
>  			goto out_bad;
> @@ -505,18 +530,19 @@
>  	if (NFS_STALE(inode))
>  		goto out_bad;
>  
> -	error = NFS_PROTO(dir)->lookup(dir, &dentry->d_name, &fhandle, &fattr);
> +	error = NFS_PROTO(dir)->lookup(dir, &dentry->d_name, fhandle, fattr);
>  	if (error)
>  		goto out_bad;
> -	if (memcmp(NFS_FH(inode), &fhandle, sizeof(struct nfs_fh))!= 0)
> +	if (memcmp(NFS_FH(inode), fhandle, sizeof(struct nfs_fh))!= 0)
>  		goto out_bad;
> -	if ((error = nfs_refresh_inode(inode, &fattr)) != 0)
> +	if ((error = nfs_refresh_inode(inode, fattr)) != 0)
>  		goto out_bad;
>  
>  	nfs_renew_times(dentry);
>   out_valid:
> -	unlock_kernel();
> -	return 1;
> + 	rc = 1;
> +	goto out;
> +
>   out_bad:
>  	NFS_CACHEINV(dir);
>  	if (inode && S_ISDIR(inode->i_mode)) {
> @@ -528,8 +554,14 @@
>  		shrink_dcache_parent(dentry);
>  	}
>  	d_drop(dentry);
> +	rc = 0;
> +	/* fall through */
> +
> +out:
>  	unlock_kernel();
> -	return 0;
> +	if (mem)
> +		kfree(mem);
> +	return rc;
>  }
>  
>  /*
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs