Review: fix truncate vs null files issues

Review: fix truncate vs null files issues

[David Chinner]

When we change the file size via xfs_setattr(), we log the
new, in-memory file size in the transaction. We do this without
having flushed any dirty data so if we have previously extended
the file we change the on disk file size without having written
the data first.

This is a problem for both growing the file and truncating the
file. The truncate case is partially hidden by the VTRUNCATE
code, but if the file has not been closed before a crash has
occurred we can still get NULLs appearing in files. 

The following patch fixes this problem by flushing the range
between the on-disk filesize and the new file size as long
as the new file size is larger than the on disk file size.

Comments?

Cheers,

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

---
 fs/xfs/xfs_vnodeops.c |   25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

Index: 2.6.x-xfs-new/fs/xfs/xfs_vnodeops.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_vnodeops.c	2007-06-20 17:59:35.050768978 +1000
+++ 2.6.x-xfs-new/fs/xfs/xfs_vnodeops.c	2007-06-21 15:18:17.187347809 +1000
@@ -595,7 +595,30 @@ xfs_setattr(
 			code = xfs_igrow_start(ip, vap->va_size, credp);
 		}
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);
-		vn_iowait(vp); /* wait for the completion of any pending DIOs */
+
+		/*
+		 * We are going to log the inode size change in this
+		 * transaction so any previous writes that are beyond the on
+		 * disk EOF and the new EOF that have not been written out need
+		 * to be written here. If we do not write the data out, we
+		 * expose ourselves to the null files problem.
+		 *
+		 * Only flush from the on disk size to the smaller of the in
+		 * memory file size or the new size as that's the range we
+		 * really care about here and prevents waiting for other data
+		 * not within the range we care about here.
+		 */
+		if (!code &&
+		    (ip->i_size != ip->i_d.di_size) &&
+		    (vap->va_size > ip->i_d.di_size)) {
+			code = bhv_vop_flush_pages(XFS_ITOV(ip),
+					ip->i_d.di_size, vap->va_size,
+					XFS_B_ASYNC, FI_NONE);
+		}
+
+		/* wait for all I/O to complete */
+		vn_iowait(vp);
+
 		if (!code)
 			code = xfs_itruncate_data(ip, vap->va_size);
 		if (code) {

Re: Review: fix truncate vs null files issues

[Christoph Hellwig]

On Mon, Jul 09, 2007 at 05:38:26PM +1000, David Chinner wrote:
> 
> When we change the file size via xfs_setattr(), we log the
> new, in-memory file size in the transaction. We do this without
> having flushed any dirty data so if we have previously extended
> the file we change the on disk file size without having written
> the data first.
> 
> This is a problem for both growing the file and truncating the
> file. The truncate case is partially hidden by the VTRUNCATE
> code, but if the file has not been closed before a crash has
> occurred we can still get NULLs appearing in files. 
> 
> The following patch fixes this problem by flushing the range
> between the on-disk filesize and the new file size as long
> as the new file size is larger than the on disk file size.
> 
> Comments?

Looks good.

> +		    (ip->i_size != ip->i_d.di_size) &&
> +		    (vap->va_size > ip->i_d.di_size)) {

And if I'd really want to get picky I'd say please remove all the superflous
braces here..