Date:	Wed, 2 Sep 2009 22:32:07 -0700
From:	Jiaying Zhang <jiayingz@...gle.com>
To:	Andreas Dilger <adilger@....com>
Cc:	Theodore Tso <tytso@....edu>, Frank Mayhar <fmayhar@...gle.com>,
	Eric Sandeen <sandeen@...hat.com>,
	Curt Wohlgemuth <curtw@...gle.com>,
	ext4 development <linux-ext4@...r.kernel.org>
Subject: Re: Question on fallocate/ftruncate sequence

On Wed, Sep 2, 2009 at 10:20 PM, Jiaying Zhang<jiayingz@...gle.com> wrote:
> On Wed, Sep 2, 2009 at 1:41 AM, Andreas Dilger<adilger@....com> wrote:
>> 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.
>
> We were thinking to clear this flag when we need to allocate new
> blocks, but I was not sure how to get the current max allocated
> block -- that is mostly because I just started working on the ext4
> code. After digging into the ext4 allocation code today, I think we
> can put the check&clear in ext4_ext_get_blocks:
>
> @@ -2968,6 +2968,14 @@ int ext4_ext_get_blocks(handle_t *handle
>        newex.ee_len = cpu_to_le16(ar.len);
>        if (create == EXT4_CREATE_UNINITIALIZED_EXT)  /* Mark uninitialized */
>                ext4_ext_mark_uninitialized(&newex);
> +
> +       if (unlikely(inode->i_flags & EXT4_EOFBLOCKS_FL)) {
> +               BUG_ON(!eh->eh_entries);
> +               last_ex = EXT_LAST_EXTENT(eh);
> +               if (iblock + max_blocks > le32_to_cpu(last_ex->ee_block)
> +                                       + ext4_ext_get_actual_len(last_ex))
> +                       inode->i_flags &= ~EXT4_EOFBLOCKS_FL;
> +       }
>        err = ext4_ext_insert_extent(handle, inode, path, &newex);
>        if (err) {
>                /* free data blocks we just allocated */
>
> Again, I just started looking at this part of code, so please let me know
> if I am in the right direction.
>
> Another thing I am not sure is whether we can allocate a non-data block,
> like extended attributes, beyond the current max block without changing
> the i_size. In that case, clearing the EOFBLOCKS flag will be wrong.
>
>>>  #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.
>
> So to make it user-settable, we need to add the handling in ext4_ioctl
> that calls vmtruncate when the flag to be cleared. But how can we get
> the right size to truncate in that case? Can we just set that to the
> max initialized block shift with block size? But that may also truncate
> the blocks reserved without the KEEP_SIZE flag.

Never mind, that is a stupid question. We can just truncate to the
current i_size.

Jiaying

>
> Jiaying
>
>>
>> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists