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