[PATCH 00/17] btrfs: restrain lock extent usage during writeback

[PATCH 00/17] btrfs: restrain lock extent usage during writeback

[Josef Bacik]

Hello,

We use the extent lock to cover a pretty wide range of operations during
writeback, and this has made things like the iomap conversion more annoying.
It's also relatively complex how the rules work, and in fact are different
between compressed and normal writeback.

This series is meant to address this by pushing the extent lock down to cover a
discreet set of operations.  Being able to clearly define what the extent lock
is protecting will give us a better idea of how to reduce it's usage or possibly
replace it in the future.

The current state of affairs has been we lock the page for the range we're going
to write back, and then we lock the extent, and then we start writeback.  This
looks a few different ways

COW:
lock page
  lock extent
    allocate the extent
    insert the pinned extent
    allocate the ordered extent
    clear DELALLOC
  unlock extent
unlock page

NOCOW
lock page
  lock extent
    lookup the file extent
      check if the file extent can be used nocow
        if no:
          cow_file_range
        if yes:
          insert pinned extent if prealloc
          allocate ordered extent
          clear delalloc
  unlock extent
unlock page

COMPRESS
lock page
  lock extent
  unlock extent
    create async objects for the page ranges
    compress the pages
    submit the compressed extents
      lock extent
        allocate the extent
        insert the pinned extent
        allocate the ordered extent
        clear DELALLOC
      unlock extent
      unlock pages

As you can see there's some variations above, but in essence the extent lock
protects

- Calling into the allocator.
- The em_tree modifications (pinned extent).
- Allocating and attaching the ordered extent.
- Clearing the various bits on the extent_io_tree.
- Looking up the file extent items on disk in the NOCOW case.

We don't actually need to protect the allocator when we're allocating, that has
it's own locking, we're not gaining anything from the lock being held there.

We also don't need to protect looking up the file extent items in the NOCOW
case, we are protected by the page lock here.  There are 3 cases we would need
to be worried about here

- Prealloc.  This locks the inode and calls btrfs_wait_ordered_range() before
  doing the preallocation, so there will be no dirty pages in this range before
  it begins the preallocation.
- Zero range.  Same as above.
- O_DIRECT.  We invalidate the page range before we start the write, so again
  we're protected here.

This allows us to reduce the scope of the extent lock to the 3 things that
really matter

- Protecting the em tree modifications.  This makes sense, this is directly
  related to the inode range.
- Protecting the extent_io_tree modifications.  Again this is clear.
- Allocating and attaching the ordered extent.  Also clear.

With this in mind we can push the extent locking down into the functions that do
these operations, and drastically simplify the locking that we're doing here.

I've run this through several runs of the CI to validate that this doesn't break
anything.  Nothing has failed yet, but it is a rather scary looking change, so a
sharp review is absolutely necessary, and having a decent soak time.  I would
like to get this merged ASAP so we can start pounding on it and make sure it's
solid before I attempt other locking related changes.  Thanks,

Josef
        

Josef Bacik (17):
  btrfs: handle errors in btrfs_reloc_clone_csums properly
  btrfs: push all inline logic into cow_file_range
  btrfs: unlock all the pages with successful inline extent creation
  btrfs: move extent bit and page cleanup into cow_file_range_inline
  btrfs: lock extent when doing inline extent in compression
  btrfs: push the extent lock into btrfs_run_delalloc_range
  btrfs: push extent lock into run_delalloc_nocow
  btrfs: adjust while loop condition in run_delalloc_nocow
  btrfs: push extent lock down in run_delalloc_nocow
  btrfs: remove unlock_extent from run_delalloc_compressed
  btrfs: push extent lock into run_delalloc_cow
  btrfs: push extent lock into cow_file_range
  btrfs: push lock_extent into cow_file_range_inline
  btrfs: move can_cow_file_range_inline() outside of the extent lock
  btrfs: push lock_extent down in cow_file_range()
  btrfs: push extent lock down in submit_one_async_extent
  btrfs: add a cached state to extent_clear_unlock_delalloc

 fs/btrfs/extent_io.c    |   8 +-
 fs/btrfs/extent_io.h    |   2 +
 fs/btrfs/inode.c        | 279 +++++++++++++++++++++++-----------------
 fs/btrfs/ordered-data.c |   6 +
 fs/btrfs/ordered-data.h |   1 +
 fs/btrfs/relocation.c   |   4 +-
 6 files changed, 177 insertions(+), 123 deletions(-)

-- 
2.43.0

[PATCH 01/17] btrfs: handle errors in btrfs_reloc_clone_csums properly

[Josef Bacik]

In the cow path we will clone the reloc csums for relocated data
extents, and if there's an error we already have an ordered extent and
rely on the ordered extent finishing to clean everything up.

There's a problem however, we don't mark the ordered extent with an
error, we pretend like everything was just fine.  If we were at the end
of our range we won't actually bubble up this error anywhere, and we
could end up inserting an extent that doesn't have csums where it should
have them.

Fix this by adding a helper to mark the ordered extent with an error,
and then use this when we fail to lookup the csums in
btrfs_reloc_clone_csums.  Use this helper in the other place where we
use the same pattern while we're here.

This will prevent us from erroneously inserting the extent that doesn't
have the required checksums.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c        | 5 ++---
 fs/btrfs/ordered-data.c | 6 ++++++
 fs/btrfs/ordered-data.h | 1 +
 fs/btrfs/relocation.c   | 4 +++-
 4 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 1dde8085271e..94d9a74a912c 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3183,9 +3183,8 @@ int btrfs_finish_one_ordered(struct btrfs_ordered_extent *ordered_extent)
 		 * set the mapping error, so we need to set it if we're the ones
 		 * marking this ordered extent as failed.
 		 */
-		if (ret && !test_and_set_bit(BTRFS_ORDERED_IOERR,
-					     &ordered_extent->flags))
-			mapping_set_error(ordered_extent->inode->i_mapping, -EIO);
+		if (ret)
+			btrfs_mark_ordered_extent_error(ordered_extent);
 
 		if (truncated)
 			unwritten_start += logical_len;
diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
index 03b2f646b2f9..1fe64625d006 100644
--- a/fs/btrfs/ordered-data.c
+++ b/fs/btrfs/ordered-data.c
@@ -294,6 +294,12 @@ void btrfs_add_ordered_sum(struct btrfs_ordered_extent *entry,
 	spin_unlock_irq(&inode->ordered_tree_lock);
 }
 
