On Thu, Sep 13, 2007 at 12:20:46PM +0100, Christoph Hellwig wrote:
> This has never been out for review, has it?
> On Thu, Sep 13, 2007 at 01:30:16PM +1000, Lachlan McIlroy wrote:
> > fs/xfs/xfs_vnodeops.c - 1.720 - changed
> > http://oss.sgi.com/cgi-bin/cvsweb.cgi/xfs-linux/xfs_vnodeops.c.diff?r1=text&tr1=1.720&r2=text&tr2=1.719&f=h
> > - Ensure file size updates have been completed before writing inode to
> > disk.
> I think you want to at least add a comment above the filemap_fdatawait
> call why we have it that early compared to where the generic code calls
> it (again).
true - obvious to lachlan and myself, maybe not others. :/
> But hopefully I'll push changes to the core code soon
> to move the filemap_datawrite/wait into fs domain completely.
What timeframe are we looking at there?
> I also don't like idioms like vn_to_inode(XFS_ITOV(ip)) at all. Just
> doing a direct ip->i_vnode deference sounds perfectly fine.
Sounds like another cleanup patch for Eric ;)
> Why is removing the ipincount check in xfs_inode_flush okay? Trying
> to flush pinned inodes doesn't seem that much of a good idea.
The code is .... convoluted. In the case where we are doing an async
flush, we check the pin count once we've got the locks so the pin
count check is not really needed here. In the case of a sync flush,
we'd return EAGAIN, which would then call use again with the FLUSH_LOG
flag and so we'd do a log force to unpin the inode.
If we then race with something to else, the inode could end up pinned
again before we go to flush the inode and hence we'd end up not
flushing the inode because the pin count was set again.
Anyway, if we are doing a sync flush, it's a blocking operation
and we want to push the log, which is exactly what will happen
in xfs_iflush() when it calls xfs_iunpin_wait() if the inode is
pinned. hence the check for xfs_ipincount() in the higher level
is pretty much redundant as the correct log force will occur
just by allowing the inode flush to continue.
SGI Australian Software Group