[PATCH 0/6] Refactor nocow path

[PATCH 0/6] Refactor nocow path

[Nikolay Borisov]

This series aims at making the nocow path code more understanble. This done by 
doing the following things: 

1. Re-arranging and renaming some variables so that they have more expressive
names, as well as reducing their scope. Patch 1 does this. 

2. Since run_delalloc_nocow open-codes traversal of the btree it contains a lot
of checks which do not pertain to the nocow logic per-se, but are there to 
ensure the code has found the correct EXTENT_ITEM. The nocow logic itself 
contains some subtle checks which are non-obvious at first. Patch 2 rectifies 
this by adding appropriate comments.

3. Patch 3 duplicates the call to btrfs_add_ordered_extent into each branch for 
REGULAR or PREALLOC extents. Despite this duplication I think the code flow 
becomes more streamlined and easier to understand. It also does away with one 
of the local variables. 

4. Patch 4 moves extent checking code into the branch it pertains to. 

5. Patch 5 simplifies the conditions of the main 'if' in that function 

6. Finally, patch 6 removes the BUG_ON that will be triggered in case 
btrfs_add_ordered_extent returned ENOMEM. Now it's replaced with proper graceful
error handling. 

This patchset has been tested with a full xfstest run with -onodatacow option
mount options set. 

Nikolay Borisov (6):
  btrfs: Refactor run_delalloc_nocow
  btrfs: Improve comments around nocow path
  btrfs: Simplify run_delalloc_nocow
  btrfs: Streamline code in run_delalloc_nocow in case of inline extents
  btrfs: Simplify extent type check
  btrfs: Remove BUG_ON from run_delalloc_nocow

 fs/btrfs/extent-tree.c |   3 +
 fs/btrfs/inode.c       | 156 ++++++++++++++++++++++++++---------------
 2 files changed, 102 insertions(+), 57 deletions(-)

-- 
2.17.1

[PATCH 1/6] btrfs: Refactor run_delalloc_nocow

[Nikolay Borisov]

Of the 22 (!!!) local variables declared in this function only 9 have
function-wide context. Of the remaining 13, 12 are needed in the main
while loop of the function and 1 is needed in a tiny if branch, only in
case we have prealloc extent. This commit reduces the lifespan of every
variable to its bare minimum. It also renames the 'nolock'boolean to
freespace_inode to clearly indicate its purpose.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/inode.c | 62 ++++++++++++++++++++++--------------------------
 1 file changed, 29 insertions(+), 33 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index f01a2f308492..4393f986e5ad 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1301,30 +1301,18 @@ static noinline int csum_exist_in_range(struct btrfs_fs_info *fs_info,
  */
 static noinline int run_delalloc_nocow(struct inode *inode,
 				       struct page *locked_page,
-			      u64 start, u64 end, int *page_started, int force,
-			      unsigned long *nr_written)
+				       const u64 start, const u64 end,
+				       int *page_started, int force,
+				       unsigned long *nr_written)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct btrfs_root *root = BTRFS_I(inode)->root;
-	struct extent_buffer *leaf;
 	struct btrfs_path *path;
-	struct btrfs_file_extent_item *fi;
-	struct btrfs_key found_key;
-	struct extent_map *em;
-	u64 cow_start;
-	u64 cur_offset;
-	u64 extent_end;
-	u64 extent_offset;
-	u64 disk_bytenr;
-	u64 num_bytes;
-	u64 disk_num_bytes;
-	u64 ram_bytes;
-	int extent_type;
+	u64 cow_start = (u64)-1;
+	u64 cur_offset = start;
 	int ret;
-	int type;
-	int nocow;
-	int check_prev = 1;
-	bool nolock;
+	bool check_prev = true;
+	bool freespace_inode = btrfs_is_free_space_inode(BTRFS_I(inode));
 	u64 ino = btrfs_ino(BTRFS_I(inode));
 
 	path = btrfs_alloc_path();
@@ -1339,11 +1327,20 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 		return -ENOMEM;
 	}
 
-	nolock = btrfs_is_free_space_inode(BTRFS_I(inode));
-
-	cow_start = (u64)-1;
-	cur_offset = start;
 	while (1) {
+		struct btrfs_key found_key;
+		struct btrfs_file_extent_item *fi;
+		struct extent_buffer *leaf;
+		u64 extent_end;
+		u64 extent_offset;
+		u64 disk_bytenr = 0;
+		u64 num_bytes = 0;
+		u64 disk_num_bytes;
+		int type;
+		u64 ram_bytes;
+		int extent_type;
+		bool nocow = false;
+
 		ret = btrfs_lookup_file_extent(NULL, root, path, ino,
 					       cur_offset, 0);
 		if (ret < 0)
@@ -1356,7 +1353,8 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 			    found_key.type == BTRFS_EXTENT_DATA_KEY)
 				path->slots[0]--;
 		}
-		check_prev = 0;
+		/* Check previous only on first pass */
+		check_prev = false;
 next_slot:
 		leaf = path->nodes[0];
 		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
@@ -1371,9 +1369,6 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 			leaf = path->nodes[0];
 		}
 
-		nocow = 0;
-		disk_bytenr = 0;
-		num_bytes = 0;
 		btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
 
 		if (found_key.objectid > ino)
@@ -1420,7 +1415,7 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 			 * Do the same check as in btrfs_cross_ref_exist but
 			 * without the unnecessary search.
 			 */
-			if (!nolock &&
+			if (!freespace_inode &&
 			    btrfs_file_extent_generation(leaf, fi) <=
 			    btrfs_root_last_snapshot(&root->root_item))
 				goto out_check;
@@ -1442,7 +1437,7 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 					goto error;
 				}
 
-				WARN_ON_ONCE(nolock);
+				WARN_ON_ONCE(freespace_inode);
 				goto out_check;
 			}
 			disk_bytenr += extent_offset;
@@ -1452,7 +1447,7 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 			 * if there are pending snapshots for this root,
 			 * we fall into common COW way.
 			 */
-			if (!nolock && atomic_read(&root->snapshot_force_cow))
+			if (!freespace_inode && atomic_read(&root->snapshot_force_cow))
 				goto out_check;
 			/*
 			 * force cow if csum exists in the range.
@@ -1471,12 +1466,12 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 						cur_offset = cow_start;
 					goto error;
 				}
-				WARN_ON_ONCE(nolock);
+				WARN_ON_ONCE(freespace_inode);
 				goto out_check;
 			}
 			if (!btrfs_inc_nocow_writers(fs_info, disk_bytenr))
 				goto out_check;
-			nocow = 1;
+			nocow = true;
 		} else if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
 			extent_end = found_key.offset +
 				btrfs_file_extent_ram_bytes(leaf, fi);
@@ -1518,6 +1513,7 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 
 		if (extent_type == BTRFS_FILE_EXTENT_PREALLOC) {
 			u64 orig_start = found_key.offset - extent_offset;
+			struct extent_map *em;
 
 			em = create_io_em(inode, cur_offset, num_bytes,
 					  orig_start,
@@ -1543,7 +1539,7 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 		}
 
 		ret = btrfs_add_ordered_extent(inode, cur_offset, disk_bytenr,
-					       num_bytes, num_bytes, type);
+					       num_bytes, num_bytes,type);
 		if (nocow)
 			btrfs_dec_nocow_writers(fs_info, disk_bytenr);
 		BUG_ON(ret); /* -ENOMEM */
-- 
2.17.1

[PATCH v2 1/2] btrfs: Refactor run_delalloc_nocow

[Nikolay Borisov]

Of the 22 (!!!) local variables declared in this function only 9 have
function-wide context. Of the remaining 13, 12 are needed in the main
while loop of the function and 1 is needed in a tiny if branch, only in
case we have prealloc extent. This commit reduces the lifespan of every
variable to its bare minimum. It also renames the 'nolock'boolean to
freespace_inode to clearly indicate its purpose.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

V2: 
 * Don't add comment before assignment of prev_check

 fs/btrfs/inode.c | 61 ++++++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 33 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index ee582a36653d..fc6a8f9abb40 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1310,30 +1310,18 @@ static noinline int csum_exist_in_range(struct btrfs_fs_info *fs_info,
  */
 static noinline int run_delalloc_nocow(struct inode *inode,
 				       struct page *locked_page,
-			      u64 start, u64 end, int *page_started, int force,
-			      unsigned long *nr_written)
+				       const u64 start, const u64 end,
+				       int *page_started, int force,
+				       unsigned long *nr_written)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct btrfs_root *root = BTRFS_I(inode)->root;
-	struct extent_buffer *leaf;
 	struct btrfs_path *path;
-	struct btrfs_file_extent_item *fi;
-	struct btrfs_key found_key;
-	struct extent_map *em;
-	u64 cow_start;
-	u64 cur_offset;
-	u64 extent_end;
-	u64 extent_offset;
-	u64 disk_bytenr;
-	u64 num_bytes;
-	u64 disk_num_bytes;
-	u64 ram_bytes;
-	int extent_type;
+	u64 cow_start = (u64)-1;
+	u64 cur_offset = start;
 	int ret;
