-------- Original Message --------
Subject: Re: [PATCH] btrfs: Fix and enhance merge_extent_mapping() to
insert best fitted extent map
From: Liu Bo <bo.li.liu@xxxxxxxxxx>
To: Qu Wenruo <quwenruo@xxxxxxxxxxxxxx>
Date: 2014年09月18日 16:20
On Thu, Sep 18, 2014 at 03:58:26PM +0800, Qu Wenruo wrote:
[...]
original: (start >= existing->start && start < extent_map_end(existing))
this patch: (start < extent_map_end(existing) && start + len > existing->start)
(start + len > existing->start) doesn't equal to start >= existing->start,
here is a case of (start+len > existing->start) but (start <= existing->start).
|--------| -->(existing)
|--------| -->[start, start+len)
And calling search_extent_mapping() doesn't make sure that
(start >= existing->start) is true, either.
All right, that case is right and I'm wrong.
I'll change the if() to use start >= existing->start.
BTW, Any other problem?
Others look good.
thanks,
-liubo
Would you mind me adding your reviewed-by?
Thanks
Qu
Thanks,
Qu
And one of overlapping cases is (existing->start > start), ie. em->start > start, this is
against our rule of btrfs_get_extent,
Nope again, this overlapping in fact is quite normal in multithread
random read/write.
The files's [0~16) is a preallocated one,
Thread A:
write [4K, 8K) into the file, but not committed yet.
extent map tree contains [0,16K) only
Thread B:
btrfs_get_extent()
the map_start is 8K, len is 4K as an example
grab a large em, take [0,16K), since [4K,8K) write is not committed.
comes to insert: btrfs_release_path(path);
Thread A:
[4K, 8K) is not committed
the extent map is now [0, 4K) [4K, 8K) [8K, 16K).
Thread B:
goes to insert: add_extent_mapping()
the [0,16K) is overlapping, and the returned existing one is [8K, 16K).
which contains the [map_start, map_start + len).
So this's an example of existing->start == start (both are 8K), not
existing->start > start.
See __extent_writepage_io(),
{
...
em = epd->get_extent(inode, page, pg_offset, cur,
end - cur + 1, 1);
if (IS_ERR_OR_NULL(em)) {
SetPageError(page);
ret = PTR_ERR_OR_ZERO(em);
break;
}
extent_offset = cur - em->start;
^^^^^^^^^^^^^^^^^^^^^^^^^^^ it needs to be (em->start <= cur)
...
}
thanks,
-liubo
struct extent_map *btrfs_get_extent(...)
{
[...]
insert:
btrfs_release_path(path);
if (em->start > start || extent_map_end(em) <= start) {
btrfs_err(root->fs_info, "bad extent! em: [%llu %llu] passed
[%llu %llu]",
em->start, em->len, start, len);
err = -EIO;
goto out;
}
[...]
}
thanks,
-liubo
+ /*
+ * The existing extent map is the one nearest to
+ * the [start, start + len) range which overlaps
+ */
+ err = merge_extent_mapping(em_tree, existing,
+ em, start);
free_extent_map(existing);
- existing = NULL;
- }
- if (!existing) {
- existing = lookup_extent_mapping(em_tree, em->start,
- em->len);
- if (existing) {
- err = merge_extent_mapping(em_tree, existing,
- em, start);
- free_extent_map(existing);
- if (err) {
- free_extent_map(em);
- em = NULL;
- }
- } else {
- err = -EIO;
+ if (err) {
free_extent_map(em);
em = NULL;
}
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