+void btrfs_mark_ordered_extent_error(struct btrfs_ordered_extent *ordered)
+{
+	if (!test_and_set_bit(BTRFS_ORDERED_IOERR, &ordered->flags))
+		mapping_set_error(ordered->inode->i_mapping, -EIO);
+}
+
 static void finish_ordered_fn(struct btrfs_work *work)
 {
 	struct btrfs_ordered_extent *ordered_extent;
diff --git a/fs/btrfs/ordered-data.h b/fs/btrfs/ordered-data.h
index 34413fc5b4bd..b6f6c6b91732 100644
--- a/fs/btrfs/ordered-data.h
+++ b/fs/btrfs/ordered-data.h
@@ -203,6 +203,7 @@ bool btrfs_try_lock_ordered_range(struct btrfs_inode *inode, u64 start, u64 end,
 				  struct extent_state **cached_state);
 struct btrfs_ordered_extent *btrfs_split_ordered_extent(
 			struct btrfs_ordered_extent *ordered, u64 len);
+void btrfs_mark_ordered_extent_error(struct btrfs_ordered_extent *ordered);
 int __init ordered_data_init(void);
 void __cold ordered_data_exit(void);
 
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 7e7799b4560b..0ef84dd5762e 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -4411,8 +4411,10 @@ int btrfs_reloc_clone_csums(struct btrfs_ordered_extent *ordered)
 	ret = btrfs_lookup_csums_list(csum_root, disk_bytenr,
 				      disk_bytenr + ordered->num_bytes - 1,
 				      &list, false);
-	if (ret < 0)
+	if (ret < 0) {
+		btrfs_mark_ordered_extent_error(ordered);
 		return ret;
+	}
 
 	while (!list_empty(&list)) {
 		struct btrfs_ordered_sum *sums =
-- 
2.43.0

Re: [PATCH 01/17] btrfs: handle errors in btrfs_reloc_clone_csums properly

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 01/17] btrfs: handle errors in btrfs_reloc_clone_csums properly

[David Sterba]

On Wed, Apr 17, 2024 at 10:35:45AM -0400, Josef Bacik wrote:
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -3183,9 +3183,8 @@ int btrfs_finish_one_ordered(struct btrfs_ordered_extent *ordered_extent)
>  		 * set the mapping error, so we need to set it if we're the ones
>  		 * marking this ordered extent as failed.
>  		 */
> -		if (ret && !test_and_set_bit(BTRFS_ORDERED_IOERR,
> -					     &ordered_extent->flags))
> -			mapping_set_error(ordered_extent->inode->i_mapping, -EIO);
> +		if (ret)
> +			btrfs_mark_ordered_extent_error(ordered_extent);

This change makes the comment above the code out of sync, it explains
why the BTRFS_ORDERED_IOERR is needed, you remove it.

[PATCH 02/17] btrfs: push all inline logic into cow_file_range

[Josef Bacik]

Currently we have a lot of duplicated checks of

if (start == 0 && fs_info->sectorsize == PAGE_SIZE)
	cow_file_range_inline();

Instead of duplicating this check everywhere, consolidate all of the
inline extent logic into a helper which documents all of the checks and
then use that helper inside of cow_file_range_inline().  With this we
can clean up all of the calls to either unconditionally call
cow_file_range_inline(), or at least reduce the checks we're doing
before we call cow_file_range_inline();

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c | 143 +++++++++++++++++++++++++++--------------------
 1 file changed, 81 insertions(+), 62 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 94d9a74a912c..925a53d886b4 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -614,14 +614,56 @@ static int insert_inline_extent(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
+static bool can_cow_file_range_inline(struct btrfs_inode *inode,
+				      u64 offset, u64 size,
+				      size_t compressed_size)
+{
+	struct btrfs_fs_info *fs_info = inode->root->fs_info;
+	u64 data_len = (compressed_size ?: size);
+
+	/* Inline extents must start at offset 0. */
+	if (offset != 0)
+		return false;
+
+	/*
+	 * Due to the page size limit, for subpage we can only trigger the
+	 * writeback for the dirty sectors of page, that means data writeback
+	 * is doing more writeback than what we want.
+	 *
+	 * This is especially unexpected for some call sites like fallocate,
+	 * where we only increase i_size after everything is done.
+	 * This means we can trigger inline extent even if we didn't want to.
+	 * So here we skip inline extent creation completely.
+	 */
+	if (fs_info->sectorsize != PAGE_SIZE)
+		return false;
+
+	/* Inline extents are limited to sectorsize. */
+	if (size > fs_info->sectorsize)
+		return false;
+
+	/* We cannot exceed the maximum inline data size. */
+	if (data_len > BTRFS_MAX_INLINE_DATA_SIZE(fs_info))
+		return false;
+
+	/* We cannot exceed the user specified max_inline size. */
+	if (data_len > fs_info->max_inline)
+		return false;
+
+	/* Inline extents must be the entirety of the file. */
+	if (size < i_size_read(&inode->vfs_inode))
+		return false;
+
+	return true;
+}
 
 /*
  * conditionally insert an inline extent into the file.  This
  * does the checks required to make sure the data is small enough
  * to fit as an inline extent.
  */
-static noinline int cow_file_range_inline(struct btrfs_inode *inode, u64 size,
-					  size_t compressed_size,
+static noinline int cow_file_range_inline(struct btrfs_inode *inode, u64 offset,
+					  u64 size, size_t compressed_size,
 					  int compress_type,
 					  struct folio *compressed_folio,
 					  bool update_i_size)
@@ -634,16 +676,7 @@ static noinline int cow_file_range_inline(struct btrfs_inode *inode, u64 size,
 	int ret;
 	struct btrfs_path *path;
 
-	/*
-	 * We can create an inline extent if it ends at or beyond the current
-	 * i_size, is no larger than a sector (decompressed), and the (possibly
-	 * compressed) data fits in a leaf and the configured maximum inline
-	 * size.
-	 */
-	if (size < i_size_read(&inode->vfs_inode) ||
-	    size > fs_info->sectorsize ||
-	    data_len > BTRFS_MAX_INLINE_DATA_SIZE(fs_info) ||
-	    data_len > fs_info->max_inline)
+	if (!can_cow_file_range_inline(inode, offset, size, compressed_size))
 		return 1;
 
 	path = btrfs_alloc_path();
@@ -971,43 +1004,38 @@ static void compress_file_range(struct btrfs_work *work)
 	 * Check cow_file_range() for why we don't even try to create inline
 	 * extent for the subpage case.
 	 */
-	if (start == 0 && fs_info->sectorsize == PAGE_SIZE) {
-		if (total_in < actual_end) {
-			ret = cow_file_range_inline(inode, actual_end, 0,
-						    BTRFS_COMPRESS_NONE, NULL,
-						    false);
-		} else {
-			ret = cow_file_range_inline(inode, actual_end,
-						    total_compressed,
-						    compress_type, folios[0],
-						    false);
-		}
-		if (ret <= 0) {
-			unsigned long clear_flags = EXTENT_DELALLOC |
-				EXTENT_DELALLOC_NEW | EXTENT_DEFRAG |
-				EXTENT_DO_ACCOUNTING;
+	if (total_in < actual_end)
+		ret = cow_file_range_inline(inode, start, actual_end, 0,
+					    BTRFS_COMPRESS_NONE, NULL, false);
+	else
+		ret = cow_file_range_inline(inode, start, actual_end,
+					    total_compressed, compress_type,
+					    folios[0], false);
+	if (ret <= 0) {
+		unsigned long clear_flags = EXTENT_DELALLOC |
+			EXTENT_DELALLOC_NEW | EXTENT_DEFRAG |
+			EXTENT_DO_ACCOUNTING;
 
-			if (ret < 0)
-				mapping_set_error(mapping, -EIO);
+		if (ret < 0)
+			mapping_set_error(mapping, -EIO);
 
-			/*
-			 * inline extent creation worked or returned error,
-			 * we don't need to create any more async work items.
-			 * Unlock and free up our temp pages.
-			 *
-			 * We use DO_ACCOUNTING here because we need the
-			 * delalloc_release_metadata to be done _after_ we drop
-			 * our outstanding extent for clearing delalloc for this
-			 * range.
-			 */
-			extent_clear_unlock_delalloc(inode, start, end,
-						     NULL,
-						     clear_flags,
-						     PAGE_UNLOCK |
-						     PAGE_START_WRITEBACK |
-						     PAGE_END_WRITEBACK);
-			goto free_pages;
-		}
+		/*
+		 * inline extent creation worked or returned error,
+		 * we don't need to create any more async work items.
+		 * Unlock and free up our temp pages.
+		 *
+		 * We use DO_ACCOUNTING here because we need the
+		 * delalloc_release_metadata to be done _after_ we drop
+		 * our outstanding extent for clearing delalloc for this
+		 * range.
+		 */
+		extent_clear_unlock_delalloc(inode, start, end,
+					     NULL,
+					     clear_flags,
+					     PAGE_UNLOCK |
+					     PAGE_START_WRITEBACK |
+					     PAGE_END_WRITEBACK);
+		goto free_pages;
 	}
 
 	/*
@@ -1315,22 +1343,12 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 
 	inode_should_defrag(inode, start, end, num_bytes, SZ_64K);
 
-	/*
-	 * Due to the page size limit, for subpage we can only trigger the
-	 * writeback for the dirty sectors of page, that means data writeback
-	 * is doing more writeback than what we want.
-	 *
-	 * This is especially unexpected for some call sites like fallocate,
-	 * where we only increase i_size after everything is done.
-	 * This means we can trigger inline extent even if we didn't want to.
-	 * So here we skip inline extent creation completely.
-	 */
-	if (start == 0 && fs_info->sectorsize == PAGE_SIZE && !no_inline) {
+	if (!no_inline) {
 		u64 actual_end = min_t(u64, i_size_read(&inode->vfs_inode),
 				       end + 1);
 
 		/* lets try to make an inline extent */
-		ret = cow_file_range_inline(inode, actual_end, 0,
+		ret = cow_file_range_inline(inode, start, actual_end, 0,
 					    BTRFS_COMPRESS_NONE, NULL, false);
 		if (ret == 0) {
 			/*
@@ -10229,10 +10247,11 @@ ssize_t btrfs_do_encoded_write(struct kiocb *iocb, struct iov_iter *from,
 		goto out_qgroup_free_data;
 
 	/* Try an inline extent first. */
-	if (start == 0 && encoded->unencoded_len == encoded->len &&
+	if (encoded->unencoded_len == encoded->len &&
 	    encoded->unencoded_offset == 0) {
-		ret = cow_file_range_inline(inode, encoded->len, orig_count,
-					    compression, folios[0], true);
+		ret = cow_file_range_inline(inode, start, encoded->len,
+					    orig_count, compression, folios[0],
+					    true);
 		if (ret <= 0) {
 			if (ret == 0)
 				ret = orig_count;
-- 
2.43.0

Re: [PATCH 02/17] btrfs: push all inline logic into cow_file_range

[Goldwyn Rodrigues]

On 10:35 17/04, Josef Bacik wrote:
> Currently we have a lot of duplicated checks of
> 
> if (start == 0 && fs_info->sectorsize == PAGE_SIZE)
> 	cow_file_range_inline();
> 
> Instead of duplicating this check everywhere, consolidate all of the
> inline extent logic into a helper which documents all of the checks and
> then use that helper inside of cow_file_range_inline().  With this we
> can clean up all of the calls to either unconditionally call
> cow_file_range_inline(), or at least reduce the checks we're doing
> before we call cow_file_range_inline();
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

[PATCH 03/17] btrfs: unlock all the pages with successful inline extent creation

[Josef Bacik]

Since 4750af3bbe5d ("btrfs: prevent extent_clear_unlock_delalloc() to
unlock page not locked by __process_pages_contig()") we have been
unlocking the locked page manually instead of via
extent_clear_unlock_delalloc() because of subpage blocksize support.
However we actually disable inline extent creation for subpage blocksize
support, so this behavior isn't necessary.  Remove this code and
comment, if at some point the subpage blocksize code grows support for
inline extents this can be re-evaluated.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 925a53d886b4..19579439f7f6 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1358,24 +1358,11 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 			 * range.
 			 */
 			extent_clear_unlock_delalloc(inode, start, end,
-				     locked_page,
+				     NULL,
 				     EXTENT_LOCKED | EXTENT_DELALLOC |
 				     EXTENT_DELALLOC_NEW | EXTENT_DEFRAG |
 				     EXTENT_DO_ACCOUNTING, PAGE_UNLOCK |
 				     PAGE_START_WRITEBACK | PAGE_END_WRITEBACK);
-			/*
-			 * locked_page is locked by the caller of
-			 * writepage_delalloc(), not locked by
-			 * __process_pages_contig().
-			 *
-			 * We can't let __process_pages_contig() to unlock it,
-			 * as it doesn't have any subpage::writers recorded.
-			 *
-			 * Here we manually unlock the page, since the caller
-			 * can't determine if it's an inline extent or a
-			 * compressed extent.
-			 */
-			unlock_page(locked_page);
 			ret = 1;
 			goto done;
 		} else if (ret < 0) {
-- 
2.43.0

Re: [PATCH 03/17] btrfs: unlock all the pages with successful inline extent creation

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 04/17] btrfs: move extent bit and page cleanup into cow_file_range_inline

[Josef Bacik]

We duplicate the extent cleanup for cow_file_range_inline() in the cow
and compressed case.  The encoded case doesn't need to do cleanup the
same way, so rename cow_file_range_inline to __cow_file_range_inline and
then make cow_file_range_inline handle the extent cleanup appropriately,
and update the callers.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c | 104 +++++++++++++++++++++++------------------------
 1 file changed, 51 insertions(+), 53 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 19579439f7f6..6fd866a793bb 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -662,11 +662,11 @@ static bool can_cow_file_range_inline(struct btrfs_inode *inode,
  * does the checks required to make sure the data is small enough
  * to fit as an inline extent.
  */
-static noinline int cow_file_range_inline(struct btrfs_inode *inode, u64 offset,
-					  u64 size, size_t compressed_size,
-					  int compress_type,
-					  struct folio *compressed_folio,
-					  bool update_i_size)
+static noinline int __cow_file_range_inline(struct btrfs_inode *inode, u64 offset,
+					    u64 size, size_t compressed_size,
+					    int compress_type,
+					    struct folio *compressed_folio,
+					    bool update_i_size)
 {
 	struct btrfs_drop_extents_args drop_args = { 0 };
 	struct btrfs_root *root = inode->root;
@@ -737,6 +737,33 @@ static noinline int cow_file_range_inline(struct btrfs_inode *inode, u64 offset,
 	return ret;
 }
 
+static noinline int cow_file_range_inline(struct btrfs_inode *inode, u64 offset,
+					  u64 end,
+					  size_t compressed_size,
+					  int compress_type,
+					  struct folio *compressed_folio,
+					  bool update_i_size, bool locked)
+{
+	unsigned long clear_flags = EXTENT_DELALLOC | EXTENT_DELALLOC_NEW |
+		EXTENT_DEFRAG | EXTENT_DO_ACCOUNTING;
+	u64 size = min_t(u64, i_size_read(&inode->vfs_inode), end + 1);
+	int ret;
+
+	ret = __cow_file_range_inline(inode, offset, size, compressed_size,
+				      compress_type, compressed_folio,
+				      update_i_size);
+	if (ret > 0)
+		return ret;
+
+	if (locked)
+		clear_flags |= EXTENT_LOCKED;
+
+	extent_clear_unlock_delalloc(inode, offset, end, NULL, clear_flags,
+				     PAGE_UNLOCK | PAGE_START_WRITEBACK |
+				     PAGE_END_WRITEBACK);
+	return ret;
+}
+
 struct async_extent {
 	u64 start;
 	u64 ram_size;
@@ -1005,36 +1032,15 @@ static void compress_file_range(struct btrfs_work *work)
 	 * extent for the subpage case.
 	 */
 	if (total_in < actual_end)
-		ret = cow_file_range_inline(inode, start, actual_end, 0,
-					    BTRFS_COMPRESS_NONE, NULL, false);
+		ret = cow_file_range_inline(inode, start, end, 0,
+					    BTRFS_COMPRESS_NONE, NULL, false,
+					    false);
 	else
-		ret = cow_file_range_inline(inode, start, actual_end,
-					    total_compressed, compress_type,
-					    folios[0], false);
+		ret = cow_file_range_inline(inode, start, end, total_compressed,
+					    compress_type, folios[0], false, false);
 	if (ret <= 0) {
-		unsigned long clear_flags = EXTENT_DELALLOC |
-			EXTENT_DELALLOC_NEW | EXTENT_DEFRAG |
-			EXTENT_DO_ACCOUNTING;
-
 		if (ret < 0)
 			mapping_set_error(mapping, -EIO);
-
-		/*
-		 * inline extent creation worked or returned error,
-		 * we don't need to create any more async work items.
-		 * Unlock and free up our temp pages.
-		 *
-		 * We use DO_ACCOUNTING here because we need the
-		 * delalloc_release_metadata to be done _after_ we drop
-		 * our outstanding extent for clearing delalloc for this
-		 * range.
-		 */
-		extent_clear_unlock_delalloc(inode, start, end,
-					     NULL,
-					     clear_flags,
-					     PAGE_UNLOCK |
-					     PAGE_START_WRITEBACK |
-					     PAGE_END_WRITEBACK);
 		goto free_pages;
 	}
 
@@ -1344,29 +1350,21 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 	inode_should_defrag(inode, start, end, num_bytes, SZ_64K);
 
 	if (!no_inline) {
-		u64 actual_end = min_t(u64, i_size_read(&inode->vfs_inode),
-				       end + 1);
-
 		/* lets try to make an inline extent */
-		ret = cow_file_range_inline(inode, start, actual_end, 0,
-					    BTRFS_COMPRESS_NONE, NULL, false);
-		if (ret == 0) {
+		ret = cow_file_range_inline(inode, start, end, 0,
+					    BTRFS_COMPRESS_NONE, NULL, false,
+					    true);
+		if (ret <= 0) {
 			/*
-			 * We use DO_ACCOUNTING here because we need the
-			 * delalloc_release_metadata to be run _after_ we drop
-			 * our outstanding extent for clearing delalloc for this
-			 * range.
+			 * We succeeded, return 1 so the caller knows we're done
+			 * with this page and already handled the IO.
+			 *
+			 * If there was an error then cow_file_range_inline() has
+			 * already done the cleanup.
 			 */
-			extent_clear_unlock_delalloc(inode, start, end,
-				     NULL,
-				     EXTENT_LOCKED | EXTENT_DELALLOC |
-				     EXTENT_DELALLOC_NEW | EXTENT_DEFRAG |
-				     EXTENT_DO_ACCOUNTING, PAGE_UNLOCK |
-				     PAGE_START_WRITEBACK | PAGE_END_WRITEBACK);
-			ret = 1;
+			if (ret == 0)
+				ret = 1;
 			goto done;
-		} else if (ret < 0) {
-			goto out_unlock;
 		}
 	}
 
@@ -10236,9 +10234,9 @@ ssize_t btrfs_do_encoded_write(struct kiocb *iocb, struct iov_iter *from,
 	/* Try an inline extent first. */
 	if (encoded->unencoded_len == encoded->len &&
 	    encoded->unencoded_offset == 0) {
-		ret = cow_file_range_inline(inode, start, encoded->len,
-					    orig_count, compression, folios[0],
-					    true);
+		ret = __cow_file_range_inline(inode, start, encoded->len,
+					      orig_count, compression, folios[0],
+					      true);
 		if (ret <= 0) {
 			if (ret == 0)
 				ret = orig_count;
-- 
2.43.0

Re: [PATCH 04/17] btrfs: move extent bit and page cleanup into cow_file_range_inline

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 05/17] btrfs: lock extent when doing inline extent in compression

[Josef Bacik]

We currently don't lock the extent when we're doing a
cow_file_range_inline() for a compressed extent.  This isn't a problem
necessarily, but it's inconsistent with the rest of our usage of
cow_file_range_inline().  This also leads to some extra weird logic
around whether the extent is locked or not.  Fix this to lock the extent
before calling cow_file_range_inline() in compression to make it
consistent with the rest of the inline users.  In future patches this
will be pushed down into the cow_file_range_inline() helper, so we're
fine with the quick and dirty locking here.  This patch exists to make
the behavior change obvious.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 6fd866a793bb..ba74476ac423 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -742,10 +742,10 @@ static noinline int cow_file_range_inline(struct btrfs_inode *inode, u64 offset,
 					  size_t compressed_size,
 					  int compress_type,
 					  struct folio *compressed_folio,
-					  bool update_i_size, bool locked)
+					  bool update_i_size)
 {
 	unsigned long clear_flags = EXTENT_DELALLOC | EXTENT_DELALLOC_NEW |
-		EXTENT_DEFRAG | EXTENT_DO_ACCOUNTING;
+		EXTENT_DEFRAG | EXTENT_DO_ACCOUNTING | EXTENT_LOCKED;
 	u64 size = min_t(u64, i_size_read(&inode->vfs_inode), end + 1);
 	int ret;
 
@@ -755,9 +755,6 @@ static noinline int cow_file_range_inline(struct btrfs_inode *inode, u64 offset,
 	if (ret > 0)
 		return ret;
 
-	if (locked)
-		clear_flags |= EXTENT_LOCKED;
-
 	extent_clear_unlock_delalloc(inode, offset, end, NULL, clear_flags,
 				     PAGE_UNLOCK | PAGE_START_WRITEBACK |
 				     PAGE_END_WRITEBACK);
@@ -1031,18 +1028,19 @@ static void compress_file_range(struct btrfs_work *work)
 	 * Check cow_file_range() for why we don't even try to create inline
 	 * extent for the subpage case.
 	 */
+	lock_extent(&inode->io_tree, start, end, NULL);
 	if (total_in < actual_end)
 		ret = cow_file_range_inline(inode, start, end, 0,
-					    BTRFS_COMPRESS_NONE, NULL, false,
-					    false);
+					    BTRFS_COMPRESS_NONE, NULL, false);
 	else
 		ret = cow_file_range_inline(inode, start, end, total_compressed,
-					    compress_type, folios[0], false, false);
+					    compress_type, folios[0], false);
 	if (ret <= 0) {
 		if (ret < 0)
 			mapping_set_error(mapping, -EIO);
 		goto free_pages;
 	}
+	unlock_extent(&inode->io_tree, start, end, NULL);
 
 	/*
 	 * We aren't doing an inline extent. Round the compressed size up to a
@@ -1352,8 +1350,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 	if (!no_inline) {
 		/* lets try to make an inline extent */
 		ret = cow_file_range_inline(inode, start, end, 0,
-					    BTRFS_COMPRESS_NONE, NULL, false,
-					    true);
+					    BTRFS_COMPRESS_NONE, NULL, false);
 		if (ret <= 0) {
 			/*
 			 * We succeeded, return 1 so the caller knows we're done
-- 
2.43.0

Re: [PATCH 05/17] btrfs: lock extent when doing inline extent in compression

[Johannes Thumshirn]

On 17.04.24 16:36, Josef Bacik wrote:
> We currently don't lock the extent when we're doing a
> cow_file_range_inline() for a compressed extent.  This isn't a problem
> necessarily, but it's inconsistent with the rest of our usage of
> cow_file_range_inline().  This also leads to some extra weird logic
> around whether the extent is locked or not.  Fix this to lock the extent
> before calling cow_file_range_inline() in compression to make it
> consistent with the rest of the inline users.  In future patches this
> will be pushed down into the cow_file_range_inline() helper, so we're
> fine with the quick and dirty locking here.  This patch exists to make
> the behavior change obvious.

But in btrfs_do_encoded_write() we're still not locking the compressed 
extents and call __cow_file_range_inline().

This deviation should at least be documented here.

Re: [PATCH 05/17] btrfs: lock extent when doing inline extent in compression

[Josef Bacik]

On Wed, Apr 17, 2024 at 04:29:52PM +0000, Johannes Thumshirn wrote:
> On 17.04.24 16:36, Josef Bacik wrote:
> > We currently don't lock the extent when we're doing a
> > cow_file_range_inline() for a compressed extent.  This isn't a problem
> > necessarily, but it's inconsistent with the rest of our usage of
> > cow_file_range_inline().  This also leads to some extra weird logic
> > around whether the extent is locked or not.  Fix this to lock the extent
> > before calling cow_file_range_inline() in compression to make it
> > consistent with the rest of the inline users.  In future patches this
> > will be pushed down into the cow_file_range_inline() helper, so we're
> > fine with the quick and dirty locking here.  This patch exists to make
> > the behavior change obvious.
> 
> But in btrfs_do_encoded_write() we're still not locking the compressed 
> extents and call __cow_file_range_inline().

We are though, the extent is locked the entire operation during encoded writes.
Thanks,

Josef

[PATCH 06/17] btrfs: push the extent lock into btrfs_run_delalloc_range

[Josef Bacik]

We want to limit the scope of the extent lock to be around operations
that can change in flight.  Currently we hold the extent lock through
the entire writepage operation, which isn't really necessary.

We want to protect to make sure nobody has updated DELALLOC.  In
find_lock_delalloc_range we must lock the range in order to validate the
contents of our io_tree.  However once we've done that we're safe to
unlock the range and continue, as we have the page lock already held for
the range.

We are protected from all operations at this point.

* mmap() - we're holding the page lock, thus are protected.
* buffered writes - again, we're protected because we take the page lock
  for the first and last page in our range for buffered writes so we
  won't create new delalloc ranges in this area.
* direct IO - we invalidate pagecache before attempting to write a new
  area, which requires the page lock, so again are protected once we're
  holding the page lock on this range.

Additionally this behavior actually already exists for compressed, we
unlock the range as soon as we start to process the async extents, and
re-lock it during compression.  So this is completely safe, and makes
the locking more consistent.

Make this simple by just pushing the extent lock into
btrfs_run_delalloc_range.  From there followup patches will push the
lock further down into its users.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/extent_io.c | 5 ++---
 fs/btrfs/inode.c     | 5 +++++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 7b10f47d8f83..c09f46f969b1 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -396,15 +396,14 @@ noinline_for_stack bool find_lock_delalloc_range(struct inode *inode,
 	/* then test to make sure it is all still delalloc */
 	ret = test_range_bit(tree, delalloc_start, delalloc_end,
 			     EXTENT_DELALLOC, cached_state);
+
+	unlock_extent(tree, delalloc_start, delalloc_end, &cached_state);
 	if (!ret) {
-		unlock_extent(tree, delalloc_start, delalloc_end,
-			      &cached_state);
 		__unlock_for_delalloc(inode, locked_page,
 			      delalloc_start, delalloc_end);
 		cond_resched();
 		goto again;
 	}
-	free_extent_state(cached_state);
 	*start = delalloc_start;
 	*end = delalloc_end;
 out_failed:
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index ba74476ac423..2083005f2828 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2249,6 +2249,11 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
 	const bool zoned = btrfs_is_zoned(inode->root->fs_info);
 	int ret;
 
+	/*
+	 * We're unlocked by the different fill functions below.
+	 */
+	lock_extent(&inode->io_tree, start, end, NULL);
+
 	/*
 	 * The range must cover part of the @locked_page, or a return of 1
 	 * can confuse the caller.
-- 
2.43.0

Re: [PATCH 06/17] btrfs: push the extent lock into btrfs_run_delalloc_range

[Goldwyn Rodrigues]

On 10:35 17/04, Josef Bacik wrote:
> We want to limit the scope of the extent lock to be around operations
> that can change in flight.  Currently we hold the extent lock through
> the entire writepage operation, which isn't really necessary.
> 
> We want to protect to make sure nobody has updated DELALLOC.  In
> find_lock_delalloc_range we must lock the range in order to validate the
> contents of our io_tree.  However once we've done that we're safe to
> unlock the range and continue, as we have the page lock already held for
> the range.
> 
> We are protected from all operations at this point.
> 
> * mmap() - we're holding the page lock, thus are protected.
> * buffered writes - again, we're protected because we take the page lock
>   for the first and last page in our range for buffered writes so we
>   won't create new delalloc ranges in this area.
> * direct IO - we invalidate pagecache before attempting to write a new
>   area, which requires the page lock, so again are protected once we're
>   holding the page lock on this range.
> 
> Additionally this behavior actually already exists for compressed, we
> unlock the range as soon as we start to process the async extents, and
> re-lock it during compression.  So this is completely safe, and makes
> the locking more consistent.
> 
> Make this simple by just pushing the extent lock into
> btrfs_run_delalloc_range.  From there followup patches will push the
> lock further down into its users.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

[PATCH 07/17] btrfs: push extent lock into run_delalloc_nocow

[Josef Bacik]

run_delalloc_nocow is a bit special as it walks through the file extents
for the inode and determines what it can nocow and what it can't.  This
is the more complicated area for extent locking, so start with this
function.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 2083005f2828..f14b3cecce47 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1977,6 +1977,8 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 	 */
 	ASSERT(!btrfs_is_zoned(fs_info) || btrfs_is_data_reloc_root(root));
 
+	lock_extent(&inode->io_tree, start, end, NULL);
+
 	path = btrfs_alloc_path();
 	if (!path) {
 		ret = -ENOMEM;
@@ -2249,11 +2251,6 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
 	const bool zoned = btrfs_is_zoned(inode->root->fs_info);
 	int ret;
 
-	/*
-	 * We're unlocked by the different fill functions below.
-	 */
-	lock_extent(&inode->io_tree, start, end, NULL);
-
 	/*
 	 * The range must cover part of the @locked_page, or a return of 1
 	 * can confuse the caller.
@@ -2266,6 +2263,11 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
 		goto out;
 	}
 
+	/*
+	 * We're unlocked by the different fill functions below.
+	 */
+	lock_extent(&inode->io_tree, start, end, NULL);
+
 	if (btrfs_inode_can_compress(inode) &&
 	    inode_need_compress(inode, start, end) &&
 	    run_delalloc_compressed(inode, locked_page, start, end, wbc))
-- 
2.43.0

Re: [PATCH 07/17] btrfs: push extent lock into run_delalloc_nocow

[Goldwyn Rodrigues]

On 10:35 17/04, Josef Bacik wrote:
> run_delalloc_nocow is a bit special as it walks through the file extents
> for the inode and determines what it can nocow and what it can't.  This
> is the more complicated area for extent locking, so start with this
> function.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>


> ---
>  fs/btrfs/inode.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 2083005f2828..f14b3cecce47 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1977,6 +1977,8 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
>  	 */
>  	ASSERT(!btrfs_is_zoned(fs_info) || btrfs_is_data_reloc_root(root));
>  
> +	lock_extent(&inode->io_tree, start, end, NULL);
> +
>  	path = btrfs_alloc_path();
>  	if (!path) {
>  		ret = -ENOMEM;
> @@ -2249,11 +2251,6 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
>  	const bool zoned = btrfs_is_zoned(inode->root->fs_info);
>  	int ret;
>  
> -	/*
> -	 * We're unlocked by the different fill functions below.
> -	 */
> -	lock_extent(&inode->io_tree, start, end, NULL);
> -

So, you are adding this hunk in the previous patch and (re)moving it here.
Do you think it would be better to merge this with the previous patch?

>  	/*
>  	 * The range must cover part of the @locked_page, or a return of 1
>  	 * can confuse the caller.
> @@ -2266,6 +2263,11 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
>  		goto out;
>  	}
>  
> +	/*
> +	 * We're unlocked by the different fill functions below.
> +	 */
> +	lock_extent(&inode->io_tree, start, end, NULL);
> +
>  	if (btrfs_inode_can_compress(inode) &&
>  	    inode_need_compress(inode, start, end) &&
>  	    run_delalloc_compressed(inode, locked_page, start, end, wbc))
> -- 
> 2.43.0
> 
> 

-- 
Goldwyn

Re: [PATCH 07/17] btrfs: push extent lock into run_delalloc_nocow

[Josef Bacik]

On Tue, Apr 23, 2024 at 06:33:25AM -0500, Goldwyn Rodrigues wrote:
> On 10:35 17/04, Josef Bacik wrote:
> > run_delalloc_nocow is a bit special as it walks through the file extents
> > for the inode and determines what it can nocow and what it can't.  This
> > is the more complicated area for extent locking, so start with this
> > function.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> 
> 
> > ---
> >  fs/btrfs/inode.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 2083005f2828..f14b3cecce47 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -1977,6 +1977,8 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
> >  	 */
> >  	ASSERT(!btrfs_is_zoned(fs_info) || btrfs_is_data_reloc_root(root));
> >  
> > +	lock_extent(&inode->io_tree, start, end, NULL);
> > +
> >  	path = btrfs_alloc_path();
> >  	if (!path) {
> >  		ret = -ENOMEM;
> > @@ -2249,11 +2251,6 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
> >  	const bool zoned = btrfs_is_zoned(inode->root->fs_info);
> >  	int ret;
> >  
> > -	/*
> > -	 * We're unlocked by the different fill functions below.
> > -	 */
> > -	lock_extent(&inode->io_tree, start, end, NULL);
> > -
> 
> So, you are adding this hunk in the previous patch and (re)moving it here.
> Do you think it would be better to merge this with the previous patch?

It depends?  I did it like this so people could follow closely as I pushed the
locking down.  Most of these "push extent lock into.." patches do a variation of
this, where I push it down into a function and move the lock down in the set of
things.  I'm happy to merge them together, but I split it this way so my logic
could be followed.  Thanks,

Josef

Re: [PATCH 07/17] btrfs: push extent lock into run_delalloc_nocow

[David Sterba]

On Tue, Apr 23, 2024 at 12:49:14PM -0400, Josef Bacik wrote:
> On Tue, Apr 23, 2024 at 06:33:25AM -0500, Goldwyn Rodrigues wrote:
> > On 10:35 17/04, Josef Bacik wrote:
> > > run_delalloc_nocow is a bit special as it walks through the file extents
> > > for the inode and determines what it can nocow and what it can't.  This
> > > is the more complicated area for extent locking, so start with this
> > > function.
> > > 
> > > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > 
> > 
> > > ---
> > >  fs/btrfs/inode.c | 12 +++++++-----
> > >  1 file changed, 7 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > > index 2083005f2828..f14b3cecce47 100644
> > > --- a/fs/btrfs/inode.c
> > > +++ b/fs/btrfs/inode.c
> > > @@ -1977,6 +1977,8 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
> > >  	 */
> > >  	ASSERT(!btrfs_is_zoned(fs_info) || btrfs_is_data_reloc_root(root));
> > >  
> > > +	lock_extent(&inode->io_tree, start, end, NULL);
> > > +
> > >  	path = btrfs_alloc_path();
> > >  	if (!path) {
> > >  		ret = -ENOMEM;
> > > @@ -2249,11 +2251,6 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
> > >  	const bool zoned = btrfs_is_zoned(inode->root->fs_info);
> > >  	int ret;
> > >  
> > > -	/*
> > > -	 * We're unlocked by the different fill functions below.
> > > -	 */
> > > -	lock_extent(&inode->io_tree, start, end, NULL);
> > > -
> > 
> > So, you are adding this hunk in the previous patch and (re)moving it here.
> > Do you think it would be better to merge this with the previous patch?
> 
> It depends?  I did it like this so people could follow closely as I pushed the
> locking down.  Most of these "push extent lock into.." patches do a variation of
> this, where I push it down into a function and move the lock down in the set of
> things.  I'm happy to merge them together, but I split it this way so my logic
> could be followed.

The incremental changes are better for review, moving locks affects code
before and after and it's more convienient to review each step as the
amount of information and context is bearable. So I'm fine with the way
you did it.

Re: [PATCH 07/17] btrfs: push extent lock into run_delalloc_nocow

[Goldwyn Rodrigues]

On  6:33 23/04, Goldwyn Rodrigues wrote:
> On 10:35 17/04, Josef Bacik wrote:
> > run_delalloc_nocow is a bit special as it walks through the file extents
> > for the inode and determines what it can nocow and what it can't.  This
> > is the more complicated area for extent locking, so start with this
> > function.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> 
> 
> > ---
> >  fs/btrfs/inode.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 2083005f2828..f14b3cecce47 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -1977,6 +1977,8 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
> >  	 */
> >  	ASSERT(!btrfs_is_zoned(fs_info) || btrfs_is_data_reloc_root(root));
> >  
> > +	lock_extent(&inode->io_tree, start, end, NULL);
> > +
> >  	path = btrfs_alloc_path();
> >  	if (!path) {
> >  		ret = -ENOMEM;
> > @@ -2249,11 +2251,6 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
> >  	const bool zoned = btrfs_is_zoned(inode->root->fs_info);
> >  	int ret;
> >  
> > -	/*
> > -	 * We're unlocked by the different fill functions below.
> > -	 */
> > -	lock_extent(&inode->io_tree, start, end, NULL);
> > -
> 
> So, you are adding this hunk in the previous patch and (re)moving it here.
> Do you think it would be better to merge this with the previous patch?

Ignore this comment. This patch series is doing just that and I should
have commented after looking at the series as a whole.

Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

[PATCH 08/17] btrfs: adjust while loop condition in run_delalloc_nocow

[Josef Bacik]

We have the following pattern

while (1) {
	if (cur_offset > end)
		break;
}

Which is just

while (cur_offset <= end) {
}

so adjust the code to be more clear.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index f14b3cecce47..80e92d37af34 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1988,7 +1988,7 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 	nocow_args.end = end;
 	nocow_args.writeback_path = true;
 
-	while (1) {
+	while (cur_offset <= end) {
 		struct btrfs_block_group *nocow_bg = NULL;
 		struct btrfs_ordered_extent *ordered;
 		struct btrfs_key found_key;
@@ -2192,8 +2192,6 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 		 */
 		if (ret)
 			goto error;
-		if (cur_offset > end)
-			break;
 	}
 	btrfs_release_path(path);
 
-- 
2.43.0

Re: [PATCH 08/17] btrfs: adjust while loop condition in run_delalloc_nocow

[Goldwyn Rodrigues]

On 10:35 17/04, Josef Bacik wrote:
> We have the following pattern
> 
> while (1) {
> 	if (cur_offset > end)
> 		break;
> }
> 
> Which is just
> 
> while (cur_offset <= end) {
> }
> 
> so adjust the code to be more clear.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/inode.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index f14b3cecce47..80e92d37af34 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1988,7 +1988,7 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
>  	nocow_args.end = end;
>  	nocow_args.writeback_path = true;
>  
> -	while (1) {
> +	while (cur_offset <= end) {
>  		struct btrfs_block_group *nocow_bg = NULL;
>  		struct btrfs_ordered_extent *ordered;
>  		struct btrfs_key found_key;
> @@ -2192,8 +2192,6 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
>  		 */
>  		if (ret)
>  			goto error;
> -		if (cur_offset > end)
> -			break;
>  	}
>  	btrfs_release_path(path);
>  

Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

-- 
Goldwyn

[PATCH 09/17] btrfs: push extent lock down in run_delalloc_nocow

[Josef Bacik]

run_delalloc_nocow is a little special because we use the file extents
to see if we can nocow a range.  We don't actually need the protection
of the extent lock to look at the file extents at this point however.
We are currently holding the page lock for this range, so we are
protected from anybody who would simultaneously be modifying the file
extent items for this range.

* mmap() - we're holding the page lock.
* buffered writes - we're holding the page lock.
* direct writes - we're holding the page lock and direct IO has to flush
  page cache before it's able to continue.
* fallocate() - all callers flush the range and wait on ordered extents
  while holding the inode lock and the mmap lock, so we are again saved
  by the page lock.

We want to use the extent lock to protect

1) The mapping tree for the given range.
2) The ordered extents for the given range.
3) The io_tree for the given range.

Push the extent lock down to cover these operations.  In the
fallback_to_cow() case we simply lock before doing anything and rely on
the cow_file_range() helper to handle it's range properly.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 80e92d37af34..ab3d6ebbce6a 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1747,6 +1747,8 @@ static int fallback_to_cow(struct btrfs_inode *inode, struct page *locked_page,
 	u64 count;
 	int ret;
 
+	lock_extent(io_tree, start, end, NULL);
+
 	/*
 	 * If EXTENT_NORESERVE is set it means that when the buffered write was
 	 * made we had not enough available data space and therefore we did not
@@ -1977,8 +1979,6 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 	 */
 	ASSERT(!btrfs_is_zoned(fs_info) || btrfs_is_data_reloc_root(root));
 
-	lock_extent(&inode->io_tree, start, end, NULL);
-
 	path = btrfs_alloc_path();
 	if (!path) {
 		ret = -ENOMEM;
@@ -1994,6 +1994,7 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 		struct btrfs_key found_key;
 		struct btrfs_file_extent_item *fi;
 		struct extent_buffer *leaf;
+		struct extent_state *cached_state = NULL;
 		u64 extent_end;
 		u64 ram_bytes;
 		u64 nocow_end;
@@ -2131,6 +2132,8 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 		}
 
 		nocow_end = cur_offset + nocow_args.num_bytes - 1;
+		lock_extent(&inode->io_tree, cur_offset, nocow_end, &cached_state);
+
 		is_prealloc = extent_type == BTRFS_FILE_EXTENT_PREALLOC;
 		if (is_prealloc) {
 			u64 orig_start = found_key.offset - nocow_args.extent_offset;
@@ -2144,6 +2147,8 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 					  ram_bytes, BTRFS_COMPRESS_NONE,
 					  BTRFS_ORDERED_PREALLOC);
 			if (IS_ERR(em)) {
+				unlock_extent(&inode->io_tree, cur_offset,
+					      nocow_end, &cached_state);
 				btrfs_dec_nocow_writers(nocow_bg);
 				ret = PTR_ERR(em);
 				goto error;
@@ -2164,6 +2169,8 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 				btrfs_drop_extent_map_range(inode, cur_offset,
 							    nocow_end, false);
 			}
+			unlock_extent(&inode->io_tree, cur_offset,
+				      nocow_end, &cached_state);
 			ret = PTR_ERR(ordered);
 			goto error;
 		}
@@ -2182,6 +2189,7 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 					     EXTENT_DELALLOC |
 					     EXTENT_CLEAR_DATA_RESV,
 					     PAGE_UNLOCK | PAGE_SET_ORDERED);
+		free_extent_state(cached_state);
 
 		cur_offset = extent_end;
 
@@ -2217,13 +2225,20 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 	 */
 	if (cow_start != (u64)-1)
 		cur_offset = cow_start;
-	if (cur_offset < end)
+
+	/*
+	 * We need to lock the extent here because we're clearing DELALLOC and
+	 * we're not locked at this point.
+	 */
+	if (cur_offset < end) {
+		lock_extent(&inode->io_tree, cur_offset, end, NULL);
 		extent_clear_unlock_delalloc(inode, cur_offset, end,
 					     locked_page, EXTENT_LOCKED |
 					     EXTENT_DELALLOC | EXTENT_DEFRAG |
 					     EXTENT_DO_ACCOUNTING, PAGE_UNLOCK |
 					     PAGE_START_WRITEBACK |
 					     PAGE_END_WRITEBACK);
+	}
 	btrfs_free_path(path);
 	return ret;
 }
-- 
2.43.0

Re: [PATCH 09/17] btrfs: push extent lock down in run_delalloc_nocow

[Goldwyn Rodrigues]

On 10:35 17/04, Josef Bacik wrote:
> run_delalloc_nocow is a little special because we use the file extents
> to see if we can nocow a range.  We don't actually need the protection
> of the extent lock to look at the file extents at this point however.
> We are currently holding the page lock for this range, so we are
> protected from anybody who would simultaneously be modifying the file
> extent items for this range.
> 
> * mmap() - we're holding the page lock.
> * buffered writes - we're holding the page lock.
> * direct writes - we're holding the page lock and direct IO has to flush
>   page cache before it's able to continue.
> * fallocate() - all callers flush the range and wait on ordered extents
>   while holding the inode lock and the mmap lock, so we are again saved
>   by the page lock.
> 
> We want to use the extent lock to protect
> 
> 1) The mapping tree for the given range.
> 2) The ordered extents for the given range.
> 3) The io_tree for the given range.
> 
> Push the extent lock down to cover these operations.  In the
> fallback_to_cow() case we simply lock before doing anything and rely on
> the cow_file_range() helper to handle it's range properly.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

[PATCH 10/17] btrfs: remove unlock_extent from run_delalloc_compressed

[Josef Bacik]

Since we immediately unlock the extent range when we enter
run_delalloc_compressed() simply move the lock_extent() down to cover
cow_file_range() and then remove the unlock_extent() from
run_delalloc_compressed.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index ab3d6ebbce6a..9066d248b9aa 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1642,7 +1642,6 @@ static bool run_delalloc_compressed(struct btrfs_inode *inode,
 	if (!ctx)
 		return false;
 
-	unlock_extent(&inode->io_tree, start, end, NULL);
 	set_bit(BTRFS_INODE_HAS_ASYNC_EXTENT, &inode->runtime_flags);
 
 	async_chunk = ctx->chunks;
@@ -2276,16 +2275,16 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
 		goto out;
 	}
 
-	/*
-	 * We're unlocked by the different fill functions below.
-	 */
-	lock_extent(&inode->io_tree, start, end, NULL);
-
 	if (btrfs_inode_can_compress(inode) &&
 	    inode_need_compress(inode, start, end) &&
 	    run_delalloc_compressed(inode, locked_page, start, end, wbc))
 		return 1;
 
+	/*
+	 * We're unlocked by the different fill functions below.
+	 */
+	lock_extent(&inode->io_tree, start, end, NULL);
+
 	if (zoned)
 		ret = run_delalloc_cow(inode, locked_page, start, end, wbc,
 				       true);
-- 
2.43.0

Re: [PATCH 10/17] btrfs: remove unlock_extent from run_delalloc_compressed

[Goldwyn Rodrigues]

On 10:35 17/04, Josef Bacik wrote:
> Since we immediately unlock the extent range when we enter
> run_delalloc_compressed() simply move the lock_extent() down to cover
> cow_file_range() and then remove the unlock_extent() from
> run_delalloc_compressed.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

[PATCH 11/17] btrfs: push extent lock into run_delalloc_cow

[Josef Bacik]

This is used by zoned but also as the fallback for uncompressed extents
when we fail to compress the ranges.  Push the extent lock into
run_dealloc_cow(), and adjust the compression case to take the extent
lock after calling run_delalloc_cow().

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 9066d248b9aa..b1ae3308ecad 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1167,13 +1167,13 @@ static void submit_one_async_extent(struct async_chunk *async_chunk,
 		if (!(start >= locked_page_end || end <= locked_page_start))
 			locked_page = async_chunk->locked_page;
 	}
-	lock_extent(io_tree, start, end, NULL);
 
 	if (async_extent->compress_type == BTRFS_COMPRESS_NONE) {
 		submit_uncompressed_range(inode, async_extent, locked_page);
 		goto done;
 	}
 
+	lock_extent(io_tree, start, end, NULL);
 	ret = btrfs_reserve_extent(root, async_extent->ram_size,
 				   async_extent->compressed_size,
 				   async_extent->compressed_size,
@@ -1722,6 +1722,8 @@ static noinline int run_delalloc_cow(struct btrfs_inode *inode,
 	u64 done_offset = end;
 	int ret;
 
+	lock_extent(&inode->io_tree, start, end, NULL);
+
 	while (start <= end) {
 		ret = cow_file_range(inode, locked_page, start, end, &done_offset,
 				     true, false);
@@ -2280,17 +2282,14 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
 	    run_delalloc_compressed(inode, locked_page, start, end, wbc))
 		return 1;
 
-	/*
-	 * We're unlocked by the different fill functions below.
-	 */
-	lock_extent(&inode->io_tree, start, end, NULL);
-
-	if (zoned)
+	if (zoned) {
 		ret = run_delalloc_cow(inode, locked_page, start, end, wbc,
 				       true);
-	else
+	} else {
+		lock_extent(&inode->io_tree, start, end, NULL);
 		ret = cow_file_range(inode, locked_page, start, end, NULL,
 				     false, false);
+	}
 
 out:
 	if (ret < 0)
-- 
2.43.0

Re: [PATCH 11/17] btrfs: push extent lock into run_delalloc_cow

[Goldwyn Rodrigues]

On 10:35 17/04, Josef Bacik wrote:
> This is used by zoned but also as the fallback for uncompressed extents
> when we fail to compress the ranges.  Push the extent lock into
> run_dealloc_cow(), and adjust the compression case to take the extent
> lock after calling run_delalloc_cow().
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

[PATCH 12/17] btrfs: push extent lock into cow_file_range

[Josef Bacik]

Now that cow_file_range is the only function that is called with the
range locked, push this call into cow_file_range so we can further
narrow the scope.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index b1ae3308ecad..e076b91376a9 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1336,6 +1336,8 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 	bool extent_reserved = false;
 	int ret = 0;
 
+	lock_extent(&inode->io_tree, start, end, NULL);
+
 	if (btrfs_is_free_space_inode(inode)) {
 		ret = -EINVAL;
 		goto out_unlock;
@@ -1722,8 +1724,6 @@ static noinline int run_delalloc_cow(struct btrfs_inode *inode,
 	u64 done_offset = end;
 	int ret;
 
-	lock_extent(&inode->io_tree, start, end, NULL);
-
 	while (start <= end) {
 		ret = cow_file_range(inode, locked_page, start, end, &done_offset,
 				     true, false);
@@ -1744,12 +1744,11 @@ static int fallback_to_cow(struct btrfs_inode *inode, struct page *locked_page,
 	const bool is_reloc_ino = btrfs_is_data_reloc_root(inode->root);
 	const u64 range_bytes = end + 1 - start;
 	struct extent_io_tree *io_tree = &inode->io_tree;
+	struct extent_state *cached_state = NULL;
 	u64 range_start = start;
 	u64 count;
 	int ret;
 
-	lock_extent(io_tree, start, end, NULL);
-
 	/*
 	 * If EXTENT_NORESERVE is set it means that when the buffered write was
 	 * made we had not enough available data space and therefore we did not
@@ -1782,6 +1781,7 @@ static int fallback_to_cow(struct btrfs_inode *inode, struct page *locked_page,
 	 * group that contains that extent to RO mode and therefore force COW
 	 * when starting writeback.
 	 */
+	lock_extent(io_tree, start, end, &cached_state);
 	count = count_range_bits(io_tree, &range_start, end, range_bytes,
 				 EXTENT_NORESERVE, 0, NULL);
 	if (count > 0 || is_space_ino || is_reloc_ino) {
@@ -1800,6 +1800,7 @@ static int fallback_to_cow(struct btrfs_inode *inode, struct page *locked_page,
 			clear_extent_bit(io_tree, start, end, EXTENT_NORESERVE,
 					 NULL);
 	}
+	unlock_extent(io_tree, start, end, &cached_state);
 
 	/*
 	 * Don't try to create inline extents, as a mix of inline extent that
@@ -2282,14 +2283,12 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
 	    run_delalloc_compressed(inode, locked_page, start, end, wbc))
 		return 1;
 
-	if (zoned) {
+	if (zoned)
 		ret = run_delalloc_cow(inode, locked_page, start, end, wbc,
 				       true);
-	} else {
-		lock_extent(&inode->io_tree, start, end, NULL);
+	else
 		ret = cow_file_range(inode, locked_page, start, end, NULL,
 				     false, false);
-	}
 
 out:
 	if (ret < 0)
-- 
2.43.0

Re: [PATCH 12/17] btrfs: push extent lock into cow_file_range

[Goldwyn Rodrigues]

On 10:35 17/04, Josef Bacik wrote:
> Now that cow_file_range is the only function that is called with the
> range locked, push this call into cow_file_range so we can further
> narrow the scope.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

[PATCH 13/17] btrfs: push lock_extent into cow_file_range_inline

[Josef Bacik]

Now that we've pushed the lock_extent() into cow_file_range() we can
push the extent locking into cow_file_range_inline() and move the
lock_extent in cow_file_range() to after we call
cow_file_range_inline().

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index e076b91376a9..fcad740b1e28 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -744,17 +744,22 @@ static noinline int cow_file_range_inline(struct btrfs_inode *inode, u64 offset,
 					  struct folio *compressed_folio,
 					  bool update_i_size)
 {
+	struct extent_state *cached = NULL;
 	unsigned long clear_flags = EXTENT_DELALLOC | EXTENT_DELALLOC_NEW |
 		EXTENT_DEFRAG | EXTENT_DO_ACCOUNTING | EXTENT_LOCKED;
 	u64 size = min_t(u64, i_size_read(&inode->vfs_inode), end + 1);
 	int ret;
 
+	lock_extent(&inode->io_tree, offset, end, &cached);
 	ret = __cow_file_range_inline(inode, offset, size, compressed_size,
 				      compress_type, compressed_folio,
 				      update_i_size);
-	if (ret > 0)
+	if (ret > 0) {
+		unlock_extent(&inode->io_tree, offset, end, &cached);
 		return ret;
+	}
 
+	free_extent_state(cached);
 	extent_clear_unlock_delalloc(inode, offset, end, NULL, clear_flags,
 				     PAGE_UNLOCK | PAGE_START_WRITEBACK |
 				     PAGE_END_WRITEBACK);
@@ -1028,7 +1033,6 @@ static void compress_file_range(struct btrfs_work *work)
 	 * Check cow_file_range() for why we don't even try to create inline
 	 * extent for the subpage case.
 	 */
-	lock_extent(&inode->io_tree, start, end, NULL);
 	if (total_in < actual_end)
 		ret = cow_file_range_inline(inode, start, end, 0,
 					    BTRFS_COMPRESS_NONE, NULL, false);
@@ -1040,7 +1044,6 @@ static void compress_file_range(struct btrfs_work *work)
 			mapping_set_error(mapping, -EIO);
 		goto free_pages;
 	}
-	unlock_extent(&inode->io_tree, start, end, NULL);
 
 	/*
 	 * We aren't doing an inline extent. Round the compressed size up to a
@@ -1336,8 +1339,6 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 	bool extent_reserved = false;
 	int ret = 0;
 
-	lock_extent(&inode->io_tree, start, end, NULL);
-
 	if (btrfs_is_free_space_inode(inode)) {
 		ret = -EINVAL;
 		goto out_unlock;
@@ -1367,6 +1368,8 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 		}
 	}
 
+	lock_extent(&inode->io_tree, start, end, NULL);
+
 	alloc_hint = get_extent_allocation_hint(inode, start, num_bytes);
 
 	/*
-- 
2.43.0

Re: [PATCH 13/17] btrfs: push lock_extent into cow_file_range_inline

[Goldwyn Rodrigues]

On 10:35 17/04, Josef Bacik wrote:
> Now that we've pushed the lock_extent() into cow_file_range() we can
> push the extent locking into cow_file_range_inline() and move the
> lock_extent in cow_file_range() to after we call
> cow_file_range_inline().
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

[PATCH 14/17] btrfs: move can_cow_file_range_inline() outside of the extent lock

[Josef Bacik]

These checks aren't reliant on the extent lock.  Move this up into
cow_file_range_inline(), and then update encoded writes to call this
check before calling __cow_file_range_inline().  This will allow us to
skip the extent lock if we're not able to inline the given extent.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index fcad740b1e28..07bdad6f1be0 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -661,6 +661,9 @@ static bool can_cow_file_range_inline(struct btrfs_inode *inode,
  * conditionally insert an inline extent into the file.  This
  * does the checks required to make sure the data is small enough
  * to fit as an inline extent.
+ *
+ * If being used directly, you must have already checked we're allowed to cow
+ * the range by getting true from can_cow_file_range_inline().
  */
 static noinline int __cow_file_range_inline(struct btrfs_inode *inode, u64 offset,
 					    u64 size, size_t compressed_size,
@@ -676,9 +679,6 @@ static noinline int __cow_file_range_inline(struct btrfs_inode *inode, u64 offse
 	int ret;
 	struct btrfs_path *path;
 
-	if (!can_cow_file_range_inline(inode, offset, size, compressed_size))
-		return 1;
-
 	path = btrfs_alloc_path();
 	if (!path)
 		return -ENOMEM;
@@ -750,6 +750,9 @@ static noinline int cow_file_range_inline(struct btrfs_inode *inode, u64 offset,
 	u64 size = min_t(u64, i_size_read(&inode->vfs_inode), end + 1);
 	int ret;
 
+	if (!can_cow_file_range_inline(inode, offset, size, compressed_size))
+		return 1;
+
 	lock_extent(&inode->io_tree, offset, end, &cached);
 	ret = __cow_file_range_inline(inode, offset, size, compressed_size,
 				      compress_type, compressed_folio,
@@ -10250,7 +10253,8 @@ ssize_t btrfs_do_encoded_write(struct kiocb *iocb, struct iov_iter *from,
 
 	/* Try an inline extent first. */
 	if (encoded->unencoded_len == encoded->len &&
-	    encoded->unencoded_offset == 0) {
+	    encoded->unencoded_offset == 0 &&
+	    can_cow_file_range_inline(inode, start, encoded->len, orig_count)) {
 		ret = __cow_file_range_inline(inode, start, encoded->len,
 					      orig_count, compression, folios[0],
 					      true);
-- 
2.43.0

Re: [PATCH 14/17] btrfs: move can_cow_file_range_inline() outside of the extent lock

[Goldwyn Rodrigues]

On 10:35 17/04, Josef Bacik wrote:
> These checks aren't reliant on the extent lock.  Move this up into
> cow_file_range_inline(), and then update encoded writes to call this
> check before calling __cow_file_range_inline().  This will allow us to
> skip the extent lock if we're not able to inline the given extent.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Looks good.
Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

-- 
Goldwyn

[PATCH 15/17] btrfs: push lock_extent down in cow_file_range()

[Josef Bacik]

Now that we've got the extent lock pushed into cow_file_range() we can
push it further down into the allocation loop.  This allows us to only
hold the extent lock during the dropping of the extent map range and
inserting the ordered extent.

This makes the error case a little trickier as we'll now have to lock
the range before clearing any of the other extent bits for the range,
but this is the error path so is less performance critical.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 07bdad6f1be0..3cd092fab31d 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1371,8 +1371,6 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 		}
 	}
 
-	lock_extent(&inode->io_tree, start, end, NULL);
-
 	alloc_hint = get_extent_allocation_hint(inode, start, num_bytes);
 
 	/*
@@ -1429,6 +1427,9 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 		extent_reserved = true;
 
 		ram_size = ins.offset;
+
+		lock_extent(&inode->io_tree, start, start + ram_size - 1, NULL);
+
 		em = create_io_em(inode, start, ins.offset, /* len */
 				  start, /* orig_start */
 				  ins.objectid, /* block_start */
@@ -1438,6 +1439,8 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 				  BTRFS_COMPRESS_NONE, /* compress_type */
 				  BTRFS_ORDERED_REGULAR /* type */);
 		if (IS_ERR(em)) {
+			unlock_extent(&inode->io_tree, start,
+				      start + ram_size - 1, NULL);
 			ret = PTR_ERR(em);
 			goto out_reserve;
 		}
@@ -1448,6 +1451,8 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 					0, 1 << BTRFS_ORDERED_REGULAR,
 					BTRFS_COMPRESS_NONE);
 		if (IS_ERR(ordered)) {
+			unlock_extent(&inode->io_tree, start,
+				      start + ram_size - 1, NULL);
 			ret = PTR_ERR(ordered);
 			goto out_drop_extent_cache;
 		}
@@ -1549,6 +1554,13 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 					     locked_page, 0, page_ops);
 	}
 
+	/*
+	 * At this point we're unlocked, we want to make sure we're only
+	 * clearing these flags under the extent lock, so lock the rest of the
+	 * range and clear everything up.
+	 */
+	lock_extent(&inode->io_tree, start, end, NULL);
+
 	/*
 	 * For the range (2). If we reserved an extent for our delalloc range
 	 * (or a subrange) and failed to create the respective ordered extent,
-- 
2.43.0

Re: [PATCH 15/17] btrfs: push lock_extent down in cow_file_range()

[Goldwyn Rodrigues]

On 10:35 17/04, Josef Bacik wrote:
> Now that we've got the extent lock pushed into cow_file_range() we can
> push it further down into the allocation loop.  This allows us to only
> hold the extent lock during the dropping of the extent map range and
> inserting the ordered extent.
> 
> This makes the error case a little trickier as we'll now have to lock
> the range before clearing any of the other extent bits for the range,
> but this is the error path so is less performance critical.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

[PATCH 16/17] btrfs: push extent lock down in submit_one_async_extent

[Josef Bacik]

We don't need to include the time we spend in the allocator under our
extent lock protection, move it after the allocator and make sure we
lock the extent in the error case to ensure we're not clearing these
bits without the extent lock held.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 3cd092fab31d..a96f68f61495 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1179,7 +1179,6 @@ static void submit_one_async_extent(struct async_chunk *async_chunk,
 		goto done;
 	}
 
-	lock_extent(io_tree, start, end, NULL);
 	ret = btrfs_reserve_extent(root, async_extent->ram_size,
 				   async_extent->compressed_size,
 				   async_extent->compressed_size,
@@ -1195,6 +1194,8 @@ static void submit_one_async_extent(struct async_chunk *async_chunk,
 		goto done;
 	}
 
+	lock_extent(io_tree, start, end, NULL);
+
 	/* Here we're doing allocation and writeback of the compressed pages */
 	em = create_io_em(inode, start,
 			  async_extent->ram_size,	/* len */
-- 
2.43.0

Re: [PATCH 16/17] btrfs: push extent lock down in submit_one_async_extent

[Goldwyn Rodrigues]

On 10:36 17/04, Josef Bacik wrote:
> We don't need to include the time we spend in the allocator under our
> extent lock protection, move it after the allocator and make sure we
> lock the extent in the error case to ensure we're not clearing these
> bits without the extent lock held.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Looks good.
Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

-- 
Goldwyn

[PATCH 17/17] btrfs: add a cached state to extent_clear_unlock_delalloc

[Josef Bacik]

Now that we have the lock_extent tightly coupled with
extent_clear_unlock_delalloc we can add a cached state to
extent_clear_unlock_delalloc and benefit from skipping the extra lookup
when we're doing cow.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/extent_io.c |  3 ++-
 fs/btrfs/extent_io.h |  2 ++
 fs/btrfs/inode.c     | 42 ++++++++++++++++++++++++------------------
 3 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index c09f46f969b1..8ec5f5c7bfe1 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -412,9 +412,10 @@ noinline_for_stack bool find_lock_delalloc_range(struct inode *inode,
 
 void extent_clear_unlock_delalloc(struct btrfs_inode *inode, u64 start, u64 end,
 				  struct page *locked_page,
+				  struct extent_state **cached,
 				  u32 clear_bits, unsigned long page_ops)
 {
-	clear_extent_bit(&inode->io_tree, start, end, clear_bits, NULL);
+	clear_extent_bit(&inode->io_tree, start, end, clear_bits, cached);
 
 	__process_pages_contig(inode->vfs_inode.i_mapping, locked_page,
 			       start, end, page_ops);
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index c81a9b546c9f..e62dc720e1d2 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -27,6 +27,7 @@ struct address_space;
 struct writeback_control;
 struct extent_io_tree;
 struct extent_map_tree;
+struct extent_state;
 struct btrfs_block_group;
 struct btrfs_fs_info;
 struct btrfs_inode;
@@ -352,6 +353,7 @@ void clear_extent_buffer_uptodate(struct extent_buffer *eb);
 void extent_range_clear_dirty_for_io(struct inode *inode, u64 start, u64 end);
 void extent_clear_unlock_delalloc(struct btrfs_inode *inode, u64 start, u64 end,
 				  struct page *locked_page,
+				  struct extent_state **cached,
 				  u32 bits_to_clear, unsigned long page_ops);
 int extent_invalidate_folio(struct extent_io_tree *tree,
 			    struct folio *folio, size_t offset);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index a96f68f61495..7a738ec41095 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -762,8 +762,8 @@ static noinline int cow_file_range_inline(struct btrfs_inode *inode, u64 offset,
 		return ret;
 	}
 
-	free_extent_state(cached);
-	extent_clear_unlock_delalloc(inode, offset, end, NULL, clear_flags,
+	extent_clear_unlock_delalloc(inode, offset, end, NULL, &cached,
+				     clear_flags,
 				     PAGE_UNLOCK | PAGE_START_WRITEBACK |
 				     PAGE_END_WRITEBACK);
 	return ret;
@@ -1154,6 +1154,7 @@ static void submit_one_async_extent(struct async_chunk *async_chunk,
 	struct btrfs_ordered_extent *ordered;
 	struct btrfs_key ins;
 	struct page *locked_page = NULL;
+	struct extent_state *cached = NULL;
 	struct extent_map *em;
 	int ret = 0;
 	u64 start = async_extent->start;
@@ -1194,7 +1195,7 @@ static void submit_one_async_extent(struct async_chunk *async_chunk,
 		goto done;
 	}
 
-	lock_extent(io_tree, start, end, NULL);
+	lock_extent(io_tree, start, end, &cached);
 
 	/* Here we're doing allocation and writeback of the compressed pages */
 	em = create_io_em(inode, start,
@@ -1229,7 +1230,7 @@ static void submit_one_async_extent(struct async_chunk *async_chunk,
 
 	/* Clear dirty, set writeback and unlock the pages. */
 	extent_clear_unlock_delalloc(inode, start, end,
-			NULL, EXTENT_LOCKED | EXTENT_DELALLOC,
+			NULL, &cached, EXTENT_LOCKED | EXTENT_DELALLOC,
 			PAGE_UNLOCK | PAGE_START_WRITEBACK);
 	btrfs_submit_compressed_write(ordered,
 			    async_extent->folios,	/* compressed_folios */
@@ -1247,7 +1248,8 @@ static void submit_one_async_extent(struct async_chunk *async_chunk,
 	btrfs_free_reserved_extent(fs_info, ins.objectid, ins.offset, 1);
 	mapping_set_error(inode->vfs_inode.i_mapping, -EIO);
 	extent_clear_unlock_delalloc(inode, start, end,
-				     NULL, EXTENT_LOCKED | EXTENT_DELALLOC |
+				     NULL, &cached,
+				     EXTENT_LOCKED | EXTENT_DELALLOC |
 				     EXTENT_DELALLOC_NEW |
 				     EXTENT_DEFRAG | EXTENT_DO_ACCOUNTING,
 				     PAGE_UNLOCK | PAGE_START_WRITEBACK |
@@ -1329,6 +1331,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 {
 	struct btrfs_root *root = inode->root;
 	struct btrfs_fs_info *fs_info = root->fs_info;
+	struct extent_state *cached = NULL;
 	u64 alloc_hint = 0;
 	u64 orig_start = start;
 	u64 num_bytes;
@@ -1429,7 +1432,8 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 
 		ram_size = ins.offset;
 
-		lock_extent(&inode->io_tree, start, start + ram_size - 1, NULL);
+		lock_extent(&inode->io_tree, start, start + ram_size - 1,
+			    &cached);
 
 		em = create_io_em(inode, start, ins.offset, /* len */
 				  start, /* orig_start */
@@ -1441,7 +1445,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 				  BTRFS_ORDERED_REGULAR /* type */);
 		if (IS_ERR(em)) {
 			unlock_extent(&inode->io_tree, start,
-				      start + ram_size - 1, NULL);
+				      start + ram_size - 1, &cached);
 			ret = PTR_ERR(em);
 			goto out_reserve;
 		}
@@ -1453,7 +1457,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 					BTRFS_COMPRESS_NONE);
 		if (IS_ERR(ordered)) {
 			unlock_extent(&inode->io_tree, start,
-				      start + ram_size - 1, NULL);
+				      start + ram_size - 1, &cached);
 			ret = PTR_ERR(ordered);
 			goto out_drop_extent_cache;
 		}
@@ -1493,7 +1497,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 		page_ops |= PAGE_SET_ORDERED;
 
 		extent_clear_unlock_delalloc(inode, start, start + ram_size - 1,
-					     locked_page,
+					     locked_page, &cached,
 					     EXTENT_LOCKED | EXTENT_DELALLOC,
 					     page_ops);
 		if (num_bytes < cur_alloc_size)
@@ -1552,7 +1556,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 		if (!locked_page)
 			mapping_set_error(inode->vfs_inode.i_mapping, ret);
 		extent_clear_unlock_delalloc(inode, orig_start, start - 1,
-					     locked_page, 0, page_ops);
+					     locked_page, NULL, 0, page_ops);
 	}
 
 	/*
@@ -1575,7 +1579,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 	if (extent_reserved) {
 		extent_clear_unlock_delalloc(inode, start,
 					     start + cur_alloc_size - 1,
-					     locked_page,
+					     locked_page, &cached,
 					     clear_bits,
 					     page_ops);
 		start += cur_alloc_size;
@@ -1590,7 +1594,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 	if (start < end) {
 		clear_bits |= EXTENT_CLEAR_DATA_RESV;
 		extent_clear_unlock_delalloc(inode, start, end, locked_page,
-					     clear_bits, page_ops);
+					     &cached, clear_bits, page_ops);
 	}
 	return ret;
 }
@@ -2206,11 +2210,10 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 		btrfs_put_ordered_extent(ordered);
 
 		extent_clear_unlock_delalloc(inode, cur_offset, nocow_end,
-					     locked_page, EXTENT_LOCKED |
-					     EXTENT_DELALLOC |
+					     locked_page, &cached_state,
+					     EXTENT_LOCKED | EXTENT_DELALLOC |
 					     EXTENT_CLEAR_DATA_RESV,
 					     PAGE_UNLOCK | PAGE_SET_ORDERED);
-		free_extent_state(cached_state);
 
 		cur_offset = extent_end;
 
@@ -2252,10 +2255,13 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 	 * we're not locked at this point.
 	 */
 	if (cur_offset < end) {
-		lock_extent(&inode->io_tree, cur_offset, end, NULL);
+		struct extent_state *cached = NULL;
+
+		lock_extent(&inode->io_tree, cur_offset, end, &cached);
 		extent_clear_unlock_delalloc(inode, cur_offset, end,
-					     locked_page, EXTENT_LOCKED |
-					     EXTENT_DELALLOC | EXTENT_DEFRAG |
+					     locked_page, &cached,
+					     EXTENT_LOCKED | EXTENT_DELALLOC |
+					     EXTENT_DEFRAG |
 					     EXTENT_DO_ACCOUNTING, PAGE_UNLOCK |
 					     PAGE_START_WRITEBACK |
 					     PAGE_END_WRITEBACK);
-- 
2.43.0

Re: [PATCH 17/17] btrfs: add a cached state to extent_clear_unlock_delalloc

[Goldwyn Rodrigues]

On 10:36 17/04, Josef Bacik wrote:
> Now that we have the lock_extent tightly coupled with
> extent_clear_unlock_delalloc we can add a cached state to
> extent_clear_unlock_delalloc and benefit from skipping the extra lookup
> when we're doing cow.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Yay, for caching!
Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

-- 
Goldwyn

Re: [PATCH 17/17] btrfs: add a cached state to extent_clear_unlock_delalloc

[Goldwyn Rodrigues]

On 10:36 17/04, Josef Bacik wrote:
> Now that we have the lock_extent tightly coupled with
> extent_clear_unlock_delalloc we can add a cached state to
> extent_clear_unlock_delalloc and benefit from skipping the extra lookup
> when we're doing cow.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Looks good.
Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

Re: [PATCH 00/17] btrfs: restrain lock extent usage during writeback

[Christoph Hellwig]

On Wed, Apr 17, 2024 at 10:35:44AM -0400, Josef Bacik wrote:
> discreet set of operations.  Being able to clearly define what the extent lock
> is protecting will give us a better idea of how to reduce it's usage or possibly
> replace it in the future.

It should also allow to stop taking it in ->read_folio and ->readahead,
which is what is making the btrfs I/O path so weird and incompatbile
with the rest of the kernel.  I tried to get rid of just that a while
ago but spectacularly failed.  Maybe doing this in smaller steps and
by someone knowning the code better is going to be more successful.

Re: Re: [PATCH 00/17] btrfs: restrain lock extent usage during writeback

[Goldwyn Rodrigues]

On 22:54 17/04, Christoph Hellwig wrote:
> On Wed, Apr 17, 2024 at 10:35:44AM -0400, Josef Bacik wrote:
> > discreet set of operations.  Being able to clearly define what the extent lock
> > is protecting will give us a better idea of how to reduce it's usage or possibly
> > replace it in the future.
> 
> It should also allow to stop taking it in ->read_folio and ->readahead,
> which is what is making the btrfs I/O path so weird and incompatbile
> with the rest of the kernel.  I tried to get rid of just that a while
> ago but spectacularly failed.  Maybe doing this in smaller steps and
> by someone knowning the code better is going to be more successful.
> 

The only reason I have encountered for taking extent locks during reads
is for checksums. read()s collects checksums before submitting the bio
where as writeback() adds the checksums during bio completion.

So, there is a small window where a read() performed immediately after
writeback+truncate pages would give an EIO because the checksum is
not in the checksum tree and does not match the calculated checksum.

If we can delay retrieving the checksum or wait for ordered extents to
complete before performing the read, I think avoiding extent locks
during read is possible.


-- 
Goldwyn

Re: [PATCH 00/17] btrfs: restrain lock extent usage during writeback

[Christoph Hellwig]

On Thu, Apr 18, 2024 at 08:45:35AM -0500, Goldwyn Rodrigues wrote:
> The only reason I have encountered for taking extent locks during reads
> is for checksums. read()s collects checksums before submitting the bio
> where as writeback() adds the checksums during bio completion.
> 
> So, there is a small window where a read() performed immediately after
> writeback+truncate pages would give an EIO because the checksum is
> not in the checksum tree and does not match the calculated checksum.
> 
> If we can delay retrieving the checksum or wait for ordered extents to
> complete before performing the read, I think avoiding extent locks
> during read is possible.

And the fix for that is to only clear the writeback bit once the
ordered extent processing has finished, which is the other bit
making btrfs I/O so different from the core kernels expectations.
It is highly coupled with the extent lock semantics as far as I
can tell.

Re: [PATCH 00/17] btrfs: restrain lock extent usage during writeback

[Josef Bacik]

On Thu, Apr 18, 2024 at 07:47:26AM -0700, Christoph Hellwig wrote:
> On Thu, Apr 18, 2024 at 08:45:35AM -0500, Goldwyn Rodrigues wrote:
> > The only reason I have encountered for taking extent locks during reads
> > is for checksums. read()s collects checksums before submitting the bio
> > where as writeback() adds the checksums during bio completion.
> > 
> > So, there is a small window where a read() performed immediately after
> > writeback+truncate pages would give an EIO because the checksum is
> > not in the checksum tree and does not match the calculated checksum.
> > 
> > If we can delay retrieving the checksum or wait for ordered extents to
> > complete before performing the read, I think avoiding extent locks
> > during read is possible.
> 
> And the fix for that is to only clear the writeback bit once the
> ordered extent processing has finished, which is the other bit
> making btrfs I/O so different from the core kernels expectations.
> It is highly coupled with the extent lock semantics as far as I
> can tell.
> 

They aren't coupled, but they are related.  My follow-up work is to make this
change and see what breaks, and then tackle the read side.  This should pave the
way for a straightforward iomap conversion.  Thanks,

Josef