-	int type;
-	int nocow;
-	int check_prev = 1;
-	bool nolock;
+	bool check_prev = true;
+	bool freespace_inode = btrfs_is_free_space_inode(BTRFS_I(inode));
 	u64 ino = btrfs_ino(BTRFS_I(inode));
 
 	path = btrfs_alloc_path();
@@ -1349,11 +1337,20 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 		return -ENOMEM;
 	}
 
-	nolock = btrfs_is_free_space_inode(BTRFS_I(inode));
-
-	cow_start = (u64)-1;
-	cur_offset = start;
 	while (1) {
+		struct btrfs_key found_key;
+		struct btrfs_file_extent_item *fi;
+		struct extent_buffer *leaf;
+		u64 extent_end;
+		u64 extent_offset;
+		u64 disk_bytenr = 0;
+		u64 num_bytes = 0;
+		u64 disk_num_bytes;
+		int type;
+		u64 ram_bytes;
+		int extent_type;
+		bool nocow = false;
+
 		ret = btrfs_lookup_file_extent(NULL, root, path, ino,
 					       cur_offset, 0);
 		if (ret < 0)
@@ -1366,7 +1363,7 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 			    found_key.type == BTRFS_EXTENT_DATA_KEY)
 				path->slots[0]--;
 		}
-		check_prev = 0;
+		check_prev = false;
 next_slot:
 		leaf = path->nodes[0];
 		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
@@ -1381,9 +1378,6 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 			leaf = path->nodes[0];
 		}
 
-		nocow = 0;
-		disk_bytenr = 0;
-		num_bytes = 0;
 		btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
 
 		if (found_key.objectid > ino)
@@ -1430,7 +1424,7 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 			 * Do the same check as in btrfs_cross_ref_exist but
 			 * without the unnecessary search.
 			 */
-			if (!nolock &&
+			if (!freespace_inode &&
 			    btrfs_file_extent_generation(leaf, fi) <=
 			    btrfs_root_last_snapshot(&root->root_item))
 				goto out_check;
@@ -1452,7 +1446,7 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 					goto error;
 				}
 
-				WARN_ON_ONCE(nolock);
+				WARN_ON_ONCE(freespace_inode);
 				goto out_check;
 			}
 			disk_bytenr += extent_offset;
@@ -1462,7 +1456,7 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 			 * if there are pending snapshots for this root,
 			 * we fall into common COW way.
 			 */
-			if (!nolock && atomic_read(&root->snapshot_force_cow))
+			if (!freespace_inode && atomic_read(&root->snapshot_force_cow))
 				goto out_check;
 			/*
 			 * force cow if csum exists in the range.
@@ -1481,12 +1475,12 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 						cur_offset = cow_start;
 					goto error;
 				}
-				WARN_ON_ONCE(nolock);
+				WARN_ON_ONCE(freespace_inode);
 				goto out_check;
 			}
 			if (!btrfs_inc_nocow_writers(fs_info, disk_bytenr))
 				goto out_check;
-			nocow = 1;
+			nocow = true;
 		} else if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
 			extent_end = found_key.offset +
 				btrfs_file_extent_ram_bytes(leaf, fi);
@@ -1529,6 +1523,7 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 
 		if (extent_type == BTRFS_FILE_EXTENT_PREALLOC) {
 			u64 orig_start = found_key.offset - extent_offset;
+			struct extent_map *em;
 
 			em = create_io_em(inode, cur_offset, num_bytes,
 					  orig_start,
@@ -1554,7 +1549,7 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 		}
 
 		ret = btrfs_add_ordered_extent(inode, cur_offset, disk_bytenr,
-					       num_bytes, num_bytes, type);
+					       num_bytes, num_bytes,type);
 		if (nocow)
 			btrfs_dec_nocow_writers(fs_info, disk_bytenr);
 		BUG_ON(ret); /* -ENOMEM */
-- 
2.17.1

Re: [PATCH v2 1/2] btrfs: Refactor run_delalloc_nocow

[David Sterba]

On Wed, Aug 21, 2019 at 10:42:03AM +0300, Nikolay Borisov wrote:
> Of the 22 (!!!) local variables declared in this function only 9 have
> function-wide context.

That's the evolution of code :)

> Of the remaining 13, 12 are needed in the main
> while loop of the function and 1 is needed in a tiny if branch, only in
> case we have prealloc extent. This commit reduces the lifespan of every
> variable to its bare minimum. It also renames the 'nolock'boolean to
> freespace_inode to clearly indicate its purpose.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> V2: 
>  * Don't add comment before assignment of prev_check
> 
>  fs/btrfs/inode.c | 61 ++++++++++++++++++++++--------------------------
>  1 file changed, 28 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index ee582a36653d..fc6a8f9abb40 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1310,30 +1310,18 @@ static noinline int csum_exist_in_range(struct btrfs_fs_info *fs_info,
>   */
>  static noinline int run_delalloc_nocow(struct inode *inode,
>  				       struct page *locked_page,
> -			      u64 start, u64 end, int *page_started, int force,
> -			      unsigned long *nr_written)
> +				       const u64 start, const u64 end,
> +				       int *page_started, int force,
> +				       unsigned long *nr_written)
>  {
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>  	struct btrfs_root *root = BTRFS_I(inode)->root;
> -	struct extent_buffer *leaf;
>  	struct btrfs_path *path;
> -	struct btrfs_file_extent_item *fi;
> -	struct btrfs_key found_key;
> -	struct extent_map *em;
> -	u64 cow_start;
> -	u64 cur_offset;
> -	u64 extent_end;
> -	u64 extent_offset;
> -	u64 disk_bytenr;
> -	u64 num_bytes;
> -	u64 disk_num_bytes;
> -	u64 ram_bytes;
> -	int extent_type;
> +	u64 cow_start = (u64)-1;
> +	u64 cur_offset = start;
>  	int ret;
> -	int type;
> -	int nocow;
> -	int check_prev = 1;
> -	bool nolock;
> +	bool check_prev = true;
> +	bool freespace_inode = btrfs_is_free_space_inode(BTRFS_I(inode));

You add 'const' to the parameters, I added one here too.

Reviewed-by: David Sterba <dsterba@suse.com>

[PATCH 2/6] btrfs: Improve comments around nocow path

[Nikolay Borisov]

run_delalloc_nocow contains numerous, somewhat subtle, checks when
figuring out whether a particular extent should be CoW'ed or not. This
patch explicitly states the assumptions those checks verify. As a
result also document 2 of the more subtle checks in check_committed_ref
as well.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent-tree.c |  3 +++
 fs/btrfs/inode.c       | 50 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 163db9a560e2..1fc795fda4c2 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -2868,16 +2868,19 @@ static noinline int check_committed_ref(struct btrfs_root *root,
 	item_size = btrfs_item_size_nr(leaf, path->slots[0]);
 	ei = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_extent_item);
 
+	/* If extent item has more than 1 inline ref then it has references */
 	if (item_size != sizeof(*ei) +
 	    btrfs_extent_inline_ref_size(BTRFS_EXTENT_DATA_REF_KEY))
 		goto out;
 
+	/* If extent created before last snapshot => it's definitely referenced */
 	if (btrfs_extent_generation(leaf, ei) <=
 	    btrfs_root_last_snapshot(&root->root_item))
 		goto out;
 
 	iref = (struct btrfs_extent_inline_ref *)(ei + 1);
 
+	/* If this extent has SHARED_DATA_REF then it's referenced */
 	type = btrfs_get_extent_inline_ref_type(leaf, iref, BTRFS_REF_TYPE_DATA);
 	if (type != BTRFS_EXTENT_DATA_REF_KEY)
 		goto out;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 4393f986e5ad..aa5e017e31ab 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1345,6 +1345,11 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 					       cur_offset, 0);
 		if (ret < 0)
 			goto error;
+
+		/*
+		 * If nothing found and this is the initial search get the last
+		 * extent of this file
+		 */
 		if (ret > 0 && path->slots[0] > 0 && check_prev) {
 			leaf = path->nodes[0];
 			btrfs_item_key_to_cpu(leaf, &found_key,
@@ -1353,9 +1358,9 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 			    found_key.type == BTRFS_EXTENT_DATA_KEY)
 				path->slots[0]--;
 		}
-		/* Check previous only on first pass */
 		check_prev = false;
 next_slot:
+		/* If we are beyond end of leaf break */
 		leaf = path->nodes[0];
 		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
 			ret = btrfs_next_leaf(root, path);
@@ -1371,23 +1376,39 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 
 		btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
 
+		/* Didn't find anything for our INO */
 		if (found_key.objectid > ino)
 			break;
+		/*
+		 * Found a different inode or no extents for our file,
+		 * goto next slot
+		 */
 		if (WARN_ON_ONCE(found_key.objectid < ino) ||
 		    found_key.type < BTRFS_EXTENT_DATA_KEY) {
 			path->slots[0]++;
 			goto next_slot;
 		}
+
+		/* Found key is not EXTENT_DATA_KEY or starts after req range */
 		if (found_key.type > BTRFS_EXTENT_DATA_KEY ||
 		    found_key.offset > end)
 			break;
 
+		/*
+		 * If the found extent starts after requested offset, then
+		 * adjust extent_end to be right before this extent begins
+		 */
 		if (found_key.offset > cur_offset) {
 			extent_end = found_key.offset;
 			extent_type = 0;
 			goto out_check;
 		}
 
+
+		/*
+		 * Found extent which begins before our range and has the
+		 * potential to intersect it.
+		 */
 		fi = btrfs_item_ptr(leaf, path->slots[0],
 				    struct btrfs_file_extent_item);
 		extent_type = btrfs_file_extent_type(leaf, fi);
@@ -1401,19 +1422,28 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 				btrfs_file_extent_num_bytes(leaf, fi);
 			disk_num_bytes =
 				btrfs_file_extent_disk_num_bytes(leaf, fi);
+			/*
+			 * If extent we got ends before our range starts,
+			 * skip to next extent
+			 */
 			if (extent_end <= start) {
 				path->slots[0]++;
 				goto next_slot;
 			}
+			/* skip holes */
 			if (disk_bytenr == 0)
 				goto out_check;
+			/* skip compressed/encrypted/encoded extents */
 			if (btrfs_file_extent_compression(leaf, fi) ||
 			    btrfs_file_extent_encryption(leaf, fi) ||
 			    btrfs_file_extent_other_encoding(leaf, fi))
 				goto out_check;
 			/*
-			 * Do the same check as in btrfs_cross_ref_exist but
-			 * without the unnecessary search.
+			 * If extent is created before the last volume's snapshot
+			 * this implies the extent is shared, hence we can't do
+			 * nocow. This is the same check as in
+			 * btrfs_cross_ref_exist but without calling
+			 * btrfs_search_slot.
 			 */
 			if (!freespace_inode &&
 			    btrfs_file_extent_generation(leaf, fi) <=
@@ -1421,6 +1451,7 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 				goto out_check;
 			if (extent_type == BTRFS_FILE_EXTENT_REG && !force)
 				goto out_check;
+			/* If extent RO, we must CoW it */
 			if (btrfs_extent_readonly(fs_info, disk_bytenr))
 				goto out_check;
 			ret = btrfs_cross_ref_exist(root, ino,
@@ -1444,7 +1475,7 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 			disk_bytenr += cur_offset - found_key.offset;
 			num_bytes = min(end + 1, extent_end) - cur_offset;
 			/*
-			 * if there are pending snapshots for this root,
+			 * If there are pending snapshots for this root,
 			 * we fall into common COW way.
 			 */
 			if (!freespace_inode && atomic_read(&root->snapshot_force_cow))
@@ -1481,12 +1512,17 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 			BUG();
 		}
 out_check:
+		/* Skip extents outside of our requested range */
 		if (extent_end <= start) {
 			path->slots[0]++;
 			if (nocow)
 				btrfs_dec_nocow_writers(fs_info, disk_bytenr);
 			goto next_slot;
 		}
+		/*
+		 * If nocow is false then record the beginning of the range
+		 * that needs to be CoWed
+		 */
 		if (!nocow) {
 			if (cow_start == (u64)-1)
 				cow_start = cur_offset;
@@ -1498,6 +1534,12 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 		}
 
 		btrfs_release_path(path);
+
+		/*
+		 * CoW range from cow_start to found_key.offset - 1. As the key
+		 * will contains the beginning of the first extent that can be
+		 * NOCOW, following one which needs to be CoW'ed
+		 */
 		if (cow_start != (u64)-1) {
 			ret = cow_file_range(inode, locked_page,
 					     cow_start, found_key.offset - 1,
-- 
2.17.1

Re: [PATCH 2/6] btrfs: Improve comments around nocow path

[Filipe Manana]

On Mon, Aug 5, 2019 at 3:48 PM Nikolay Borisov <nborisov@suse.com> wrote:
>
> run_delalloc_nocow contains numerous, somewhat subtle, checks when
> figuring out whether a particular extent should be CoW'ed or not. This
> patch explicitly states the assumptions those checks verify. As a
> result also document 2 of the more subtle checks in check_committed_ref
> as well.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/extent-tree.c |  3 +++
>  fs/btrfs/inode.c       | 50 ++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 49 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 163db9a560e2..1fc795fda4c2 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -2868,16 +2868,19 @@ static noinline int check_committed_ref(struct btrfs_root *root,
>         item_size = btrfs_item_size_nr(leaf, path->slots[0]);
>         ei = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_extent_item);
>
> +       /* If extent item has more than 1 inline ref then it has references */
>         if (item_size != sizeof(*ei) +
>             btrfs_extent_inline_ref_size(BTRFS_EXTENT_DATA_REF_KEY))
>                 goto out;
>
> +       /* If extent created before last snapshot => it's definitely referenced */

Extents are always referenced, otherwise we have a corruption, so:
referenced -> shared

>         if (btrfs_extent_generation(leaf, ei) <=
>             btrfs_root_last_snapshot(&root->root_item))
>                 goto out;
>
>         iref = (struct btrfs_extent_inline_ref *)(ei + 1);
>
> +       /* If this extent has SHARED_DATA_REF then it's referenced */

referenced -> shared

>         type = btrfs_get_extent_inline_ref_type(leaf, iref, BTRFS_REF_TYPE_DATA);
>         if (type != BTRFS_EXTENT_DATA_REF_KEY)
>                 goto out;
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 4393f986e5ad..aa5e017e31ab 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1345,6 +1345,11 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>                                                cur_offset, 0);
>                 if (ret < 0)
>                         goto error;
> +
> +               /*
> +                * If nothing found and this is the initial search get the last
> +                * extent of this file

Last extent?
No, we want to check if the previous slot points to an extent item,
and if so, use it. That does not mean it's the last extent in the
file, it can be any in the middle or in the beginning.
This is hit when there's no extent that starts at the given search
offset, so we end up in the next one and we have to go 1 slot behind
because the previous one contains the search offset.

> +                */
>                 if (ret > 0 && path->slots[0] > 0 && check_prev) {
>                         leaf = path->nodes[0];
>                         btrfs_item_key_to_cpu(leaf, &found_key,
> @@ -1353,9 +1358,9 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>                             found_key.type == BTRFS_EXTENT_DATA_KEY)
>                                 path->slots[0]--;
>                 }
> -               /* Check previous only on first pass */

Previous patch added this comment, this one removes it. Why?

>                 check_prev = false;
>  next_slot:
> +               /* If we are beyond end of leaf break */

No, we don't break. We iterate to the next leaf. We break only if
there isn't a next leaf (we are at the last one).

>                 leaf = path->nodes[0];
>                 if (path->slots[0] >= btrfs_header_nritems(leaf)) {
>                         ret = btrfs_next_leaf(root, path);
> @@ -1371,23 +1376,39 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>
>                 btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
>
> +               /* Didn't find anything for our INO */
>                 if (found_key.objectid > ino)
>                         break;
> +               /*
> +                * Found a different inode or no extents for our file,
> +                * goto next slot

No. This does not mean that there are no extents for the file. If
there weren't any, we would break instead of iterating to the next
slot.
One example described at
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1d512cb77bdbda80f0dd0620a3b260d697fd581d

> +                */
>                 if (WARN_ON_ONCE(found_key.objectid < ino) ||
>                     found_key.type < BTRFS_EXTENT_DATA_KEY) {
>                         path->slots[0]++;
>                         goto next_slot;
>                 }
> +
> +               /* Found key is not EXTENT_DATA_KEY or starts after req range */
>                 if (found_key.type > BTRFS_EXTENT_DATA_KEY ||
>                     found_key.offset > end)
>                         break;
>
> +               /*
> +                * If the found extent starts after requested offset, then
> +                * adjust extent_end to be right before this extent begins
> +                */
>                 if (found_key.offset > cur_offset) {
>                         extent_end = found_key.offset;
>                         extent_type = 0;
>                         goto out_check;
>                 }
>
> +
> +               /*
> +                * Found extent which begins before our range and has the
> +                * potential to intersect it.
> +                */
>                 fi = btrfs_item_ptr(leaf, path->slots[0],
>                                     struct btrfs_file_extent_item);
>                 extent_type = btrfs_file_extent_type(leaf, fi);
> @@ -1401,19 +1422,28 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>                                 btrfs_file_extent_num_bytes(leaf, fi);
>                         disk_num_bytes =
>                                 btrfs_file_extent_disk_num_bytes(leaf, fi);
> +                       /*
> +                        * If extent we got ends before our range starts,
> +                        * skip to next extent
> +                        */
>                         if (extent_end <= start) {
>                                 path->slots[0]++;
>                                 goto next_slot;
>                         }
> +                       /* skip holes */
>                         if (disk_bytenr == 0)
>                                 goto out_check;
> +                       /* skip compressed/encrypted/encoded extents */
>                         if (btrfs_file_extent_compression(leaf, fi) ||
>                             btrfs_file_extent_encryption(leaf, fi) ||
>                             btrfs_file_extent_other_encoding(leaf, fi))
>                                 goto out_check;
>                         /*
> -                        * Do the same check as in btrfs_cross_ref_exist but
> -                        * without the unnecessary search.
> +                        * If extent is created before the last volume's snapshot
> +                        * this implies the extent is shared, hence we can't do
> +                        * nocow. This is the same check as in
> +                        * btrfs_cross_ref_exist but without calling
> +                        * btrfs_search_slot.
>                          */
>                         if (!freespace_inode &&
>                             btrfs_file_extent_generation(leaf, fi) <=
> @@ -1421,6 +1451,7 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>                                 goto out_check;
>                         if (extent_type == BTRFS_FILE_EXTENT_REG && !force)
>                                 goto out_check;
> +                       /* If extent RO, we must CoW it */
>                         if (btrfs_extent_readonly(fs_info, disk_bytenr))
>                                 goto out_check;
>                         ret = btrfs_cross_ref_exist(root, ino,
> @@ -1444,7 +1475,7 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>                         disk_bytenr += cur_offset - found_key.offset;
>                         num_bytes = min(end + 1, extent_end) - cur_offset;
>                         /*
> -                        * if there are pending snapshots for this root,
> +                        * If there are pending snapshots for this root,
>                          * we fall into common COW way.
>                          */
>                         if (!freespace_inode && atomic_read(&root->snapshot_force_cow))
> @@ -1481,12 +1512,17 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>                         BUG();
>                 }
>  out_check:
> +               /* Skip extents outside of our requested range */
>                 if (extent_end <= start) {
>                         path->slots[0]++;
>                         if (nocow)
>                                 btrfs_dec_nocow_writers(fs_info, disk_bytenr);
>                         goto next_slot;
>                 }
> +               /*
> +                * If nocow is false then record the beginning of the range
> +                * that needs to be CoWed
> +                */
>                 if (!nocow) {
>                         if (cow_start == (u64)-1)
>                                 cow_start = cur_offset;
> @@ -1498,6 +1534,12 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>                 }
>
>                 btrfs_release_path(path);
> +
> +               /*
> +                * CoW range from cow_start to found_key.offset - 1. As the key
> +                * will contains the beginning of the first extent that can be
> +                * NOCOW, following one which needs to be CoW'ed
> +                */
>                 if (cow_start != (u64)-1) {
>                         ret = cow_file_range(inode, locked_page,
>                                              cow_start, found_key.offset - 1,
> --
> 2.17.1
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH 2/6] btrfs: Improve comments around nocow path

[Nikolay Borisov]

On 6.08.19 г. 13:09 ч., Filipe Manana wrote:
> On Mon, Aug 5, 2019 at 3:48 PM Nikolay Borisov <nborisov@suse.com> wrote:

<snip>

>> @@ -1371,23 +1376,39 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>>
>>                 btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
>>
>> +               /* Didn't find anything for our INO */
>>                 if (found_key.objectid > ino)
>>                         break;
>> +               /*
>> +                * Found a different inode or no extents for our file,
>> +                * goto next slot
> 
> No. This does not mean that there are no extents for the file. If
> there weren't any, we would break instead of iterating to the next
> slot.
> One example described at
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1d512cb77bdbda80f0dd0620a3b260d697fd581d

I see, thanks for the pointer. How about the following :

/*
                 * Keep searching until we find an EXTENT ITEM or are
sure
                 * there are no more extents for this inode

                 */

While it doesn't mention the race condition this check, coupled with the
next one (where we break if type > EXTENT_DATA_KEY), it reflects reality
close enough?


> 
>> +                */
>>                 if (WARN_ON_ONCE(found_key.objectid < ino) ||
>>                     found_key.type < BTRFS_EXTENT_DATA_KEY) {
>>                         path->slots[0]++;
>>                         goto next_slot;
>>                 }
>> +
>> +               /* Found key is not EXTENT_DATA_KEY or starts after req range */
>>                 if (found_key.type > BTRFS_EXTENT_DATA_KEY ||
>>                     found_key.offset > end)
>>                         break;
>>
>> +               /*
>> +                * If the found extent starts after requested offset, then
>> +                * adjust extent_end to be right before this extent begins
>> +                */
>>                 if (found_key.offset > cur_offset) {
>>                         extent_end = found_key.offset;
>>                         extent_type = 0;
>>                         goto out_check;
>>                 }
>>
>> +
>> +               /*
>> +                * Found extent which begins before our range and has the
>> +                * potential to intersect it.
>> +                */
>>                 fi = btrfs_item_ptr(leaf, path->slots[0],
>>                                     struct btrfs_file_extent_item);
>>                 extent_type = btrfs_file_extent_type(leaf, fi);
<snip>

Re: [PATCH 2/6] btrfs: Improve comments around nocow path

[Filipe Manana]

On Wed, Aug 7, 2019 at 9:16 AM Nikolay Borisov <nborisov@suse.com> wrote:
>
>
>
> On 6.08.19 г. 13:09 ч., Filipe Manana wrote:
> > On Mon, Aug 5, 2019 at 3:48 PM Nikolay Borisov <nborisov@suse.com> wrote:
>
> <snip>
>
> >> @@ -1371,23 +1376,39 @@ static noinline int run_delalloc_nocow(struct inode *inode,
> >>
> >>                 btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
> >>
> >> +               /* Didn't find anything for our INO */
> >>                 if (found_key.objectid > ino)
> >>                         break;
> >> +               /*
> >> +                * Found a different inode or no extents for our file,
> >> +                * goto next slot
> >
> > No. This does not mean that there are no extents for the file. If
> > there weren't any, we would break instead of iterating to the next
> > slot.
> > One example described at
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1d512cb77bdbda80f0dd0620a3b260d697fd581d
>
> I see, thanks for the pointer. How about the following :
>
> /*
>                  * Keep searching until we find an EXTENT ITEM or are
> sure
>                  * there are no more extents for this inode
>
>                  */

Yes, that's just fine.

Thanks.

>
> While it doesn't mention the race condition this check, coupled with the
> next one (where we break if type > EXTENT_DATA_KEY), it reflects reality
> close enough?
>
>
> >
> >> +                */
> >>                 if (WARN_ON_ONCE(found_key.objectid < ino) ||
> >>                     found_key.type < BTRFS_EXTENT_DATA_KEY) {
> >>                         path->slots[0]++;
> >>                         goto next_slot;
> >>                 }
> >> +
> >> +               /* Found key is not EXTENT_DATA_KEY or starts after req range */
> >>                 if (found_key.type > BTRFS_EXTENT_DATA_KEY ||
> >>                     found_key.offset > end)
> >>                         break;
> >>
> >> +               /*
> >> +                * If the found extent starts after requested offset, then
> >> +                * adjust extent_end to be right before this extent begins
> >> +                */
> >>                 if (found_key.offset > cur_offset) {
> >>                         extent_end = found_key.offset;
> >>                         extent_type = 0;
> >>                         goto out_check;
> >>                 }
> >>
> >> +
> >> +               /*
> >> +                * Found extent which begins before our range and has the
> >> +                * potential to intersect it.
> >> +                */
> >>                 fi = btrfs_item_ptr(leaf, path->slots[0],
> >>                                     struct btrfs_file_extent_item);
> >>                 extent_type = btrfs_file_extent_type(leaf, fi);
> <snip>



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

[PATCH v2 2/2] btrfs: Improve comments around nocow path

[Nikolay Borisov]

run_delalloc_nocow contains numerous, somewhat subtle, checks when
figuring out whether a particular extent should be CoW'ed or not. This
patch explicitly states the assumptions those checks verify. As a
result also document 2 of the more subtle checks in check_committed_ref
as well.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

V2:
 * Use 'shared' instead of 'referenced' when referring to extents 
 * Clarify some comments in run_delalloc_nocow 

 fs/btrfs/extent-tree.c |  3 +++
 fs/btrfs/inode.c       | 50 +++++++++++++++++++++++++++++++++++++++---
 2 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index d3b58e388535..9895698cbde9 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -2963,16 +2963,19 @@ static noinline int check_committed_ref(struct btrfs_root *root,
 	item_size = btrfs_item_size_nr(leaf, path->slots[0]);
 	ei = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_extent_item);
 
+	/* If extent item has more than 1 inline ref then it's shared*/
 	if (item_size != sizeof(*ei) +
 	    btrfs_extent_inline_ref_size(BTRFS_EXTENT_DATA_REF_KEY))
 		goto out;
 
+	/* If extent created before last snapshot => it's definitely shared */
 	if (btrfs_extent_generation(leaf, ei) <=
 	    btrfs_root_last_snapshot(&root->root_item))
 		goto out;
 
 	iref = (struct btrfs_extent_inline_ref *)(ei + 1);
 
+	/* If this extent has SHARED_DATA_REF then it's shared */
 	type = btrfs_get_extent_inline_ref_type(leaf, iref, BTRFS_REF_TYPE_DATA);
 	if (type != BTRFS_EXTENT_DATA_REF_KEY)
 		goto out;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index fc6a8f9abb40..223d424532fd 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1355,6 +1355,12 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 					       cur_offset, 0);
 		if (ret < 0)
 			goto error;
+
+		/*
+		 * If there is no extent for our range when doing the initial
+		 * search, then go back to the previous slot as it will be the
+		 * one containing the search offset
+		 */
 		if (ret > 0 && path->slots[0] > 0 && check_prev) {
 			leaf = path->nodes[0];
 			btrfs_item_key_to_cpu(leaf, &found_key,
@@ -1365,6 +1371,7 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 		}
 		check_prev = false;
 next_slot:
+		/* Go to next leaf if we have exhausted the current one */
 		leaf = path->nodes[0];
 		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
 			ret = btrfs_next_leaf(root, path);
@@ -1380,23 +1387,39 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 
 		btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
 
+		/* Didn't find anything for our INO */
 		if (found_key.objectid > ino)
 			break;
+		/*
+		 * Keep searching until we find an EXTENT_ITEM or there are no
+		 * more extents for this inode
+		 */
 		if (WARN_ON_ONCE(found_key.objectid < ino) ||
 		    found_key.type < BTRFS_EXTENT_DATA_KEY) {
 			path->slots[0]++;
 			goto next_slot;
 		}
+
+		/* Found key is not EXTENT_DATA_KEY or starts after req range */
 		if (found_key.type > BTRFS_EXTENT_DATA_KEY ||
 		    found_key.offset > end)
 			break;
 
+		/*
+		 * If the found extent starts after requested offset, then
+		 * adjust extent_end to be right before this extent begins
+		 */
 		if (found_key.offset > cur_offset) {
 			extent_end = found_key.offset;
 			extent_type = 0;
 			goto out_check;
 		}
 
+
+		/*
+		 * Found extent which begins before our range and potentially
+		 * intersect it.
+		 */
 		fi = btrfs_item_ptr(leaf, path->slots[0],
 				    struct btrfs_file_extent_item);
 		extent_type = btrfs_file_extent_type(leaf, fi);
@@ -1410,19 +1433,28 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 				btrfs_file_extent_num_bytes(leaf, fi);
 			disk_num_bytes =
 				btrfs_file_extent_disk_num_bytes(leaf, fi);
+			/*
+			 * If extent we got ends before our range starts,
+			 * skip to next extent
+			 */
 			if (extent_end <= start) {
 				path->slots[0]++;
 				goto next_slot;
 			}
+			/* skip holes */
 			if (disk_bytenr == 0)
 				goto out_check;
+			/* skip compressed/encrypted/encoded extents */
 			if (btrfs_file_extent_compression(leaf, fi) ||
 			    btrfs_file_extent_encryption(leaf, fi) ||
 			    btrfs_file_extent_other_encoding(leaf, fi))
 				goto out_check;
 			/*
-			 * Do the same check as in btrfs_cross_ref_exist but
-			 * without the unnecessary search.
+			 * If extent is created before the last volume's snapshot
+			 * this implies the extent is shared, hence we can't do
+			 * nocow. This is the same check as in
+			 * btrfs_cross_ref_exist but without calling
+			 * btrfs_search_slot.
 			 */
 			if (!freespace_inode &&
 			    btrfs_file_extent_generation(leaf, fi) <=
@@ -1430,6 +1462,7 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 				goto out_check;
 			if (extent_type == BTRFS_FILE_EXTENT_REG && !force)
 				goto out_check;
+			/* If extent RO, we must CoW it */
 			if (btrfs_extent_readonly(fs_info, disk_bytenr))
 				goto out_check;
 			ret = btrfs_cross_ref_exist(root, ino,
@@ -1453,7 +1486,7 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 			disk_bytenr += cur_offset - found_key.offset;
 			num_bytes = min(end + 1, extent_end) - cur_offset;
 			/*
-			 * if there are pending snapshots for this root,
+			 * If there are pending snapshots for this root,
 			 * we fall into common COW way.
 			 */
 			if (!freespace_inode && atomic_read(&root->snapshot_force_cow))
@@ -1490,12 +1523,17 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 			BUG();
 		}
 out_check:
+		/* Skip extents outside of our requested range */
 		if (extent_end <= start) {
 			path->slots[0]++;
 			if (nocow)
 				btrfs_dec_nocow_writers(fs_info, disk_bytenr);
 			goto next_slot;
 		}
+		/*
+		 * If nocow is false then record the beginning of the range
+		 * that needs to be CoWed
+		 */
 		if (!nocow) {
 			if (cow_start == (u64)-1)
 				cow_start = cur_offset;
@@ -1507,6 +1545,12 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 		}
 
 		btrfs_release_path(path);
+
+		/*
+		 * CoW range from cow_start to found_key.offset - 1. As the key
+		 * will contains the beginning of the first extent that can be
+		 * NOCOW, following one which needs to be CoW'ed
+		 */
 		if (cow_start != (u64)-1) {
 			ret = cow_file_range(inode, locked_page,
 					     cow_start, found_key.offset - 1,
-- 
2.17.1

Re: [PATCH v2 2/2] btrfs: Improve comments around nocow path

[David Sterba]

On Wed, Aug 21, 2019 at 10:42:57AM +0300, Nikolay Borisov wrote:
> run_delalloc_nocow contains numerous, somewhat subtle, checks when
> figuring out whether a particular extent should be CoW'ed or not. This
> patch explicitly states the assumptions those checks verify. As a
> result also document 2 of the more subtle checks in check_committed_ref
> as well.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>

with typos and whitespace fixes here and there.

[PATCH 3/6] btrfs: Simplify run_delalloc_nocow

[Nikolay Borisov]

There is no point in checking the type of the extent again just to se
the 'type' variable, when this check has already been performed before.
Instead, extend the original if branch with an 'else' clause. This
allows to remove one local variable and make it obvious how the code
flow differs for prealloc/regular extents.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/inode.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index aa5e017e31ab..49db8090e62f 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1336,7 +1336,6 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 		u64 disk_bytenr = 0;
 		u64 num_bytes = 0;
 		u64 disk_num_bytes;
-		int type;
 		u64 ram_bytes;
 		int extent_type;
 		bool nocow = false;
@@ -1572,16 +1571,17 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 				goto error;
 			}
 			free_extent_map(em);
-		}
-
-		if (extent_type == BTRFS_FILE_EXTENT_PREALLOC) {
-			type = BTRFS_ORDERED_PREALLOC;
+			ret = btrfs_add_ordered_extent(inode, cur_offset,
+						       disk_bytenr, num_bytes,
+						       num_bytes,
+						       BTRFS_ORDERED_PREALLOC);
 		} else {
-			type = BTRFS_ORDERED_NOCOW;
+			ret = btrfs_add_ordered_extent(inode, cur_offset,
+						       disk_bytenr, num_bytes,
+						       num_bytes,
+						       BTRFS_ORDERED_NOCOW);
 		}
 
-		ret = btrfs_add_ordered_extent(inode, cur_offset, disk_bytenr,
-					       num_bytes, num_bytes,type);
 		if (nocow)
 			btrfs_dec_nocow_writers(fs_info, disk_bytenr);
 		BUG_ON(ret); /* -ENOMEM */
-- 
2.17.1

Re: [PATCH 3/6] btrfs: Simplify run_delalloc_nocow

[Johannes Thumshirn]

On 05/08/2019 16:47, Nikolay Borisov wrote:
> There is no point in checking the type of the extent again just to se
                                                                set ~^

Otherwise,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 4/6] btrfs: Streamline code in run_delalloc_nocow in case of inline extents

[Nikolay Borisov]

The extent range check right after the "out_check" label is redundant,
because the only way it can trigger is if we have an inline extent. In
this case it makes more sense to actually move it in the branch
explictly dealing with inlines extents. What's more, the nested
'if (nocow)' can never be true because for inline extents we always do
CoW and there is no chance 'noco' can be true, just remove that check.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/inode.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 49db8090e62f..8e24b7641247 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1507,17 +1507,15 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 				btrfs_file_extent_ram_bytes(leaf, fi);
 			extent_end = ALIGN(extent_end,
 					   fs_info->sectorsize);
+			/* Skip extents outside of our requested range */
+			if (extent_end <= start) {
+				path->slots[0]++;
+				goto next_slot;
+			}
 		} else {
 			BUG();
 		}
 out_check:
-		/* Skip extents outside of our requested range */
-		if (extent_end <= start) {
-			path->slots[0]++;
-			if (nocow)
-				btrfs_dec_nocow_writers(fs_info, disk_bytenr);
-			goto next_slot;
-		}
 		/*
 		 * If nocow is false then record the beginning of the range
 		 * that needs to be CoWed
-- 
2.17.1

Re: [PATCH 4/6] btrfs: Streamline code in run_delalloc_nocow in case of inline extents

[David Sterba]

On Mon, Aug 05, 2019 at 05:47:06PM +0300, Nikolay Borisov wrote:
> The extent range check right after the "out_check" label is redundant,
> because the only way it can trigger is if we have an inline extent. In
> this case it makes more sense to actually move it in the branch
> explictly dealing with inlines extents. What's more, the nested
> 'if (nocow)' can never be true because for inline extents we always do
> CoW and there is no chance 'noco' can be true, just remove that check.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>

[PATCH 5/6] btrfs: Simplify extent type check

[Nikolay Borisov]

Extent type can only be regular/prealloc/inline. The main branch of the
'if' already handles the first two, leaving the 'else' to handle inline.
Furthermore, tree-checker ensures that leaf items are correct.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/inode.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 8e24b7641247..6c3f9f3a7ed1 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1502,18 +1502,14 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 			if (!btrfs_inc_nocow_writers(fs_info, disk_bytenr))
 				goto out_check;
 			nocow = true;
-		} else if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
-			extent_end = found_key.offset +
-				btrfs_file_extent_ram_bytes(leaf, fi);
-			extent_end = ALIGN(extent_end,
-					   fs_info->sectorsize);
+		} else {
+			extent_end = found_key.offset + ram_bytes;
+			extent_end = ALIGN(extent_end, fs_info->sectorsize);
 			/* Skip extents outside of our requested range */
 			if (extent_end <= start) {
 				path->slots[0]++;
 				goto next_slot;
 			}
-		} else {
-			BUG();
 		}
 out_check:
 		/*
-- 
2.17.1

Re: [PATCH 5/6] btrfs: Simplify extent type check

[Filipe Manana]

On Mon, Aug 5, 2019 at 3:47 PM Nikolay Borisov <nborisov@suse.com> wrote:
>
> Extent type can only be regular/prealloc/inline. The main branch of the
> 'if' already handles the first two, leaving the 'else' to handle inline.
> Furthermore, tree-checker ensures that leaf items are correct.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

> ---
>  fs/btrfs/inode.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 8e24b7641247..6c3f9f3a7ed1 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1502,18 +1502,14 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>                         if (!btrfs_inc_nocow_writers(fs_info, disk_bytenr))
>                                 goto out_check;
>                         nocow = true;
> -               } else if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
> -                       extent_end = found_key.offset +
> -                               btrfs_file_extent_ram_bytes(leaf, fi);
> -                       extent_end = ALIGN(extent_end,
> -                                          fs_info->sectorsize);
> +               } else {
> +                       extent_end = found_key.offset + ram_bytes;
> +                       extent_end = ALIGN(extent_end, fs_info->sectorsize);
>                         /* Skip extents outside of our requested range */
>                         if (extent_end <= start) {
>                                 path->slots[0]++;
>                                 goto next_slot;
>                         }
> -               } else {
> -                       BUG();
>                 }
>  out_check:
>                 /*
> --
> 2.17.1
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH 5/6] btrfs: Simplify extent type check

[David Sterba]

On Mon, Aug 05, 2019 at 05:47:07PM +0300, Nikolay Borisov wrote:
> Extent type can only be regular/prealloc/inline. The main branch of the
> 'if' already handles the first two, leaving the 'else' to handle inline.
> Furthermore, tree-checker ensures that leaf items are correct.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/inode.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 8e24b7641247..6c3f9f3a7ed1 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1502,18 +1502,14 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>  			if (!btrfs_inc_nocow_writers(fs_info, disk_bytenr))
>  				goto out_check;
>  			nocow = true;
> -		} else if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
> -			extent_end = found_key.offset +
> -				btrfs_file_extent_ram_bytes(leaf, fi);
> -			extent_end = ALIGN(extent_end,
> -					   fs_info->sectorsize);
> +		} else {
> +			extent_end = found_key.offset + ram_bytes;
> +			extent_end = ALIGN(extent_end, fs_info->sectorsize);
>  			/* Skip extents outside of our requested range */
>  			if (extent_end <= start) {
>  				path->slots[0]++;
>  				goto next_slot;
>  			}
> -		} else {
> -			BUG();

I am not sure if we should delete this or leave it (with a message what
happened). There are other places that switch value from a known set and
have a catch-all branch.

With your change the 'catch-all' is the inline extent type. It's true
that the checker should not let an unknown type appear in this code,
however I'd rather make it explicit that something is seriously wrong if
there's an unexpected type rather than silently continuing.

The BUG can be turned to actual error handling so we don't need to crash
at least.

Re: [PATCH 5/6] btrfs: Simplify extent type check

[Qu Wenruo]

On 2019/8/21 下午11:40, David Sterba wrote:
> On Mon, Aug 05, 2019 at 05:47:07PM +0300, Nikolay Borisov wrote:
>> Extent type can only be regular/prealloc/inline. The main branch of the
>> 'if' already handles the first two, leaving the 'else' to handle inline.
>> Furthermore, tree-checker ensures that leaf items are correct.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>  fs/btrfs/inode.c | 10 +++-------
>>  1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 8e24b7641247..6c3f9f3a7ed1 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -1502,18 +1502,14 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>>  			if (!btrfs_inc_nocow_writers(fs_info, disk_bytenr))
>>  				goto out_check;
>>  			nocow = true;
>> -		} else if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
>> -			extent_end = found_key.offset +
>> -				btrfs_file_extent_ram_bytes(leaf, fi);
>> -			extent_end = ALIGN(extent_end,
>> -					   fs_info->sectorsize);
>> +		} else {
>> +			extent_end = found_key.offset + ram_bytes;
>> +			extent_end = ALIGN(extent_end, fs_info->sectorsize);
>>  			/* Skip extents outside of our requested range */
>>  			if (extent_end <= start) {
>>  				path->slots[0]++;
>>  				goto next_slot;
>>  			}
>> -		} else {
>> -			BUG();
>
> I am not sure if we should delete this or leave it (with a message what
> happened). There are other places that switch value from a known set and
> have a catch-all branch.

We can just delete it IHMO.

That's why we have tree-checker, we have ensured at least EXTENT_DATA
item read from disk doesn't contain invalid type.
So removing the BUG() here should be OK.

Although converting it to a better error handler won't hurt.
In that case it can catch runtime memory corruption earlier.

Thanks,
Qu

>
> With your change the 'catch-all' is the inline extent type. It's true
> that the checker should not let an unknown type appear in this code,
> however I'd rather make it explicit that something is seriously wrong if
> there's an unexpected type rather than silently continuing.
>
> The BUG can be turned to actual error handling so we don't need to crash
> at least.
>

Re: [PATCH 5/6] btrfs: Simplify extent type check

[Nikolay Borisov]

On 22.08.19 г. 2:47 ч., Qu Wenruo wrote:
> 
> 
> On 2019/8/21 下午11:40, David Sterba wrote:
>> On Mon, Aug 05, 2019 at 05:47:07PM +0300, Nikolay Borisov wrote:
>>> Extent type can only be regular/prealloc/inline. The main branch of the
>>> 'if' already handles the first two, leaving the 'else' to handle inline.
>>> Furthermore, tree-checker ensures that leaf items are correct.
>>>
>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>> ---
>>>  fs/btrfs/inode.c | 10 +++-------
>>>  1 file changed, 3 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> index 8e24b7641247..6c3f9f3a7ed1 100644
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -1502,18 +1502,14 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>>>  			if (!btrfs_inc_nocow_writers(fs_info, disk_bytenr))
>>>  				goto out_check;
>>>  			nocow = true;
>>> -		} else if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
>>> -			extent_end = found_key.offset +
>>> -				btrfs_file_extent_ram_bytes(leaf, fi);
>>> -			extent_end = ALIGN(extent_end,
>>> -					   fs_info->sectorsize);
>>> +		} else {
>>> +			extent_end = found_key.offset + ram_bytes;
>>> +			extent_end = ALIGN(extent_end, fs_info->sectorsize);
>>>  			/* Skip extents outside of our requested range */
>>>  			if (extent_end <= start) {
>>>  				path->slots[0]++;
>>>  				goto next_slot;
>>>  			}
>>> -		} else {
>>> -			BUG();
>>
>> I am not sure if we should delete this or leave it (with a message what
>> happened). There are other places that switch value from a known set and
>> have a catch-all branch.
> 
> We can just delete it IHMO.
> 
> That's why we have tree-checker, we have ensured at least EXTENT_DATA
> item read from disk doesn't contain invalid type.
> So removing the BUG() here should be OK.
> 
> Although converting it to a better error handler won't hurt.
> In that case it can catch runtime memory corruption earlier.

IMO it's counter productive to have duplicated checks. That's just more
code that someone needs to grok down the line when they need to
understand the code. If the tree-checker was an optional feature - then
perhaps it makes sense. But since the tree-checker is, so to speak, the
outer layer of protection, ensuring the btrfs shouldn't work with
invalid data then I'd rather remove the check.

> 
> Thanks,
> Qu
> 
>>
>> With your change the 'catch-all' is the inline extent type. It's true
>> that the checker should not let an unknown type appear in this code,
>> however I'd rather make it explicit that something is seriously wrong if
>> there's an unexpected type rather than silently continuing.
>>
>> The BUG can be turned to actual error handling so we don't need to crash
>> at least.
>>
> 

[PATCH] btrfs: Streamline code in run_delalloc_nocow

[Nikolay Borisov]

Add a comment explaining why we keep the BUG also use the already read
and cached value of extent ram bytes stored in 'ram_bytes'.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/inode.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 6cb22d82e6aa..161439122a29 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1503,16 +1503,15 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 				goto out_check;
 			nocow = true;
 		} else if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
-			extent_end = found_key.offset +
-				btrfs_file_extent_ram_bytes(leaf, fi);
-			extent_end = ALIGN(extent_end,
-					   fs_info->sectorsize);
+			extent_end = found_key.offset + ram_bytes;
+			extent_end = ALIGN(extent_end, fs_info->sectorsize);
 			/* Skip extents outside of our requested range */
 			if (extent_end <= start) {
 				path->slots[0]++;
 				goto next_slot;
 			}
 		} else {
+			/* If this triggers then we have a memory corruption */
 			BUG();
 		}
 out_check:
-- 
2.17.1

Re: [PATCH] btrfs: Streamline code in run_delalloc_nocow

[David Sterba]

On Thu, Aug 22, 2019 at 05:25:23PM +0300, Nikolay Borisov wrote:
> Add a comment explaining why we keep the BUG also use the already read
> and cached value of extent ram bytes stored in 'ram_bytes'.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Added to misc-next, thanks.

[PATCH 6/6] btrfs: Remove BUG_ON from run_delalloc_nocow

[Nikolay Borisov]

Correctly handle failure cases when adding an ordered extents in case
of REGULAR or PREALLOC extents.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/inode.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 6c3f9f3a7ed1..b935c301ca72 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1569,16 +1569,26 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 						       disk_bytenr, num_bytes,
 						       num_bytes,
 						       BTRFS_ORDERED_PREALLOC);
