linux-ext4 - Re: [PATCH] ext4: refactor ext4_move

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20140729142556.GB2256@thunk.org>
Date:	Tue, 29 Jul 2014 10:25:56 -0400
From:	Theodore Ts'o <tytso@....edu>
To:	Dmitry Monakhov <dmonakhov@...nvz.org>
Cc:	linux-ext4@...r.kernel.org
Subject: Re: [PATCH] ext4: refactor ext4_move_extents code base v2

On Wed, Jul 23, 2014 at 01:00:22AM +0400, Dmitry Monakhov wrote:
> ext4_move_extents is too complex for review. It has duplicate almost each funciton
> available in the rest of other codebase. It has useless artifical restriction
> orig_offset == donor_offset. But in fact logic of ext4_move_extents
> is very simple:

Thanks for working to clean this up.  ext4_move_extents() is a
complicated hot mess.


> +
> +int
> +ext4_swap_extents(handle_t *handle, struct inode *o_inode,
> +		     struct inode *d_inode, ext4_lblk_t o_off, ext4_lblk_t d_off,
> +		     ext4_lblk_t count, int *erp)
> +{
> +	struct ext4_ext_path *o_path = NULL;
> +	struct ext4_ext_path *d_path = NULL;
> +	int replaced_count = 0;

We should document when the locking requirements are for
ext4_swap_extents.  Better yet, there should an assertion here so we
BUG if the appropriate locks (i_data_sem, i_mutex, etc.) aren't taken.

Why are you using the parameter names o_off and d_off?  "off" can make
someone think of byte offsets (which is where my mind went at first).
Maybe o_lblk and d_lblk instead?

> +		/* ext4_split_extent_at() may retult in leaf extent split,
> +		 * path must to be revalidated. */
> +		if (split)
> +			goto  repeat;
                            ^^^
Whitespace?

> +		/* If extents has different length we have to update i_blocks */

Why would the extents ever have different lengths?  Should this
(already) be disallowed above?


> +	repeat:
> +		if (o_path) {
> +			ext4_ext_drop_refs(o_path);
> +			kfree(o_path);
> +			o_path = NULL;
> +		}
> +		if (d_path) {
> +			ext4_ext_drop_refs(d_path);
> +			kfree(d_path);
> +			d_path = NULL;
> +		}
> +		o_off += len;
> +		d_off += len;
> +		replaced_count += len;
> +		count -= len;
> +	}

Why not swap things so this reads:

		o_off += len;
		d_off += len;
		replaced_count += len;
		count -= len;
	repeat:
		if (o_path) {
			ext4_ext_drop_refs(o_path);
			kfree(o_path);
			o_path = NULL;
		}
		if (d_path) {
			ext4_ext_drop_refs(d_path);
			kfree(d_path);
			d_path = NULL;
		}
	}

This way we don't have to rely on len == 0, and in one of the places
where the code currently reads "{ len = 0; goto repeat; }", the len=0;
assignment can be dropped.

Cheers,

						- Ted
--
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