[PATCH] fat: fix data past EOF resulting from fsx testsuite

[PATCH] fat: fix data past EOF resulting from fsx testsuite

[Namjae Jeon]

When running FSX with direct I/O mode, fsx resulted in DATA past EOF issues.

fsx ./file2 -Z -r 4096 -w 4096
...
..
truncating to largest ever: 0x907c
fallocating to largest ever: 0x11137
truncating to largest ever: 0x2c6fe
truncating to largest ever: 0x2cfdf
fallocating to largest ever: 0x40000
Mapped Read: non-zero data past EOF (0x18628) page offset 0x629 is 0x2a4e
...
..

The reason being, it is doing a truncate down, but the zeroing does
not happen on the last block boundary when offset is not aligned.
Even though it calls truncate_setsize()->truncate_inode_pages()->
truncate_inode_pages_range() and considers the partial zeroout but it
retrieves the page using find_lock_page() - which only looks the page in
the cache. So, zeroing out does not happen in case of direct IO.

Make a truncate page based around block_truncate_page for FAT filesystem and
invoke that helper to zerout in case the offset is not aligned with
the blocksize.

Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
Signed-off-by: Amit Sahrawat <a.sahrawat@samsung.com>
---
 fs/fat/fat.h   |  1 +
 fs/fat/file.c  |  2 ++
 fs/fat/inode.c | 12 ++++++++++++
 3 files changed, 15 insertions(+)

diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index eff027d..61801da 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -374,6 +374,7 @@ extern int fat_file_fsync(struct file *file, loff_t start, loff_t end,
 			  int datasync);
 
 /* fat/inode.c */
+extern int fat_block_truncate_page(struct inode *inode, loff_t from);
 extern void fat_attach(struct inode *inode, loff_t i_pos);
 extern void fat_detach(struct inode *inode);
 extern struct inode *fat_iget(struct super_block *sb, loff_t i_pos);
diff --git a/fs/fat/file.c b/fs/fat/file.c
index f2c73ae..a848502 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -526,6 +526,8 @@ int fat_setattr(struct dentry *dentry, struct iattr *attr)
 		down_write(&MSDOS_I(inode)->truncate_lock);
 		truncate_setsize(inode, attr->ia_size);
 		fat_truncate_blocks(inode, attr->ia_size);
+		if (inode->i_size & (inode->i_sb->s_blocksize - 1))
+			fat_block_truncate_page(inode, inode->i_size);
 		up_write(&MSDOS_I(inode)->truncate_lock);
 	}
 
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index f362796..57140f9 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -339,6 +339,18 @@ static sector_t _fat_bmap(struct address_space *mapping, sector_t block)
 	return blocknr;
 }
 
+/*
+ * fat_block_truncate_page() zeroes out a mapping from file offset `from'
+ * up to the end of the block which corresponds to `from'.
+ * This is required during truncate to physically zero out the tail end
+ * of that block so it doesn't yield old data if the file is later grown.
+ * Also, avoid causing failure from fsx for cases of "data past EOF"
+ */
+int fat_block_truncate_page(struct inode *inode, loff_t from)
+{
+	return block_truncate_page(inode->i_mapping, from, fat_get_block);
+}
+
 static const struct address_space_operations fat_aops = {
 	.readpage	= fat_readpage,
 	.readpages	= fat_readpages,
-- 
1.7.11-rc0

Re: [PATCH] fat: fix data past EOF resulting from fsx testsuite

[OGAWA Hirofumi]

Namjae Jeon <namjae.jeon@samsung.com> writes:

> When running FSX with direct I/O mode, fsx resulted in DATA past EOF issues.
>
> fsx ./file2 -Z -r 4096 -w 4096
> ...
> ..
> truncating to largest ever: 0x907c
> fallocating to largest ever: 0x11137
> truncating to largest ever: 0x2c6fe
> truncating to largest ever: 0x2cfdf
> fallocating to largest ever: 0x40000
> Mapped Read: non-zero data past EOF (0x18628) page offset 0x629 is 0x2a4e
> ...
> ..
>
> The reason being, it is doing a truncate down, but the zeroing does
> not happen on the last block boundary when offset is not aligned.
> Even though it calls truncate_setsize()->truncate_inode_pages()->
> truncate_inode_pages_range() and considers the partial zeroout but it
> retrieves the page using find_lock_page() - which only looks the page in
> the cache. So, zeroing out does not happen in case of direct IO.
>
> Make a truncate page based around block_truncate_page for FAT filesystem and
> invoke that helper to zerout in case the offset is not aligned with
> the blocksize.

Good catch. However implementation looks like strange a bit.

> @@ -374,6 +374,7 @@ extern int fat_file_fsync(struct file *file, loff_t start, loff_t end,
>  			  int datasync);
>  
>  /* fat/inode.c */
> +extern int fat_block_truncate_page(struct inode *inode, loff_t from);

We can't use "static" for fat_block_truncate_page()?

> --- a/fs/fat/file.c
> +++ b/fs/fat/file.c
> @@ -526,6 +526,8 @@ int fat_setattr(struct dentry *dentry, struct iattr *attr)
>  		down_write(&MSDOS_I(inode)->truncate_lock);
>  		truncate_setsize(inode, attr->ia_size);
>  		fat_truncate_blocks(inode, attr->ia_size);
> +		if (inode->i_size & (inode->i_sb->s_blocksize - 1))
> +			fat_block_truncate_page(inode, inode->i_size);
>  		up_write(&MSDOS_I(inode)->truncate_lock);
>  	}

Let's remove block alignment, because it was done in block_truncate_page().
Then, move fat_block_truncate_page() before truncate_setsize(). 
Looks like other fs calls it before truncate_setsize().

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

RE: [PATCH] fat: fix data past EOF resulting from fsx testsuite

[Namjae Jeon]

> >
> >  /* fat/inode.c */
> > +extern int fat_block_truncate_page(struct inode *inode, loff_t from);
> 
Hi OGAWA.
> We can't use "static" for fat_block_truncate_page()?
Block_truncate_page(., .., get_block) takes get_block as one of
its arguments and in case of fat, fat_get_block is defined and
marked static in fat/inode.c. We cannot use block_truncate_page()
directly in fat/file.c.
> 
> > --- a/fs/fat/file.c
> > +++ b/fs/fat/file.c
> > @@ -526,6 +526,8 @@ int fat_setattr(struct dentry *dentry, struct iattr *attr)
> >  		down_write(&MSDOS_I(inode)->truncate_lock);
> >  		truncate_setsize(inode, attr->ia_size);
> >  		fat_truncate_blocks(inode, attr->ia_size);
> > +		if (inode->i_size & (inode->i_sb->s_blocksize - 1))
> > +			fat_block_truncate_page(inode, inode->i_size);
> >  		up_write(&MSDOS_I(inode)->truncate_lock);
> >  	}
> 
> Let's remove block alignment, because it was done in block_truncate_page().
> Then, move fat_block_truncate_page() before truncate_setsize().
> Looks like other fs calls it before truncate_setsize().
Yes, Right. I will change it.
Thanks for review.
> 
> Thanks.
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>