Re: Question on fallocate/ftruncate sequence

[Andreas Dilger <adilger@sun.com>]

On Aug 31, 2009  12:40 -0700, Jiaying Zhang wrote:
> > It's better to define the flag as EXT4_KEEPSIZE_FL, and to use it as
> > EXT4_KEEPSIZE_FL, but make a note of that bitfield position as being
> > reserved in include/linux/fs.h.
> 
> Here is the modified patch based on your suggestions. I stick with the
> KEEPSIZE_FL approach that I think can allow us to handle the special
> truncation accordingly during fsck. Other file systems can also re-use
> this flag when they want to support fallocate with KEEP_SIZE. As you
> suggested, I moved the EXT4_KEEPSIZE_FL checking to ext4_setattr
> that now calls vmtruncate if the KEEPSIZE flag is set in the i_flag.
> Please let me know what you think about this proposed patch.
> 
> --- .pc/fallocate_keepsizse.patch/fs/ext4/extents.c	2009-08-31
> 12:08:10.000000000 -0700
> +++ fs/ext4/extents.c	2009-08-31 12:12:16.000000000 -0700
> @@ -3095,7 +3095,13 @@ static void ext4_falloc_update_inode(str
>  			i_size_write(inode, new_size);
>  		if (new_size > EXT4_I(inode)->i_disksize)
>  			ext4_update_i_disksize(inode, new_size);
> +		inode->i_flags &= ~EXT4_KEEPSIZE_FL;

Note that fallocate can be called multiple times for a file.  The
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).

>  	} else {
> +		/*
> +		 * Mark that we allocate beyond EOF so the subsequent truncate
> +		 * can proceed even if the new size is the same as i_size.
> +		 */
> +		inode->i_flags |= EXT4_KEEPSIZE_FL;

Similarly, this should only be done in case the fallocate is actually
beyond i_size.  While that is the most common case, it isn't necessarily
ALWAYS going to be true (e.g. if multiple threads are calling fallocate()
on a single file, or if a program always calls fallocate() on a file
without first checking if the file size is large enough).

> +++ include/linux/fs.h	2009-08-31 12:12:16.000000000 -0700
>  #define FS_DIRECTIO_FL			0x00100000 /* Use direct i/o */


> +++ fs/ext4/ext4.h	2009-08-31 12:12:16.000000000 -0700
>  #define EXT4_EXT_MIGRATE		0x00100000 /* Inode is migrating */

Should we redefine EXT4_EXT_MIGRATE not to conflict with FS_DIRECTIO_FL?
I don't think much, if any, use has been made of this flag, and I can
imagine a major headache in the future if this isn't changed now.

Also, EXT4_EXT_MIGRATE doesn't necessarily belong in the i_flags space,
since it is only used in-memory rather than on-disk as all of the others
are.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.