+			if (nocow)
+				btrfs_dec_nocow_writers(fs_info, disk_bytenr);
+			if (ret) {
+				btrfs_drop_extent_cache(BTRFS_I(inode),
+							cur_offset,
+							cur_offset + num_bytes - 1,
+							0);
+				goto error;
+			}
 		} else {
 			ret = btrfs_add_ordered_extent(inode, cur_offset,
 						       disk_bytenr, num_bytes,
 						       num_bytes,
 						       BTRFS_ORDERED_NOCOW);
+			if (nocow)
+				btrfs_dec_nocow_writers(fs_info, disk_bytenr);
+			if (ret)
+				goto error;
 		}
 
-		if (nocow)
-			btrfs_dec_nocow_writers(fs_info, disk_bytenr);
-		BUG_ON(ret); /* -ENOMEM */
 
 		if (root->root_key.objectid ==
 		    BTRFS_DATA_RELOC_TREE_OBJECTID)
-- 
2.17.1

Re: [PATCH 6/6] btrfs: Remove BUG_ON from run_delalloc_nocow

[Filipe Manana]

On Mon, Aug 5, 2019 at 3:47 PM Nikolay Borisov <nborisov@suse.com> wrote:
>
> Correctly handle failure cases when adding an ordered extents in case
> of REGULAR or PREALLOC extents.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

