linux-ext4 - Re: [PATCH] ext4: restart ext4_ext_remove_space() after transaction restart V2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 26 May 2010 21:22:39 +0400
From:	Dmitry Monakhov <dmonakhov@...nvz.org>
To:	tytso@....edu
Cc:	linux-ext4@...r.kernel.org, jack@...e.cz,
	aneesh.kumar@...ux.vnet.ibm.com
Subject: Re: [PATCH] ext4: restart ext4_ext_remove_space() after transaction 
	restart V2

On Wed, May 26, 2010 at 6:47 PM,  <tytso@....edu> wrote:
> On Wed, May 26, 2010 at 10:45:45AM -0400, tytso@....EDU wrote:
>> OK, but why not have the truncate *always* restart its job after
>> restarting the transaction?  #1, it's relatively rare in most
>> workloads that we need to restart the transaction at all in the first
>> place, and #2, it's easier to test if we always restart the truncate,
>> and #3, it's not like we'll be doing that much extra work if we
>> restart the truncate and the file wasn't extended significantly...
>>
>
> Like this.... (note how much simpler this patch is compared to earlier
> versions)
I've added statistic counters to truncate code and found that each
4'th rm_leaf() result in
actual transaction restart, and number of truncate/get_block races is
less than 0.001 from total
restart number and may be neglected.  So simplified version perform
false restart for each 4'th truncate.
But fairly to say what this is still invisible in terms of total truncate time.
Do we care about performance in truncate? IMHO it is not the hottest
path so it is possible to end up with
simplified version.

>
>                                                - Ted
>
> ext4: restart ext4_ext_remove_space() after transaction restart
>
> From: Dmitry Monakhov <dmonakhov@...nvz.org>
>
> If i_data_sem was internally dropped due to transaction restart, it is
> necessary to restart path look-up because extents tree was possibly
> modified by ext4_get_block().
>
> https://bugzilla.kernel.org/show_bug.cgi?id=15827
>
> Signed-off-by: Dmitry Monakhov <dmonakhov@...nvz.org>
> Signed-off-by: "Theodore Ts'o" <tytso@....edu>
> Acked-by: Jan Kara <jack@...e.cz>
> ---
>  fs/ext4/extents.c |   16 +++++++++-------
>  1 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index ffcaa11..0a47bec 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -107,11 +107,8 @@ static int ext4_ext_truncate_extend_restart(handle_t *handle,
>        if (err <= 0)
>                return err;
>        err = ext4_truncate_restart_trans(handle, inode, needed);
> -       /*
> -        * We have dropped i_data_sem so someone might have cached again
> -        * an extent we are going to truncate.
> -        */
> -       ext4_ext_invalidate_cache(inode);
> +       if (err == 0)
> +               err = -EAGAIN;
>
>        return err;
>  }
> @@ -2359,7 +2356,7 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
>        int depth = ext_depth(inode);
>        struct ext4_ext_path *path;
>        handle_t *handle;
> -       int i = 0, err = 0;
> +       int i, err;
>
>        ext_debug("truncate since %u\n", start);
>
> @@ -2368,23 +2365,26 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
>        if (IS_ERR(handle))
>                return PTR_ERR(handle);
>
> +again:
>        ext4_ext_invalidate_cache(inode);
>
>        /*
>         * We start scanning from right side, freeing all the blocks
>         * after i_size and walking into the tree depth-wise.
>         */
> +       depth = ext_depth(inode);
>        path = kzalloc(sizeof(struct ext4_ext_path) * (depth + 1), GFP_NOFS);
>        if (path == NULL) {
>                ext4_journal_stop(handle);
>                return -ENOMEM;
>        }
> +       path[0].p_depth = depth;
>        path[0].p_hdr = ext_inode_hdr(inode);
>        if (ext4_ext_check(inode, path[0].p_hdr, depth)) {
>                err = -EIO;
>                goto out;
>        }
> -       path[0].p_depth = depth;
> +       i = err = 0;
>
>        while (i >= 0 && err == 0) {
>                if (i == depth) {
> @@ -2478,6 +2478,8 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
>  out:
>        ext4_ext_drop_refs(path);
>        kfree(path);
> +       if (err == -EAGAIN)
> +               goto again;
>        ext4_journal_stop(handle);
>
>        return err;
>
--
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