[v3,0/7] convert: rollback rework
diff mbox

Message ID 20170314115554.GU14605@twin.jikos.cz
State New
Headers show

Commit Message

David Sterba March 14, 2017, 11:55 a.m. UTC
On Tue, Mar 14, 2017 at 12:55:06PM +0100, David Sterba wrote:
> On Tue, Mar 14, 2017 at 04:05:02PM +0800, Qu Wenruo wrote:
> > v2:
> >   Abstract the original code to read out data in one btrfs file to
> >   btrfs_read_file().
> >   Use simple_range and btrfs_reserved_ranges[] to cleanup open code.
> > v3:
> >   Rebased to v4.10.
> >   Squash modification in later commits to their previous owner.
> >   Fix a converity report, which doesn't exit when an error is found in
> >   check_convert_image()
> >   Fix a lot of check scripts warning
> 
> I did not mention that explicitly under v2, but the series has been
> merged to devel so incremental changes should be sent from now on. I
> went through the patches almost line by line and fixed things here and
> there. The patchset-to-patchset diff is not large (attached).

Now attached.

Comments

Qu Wenruo March 15, 2017, 12:35 a.m. UTC | #1
At 03/14/2017 07:55 PM, David Sterba wrote:
> On Tue, Mar 14, 2017 at 12:55:06PM +0100, David Sterba wrote:
>> On Tue, Mar 14, 2017 at 04:05:02PM +0800, Qu Wenruo wrote:
>>> v2:
>>>   Abstract the original code to read out data in one btrfs file to
>>>   btrfs_read_file().
>>>   Use simple_range and btrfs_reserved_ranges[] to cleanup open code.
>>> v3:
>>>   Rebased to v4.10.
>>>   Squash modification in later commits to their previous owner.
>>>   Fix a converity report, which doesn't exit when an error is found in
>>>   check_convert_image()
>>>   Fix a lot of check scripts warning
>>
>> I did not mention that explicitly under v2, but the series has been
>> merged to devel so incremental changes should be sent from now on.

Sorry, but IIRC it's recommended to base the code to latest 
stable/release branch, so I rebase them all to v4.10 code base.
(Last time you said basing code upon devel is quite a bad behavior and 
makes merging painful)

And yes, it's quite a pain to rebase them. I totally understand the pain 
during the v4.9.1->v4.10 rebase.

>> I
>> went through the patches almost line by line and fixed things here and
>> there. The patchset-to-patchset diff is not large (attached).

Sorry for the trouble and thanks for the effort.

But I'm afraid it may still happen time by time.
As you already see, we have several pending big patchset for btrfs-progs.
(Lowmem mode repair, offline scrub, and maybe some other things)

So to save your time, could you info us before merging large patchset 
and provide a base commit for us to rebase for you?

I think this would save time for both.

Thanks,
Qu
>
> Now attached.
>
>


--
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
David Sterba March 27, 2017, 5:55 p.m. UTC | #2
On Wed, Mar 15, 2017 at 08:35:06AM +0800, Qu Wenruo wrote:
> At 03/14/2017 07:55 PM, David Sterba wrote:
> > On Tue, Mar 14, 2017 at 12:55:06PM +0100, David Sterba wrote:
> >> On Tue, Mar 14, 2017 at 04:05:02PM +0800, Qu Wenruo wrote:
> >>> v2:
> >>>   Abstract the original code to read out data in one btrfs file to
> >>>   btrfs_read_file().
> >>>   Use simple_range and btrfs_reserved_ranges[] to cleanup open code.
> >>> v3:
> >>>   Rebased to v4.10.
> >>>   Squash modification in later commits to their previous owner.
> >>>   Fix a converity report, which doesn't exit when an error is found in
> >>>   check_convert_image()
> >>>   Fix a lot of check scripts warning
> >>
> >> I did not mention that explicitly under v2, but the series has been
> >> merged to devel so incremental changes should be sent from now on.
> 
> Sorry, but IIRC it's recommended to base the code to latest 
> stable/release branch, so I rebase them all to v4.10 code base.