It's correct, but:

> ---
>  fs/btrfs/inode.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 6c3f9f3a7ed1..b935c301ca72 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1569,16 +1569,26 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>                                                        disk_bytenr, num_bytes,
>                                                        num_bytes,
>                                                        BTRFS_ORDERED_PREALLOC);
> +                       if (nocow)
> +                               btrfs_dec_nocow_writers(fs_info, disk_bytenr);
> +                       if (ret) {
> +                               btrfs_drop_extent_cache(BTRFS_I(inode),
> +                                                       cur_offset,
> +                                                       cur_offset + num_bytes - 1,
> +                                                       0);
> +                               goto error;
> +                       }
>                 } else {
>                         ret = btrfs_add_ordered_extent(inode, cur_offset,
>                                                        disk_bytenr, num_bytes,
>                                                        num_bytes,
>                                                        BTRFS_ORDERED_NOCOW);
> +                       if (nocow)
> +                               btrfs_dec_nocow_writers(fs_info, disk_bytenr);
> +                       if (ret)
> +                               goto error;

We are now duplicating some error handling. Could be done like before,
outside the if branches.

>                 }
>
> -               if (nocow)
> -                       btrfs_dec_nocow_writers(fs_info, disk_bytenr);
> -               BUG_ON(ret); /* -ENOMEM */

