Re: [PATCH]: v9fs: add readpage support

[Andrew Morton <akpm@osdl.org>]

ericvh@gmail.com (Eric Van Hensbergen) wrote:
>
> Subject: [PATCH] v9fs: add readpage support
> 
> v9fs mmap support was originally removed from v9fs at Al Viro's request,
> but recently there have been requests from folks who want readpage
> functionality (primarily to enable execution of files mounted via 9P).
> This patch adds readpage support (but not writepage which contained most of
> the objectionable code).  It passes FSX (and other regressions) so it
> should be relatively safe.
> 
> +
> +static int v9fs_vfs_readpage(struct file *filp, struct page *page)
> +{  
> +	char *buffer = NULL;
> +	int retval = -EIO;
> +	loff_t offset = page_offset(page);
> +	int count = PAGE_CACHE_SIZE;
> +	struct inode *inode = filp->f_dentry->d_inode;
> +	struct v9fs_session_info *v9ses = v9fs_inode2v9ses(inode);
> +	int rsize = v9ses->maxdata - V9FS_IOHDRSZ;
> +	struct v9fs_fid *v9f = filp->private_data;
> +	struct v9fs_fcall *fcall = NULL;
> +	int fid = v9f->fid;
> +	int total = 0;
> +	int result = 0;
> +
> +	buffer = kmap(page);
> +	do {
> +		if (count < rsize)
> +			rsize = count;
> +
> +		result = v9fs_t_read(v9ses, fid, offset, rsize, &fcall);
> +
> +		if (result < 0) {
> +			printk(KERN_ERR "v9fs_t_read returned %d\n",
> +			       result);
> +
> +			kfree(fcall);
> +			goto UnmapAndUnlock;
> +		} else
> +			offset += result;
> +
> +		memcpy(buffer, fcall->params.rread.data, result);
> +
> +		count -= result;
> +		buffer += result;
> +		total += result;
> +
> +		kfree(fcall);

Minor thing: from my reading of v9fs_mux_rpc() there's potential for a
double-kfree here.  Either v9fs_mux_rpc() needs to be changed to
unambiguously zero out *rcall (or, better, v9fs_t_read does it) or you need
to zero fcall on each go around the loop.


> +		if (result < rsize)
> +			break;
> +	} while (count);
> +
> +	memset(buffer, 0, count);
> +	flush_dcache_page(page);
> +	SetPageUptodate(page);

if (result < rsize), is the page really up to date?

> +	retval = 0;
> +
> +      UnmapAndUnlock:
> +	kunmap(page);

eww, do you really indent labels like that?

> diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
> index 6852f0e..feddc5c 100644
> --- a/fs/9p/vfs_file.c
> +++ b/fs/9p/vfs_file.c
> @@ -289,6 +289,8 @@ v9fs_file_write(struct file *filp, const
>  		total += result;
>  	} while (count);
>  
> +	invalidate_inode_pages2(inode->i_mapping);
> +
>  	return total;
>  }

That's a really scary function you have there.  Can you explain the
thinking behind its use here?  What are we trying to achieve?