This applies to unmerged patchsets. Once it's in devel I do not intend
to remove it (unless there's something serious).

> (Last time you said basing code upon devel is quite a bad behavior and 
> makes merging painful)

This depends if your patchset builds on top of any changes in devel or
not. If not, any base branch should do (master, or devel). Moving a
patchset based on devel to a new rebased devel head is something I do
regularly. I do occasinally want to rebase devel, so the patch history
stays clean and we don't pile up fixups on fixups. What is not a good
practice is basing patchset on a 'next' branch of any sort. This usually
serves as a preview and any patch can disappear between two next
snapshots.

> And yes, it's quite a pain to rebase them. I totally understand the pain 
> during the v4.9.1->v4.10 rebase.
> 
> >> I
> >> went through the patches almost line by line and fixed things here and
> >> there. The patchset-to-patchset diff is not large (attached).
> 
> Sorry for the trouble and thanks for the effort.
> 
> But I'm afraid it may still happen time by time.

Of course, that's expected.

> As you already see, we have several pending big patchset for btrfs-progs.
> (Lowmem mode repair, offline scrub, and maybe some other things)
> 
> So to save your time, could you info us before merging large patchset 

If only I knew that myself :) but I'll try.

> and provide a base commit for us to rebase for you?

For big or intrusive patchsets it should be better to start on top of
master, as it's never rebased. Sometimes rebasing on top of devel
succeeds with minor conflicts, that's more like a bonus for the next
cycle if the patchset does not get merged.
--
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

Patch
diff mbox

diff --git a/convert/common.h b/convert/common.h
index 28ca14617303..0d3adeaa96cc 100644
--- a/convert/common.h
+++ b/convert/common.h
@@ -26,33 +26,6 @@ 
 #include "common-defs.h"
 #include "extent-cache.h"
 
