On 09/03/11 18:23, Jim Meyering wrote:
> Pádraig Brady wrote:
>> - i--;
>> - if (scan->ext_info[i].ext_flags & FIEMAP_EXTENT_LAST)
>> + scan->ei_count = si;
>> +
>> + si--;
>> + if (scan->ext_info[si].ext_flags & FIEMAP_EXTENT_LAST)
>> {
>> scan->hit_final_extent = true;
>> return true;
>> }
>>
>> + i--;
>> scan->scan_start = fm_extents[i].fe_logical + fm_extents[i].fe_length;
>
> The above is all fine, but unless you know that scan->ei_count
> is always positive, seeing "i" and "si" used as indices right
> after being decremented may make you think there's a risk
> of accessing some_buffer[-1].
>
> What do you think about adding an assertion, either on scan->ei_count
> before the loop, or on i and/or si after it?
Yes, it's not immediately obvious that i and si >= 1,
but it's guaranteed by the early return when fiemap->fm_mapped_extents==0.
Because of that return, an assert is overkill I think.
Actually I think we can simplify further by just
using a pointer to the last extent_info entry we're updating.
I also noticed that we shouldn't care about
the FIEMAP_EXTENT_LAST flag when merging.
Both changes are in the latest version attached.
cheers,
Pádraig.
>From 561c261ea5a9d602b752a837b307bcc397b9d6cc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com>
Date: Mon, 7 Mar 2011 08:34:35 +0000
Subject: [PATCH] copy: merge similar extents before processing
* src/extent-scan.c (extent_scan_read): Merge adjacent extents
that vary only in size, so that we may process them more efficiently.
This will be especially useful when we introduce fallocate()
to allocate extents in the destination.
---
src/extent-scan.c | 35 ++++++++++++++++++++++++-----------
1 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/src/extent-scan.c b/src/extent-scan.c
index 1ba59db..d32574a 100644
--- a/src/extent-scan.c
+++ b/src/extent-scan.c
@@ -85,24 +85,37 @@ extent_scan_read (struct extent_scan *scan)
scan->ei_count = fiemap->fm_mapped_extents;
scan->ext_info = xnmalloc (scan->ei_count, sizeof (struct extent_info));
- unsigned int i;
+ unsigned int i, si = 0;
+ struct extent_info *last_ei IF_LINT ( = NULL);
+
for (i = 0; i < scan->ei_count; i++)
{
assert (fm_extents[i].fe_logical <= OFF_T_MAX);
- scan->ext_info[i].ext_logical = fm_extents[i].fe_logical;
- scan->ext_info[i].ext_length = fm_extents[i].fe_length;
- scan->ext_info[i].ext_flags = fm_extents[i].fe_flags;
+ if (si && last_ei->ext_flags ==
+ (fm_extents[i].fe_flags & ~FIEMAP_EXTENT_LAST)
+ && (last_ei->ext_logical + last_ei->ext_length
+ == fm_extents[i].fe_logical))
+ {
+ /* Merge previous with last. */
+ last_ei->ext_length += fm_extents[i].fe_length;
+ }
+ else
+ {
+ last_ei = scan->ext_info + si;
+ last_ei->ext_logical = fm_extents[i].fe_logical;
+ last_ei->ext_length = fm_extents[i].fe_length;
+ last_ei->ext_flags = fm_extents[i].fe_flags;
+ si++;
+ }
}
- i--;
- if (scan->ext_info[i].ext_flags & FIEMAP_EXTENT_LAST)
- {
- scan->hit_final_extent = true;
- return true;
- }
+ scan->ei_count = si;
- scan->scan_start = fm_extents[i].fe_logical + fm_extents[i].fe_length;
+ if (last_ei->ext_flags & FIEMAP_EXTENT_LAST)
+ scan->hit_final_extent = true;
+ else
+ scan->scan_start = last_ei->ext_logical + last_ei->ext_length;
return true;
}
--
1.7.4
