On Aug 31, 2009 16:33 -0700, Jiaying Zhang wrote:
> > EXT4_KEEPSIZE_FL should only be cleared if there were writes to
> > the end of the fallocated space. In that regard, I think the name
> > of this flag should be changed to something like "EXT4_EOFBLOCKS_FL"
> > to indicate that blocks are allocated beyond the end of file (i_size).
>
> Thanks for catching this! I changed the patch to only clear the flag
> when the new_size is larger than i_size and changed the flag name
> as you suggested. It would be nice if we only clear the flag when we
> write beyond the fallocated space, but this seems hard to detect
> because we no longer have the allocated size once that keepsize
> fallocate call returns.
The problem is that if e2fsck depends on the EXT4_EOFBLOCKS_FL set
for fallocate-beyond-EOF then it is worse to clear it than to leave
it set. At worst, leaving the flag set results in too many truncates
on the file. Clearing the flag when not correct may result in user
visible data corruption if the file size is extended...
> Here is the new patch:
>
> --- .pc/fallocate_keepsizse.patch/fs/ext4/extents.c 2009-08-31
> 12:08:10.000000000 -0700
> +++ fs/ext4/extents.c 2009-08-31 15:51:13.000000000 -0700
> @@ -3091,11 +3091,19 @@ static void ext4_falloc_update_inode(str
> * the file size.
> */
> if (!(mode & FALLOC_FL_KEEP_SIZE)) {
> + if (new_size > i_size_read(inode)) {
> i_size_write(inode, new_size);
> + inode->i_flags &= ~EXT4_EOFBLOCKS_FL;
This again isn't quite correct, since the EOFBLOCKS_FL shouldn't
be cleared unless new_size is beyond the allocated size. The
allocation code itself might be a better place to clear this,
since it knows whether there were new blocks being added beyond
the current max allocated block.
> +#define FS_EOFBLOCKS_FL 0x00200000 /* Blocks allocated beyond EOF */
> #define FS_RESERVED_FL 0x80000000 /* reserved for ext2 lib */
>
> #define FS_FL_USER_VISIBLE 0x0003DFFF /* User visible flags */
It probably isn't a bad idea to make this flag user-visible, since it
would allow scanning for files that have excess space reserved (e.g.
if the filesystem is getting full). Making it user-settable (i.e.
clearable) would essentially mean truncating the file to i_size without
updating the timestamps so that the reserved space is discarded. I
don't think there is any value in allowing a user to turn this flag on
for a file.
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