-/*
- * Presents one simple continuous range.
- *
- * For multiple or non-continuous ranges, use extent_cache_tree from
- * extent-cache.c
- */
-struct simple_range {
-	u64 start;
-	u64 len;
-};
-
-const static struct simple_range btrfs_reserved_ranges[] = {
-	{0, SZ_1M},
-	{BTRFS_SB_MIRROR_OFFSET(1), SZ_64K},
-	{BTRFS_SB_MIRROR_OFFSET(2), SZ_64K}
-};
-
-/*
- * Simple range functions
- *
- * Get range end (exclusive)
- */
-static inline u64 range_end(const struct simple_range *range)
-{
-	return (range->start + range->len);
-}
-
 struct btrfs_mkfs_config;
 
 struct btrfs_convert_context {
diff --git a/convert/main.c b/convert/main.c
index e81a1a29a755..73c9d889ac27 100644
--- a/convert/main.c
+++ b/convert/main.c
@@ -17,8 +17,10 @@ 
  */
 
 /*
- * btrfs convert design:
+ * Btrfs convert design:
+ *
  * The overall design of btrfs convert is like the following:
+ *
  * |<------------------Old fs----------------------------->|
  * |<- used ->| |<- used ->|                    |<- used ->|
  *                           ||
@@ -29,39 +31,43 @@ 
  *
  * ODC    = Old data chunk, btrfs chunks containing old fs data
  *          Mapped 1:1 (logical address == device offset)
- * Old-FE = file extents points to old fs.
+ * Old-FE = file extents pointing to old fs.
  *
  * So old fs used space is (mostly) kept as is, while btrfs will insert
- * its chunk(Data/Meta/Sys) into large enough free space.
- * In this way, we can create different profiles for meta/data for converted fs.
+ * its chunk (Data/Meta/Sys) into large enough free space.
+ * In this way, we can create different profiles for metadata/data for
+ * converted fs.
  *
- * The DIRTY work is, we must reserve and relocate 3 ranges for btrfs:
- * [0, 1M), [btrfs_sb_offset(1), +64K), [btrfs_sb_offset(2), +64K)
+ * We must reserve and relocate 3 ranges for btrfs:
+ * * [0, 1M)                    - area never used for any data except the first
+ *                                superblock
+ * * [btrfs_sb_offset(1), +64K) - 1st superblock backup copy
+ * * [btrfs_sb_offset(2), +64K) - 2nd, dtto
  *
- * Most work are spent handling corner cases around these reserved ranges.
+ * Most work is spent handling corner cases around these reserved ranges.
  *
  * Detailed workflow is:
  * 1)   Scan old fs used space and calculate data chunk layout
  * 1.1) Scan old fs
- *      We can a map of used space of old fs
+ *      We can a map used space of old fs
  *
- * 1.2) Calculate data chunk layout <<< Complex work
- *      New data chunks must meet 3 conditions using result from 1.1)
+ * 1.2) Calculate data chunk layout - this is the hard part
+ *      New data chunks must meet 3 conditions using result fomr 1.1
  *      a. Large enough to be a chunk
- *      b. Doesn't cover reserved ranges
+ *      b. Doesn't intersect reserved ranges
  *      c. Covers all the remaining old fs used space
  *
- *      XXX: This can be simplified if we don't need to handle backup supers
+ *      NOTE: This can be simplified if we don't need to handle backup supers
  *
- * 1.3) Calculate usable space for btrfs new chunks
- *      Btrfs chunk usable space must meet 3 conditions using result from 1.2)
+ * 1.3) Calculate usable space for new btrfs chunks
+ *      Btrfs chunk usable space must meet 3 conditions using result from 1.2
  *      a. Large enough to be a chunk
- *      b. Doesn't cover reserved ranges
- *      c. Doesn't cover any data chunks in 1.1)
+ *      b. Doesn't intersect reserved ranges
+ *      c. Doesn't cover any data chunks in 1.1
  *
- * 2)   Create basic btrfs
- *      Initial metadata and sys chunks are inserted in the first available
- *      space of result of 1.3)
+ * 2)   Create basic btrfs filesystem structure
+ *      Initial metadata and sys chunks are inserted in the first availabe
+ *      space found in step 1.3
  *      Then insert all data chunks into the basic btrfs
  *
  * 3)   Create convert image
@@ -70,7 +76,8 @@ 
  *      as reflink source to create old files
  *
  * 4)   Iterate old fs to create files
- *      We just reflink any offset in old fs to new file extents
+ *      We just reflink file extents from old fs to newly created files on
+ *      btrfs.
  */
 
 #include "kerncompat.h"
@@ -211,7 +218,7 @@  static int create_image_file_range(struct btrfs_trans_handle *trans,
 	 * migrate block will fail as there is already a file extent.
 	 */
 	for (i = 0; i < ARRAY_SIZE(btrfs_reserved_ranges); i++) {
-		const struct simple_range *reserved = &btrfs_reserved_ranges[i];
+		struct simple_range *reserved = &btrfs_reserved_ranges[i];
 
 		/*
 		 * |-- reserved --|
@@ -238,7 +245,7 @@  static int create_image_file_range(struct btrfs_trans_handle *trans,
 		}
 	}
 
-	/* Check if we are going to insert regular file extent or hole */
+	/* Check if we are going to insert regular file extent, or hole */
 	cache = search_cache_extent(used, bytenr);
 	if (cache) {
 		if (cache->start <= bytenr) {
@@ -246,7 +253,7 @@  static int create_image_file_range(struct btrfs_trans_handle *trans,
 			 * |///////Used///////|
 			 *	|<--insert--->|
 			 *	bytenr
-			 * Regular file extent
+			 * Insert one real file extent
 			 */
 			len = min_t(u64, len, cache->start + cache->size -
 				    bytenr);
@@ -256,7 +263,7 @@  static int create_image_file_range(struct btrfs_trans_handle *trans,
 			 *		|//Used//|
 			 *  |<-insert-->|
 			 *  bytenr
-			 * Hole
+			 *  Insert one hole
 			 */
 			len = min(len, cache->start - bytenr);
 			disk_bytenr = 0;
@@ -267,7 +274,7 @@  static int create_image_file_range(struct btrfs_trans_handle *trans,
 		 * |//Used//|		|EOF
 		 *	    |<-insert-->|
 		 *	    bytenr
-		 * Hole
+		 * Insert one hole
 		 */
 		disk_bytenr = 0;
 		datacsum = 0;
@@ -313,7 +320,7 @@  static int migrate_one_reserved_range(struct btrfs_trans_handle *trans,
 				      struct btrfs_root *root,
 				      struct cache_tree *used,
 				      struct btrfs_inode_item *inode, int fd,
-				      u64 ino, const struct simple_range *range,
+				      u64 ino, struct simple_range *range,
 				      u32 convert_flags)
 {
 	u64 cur_off = range->start;
@@ -416,7 +423,7 @@  static int migrate_reserved_ranges(struct btrfs_trans_handle *trans,
 	int ret = 0;
 
 	for (i = 0; i < ARRAY_SIZE(btrfs_reserved_ranges); i++) {
-		const struct simple_range *range = &btrfs_reserved_ranges[i];
+		struct simple_range *range = &btrfs_reserved_ranges[i];
 
 		if (range->start > total_bytes)
 			return ret;
@@ -425,6 +432,7 @@  static int migrate_reserved_ranges(struct btrfs_trans_handle *trans,
 		if (ret < 0)
 			return ret;
 	}
+
 	return ret;
 }
 
@@ -601,7 +609,7 @@  static int wipe_reserved_ranges(struct cache_tree *tree, u64 min_stripe_size,
 	int ret;
 
 	for (i = 0; i < ARRAY_SIZE(btrfs_reserved_ranges); i++) {
-		const struct simple_range *range = &btrfs_reserved_ranges[i];
+		struct simple_range *range = &btrfs_reserved_ranges[i];
 
 		ret = wipe_one_reserved_range(tree, range->start, range->len,
 					      min_stripe_size, ensure_size);
@@ -1352,9 +1360,8 @@  static int do_convert(const char *devname, u32 convert_flags, u32 nodesize,
 }
 
 /*
- * Read out data of convert image which is in btrfs reserved range.
- *
- * So rollback can just use these data to overwrite these ranges of btrfs
+ * Read out data of convert image which is in btrfs reserved ranges so we can
+ * use them to overwrite the ranges during rollback.
  */
 static int read_reserved_ranges(struct btrfs_root *root, u64 ino,
 				u64 total_bytes, char *reserved_ranges[])
@@ -1363,7 +1370,7 @@  static int read_reserved_ranges(struct btrfs_root *root, u64 ino,
 	int ret = 0;
 
 	for (i = 0; i < ARRAY_SIZE(btrfs_reserved_ranges); i++) {
-		const struct simple_range *range = &btrfs_reserved_ranges[i];
+		struct simple_range *range = &btrfs_reserved_ranges[i];
 
 		if (range->start + range->len >= total_bytes)
 			break;
@@ -1371,7 +1378,7 @@  static int read_reserved_ranges(struct btrfs_root *root, u64 ino,
 				      reserved_ranges[i]);
 		if (ret < range->len) {
 			error(
-	"failed to read out data of convert image, offset=%llu len=%llu ret=%d",
+	"failed to read data of convert image, offset=%llu len=%llu ret=%d",
 			      range->start, range->len, ret);
 			if (ret >= 0)
 				ret = -EIO;
@@ -1388,7 +1395,7 @@  static bool is_subset_of_reserved_ranges(u64 start, u64 len)
 	bool ret = false;
 
 	for (i = 0; i < ARRAY_SIZE(btrfs_reserved_ranges); i++) {
-		const struct simple_range *range = &btrfs_reserved_ranges[i];
+		struct simple_range *range = &btrfs_reserved_ranges[i];
 
 		if (start >= range->start && start + len <= range_end(range)) {
 			ret = true;
@@ -1427,8 +1434,8 @@  static bool is_chunk_direct_mapped(struct btrfs_fs_info *fs_info, u64 start)
 /*
  * Iterate all file extents of the convert image.
  *
- * All file extents except ones in btrfs_reserved_ranges[] must be mapped 1:1
- * on disk. (Means their file_offset must match their on disk bytenr)
+ * All file extents except ones in btrfs_reserved_ranges must be mapped 1:1
+ * on disk. (Means thier file_offset must match their on disk bytenr)
  *
  * File extents in reserved ranges can be relocated to other place, and in
  * that case we will read them out for later use.
@@ -1449,10 +1456,10 @@  static int check_convert_image(struct btrfs_root *image_root, u64 ino,
 	btrfs_init_path(&path);
 	ret = btrfs_search_slot(NULL, image_root, &key, &path, 0, 0);
 	/*
-	 * It's possible that some fs doesn't store any(including sb)
+	 * It's possible that some fs doesn't store any (including sb)
 	 * data into 0~1M range, and NO_HOLES is enabled.
 	 *
-	 * So only needs to check ret < 0 case
+	 * So we only need to check if ret < 0
 	 */
 	if (ret < 0) {
 		error("failed to iterate file extents at offset 0: %s",
@@ -1510,7 +1517,7 @@  static int check_convert_image(struct btrfs_root *image_root, u64 ino,
 			goto next;
 
 		/*
-		 * Most file extent must be 1:1 mapped, which means 2 things:
+		 * Most file extents must be 1:1 mapped, which means 2 things:
 		 * 1) File extent file offset == disk_bytenr
 		 * 2) That data chunk's logical == chunk's physical
 		 *
@@ -1522,8 +1529,8 @@  static int check_convert_image(struct btrfs_root *image_root, u64 ino,
 		if (file_offset != disk_bytenr ||
 		    !is_chunk_direct_mapped(fs_info, disk_bytenr)) {
 			/*
-			 * Only file extent in btrfs reserved ranges are allow
-			 * non-1:1 mapped
+			 * Only file extent in btrfs reserved ranges are
+			 * allowed to be non-1:1 mapped
 			 */
 			if (!is_subset_of_reserved_ranges(file_offset,
 							ram_bytes)) {
@@ -1549,13 +1556,13 @@  static int check_convert_image(struct btrfs_root *image_root, u64 ino,
 	 */
 	if (!ret && !btrfs_fs_incompat(fs_info, NO_HOLES)) {
 		if (checked_bytes != total_size) {
+			ret = -EINVAL;
 			error("inode %llu has some file extents not checked",
 				ino);
-			return -EINVAL;
 		}
 	}
 
-	/* So far so good, read out old data located in btrfs reserved ranges */
+	/* So far so good, read old data located in btrfs reserved ranges */
 	ret = read_reserved_ranges(image_root, ino, total_size,
 				   reserved_ranges);
 	return ret;
@@ -1578,7 +1585,7 @@  static int check_convert_image(struct btrfs_root *image_root, u64 ino,
  * |   RSV 1   |  | Old  |   |    RSV 2  | | Old  | |   RSV 3   |
  * |   0~1M    |  | Fs   |   | SB2 + 64K | | Fs   | | SB3 + 64K |
  *
- * On the other hande, the converted fs image in btrfs is a completely
+ * On the other hande, the converted fs image in btrfs is a completely 
  * valid old fs.
  *
  * |<-----------------Converted fs image in btrfs-------------------->|
@@ -1613,7 +1620,7 @@  static int do_rollback(const char *devname)
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(btrfs_reserved_ranges); i++) {
-		const struct simple_range *range = &btrfs_reserved_ranges[i];
+		struct simple_range *range = &btrfs_reserved_ranges[i];
 
 		reserved_ranges[i] = calloc(1, range->len);
 		if (!reserved_ranges[i]) {
@@ -1636,16 +1643,14 @@  static int do_rollback(const char *devname)
 	}
 	fs_info = root->fs_info;
 
-
 	/*
 	 * Search root backref first, or after subvolume deletion (orphan),
 	 * we can still rollback the image.
 	 */
-	btrfs_init_path(&path);
-
 	key.objectid = CONV_IMAGE_SUBVOL_OBJECTID;
 	key.type = BTRFS_ROOT_BACKREF_KEY;
 	key.offset = BTRFS_FS_TREE_OBJECTID;
+	btrfs_init_path(&path);
 	ret = btrfs_search_slot(NULL, fs_info->tree_root, &key, &path, 0, 0);
 	btrfs_release_path(&path);
 	if (ret > 0) {
@@ -1662,7 +1667,6 @@  static int do_rollback(const char *devname)
 	key.objectid = CONV_IMAGE_SUBVOL_OBJECTID;
 	key.type = BTRFS_ROOT_ITEM_KEY;
 	key.offset = (u64)-1;
-
 	image_root = btrfs_read_fs_root(fs_info, &key);
 	if (IS_ERR(image_root)) {
 		ret = PTR_ERR(image_root);
@@ -1675,6 +1679,7 @@  static int do_rollback(const char *devname)
 	root_dir = btrfs_root_dirid(&image_root->root_item);
 	dir = btrfs_lookup_dir_item(NULL, image_root, &path, root_dir,
 			image_name, strlen(image_name), 0);
+
 	if (!dir || IS_ERR(dir)) {
 		btrfs_release_path(&path);
 		if (dir)
@@ -1692,6 +1697,7 @@  static int do_rollback(const char *devname)
 	ino = key.objectid;
 
 	ret = btrfs_lookup_inode(NULL, image_root, &path, &key, 0);
+
 	if (ret < 0) {
 		btrfs_release_path(&path);
 		error("unable to find inode %llu: %s", ino, strerror(-ret));
@@ -1703,8 +1709,7 @@  static int do_rollback(const char *devname)
 	btrfs_release_path(&path);
 
 	/* Check if we can rollback the image */
-	ret = check_convert_image(image_root, ino, total_bytes,
-				  reserved_ranges);
+	ret = check_convert_image(image_root, ino, total_bytes, reserved_ranges);
 	if (ret < 0) {
 		error("old fs image can't be rolled back");
 		goto close_fs;
@@ -1715,19 +1720,21 @@  static int do_rollback(const char *devname)
 	if (ret)
 		goto free_mem;
 
- 	/*
+	/*
 	 * Everything is OK, just write back old fs data into btrfs reserved
 	 * ranges
 	 *
 	 * Here, we starts from the backup blocks first, so if something goes
 	 * wrong, the fs is still mountable
 	 */
+
 	for (i = ARRAY_SIZE(btrfs_reserved_ranges) - 1; i >= 0; i--) {
 		u64 real_size;
-		const struct simple_range *range = &btrfs_reserved_ranges[i];
+		struct simple_range *range = &btrfs_reserved_ranges[i];
 
 		if (range_end(range) >= fsize)
 			continue;
+
 		real_size = min(range_end(range), fsize) - range->start;
 		ret = pwrite(fd, reserved_ranges[i], real_size, range->start);
 		if (ret < real_size) {
@@ -1741,6 +1748,7 @@  static int do_rollback(const char *devname)
 		}
 		ret = 0;
 	}
+
 free_mem:
 	for (i = 0; i < ARRAY_SIZE(btrfs_reserved_ranges); i++)
 		free(reserved_ranges[i]);
diff --git a/convert/source-fs.c b/convert/source-fs.c
index 9268639d5d7e..7cf515b0325d 100644
--- a/convert/source-fs.c
+++ b/convert/source-fs.c
@@ -22,6 +22,12 @@ 
 #include "convert/common.h"
 #include "convert/source-fs.h"
 
+struct simple_range btrfs_reserved_ranges[3] = {
+	{ 0,			     SZ_1M },
+	{ BTRFS_SB_MIRROR_OFFSET(1), SZ_64K },
+	{ BTRFS_SB_MIRROR_OFFSET(2), SZ_64K }
+};
+
 static int intersect_with_sb(u64 bytenr, u64 num_bytes)
 {
 	int i;
@@ -181,9 +187,6 @@  int read_disk_extent(struct btrfs_root *root, u64 bytenr,
  * The special point is, old disk_block can point to a reserved range.
  * So here, we don't use disk_block directly but search convert_root
  * to get the real disk_bytenr.
- *
- * TODO: Better extract this function to btrfs_reflink(), in fact we are just
- * reflinking from convert_image of convert_root.
  */
 int record_file_blocks(struct blk_iterate_data *data,
 			      u64 file_block, u64 disk_block, u64 num_blocks)
diff --git a/convert/source-fs.h b/convert/source-fs.h
index f3f96d07b9b8..9f611150e504 100644
--- a/convert/source-fs.h
+++ b/convert/source-fs.h
@@ -21,6 +21,19 @@ 
 
 #define CONV_IMAGE_SUBVOL_OBJECTID BTRFS_FIRST_FREE_OBJECTID
 
+/*
+ * Reresents a simple contiguous range.
+ *
+ * For multiple or non-contiguous ranges, use extent_cache_tree from
+ * extent-cache.c
+ */
+struct simple_range {
+	u64 start;
+	u64 len;
+};
+
+extern struct simple_range btrfs_reserved_ranges[3];
+
 struct task_info;
 
 struct task_ctx {
@@ -89,4 +102,14 @@  int read_disk_extent(struct btrfs_root *root, u64 bytenr,
 int record_file_blocks(struct blk_iterate_data *data,
 			      u64 file_block, u64 disk_block, u64 num_blocks);
 
+/*
+ * Simple range functions
+ *
+ * Get range end (exclusive)
+ */
+static inline u64 range_end(struct simple_range *range)
+{
+	return (range->start + range->len);
+}
+
 #endif
diff --git a/ctree.h b/ctree.h
index 63d6d04704e9..91d55557f91c 100644
--- a/ctree.h
+++ b/ctree.h
@@ -2778,4 +2778,5 @@  int btrfs_punch_hole(struct btrfs_trans_handle *trans,
 		     u64 ino, u64 offset, u64 len);
 int btrfs_read_file(struct btrfs_root *root, u64 ino, u64 start, int len,
 		    char *dest);
+
 #endif
diff --git a/file.c b/file.c
index 06c1e2dde49e..bf31cceffd97 100644
--- a/file.c
+++ b/file.c
@@ -173,12 +173,12 @@  int btrfs_punch_hole(struct btrfs_trans_handle *trans,
  * @dest:  where data will be stored
  *
  * NOTE:
- * 1) compression data is not supported yet.
+ * 1) compression data is not supported yet
  * 2) @start and @len must be aligned to sectorsize
  * 3) data read out is also aligned to sectorsize, not truncated to inode size
  *
  * Return < 0 for fatal error during read.
- * Otherwise return the number of suceeful read out data.
+ * Otherwise return the number of succesfully read data in bytes.
  */
 int btrfs_read_file(struct btrfs_root *root, u64 ino, u64 start, int len,
 		    char *dest)
@@ -210,8 +210,7 @@  int btrfs_read_file(struct btrfs_root *root, u64 ino, u64 start, int len,
 		goto out;
 
 	if (ret > 0) {
-		ret = btrfs_previous_item(root, &path, ino,
-					  BTRFS_EXTENT_DATA_KEY);
+		ret = btrfs_previous_item(root, &path, ino, BTRFS_EXTENT_DATA_KEY);
 		if (ret > 0) {
 			ret = -ENOENT;
 			goto out;
@@ -284,8 +283,8 @@  int btrfs_read_file(struct btrfs_root *root, u64 ino, u64 start, int len,
 		disk_bytenr = btrfs_file_extent_disk_bytenr(leaf, fi) +
 			      btrfs_file_extent_offset(leaf, fi);
 		read_len_ret = read_len;
-		ret = read_extent_data(root, dest + read_start - start,
-					disk_bytenr, &read_len_ret, 0);
+		ret = read_extent_data(root, dest + read_start - start, disk_bytenr,
+				       &read_len_ret, 0);
 		if (ret < 0)
 			break;
 		/* Short read, something went wrong */