Re: [PATCH] e4defrag: fallocate donor file only once

[Peng Tao <bergwolf@gmail.com>]

On Thu, Sep 3, 2009 at 5:30 PM, Greg Freemyer<greg.freemyer@gmail.com> wrote:
> On Thu, Sep 3, 2009 at 4:00 AM, Peng Tao<bergwolf@gmail.com> wrote:
>> On Thu, Sep 3, 2009 at 6:09 AM, Greg Freemyer<greg.freemyer@gmail.com> wrote:
>>> On Wed, Sep 2, 2009 at 11:35 AM, Peng Tao<bergwolf@gmail.com> wrote:
>>>> If we allocate the donor file once for all, it will have a better chance
>>>> to be continuous.
>>>>
>>>> Signed-off-by: "Peng Tao" <bergwolf@gmail.com>
>>>
>>> Seems like an improvement, but I'm not seeing any special handling for
>>> sparse files.  (Not before or after this patch.)
>>>
>>> Seems like there should be an outer loop that identifies contiguous
>>> data block sets in a sparse file and defrags them individually as
>>> opposed to trying to defrag the entire file at once.
>>>
>>> My impression is that with a large sparse file, e4defrag currently
>>> (with or without this patch) would fallocate a full non-sparse donor
>>> set of blocks the full size of the original file, then swap in just
>>> the truly allocated blocks?
>> Thanks for the reminder. The original code takes good care of sparse
>> files in join_extents(). Please ignore my patch.
>>
>> Sorry for the noise.
>
> RFC from a more ext4 knowledgeable person than me:
>
> The code in e4defrag still looks way to complex.  I don't see why it
> needs to know so much about extents and groups.
>
> I just looked at util/copy_sparse.c
>
> It simply loops through all the blocks in the source file and calls
> ioctl(fd, FIBMAP, &b) to see if they are allocated vs. sparse,
>
> If allocated it copies the block from src to dest.  Pretty straight
> forward and has none of the complexity of e4defrag.
>
> Seems to me e4defrag should have the actual defrag_file() rewritten to
> be something like:
>
> defrag_file()  {
>    loop through the blocks looking for the contiguous set of data blocks.
>          defrag_contiguous_data(start_block, num_blocks)
> }
>
> defrag_contiguous_data(start_block, num_blocks) {
>    // allocate one full extent at a time and donate the blocks to orig file
>    for(start=start_block; start < start_block, num_blocks; start+=chunk) {
>          fallocate(chunk);
>          move_ext(orig, donor, start, 0, chunk);
>      }
> }
>
> And then set chunk to be the max size of one extent.  Maybe the
> "chunk" should be bigger than one extent?
>
> Also, I did not put any logic in above to show testing to see if the
> new file is less fragmented than the original.  That will add to the
> complexity, but hopefully the actual defrag logic can be as relatively
> simple as the above instead of what it is now.
>
> Anyway, t would be a major change to e4defrag, but it seems that it
> would give ext4 a much better chance to reorganize itself by calling
> fallocate on full extent size chunks at minimum, instead of what the
> code currently does.
Hi, Greg,

The current e4defrag is doing most of work exactly same as your RFC,
and in a nicer manner. If you look into the code path, you'll see that
its logic is very much like the RFC except that it first fallocates a
donor file to see if a defragmentation is really necessary so it won't
have to fall back during defragmentation, which IMO is a good design
point.

Please correct me if I understand anything wrong.

>
> Greg
>

-- 
Cheers,
Peng Tao
State Key Laboratory of Networking and Switching Technology
Beijing Univ. of Posts and Telecoms.
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html