Re: [PATCH] btrfs: Fix and enhance merge_extent_mapping() to insert best fitted extent map

[Liu Bo <bo.li.liu@oracle.com>]

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

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