Re: [PATCH]: v9fs: add readpage support

[Eric Van Hensbergen]

On 1/11/06, Andrew Morton <akpm@xxxxxxxx> wrote:
> ericvh@xxxxxxxxx (Eric Van Hensbergen) wrote:
> > +
> > +             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 had a chance to go back and look into this deeper when applied
on top of Lucho's new mux code.  It appears (to Lucho and I) that the
the v9fs_mux_rpc() code zero's out *rcall really early on, so I don't
see where this could lead to a double kfree.  If you still think we
are missing something, let me know and I'll have another look.

> > 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;
> >  }
>

I went looking for an example of how to do this better.  More or less,
v9fs reads and writes are similar to DirectIO since they don't go
through the page-cache.  So, I looked at what NFS does when it gets a
DirectIO write, and it looks (to me) like it does more or less the
same thing:
(from nfs_file_direct_write() in fs/nfs/direct.c)
        retval = nfs_direct_write(inode, ctx, &iov, pos, 1);
        if (mapping->nrpages)
                invalidate_inode_pages2(mapping);

Now, that being said, it still seems to me to be a bit heavy weight --
do folks have a better pointer to code that I can use as an example of
how to do this more efficiently?

       -eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/