| Message ID | 20171221224256.18196-2-bo.li.liu@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 22.12.2017 00:42, Liu Bo wrote: > This is a prepare work for the following extent map selftest, which > runs tests against em merge logic. > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > --- > fs/btrfs/ctree.h | 2 ++ > fs/btrfs/inode.c | 101 ++++++++++++++++++++++++++++++------------------------- > 2 files changed, 58 insertions(+), 45 deletions(-) > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index b2e09fe..328f40f 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -3148,6 +3148,8 @@ struct btrfs_delalloc_work *btrfs_alloc_delalloc_work(struct inode *inode, > int delay_iput); > void btrfs_wait_and_free_delalloc_work(struct btrfs_delalloc_work *work); > > +int btrfs_add_extent_mapping(struct extent_map_tree *em_tree, > + struct extent_map **em_in, u64 start, u64 len); > struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode, > struct page *page, size_t pg_offset, u64 start, > u64 len, int create); > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index e1a7f3c..527df6f 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -6911,6 +6911,61 @@ static noinline int uncompress_inline(struct btrfs_path *path, > return ret; > } > > +int btrfs_add_extent_mapping(struct extent_map_tree *em_tree, > + struct extent_map **em_in, u64 start, u64 len) How about adding the following comment above the function: /** * btrfs_add_extent_mapping - try to add given extent mapping * @em_tree - the extent tree into which we want to add the mapping * @em_in - extent we are inserting * @start - the start of the logical range of the extent we are adding * @len - logical length of the extent * * Insert @em_in into @em_tree. In case there is an overlapping range, handle * the -EEXIST by either: * a) Returning the existing extent in @em_in if there is a full overlap * b) Merge the extents if they are near each other. * * Returns 0 on success or a negative error code * */ Also one thing which I'm not very clear is why do we need the start/len, aren't those already set in em_in ? > +{ > + int ret; > + struct extent_map *em = *em_in; > + > + ret = add_extent_mapping(em_tree, em, 0); > + /* it is possible that someone inserted the extent into the tree > + * while we had the lock dropped. It is also possible that > + * an overlapping map exists in the tree > + */ > + if (ret == -EEXIST) { > + struct extent_map *existing; > + > + ret = 0; > + > + existing = search_extent_mapping(em_tree, start, len); > + > + /* > + * existing will always be non-NULL, since there must be > + * extent causing the -EEXIST. > + */ > + if (existing->start == em->start && > + extent_map_end(existing) >= extent_map_end(em) && > + em->block_start == existing->block_start) { > + /* > + * The existing extent map already encompasses the > + * entire extent map we tried to add. > + */ > + free_extent_map(em); > + *em_in = existing; > + ret = 0; > + } else if (start >= extent_map_end(existing) || > + start <= existing->start) { > + /* > + * The existing extent map is the one nearest to > + * the [start, start + len) range which overlaps > + */ > + ret = merge_extent_mapping(em_tree, existing, > + em, start); > + free_extent_map(existing); > + if (ret) { > + free_extent_map(em); > + *em_in = NULL; > + } > + } else { > + free_extent_map(em); > + *em_in = existing; > + ret = 0; > + } > + } > + ASSERT(ret == 0 || ret == -EEXIST); > + return ret; > +} > + > /* > * a bit scary, this does extent mapping from logical file offset to the disk. > * the ugly parts come from merging extents from the disk with the in-ram > @@ -7147,51 +7202,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode, > > err = 0; > write_lock(&em_tree->lock); > - ret = add_extent_mapping(em_tree, em, 0); > - /* it is possible that someone inserted the extent into the tree > - * while we had the lock dropped. It is also possible that > - * an overlapping map exists in the tree > - */ > - if (ret == -EEXIST) { > - struct extent_map *existing; > - > - ret = 0; > - > - existing = search_extent_mapping(em_tree, start, len); > - /* > - * existing will always be non-NULL, since there must be > - * extent causing the -EEXIST. > - */ > - if (existing->start == em->start && > - extent_map_end(existing) >= extent_map_end(em) && > - em->block_start == existing->block_start) { > - /* > - * The existing extent map already encompasses the > - * entire extent map we tried to add. > - */ > - free_extent_map(em); > - em = existing; > - err = 0; > - > - } else if (start >= extent_map_end(existing) || > - start <= existing->start) { > - /* > - * 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); > - if (err) { > - free_extent_map(em); > - em = NULL; > - } > - } else { > - free_extent_map(em); > - em = existing; > - err = 0; > - } > - } > + err = btrfs_add_extent_mapping(em_tree, &em, start, len); > write_unlock(&em_tree->lock); > out: > > -- 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
On Fri, Dec 22, 2017 at 09:23:40AM +0200, Nikolay Borisov wrote: > > > On 22.12.2017 00:42, Liu Bo wrote: > > This is a prepare work for the following extent map selftest, which > > runs tests against em merge logic. > > > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > > --- > > fs/btrfs/ctree.h | 2 ++ > > fs/btrfs/inode.c | 101 ++++++++++++++++++++++++++++++------------------------- > > 2 files changed, 58 insertions(+), 45 deletions(-) > > > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > > index b2e09fe..328f40f 100644 > > --- a/fs/btrfs/ctree.h > > +++ b/fs/btrfs/ctree.h > > @@ -3148,6 +3148,8 @@ struct btrfs_delalloc_work *btrfs_alloc_delalloc_work(struct inode *inode, > > int delay_iput); > > void btrfs_wait_and_free_delalloc_work(struct btrfs_delalloc_work *work); > > > > +int btrfs_add_extent_mapping(struct extent_map_tree *em_tree, > > + struct extent_map **em_in, u64 start, u64 len); > > struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode, > > struct page *page, size_t pg_offset, u64 start, > > u64 len, int create); > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > > index e1a7f3c..527df6f 100644 > > --- a/fs/btrfs/inode.c > > +++ b/fs/btrfs/inode.c > > @@ -6911,6 +6911,61 @@ static noinline int uncompress_inline(struct btrfs_path *path, > > return ret; > > } > > > > +int btrfs_add_extent_mapping(struct extent_map_tree *em_tree, > > + struct extent_map **em_in, u64 start, u64 len) > > How about adding the following comment above the function: > > /** > * btrfs_add_extent_mapping - try to add given extent mapping > * @em_tree - the extent tree into which we want to add the mapping > * @em_in - extent we are inserting > * @start - the start of the logical range of the extent we are adding > * @len - logical length of the extent > * > * Insert @em_in into @em_tree. In case there is an overlapping range, handle > * the -EEXIST by either: > * a) Returning the existing extent in @em_in if there is a full overlap > * b) Merge the extents if they are near each other. > * > * Returns 0 on success or a negative error code > * > */ > Appreciate your comments. Sure, comments are always welcome. > Also one thing which I'm not very clear is why do we need the start/len, aren't > those already set in em_in ? > [start, start+len) is within *em_in, ie. |----*em_in--------| |------| start start+len What we really care about is [start, start+len), which is passed to btrfs_get_extent(). For example, if a file extent item on disk is [0, 32k), given a tuple (start=0, len=4k), reading [0, 4k) would end up with *em_in being [0, 32k). So if add_extent_mapping(*em_in) returns EEXIST, then we know the 'existing' em is overlapped with *em_in, then we need to check whether it's overlapped with [start, start+len). thanks, -liubo > > > > +{ > > + int ret; > > + struct extent_map *em = *em_in; > > + > > + ret = add_extent_mapping(em_tree, em, 0); > > + /* it is possible that someone inserted the extent into the tree > > + * while we had the lock dropped. It is also possible that > > + * an overlapping map exists in the tree > > + */ > > + if (ret == -EEXIST) { > > + struct extent_map *existing; > > + > > + ret = 0; > > + > > + existing = search_extent_mapping(em_tree, start, len); > > + > > + /* > > + * existing will always be non-NULL, since there must be > > + * extent causing the -EEXIST. > > + */ > > + if (existing->start == em->start && > > + extent_map_end(existing) >= extent_map_end(em) && > > + em->block_start == existing->block_start) { > > + /* > > + * The existing extent map already encompasses the > > + * entire extent map we tried to add. > > + */ > > + free_extent_map(em); > > + *em_in = existing; > > + ret = 0; > > + } else if (start >= extent_map_end(existing) || > > + start <= existing->start) { > > + /* > > + * The existing extent map is the one nearest to > > + * the [start, start + len) range which overlaps > > + */ > > + ret = merge_extent_mapping(em_tree, existing, > > + em, start); > > + free_extent_map(existing); > > + if (ret) { > > + free_extent_map(em); > > + *em_in = NULL; > > + } > > + } else { > > + free_extent_map(em); > > + *em_in = existing; > > + ret = 0; > > + } > > + } > > + ASSERT(ret == 0 || ret == -EEXIST); > > + return ret; > > +} > > + > > /* > > * a bit scary, this does extent mapping from logical file offset to the disk. > > * the ugly parts come from merging extents from the disk with the in-ram > > @@ -7147,51 +7202,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode, > > > > err = 0; > > write_lock(&em_tree->lock); > > - ret = add_extent_mapping(em_tree, em, 0); > > - /* it is possible that someone inserted the extent into the tree > > - * while we had the lock dropped. It is also possible that > > - * an overlapping map exists in the tree > > - */ > > - if (ret == -EEXIST) { > > - struct extent_map *existing; > > - > > - ret = 0; > > - > > - existing = search_extent_mapping(em_tree, start, len); > > - /* > > - * existing will always be non-NULL, since there must be > > - * extent causing the -EEXIST. > > - */ > > - if (existing->start == em->start && > > - extent_map_end(existing) >= extent_map_end(em) && > > - em->block_start == existing->block_start) { > > - /* > > - * The existing extent map already encompasses the > > - * entire extent map we tried to add. > > - */ > > - free_extent_map(em); > > - em = existing; > > - err = 0; > > - > > - } else if (start >= extent_map_end(existing) || > > - start <= existing->start) { > > - /* > > - * 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); > > - if (err) { > > - free_extent_map(em); > > - em = NULL; > > - } > > - } else { > > - free_extent_map(em); > > - em = existing; > > - err = 0; > > - } > > - } > > + err = btrfs_add_extent_mapping(em_tree, &em, start, len); > > write_unlock(&em_tree->lock); > > out: > > > > -- 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
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index b2e09fe..328f40f 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -3148,6 +3148,8 @@ struct btrfs_delalloc_work *btrfs_alloc_delalloc_work(struct inode *inode, int delay_iput); void btrfs_wait_and_free_delalloc_work(struct btrfs_delalloc_work *work); +int btrfs_add_extent_mapping(struct extent_map_tree *em_tree, + struct extent_map **em_in, u64 start, u64 len); struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode, struct page *page, size_t pg_offset, u64 start, u64 len, int create); diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index e1a7f3c..527df6f 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -6911,6 +6911,61 @@ static noinline int uncompress_inline(struct btrfs_path *path, return ret; } +int btrfs_add_extent_mapping(struct extent_map_tree *em_tree, + struct extent_map **em_in, u64 start, u64 len) +{ + int ret; + struct extent_map *em = *em_in; + + ret = add_extent_mapping(em_tree, em, 0); + /* it is possible that someone inserted the extent into the tree + * while we had the lock dropped. It is also possible that + * an overlapping map exists in the tree + */ + if (ret == -EEXIST) { + struct extent_map *existing; + + ret = 0; + + existing = search_extent_mapping(em_tree, start, len); + + /* + * existing will always be non-NULL, since there must be + * extent causing the -EEXIST. + */ + if (existing->start == em->start && + extent_map_end(existing) >= extent_map_end(em) && + em->block_start == existing->block_start) { + /* + * The existing extent map already encompasses the + * entire extent map we tried to add. + */ + free_extent_map(em); + *em_in = existing; + ret = 0; + } else if (start >= extent_map_end(existing) || + start <= existing->start) { + /* + * The existing extent map is the one nearest to + * the [start, start + len) range which overlaps + */ + ret = merge_extent_mapping(em_tree, existing, + em, start); + free_extent_map(existing); + if (ret) { + free_extent_map(em); + *em_in = NULL; + } + } else { + free_extent_map(em); + *em_in = existing; + ret = 0; + } + } + ASSERT(ret == 0 || ret == -EEXIST); + return ret; +} + /* * a bit scary, this does extent mapping from logical file offset to the disk. * the ugly parts come from merging extents from the disk with the in-ram @@ -7147,51 +7202,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode, err = 0; write_lock(&em_tree->lock); - ret = add_extent_mapping(em_tree, em, 0); - /* it is possible that someone inserted the extent into the tree - * while we had the lock dropped. It is also possible that - * an overlapping map exists in the tree - */ - if (ret == -EEXIST) { - struct extent_map *existing; - - ret = 0; - - existing = search_extent_mapping(em_tree, start, len); - /* - * existing will always be non-NULL, since there must be - * extent causing the -EEXIST. - */ - if (existing->start == em->start && - extent_map_end(existing) >= extent_map_end(em) && - em->block_start == existing->block_start) { - /* - * The existing extent map already encompasses the - * entire extent map we tried to add. - */ - free_extent_map(em); - em = existing; - err = 0; - - } else if (start >= extent_map_end(existing) || - start <= existing->start) { - /* - * 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); - if (err) { - free_extent_map(em); - em = NULL; - } - } else { - free_extent_map(em); - em = existing; - err = 0; - } - } + err = btrfs_add_extent_mapping(em_tree, &em, start, len); write_unlock(&em_tree->lock); out:
This is a prepare work for the following extent map selftest, which runs tests against em merge logic. Signed-off-by: Liu Bo <bo.li.liu@oracle.com> --- fs/btrfs/ctree.h | 2 ++ fs/btrfs/inode.c | 101 ++++++++++++++++++++++++++++++------------------------- 2 files changed, 58 insertions(+), 45 deletions(-)