Just replacing the BUG_ON(ret) with "if (ret) goto error;".

>
>                 if (root->root_key.objectid ==
>                     BTRFS_DATA_RELOC_TREE_OBJECTID)
> --
> 2.17.1
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH 6/6] btrfs: Remove BUG_ON from run_delalloc_nocow

[Nikolay Borisov]

On 6.08.19 г. 13:34 ч., Filipe Manana wrote:
> On Mon, Aug 5, 2019 at 3:47 PM Nikolay Borisov <nborisov@suse.com> wrote:
>>
>> Correctly handle failure cases when adding an ordered extents in case
>> of REGULAR or PREALLOC extents.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> 
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> 
> It's correct, but:
> 
>> ---
>>  fs/btrfs/inode.c | 16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 6c3f9f3a7ed1..b935c301ca72 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -1569,16 +1569,26 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>>                                                        disk_bytenr, num_bytes,
>>                                                        num_bytes,
>>                                                        BTRFS_ORDERED_PREALLOC);
>> +                       if (nocow)
>> +                               btrfs_dec_nocow_writers(fs_info, disk_bytenr);
>> +                       if (ret) {
>> +                               btrfs_drop_extent_cache(BTRFS_I(inode),
>> +                                                       cur_offset,
>> +                                                       cur_offset + num_bytes - 1,
>> +                                                       0);
>> +                               goto error;
>> +                       }
>>                 } else {
>>                         ret = btrfs_add_ordered_extent(inode, cur_offset,
>>                                                        disk_bytenr, num_bytes,
>>                                                        num_bytes,
>>                                                        BTRFS_ORDERED_NOCOW);
>> +                       if (nocow)
>> +                               btrfs_dec_nocow_writers(fs_info, disk_bytenr);
>> +                       if (ret)
>> +                               goto error;
> 
> We are now duplicating some error handling. Could be done like before,
> outside the if branches.
> 

Dependson the POV. IMO it's better to have all error handling for the
respective branch in one place. That way when someone is reading the
function and gets to that branch they see that in case one of the
functions fail what is the error handling. Otherwise as they are
scanning the code they'd have to look up and see if something tricky is
going on.

David, what's your take on that ?

> 

Re: [PATCH 6/6] btrfs: Remove BUG_ON from run_delalloc_nocow

[David Sterba]

On Wed, Aug 07, 2019 at 11:18:20AM +0300, Nikolay Borisov wrote:
> >> @@ -1569,16 +1569,26 @@ static noinline int run_delalloc_nocow(struct inode *inode,
> >>                                                        disk_bytenr, num_bytes,
> >>                                                        num_bytes,
> >>                                                        BTRFS_ORDERED_PREALLOC);
> >> +                       if (nocow)
> >> +                               btrfs_dec_nocow_writers(fs_info, disk_bytenr);
> >> +                       if (ret) {
> >> +                               btrfs_drop_extent_cache(BTRFS_I(inode),
> >> +                                                       cur_offset,
> >> +                                                       cur_offset + num_bytes - 1,
> >> +                                                       0);
> >> +                               goto error;
> >> +                       }
> >>                 } else {
> >>                         ret = btrfs_add_ordered_extent(inode, cur_offset,
> >>                                                        disk_bytenr, num_bytes,
> >>                                                        num_bytes,
> >>                                                        BTRFS_ORDERED_NOCOW);
> >> +                       if (nocow)
> >> +                               btrfs_dec_nocow_writers(fs_info, disk_bytenr);
> >> +                       if (ret)
> >> +                               goto error;
> > 
> > We are now duplicating some error handling. Could be done like before,
> > outside the if branches.
> > 
> 
> Dependson the POV. IMO it's better to have all error handling for the
> respective branch in one place. That way when someone is reading the
> function and gets to that branch they see that in case one of the
> functions fail what is the error handling. Otherwise as they are
> scanning the code they'd have to look up and see if something tricky is
> going on.
> 
> David, what's your take on that ?

I agree with Filipe and the common error handling style is to coalesce
the common code at the end of the function. In this function there are
several places that decrement the nocow writers and then jump to error.
So this asks for a cleanup that does something like that (untested):

--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1314,6 +1314,8 @@ static noinline int run_delalloc_nocow(struct inode *inode,
        bool check_prev = true;
        const bool freespace_inode = btrfs_is_free_space_inode(BTRFS_I(inode));
        u64 ino = btrfs_ino(BTRFS_I(inode));
+       bool nocow = false;
+       u64 disk_bytenr = 0;

        path = btrfs_alloc_path();
        if (!path) {
@@ -1333,12 +1335,12 @@ static noinline int run_delalloc_nocow(struct inode *inode,
                struct extent_buffer *leaf;
                u64 extent_end;
                u64 extent_offset;
-               u64 disk_bytenr = 0;
                u64 num_bytes = 0;
                u64 disk_num_bytes;
                u64 ram_bytes;
                int extent_type;
-               bool nocow = false;
+
+               nocow = false;
 
                ret = btrfs_lookup_file_extent(NULL, root, path, ino,
                                               cur_offset, 0);
@@ -1541,12 +1543,8 @@ static noinline int run_delalloc_nocow(struct inode *inode,
                        ret = cow_file_range(inode, locked_page,
                                             cow_start, found_key.offset - 1,
                                             page_started, nr_written, 1);
-                       if (ret) {
-                               if (nocow)
-                                       btrfs_dec_nocow_writers(fs_info,
-                                                               disk_bytenr);
+                       if (ret)
                                goto error;
-                       }
                        cow_start = (u64)-1;
                }
 
@@ -1562,9 +1560,6 @@ static noinline int run_delalloc_nocow(struct inode *inode,
                                          ram_bytes, BTRFS_COMPRESS_NONE,
                                          BTRFS_ORDERED_PREALLOC);
                        if (IS_ERR(em)) {
-                               if (nocow)
-                                       btrfs_dec_nocow_writers(fs_info,
-                                                               disk_bytenr);
                                ret = PTR_ERR(em);
                                goto error;
                        }
@@ -1583,6 +1578,7 @@ static noinline int run_delalloc_nocow(struct inode *inode,
                if (nocow)
                        btrfs_dec_nocow_writers(fs_info, disk_bytenr);
                BUG_ON(ret); /* -ENOMEM */
+               nocow = false;
 
                if (root->root_key.objectid ==
                    BTRFS_DATA_RELOC_TREE_OBJECTID)
@@ -1627,6 +1623,8 @@ static noinline int run_delalloc_nocow(struct inode *inode,
        }
 
 error:
+       if (nocow)
+               btrfs_dec_nocow_writers(fs_info, disk_bytenr);
        if (ret && cur_offset < end)
                extent_clear_unlock_delalloc(inode, cur_offset, end,
                                             locked_page, EXTENT_LOCKED |

[PATCH v2] btrfs: Remove BUG_ON from run_delalloc_nocow

[Nikolay Borisov]

Correctly handle failure cases when adding an ordered extents in case
of REGULAR or PREALLOC extents.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/inode.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 161439122a29..c8fe17cd8cb6 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1314,6 +1314,8 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 	bool check_prev = true;
 	const bool freespace_inode = btrfs_is_free_space_inode(BTRFS_I(inode));
 	u64 ino = btrfs_ino(BTRFS_I(inode));
+	bool nocow = false;
+	u64 disk_bytenr = 0;
 
 	path = btrfs_alloc_path();
 	if (!path) {
@@ -1333,12 +1335,12 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 		struct extent_buffer *leaf;
 		u64 extent_end;
 		u64 extent_offset;
-		u64 disk_bytenr = 0;
 		u64 num_bytes = 0;
 		u64 disk_num_bytes;
 		u64 ram_bytes;
 		int extent_type;
-		bool nocow = false;
+
+		nocow = false;
 
 		ret = btrfs_lookup_file_extent(NULL, root, path, ino,
 					       cur_offset, 0);
@@ -1572,16 +1574,25 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 						       disk_bytenr, num_bytes,
 						       num_bytes,
 						       BTRFS_ORDERED_PREALLOC);
+			if (ret) {
+				btrfs_drop_extent_cache(BTRFS_I(inode),
+							cur_offset,
+							cur_offset + num_bytes - 1,
+							0);
+				goto error;
+			}
 		} else {
 			ret = btrfs_add_ordered_extent(inode, cur_offset,
 						       disk_bytenr, num_bytes,
 						       num_bytes,
 						       BTRFS_ORDERED_NOCOW);
+			if (ret)
+				goto error;
 		}
 
 		if (nocow)
 			btrfs_dec_nocow_writers(fs_info, disk_bytenr);
-		BUG_ON(ret); /* -ENOMEM */
+		nocow = false;
 
 		if (root->root_key.objectid ==
 		    BTRFS_DATA_RELOC_TREE_OBJECTID)
@@ -1626,6 +1637,9 @@ static noinline int run_delalloc_nocow(struct inode *inode,
 	}
 
 error:
+	if (nocow)
+		btrfs_dec_nocow_writers(fs_info, disk_bytenr);
+
 	if (ret && cur_offset < end)
 		extent_clear_unlock_delalloc(inode, cur_offset, end,
 					     locked_page, EXTENT_LOCKED |
-- 
2.17.1

Re: [PATCH v2] btrfs: Remove BUG_ON from run_delalloc_nocow

[David Sterba]

On Thu, Aug 22, 2019 at 05:24:20PM +0300, Nikolay Borisov wrote:
> Correctly handle failure cases when adding an ordered extents in case
> of REGULAR or PREALLOC extents.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Added to misc-next, thanks.

Re: [PATCH 0/6] Refactor nocow path

[David Sterba]

On Mon, Aug 05, 2019 at 05:47:02PM +0300, Nikolay Borisov wrote:
> This series aims at making the nocow path code more understanble. This done by 
> doing the following things: 
> 
> 1. Re-arranging and renaming some variables so that they have more expressive
> names, as well as reducing their scope. Patch 1 does this. 
> 
> 2. Since run_delalloc_nocow open-codes traversal of the btree it contains a lot
> of checks which do not pertain to the nocow logic per-se, but are there to 
> ensure the code has found the correct EXTENT_ITEM. The nocow logic itself 
> contains some subtle checks which are non-obvious at first. Patch 2 rectifies 
> this by adding appropriate comments.
> 
> 3. Patch 3 duplicates the call to btrfs_add_ordered_extent into each branch for 
> REGULAR or PREALLOC extents. Despite this duplication I think the code flow 
> becomes more streamlined and easier to understand. It also does away with one 
> of the local variables. 
> 
> 4. Patch 4 moves extent checking code into the branch it pertains to. 
> 
> 5. Patch 5 simplifies the conditions of the main 'if' in that function 
> 
> 6. Finally, patch 6 removes the BUG_ON that will be triggered in case 
> btrfs_add_ordered_extent returned ENOMEM. Now it's replaced with proper graceful
> error handling. 
> 
> This patchset has been tested with a full xfstest run with -onodatacow option
> mount options set. 
> 
> Nikolay Borisov (6):
>   btrfs: Refactor run_delalloc_nocow
>   btrfs: Improve comments around nocow path
>   btrfs: Simplify run_delalloc_nocow
>   btrfs: Streamline code in run_delalloc_nocow in case of inline extents
>   btrfs: Simplify extent type check
>   btrfs: Remove BUG_ON from run_delalloc_nocow

The patchset has been in for-next, no problems so far so I'd like to
promote it to misc-next. Patch 2 has some nontrivial changes suggested,
please update and resend. Thanks.

Re: [PATCH 0/6] Refactor nocow path

[David Sterba]

On Mon, Aug 05, 2019 at 05:47:02PM +0300, Nikolay Borisov wrote:
> This series aims at making the nocow path code more understanble. This done by 
> doing the following things: 
> 
> 1. Re-arranging and renaming some variables so that they have more expressive
> names, as well as reducing their scope. Patch 1 does this. 
> 
> 2. Since run_delalloc_nocow open-codes traversal of the btree it contains a lot
> of checks which do not pertain to the nocow logic per-se, but are there to 
> ensure the code has found the correct EXTENT_ITEM. The nocow logic itself 
> contains some subtle checks which are non-obvious at first. Patch 2 rectifies 
> this by adding appropriate comments.
> 
> 3. Patch 3 duplicates the call to btrfs_add_ordered_extent into each branch for 
> REGULAR or PREALLOC extents. Despite this duplication I think the code flow 
> becomes more streamlined and easier to understand. It also does away with one 
> of the local variables. 
> 
> 4. Patch 4 moves extent checking code into the branch it pertains to. 
> 
> 5. Patch 5 simplifies the conditions of the main 'if' in that function 
> 
> 6. Finally, patch 6 removes the BUG_ON that will be triggered in case 
> btrfs_add_ordered_extent returned ENOMEM. Now it's replaced with proper graceful
> error handling. 
> 
> This patchset has been tested with a full xfstest run with -onodatacow option
> mount options set. 
> 
> Nikolay Borisov (6):
>   btrfs: Refactor run_delalloc_nocow
>   btrfs: Improve comments around nocow path
>   btrfs: Simplify run_delalloc_nocow
>   btrfs: Streamline code in run_delalloc_nocow in case of inline extents

I'll add 1-4 to misc-next, the remaining patches have some comments.