TAKE 968767 - Ensure file size updates have been completed before writing inode to disk.

TAKE 968767 - Ensure file size updates have been completed before writing inode to disk.

[Lachlan McIlroy]

Ensure file size updates have been completed before writing inode to disk.



Date:  Thu Sep 13 13:27:00 AEST 2007
Workarea:  redback.melbourne.sgi.com:/home/lachlan/isms/2.6.x-xfs
Inspected by:  dgc

The following file(s) were checked into:
  longdrop.melbourne.sgi.com:/isms/linux/2.6.x-xfs-melb


Modid:  xfs-linux-melb:xfs-kern:29675a
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.

fs/xfs/linux-2.6/xfs_super.c - 1.398 - changed
http://oss.sgi.com/cgi-bin/cvsweb.cgi/xfs-linux/linux-2.6/xfs_super.c.diff?r1=text&tr1=1.398&r2=text&tr2=1.397&f=h
	- Ensure file size updates have been completed before writing inode to disk.

fs/xfs/linux-2.6/xfs_aops.c - 1.153 - changed
http://oss.sgi.com/cgi-bin/cvsweb.cgi/xfs-linux/linux-2.6/xfs_aops.c.diff?r1=text&tr1=1.153&r2=text&tr2=1.152&f=h
	- Ensure file size updates have been completed before writing inode to disk.

Re: TAKE 968767 - Ensure file size updates have been completed before writing inode to disk.

[Christoph Hellwig]

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/linux-2.4-xfs/> xfs_vnodeops.c.diff?r1=text&tr1=1.720&r2=text&tr2=1.719&f=h
> 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).  But hopefully I'll push changes to the core code soon
to move the filemap_datawrite/wait into fs domain completely.

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.


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.

Re: TAKE 968767 - Ensure file size updates have been completed before writing inode to disk.

[David Chinner]

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/linux-2.4-xfs/> > xfs_vnodeops.c.diff?r1=text&tr1=1.720&r2=text&tr2=1.719&f=h
> http://oss.sgi.com/cgi-bin/cvsweb.cgi/linux-2.4-xfs/> xfs_vnodeops.c.diff?r1=text&tr1=1.720&r2=text&tr2=1.719&f=h
> > 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.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: TAKE 968767 - Ensure file size updates have been completed before writing inode to disk.

[Christoph Hellwig]

On Fri, Sep 14, 2007 at 10:05:56AM +1000, David Chinner wrote:
> > 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'll aim for 2.6.25, but there is some mess to sort out with the fsync
prototype first.