On Thu, 2007-08-09 at 19:52 +0100, David Howells wrote:
> Trond Myklebust <trond.myklebust@xxxxxxxxxx> wrote:
>
> > Dang, that's a lot of inlines... AFAICS, approx half of fs/nfs/fscache.h
> > should really be moved into fscache.c.
>
> If you wish. It seems a shame since a lot of them have only one caller.
...however it also forces you to export a lot of stuff which is really
private to fscache.c (the atomics etc).
> Note that due to patch #2, PG_fscache causes releasepage() and
> invalidatepage() to be called in addition to PG_private.
> > > +
> > > + if (!nfs_fscache_release_page(page, gfp))
> > > + return 0;
> >
> > This looks _very_ dubious. Why shouldn't I be able to discard a page
> > just because fscache isn't done writing it out? I may have very good
> > reasons to do so.
>
> Hmmm... Looking at the truncate routines, I suppose this ought to be okay,
> provided the cache retains a reference on the page whilst it's writing it out
> (put_page() won't can the page until we release it).
>
> It also seems dubious, though, to release the page when the filesystem is
> doing stuff to it, even if it's by proxy in the cache. I'll have to test
> that, but I'm slightly concerned that the netfs could end up releasing its
> cookie before the cache has finished with its pages. On the other hand, with
> the new asynchronous stuff I've done, I'm not sure this'll be an actual
> problem.
Actually, as long as launder_page() and invalidate_page() are doing
their thing, then your current code might be OK. The important thing is
to ensure that invalidate_inode_pages2() and truncate_inode_pages() work
as expected.
> > > static int nfs_launder_page(struct page *page)
> > > {
> > > + nfs_fscache_invalidate_page(page, page->mapping->host, 0);
> >
> > Why? The function of launder_page() is to clean the page, not to
> > truncate it.
>
> Okay.
What you should be doing here is probably to make a call to
wait_on_page_fscache_write(page). That should suffice to ensure that the
page is clean w.r.t. fscache activity afaics.
> > > @@ -1000,11 +1007,13 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
> > > if (data_stable) {
> > > inode->i_size = new_isize;
> > > invalid |= NFS_INO_INVALID_DATA;
> > > + nfs_fscache_attr_changed(inode);
> >
> > Can't fscache_attr_changed() call kmalloc(GFP_KERNEL)? You are in a
> > spinlocked context here.
>
> Hmmm... How about I move the call to fscache_attr_changed() to the callers of
> nfs_update_inode(), to just after the spinlock is unlocked. The operation is
> going to be asynchronous, so the delay shouldn't matter.
Right. You might also want to add a flag NFS_INO_INVALID_FSCACHE_ATTR or
something like that in order to trigger it.
> > I'm not too happy about the change to the binary mount interface. As far
> > as I'm concerned, the binary interface should really be considered
> > frozen, and should not be touched.
>
> I hadn't come across that. I'll have a look.
>
> > Instead, feel free to update the text-based mount interface (which can
> > be found in 2.6.23-rc1 and later). Please put any such mount interface
> > changes in a separate patch together with the Kconfig changes.
>
> If you wish, but doesn't that violate the rules of patch division laid down by
> Linus and Andrew?
Why? It should be possible to set this up so that the NFS+fscache code
compiles correctly without the mount patch.
Trond
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
