Re: [PATCH]: v9fs: add readpage support

[Eric Van Hensbergen <ericvh@gmail.com>]

On 1/11/06, Andrew Morton <akpm@osdl.org> wrote:
> 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.
>
>

Okay I'll take a look at this in the context of both the old mux and
the new mux code.

> > +             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?
>

maybe?  Its been a while since I looked at this code, but I believe
the logic is that if you are approaching the end of the file you'll
get less than rsize bytes back and then you just fill in the rest of
the page with zeros.

> > +     retval = 0;
> > +
> > +      UnmapAndUnlock:
> > +     kunmap(page);
>
> eww, do you really indent labels like that?
>

No, something funky happened and I didn't proof the patch like I should have.

> > 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?
>

Its quite possible I've done the wrong thing here.  The intent is to
make sure that any stuff that might be in the page cache due to an
mmap is flushed when I do a write.  This approach is overkill, I
should probably just flush anything in the cache that the write
affects.

I'll take another look at the mux stuff and also see if I can come up
with a less brute-force approach to invalidating the page cache.

thanks for the comments.

      -eric