On Feb 17, 2014, at 8:08 AM, Lukas Czerner <lczerner@xxxxxxxxxx> wrote:
> Currently in ext4_fallocate we would update inode size, c_time and sync
> the file with every partial allocation which is entirely unnecessary. It
> is true that if the crash happens in the middle of truncate we might end
> up with unchanged i size, or c_time which I do not think is really a
> problem - it does not mean file system corruption in any way. Note that
> xfs is doing things the same way e.g. update all of the mentioned after
> the allocation is done.
I'm OK with this part.
> This commit moves all the updates after the allocation is done. In
> addition we also need to change m_time as not only inode has been change
> bot also data regions might have changed (unwritten extents).
I don't necessarily agree about this. Calling fallocate() will not
change the user-visible data at all, so there is no reason to e.g.
do a new backup of the file or reprocess the contents, or any other
reason that an application cares about a changed mtime.
Cheers, Andreas
> Also we do
> not need to be paranoid about changing the c_time and m_time only if the
> actual allocation have happened, we can change it even if we try to
> allocate only to find out that there are already block allocated. It's
> not really a big deal and it will save us some additional complexity.
>
> Also use ext4_debug, instead of ext4_warning in #ifdef EXT4FS_DEBUG
> section.
>
> Signed-off-by: Lukas Czerner <lczerner@xxxxxxxxxx>
> ---
> fs/ext4/extents.c | 86 +++++++++++++++++++++----------------------------------
> 1 file changed, 32 insertions(+), 54 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 10cff47..6a52851 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4513,36 +4513,6 @@ retry:
> ext4_std_error(inode->i_sb, err);
> }
>
> -static void ext4_falloc_update_inode(struct inode *inode,
> - int mode, loff_t new_size, int update_ctime)
> -{
> - struct timespec now;
> -
> - if (update_ctime) {
> - now = current_fs_time(inode->i_sb);
> - if (!timespec_equal(&inode->i_ctime, &now))
> - inode->i_ctime = now;
> - }
> - /*
> - * Update only when preallocation was requested beyond
> - * the file size.
> - */
> - if (!(mode & FALLOC_FL_KEEP_SIZE)) {
> - if (new_size > i_size_read(inode))
> - i_size_write(inode, new_size);
> - if (new_size > EXT4_I(inode)->i_disksize)
> - ext4_update_i_disksize(inode, new_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.
> - */
> - if (new_size > i_size_read(inode))
> - ext4_set_inode_flag(inode, EXT4_INODE_EOFBLOCKS);
> - }
> -
> -}
> -
> /*
> * preallocate space for a file. This implements ext4's fallocate file
> * operation, which gets called from sys_fallocate system call.
> @@ -4554,7 +4524,7 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
> {
> struct inode *inode = file_inode(file);
> handle_t *handle;
> - loff_t new_size;
> + loff_t new_size = 0;
> unsigned int max_blocks;
> int ret = 0;
> int ret2 = 0;
> @@ -4594,12 +4564,15 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
> */
> credits = ext4_chunk_trans_blocks(inode, max_blocks);
> mutex_lock(&inode->i_mutex);
> - ret = inode_newsize_ok(inode, (len + offset));
> - if (ret) {
> - mutex_unlock(&inode->i_mutex);
> - trace_ext4_fallocate_exit(inode, offset, max_blocks, ret);
> - return ret;
> +
> + if (!(mode & FALLOC_FL_KEEP_SIZE) &&
> + offset + len > i_size_read(inode)) {
> + new_size = offset + len;
> + ret = inode_newsize_ok(inode, new_size);
> + if (ret)
> + goto out;
> }
> +
> flags = EXT4_GET_BLOCKS_CREATE_UNINIT_EXT;
> if (mode & FALLOC_FL_KEEP_SIZE)
> flags |= EXT4_GET_BLOCKS_KEEP_SIZE;
> @@ -4623,28 +4596,14 @@ retry:
> }
> ret = ext4_map_blocks(handle, inode, &map, flags);
> if (ret <= 0) {
> -#ifdef EXT4FS_DEBUG
> - ext4_warning(inode->i_sb,
> - "inode #%lu: block %u: len %u: "
> - "ext4_ext_map_blocks returned %d",
> - inode->i_ino, map.m_lblk,
> - map.m_len, ret);
> -#endif
> + ext4_debug("inode #%lu: block %u: len %u: "
> + "ext4_ext_map_blocks returned %d",
> + inode->i_ino, map.m_lblk,
> + map.m_len, ret);
> ext4_mark_inode_dirty(handle, inode);
> ret2 = ext4_journal_stop(handle);
> break;
> }
> - if ((map.m_lblk + ret) >= (EXT4_BLOCK_ALIGN(offset + len,
> - blkbits) >> blkbits))
> - new_size = offset + len;
> - else
> - new_size = ((loff_t) map.m_lblk + ret) << blkbits;
> -
> - ext4_falloc_update_inode(inode, mode, new_size,
> - (map.m_flags & EXT4_MAP_NEW));
> - ext4_mark_inode_dirty(handle, inode);
> - if ((file->f_flags & O_SYNC) && ret >= max_blocks)
> - ext4_handle_sync(handle);
> ret2 = ext4_journal_stop(handle);
> if (ret2)
> break;
> @@ -4654,6 +4613,25 @@ retry:
> ret = 0;
> goto retry;
> }
> +
> + handle = ext4_journal_start(inode, EXT4_HT_INODE, 2);
> + if (IS_ERR(handle))
> + goto out;
> +
> + inode->i_mtime = inode->i_ctime = ext4_current_time(inode);
> +
> + if (ret > 0 && new_size) {
> + if (new_size > i_size_read(inode))
> + i_size_write(inode, new_size);
> + if (new_size > EXT4_I(inode)->i_disksize)
> + ext4_update_i_disksize(inode, new_size);
> + }
> + ext4_mark_inode_dirty(handle, inode);
> + if (file->f_flags & O_SYNC)
> + ext4_handle_sync(handle);
> +
> + ext4_journal_stop(handle);
> +out:
> mutex_unlock(&inode->i_mutex);
> trace_ext4_fallocate_exit(inode, offset, max_blocks,
> ret > 0 ? ret2 : ret);
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP using GPGMail
_______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
