btrfs: make subpage handling be feature full

Currently reading an inline data extent will zero out all the remaining range in the page.

This is not yet causing problems even for block size < page size (subpage) cases because:

1) An inline data extent always starts at file offset 0 Meaning at page read, we always read the inline extent first, before any other blocks in the page. Then later blocks are properly read out and re-fill the zeroed out ranges.

2) Currently btrfs will read out the whole page if a buffered write is not page aligned
So a page is either fully uptodate at buffered write time (covers the whole page), or we will read out the whole page first. Meaning there is nothing to lose for such an inline extent read.

But it's still not ideal:

- We're zeroing out the page twice
One done by read_inline_extent()/uncompress_inline(), one done by btrfs_do_readpage() for ranges beyond i_size.

- We're touching blocks that doesn't belong to the inline extent In the incoming patches, we can have a partial uptodate folio, that some dirty blocks can exist while the page is not fully uptodate:

The page size is 16K and block size is 4K:

  0         4K        8K        12K        16K
  |         |         |/////////|          |

And range [8K, 12K) is dirtied by a buffered write, the remaining blocks are not uptodate.

If range [0, 4K) contains an inline data extent, and we try to read the whole page, the current behavior will overwrite range [8K, 12K) with zero and cause data loss.

So to make the behavior more consistent and in preparation for future changes, limit the inline data extents read to only zero out the range inside the first block, not the whole page.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/inode.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

fs/btrfs/inode.c +3 -2

index c432ccfba56e..fe2c6038064a 100644
@@ -6788,6 +6788,7 @@ static noinline int uncompress_inline(struct btrfs_path *path,
 {
        int ret;
        struct extent_buffer *leaf = path->nodes[0];
+       const u32 sectorsize = leaf->fs_info->sectorsize;
        char *tmp;
        size_t max_size;
        unsigned long inline_size;

@@ -6816,17 +6817,19 @@ static noinline int uncompress_inline(struct btrfs_path *path,
         * cover that region here.
         */
 
-       if (max_size < PAGE_SIZE)
-               folio_zero_range(folio, max_size, PAGE_SIZE - max_size);
+       if (max_size < sectorsize)
+               folio_zero_range(folio, max_size, sectorsize - max_size);
        kfree(tmp);
        return ret;

fs/btrfs/inode.c +3 -4

        if (!folio || folio_test_uptodate(folio))
                return 0;

@@ -6844,8 +6847,8 @@ static int read_inline_extent(struct btrfs_path *path, struct folio *folio)
        read_extent_buffer(path->nodes[0], kaddr,
                           btrfs_file_extent_inline_start(fi), copy_size);
        kunmap_local(kaddr);
-       if (copy_size < PAGE_SIZE)
-               folio_zero_range(folio, copy_size, PAGE_SIZE - copy_size);
+       if (copy_size < sectorsize)
+               folio_zero_range(folio, copy_size, sectorsize - copy_size);
        return 0;
 }
 

@@ -7020,7 +7023,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
                ASSERT(em->disk_bytenr == EXTENT_MAP_INLINE);
                ASSERT(em->len == fs_info->sectorsize);
 
-               ret = read_inline_extent(path, folio);
+               ret = read_inline_extent(fs_info, path, folio);
                if (ret < 0)
                        goto out;
                goto insert;
--

2.48.1

Inside function __cow_file_range_inline() since the inlined data no longer takes any data space, we need to free up the reserved space.

However the code is still using the old page size == sector size assumption, and will not handle subpage case well.

Thankfully it is not going to cause any problems because we have two extra safe nets:

- Inline data extents creation is disable for sector size < page size cases for now
But it won't stay that for long.

- btrfs_qgroup_free_data() will only clear ranges which are already reserved
So even if we pass a range larger than what we need, it should still be fine, especially there is only reserved space for a single block at file offset 0 for an inline data extent.

But just for the sake of consistentcy, fix the call site to use sectorsize instead of page size.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/btrfs/inode.c +1 -2

index fe2c6038064a..0efe9f005149 100644
@@ -672,7 +672,7 @@ static noinline int __cow_file_range_inline(struct btrfs_inode *inode,
         * And at reserve time, it's always aligned to page size, so
         * just free one page here.
         */
-       btrfs_qgroup_free_data(inode, NULL, 0, PAGE_SIZE, NULL);
+       btrfs_qgroup_free_data(inode, NULL, 0, fs_info->sectorsize, NULL);
        btrfs_free_path(path);
        btrfs_end_transaction(trans);
        return ret;
--

2.48.1

Quoted 65 lines · Qu Wenruo

Currently reading an inline data extent will zero out all the remaining range in the page.

This is not yet causing problems even for block size < page size (subpage) cases because:

1) An inline data extent always starts at file offset 0 Meaning at page read, we always read the inline extent first, before any other blocks in the page. Then later blocks are properly read out and re-fill the zeroed out ranges.

2) Currently btrfs will read out the whole page if a buffered write is not page aligned
So a page is either fully uptodate at buffered write time (covers the whole page), or we will read out the whole page first. Meaning there is nothing to lose for such an inline extent read.

But it's still not ideal:

- We're zeroing out the page twice
One done by read_inline_extent()/uncompress_inline(), one done by btrfs_do_readpage() for ranges beyond i_size.

- We're touching blocks that doesn't belong to the inline extent In the incoming patches, we can have a partial uptodate folio, that some dirty blocks can exist while the page is not fully uptodate:

The page size is 16K and block size is 4K:

  0         4K        8K        12K        16K
  |         |         |/////////|          |

And range [8K, 12K) is dirtied by a buffered write, the remaining blocks are not uptodate.

If range [0, 4K) contains an inline data extent, and we try to read the whole page, the current behavior will overwrite range [8K, 12K) with zero and cause data loss.

So to make the behavior more consistent and in preparation for future changes, limit the inline data extents read to only zero out the range inside the first block, not the whole page.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/inode.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

fs/btrfs/inode.c +1 -0

index c432ccfba56e..fe2c6038064a 100644
@@ -6788,6 +6788,7 @@ static noinline int uncompress_inline(struct btrfs_path *path,
 {
        int ret;
        struct extent_buffer *leaf = path->nodes[0];
+       const u32 sectorsize = leaf->fs_info->sectorsize;
        char *tmp;
        size_t max_size;
        unsigned long inline_size;

@@ -6816,17 +6817,19 @@ static noinline int uncompress_inline(struct btrfs_path *path,
         * cover that region here.
         */
 

fs/btrfs/inode.c static noinline int uncompress_inline(struct btrfs_path *path, · line 6821

Quoted 4 lines

fs/btrfs/inode.c +2 -2

-       if (max_size < PAGE_SIZE)
-               folio_zero_range(folio, max_size, PAGE_SIZE - max_size);
+       if (max_size < sectorsize)
+               folio_zero_range(folio, max_size, sectorsize - max_size);

fs/btrfs/inode.c static noinline int uncompress_inline(struct btrfs_path *path, · line 6832

Quoted 12 lines

fs/btrfs/inode.c +3 -1

        kfree(tmp);
        return ret;
 }
 
-static int read_inline_extent(struct btrfs_path *path, struct folio *folio)
+static int read_inline_extent(struct btrfs_fs_info *fs_info,
+                             struct btrfs_path *path, struct folio *folio)
 {
        struct btrfs_file_extent_item *fi;
        void *kaddr;
        size_t copy_size;
+       const u32 sectorsize = fs_info->sectorsize;

There's no need to add the fs_info argument just to get the sectorsize. We can get it like this:

const u32 sectorsize = path->nodes[0]->fs_info->sectorsize;

Otherwise it looks good, so:

Reviewed-by Filipe Manana <[email protected]>

Thanks.

Quoted 27 lines

fs/btrfs/inode.c +3 -4

        if (!folio || folio_test_uptodate(folio))
                return 0;

@@ -6844,8 +6847,8 @@ static int read_inline_extent(struct btrfs_path *path, struct folio *folio)
        read_extent_buffer(path->nodes[0], kaddr,
                           btrfs_file_extent_inline_start(fi), copy_size);
        kunmap_local(kaddr);
-       if (copy_size < PAGE_SIZE)
-               folio_zero_range(folio, copy_size, PAGE_SIZE - copy_size);
+       if (copy_size < sectorsize)
+               folio_zero_range(folio, copy_size, sectorsize - copy_size);
        return 0;
 }
 

@@ -7020,7 +7023,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
                ASSERT(em->disk_bytenr == EXTENT_MAP_INLINE);
                ASSERT(em->len == fs_info->sectorsize);
 
-               ret = read_inline_extent(path, folio);
+               ret = read_inline_extent(fs_info, path, folio);
                if (ret < 0)
                        goto out;
                goto insert;
--

2.48.1

Currently we're using btrfs_lock_and_flush_ordered_range() for both btrfs_read_folio() and btrfs_readahead(), but it has one critical problem for future subpage enhancements:

- It will call btrfs_start_ordered_extent() to writeback the involved folios

But remember we're calling btrfs_lock_and_flush_ordered_range() at read paths, meaning the folio is already locked by read path.

If we really trigger writeback, this will lead to a deadlock and writeback can not hold the folio lock.

Such dead lock is prevented by the fact that btrfs always keeps a dirty folio also uptodate, by either dirtying all blocks of the folio, or read the folio before dirtying.

But it's not the common behavior, as XFS/EXT4 both allow the folio to be dirty but not uptodate, by allowing buffered write to dirty a full block without reading the full folio.

Instead of blindly calling btrfs_start_ordered_extent(), introduce a newer helper, which is smarter in the following ways:

- Only wait and flush the ordered extent if
* The folio doesn't even have private set
* Part of the blocks of the ordered extent is not uptodate

This can happen by:
* Folio writeback finished, then get invalidated. But OE not yet finished

* Direct IO

We have to wait for the ordered extent, as it may contain to-be-inserted data checksum.
Without waiting, our read can fail due to the missing csum.

But either way, the OE should not need any extra flush inside the locked folio range.

- Skip the ordered extent completely if
* All the blocks are dirty
This happens when OE creation is caused by previous folio.

The writeback will never happen (we're holding the folio lock for read), nor will the OE finish.

Thus we must skip the range.

* All the blocks are uptodate
This happens when the writeback finished, but OE not yet finished.

Since the blocks are already uptodate, we can skip the OE range.

The newer helper, lock_extents_for_read() will do a loop for the target range by:

1) Lock the full range

2) If there is no ordered extent in the remaining range, exit

3) If there is an ordered extent that we can skip Skip to the end of the OE, and continue checking We do not trigger writeback nor wait for the OE.

4) If there is an ordered extent that we can not skip Unlock the whole extent range and start the ordered extent.

And also update btrfs_start_ordered_extent() to add two more parameters:
@nowriteback_start and @nowriteback_len, to prevent triggering flush for a certain range.

This will allow us to handle the following case properly in the future:

16K page size, 4K btrfs block size:

 16K           20K             24K              28K            32K
 |/////////////////////////////|                |              |
 |<----------- OE 2----------->|                |<--- OE 1 --->|

The folio has been written back before, thus we have an OE at [28K, 32K).
Although the OE 1 finished its IO, the OE is not yet removed from IO tree.
Later the folio got invalidated, but OE still exists.

And [16K, 24K) range is dirty and uptodate, caused by a block aligned buffered write (and future enhancements allowing btrfs to skip full folio read for such case).

Furthermore, OE 2 is created covering range [16K, 24K) by the writeback of previous folio.

Since the full folio is not uptodate, if we want to read the folio, the existing btrfs_lock_and_flush_ordered_range() will dead lock, by:

btrfs_read_folio()
| Folio 16K is already locked
|- btrfs_lock_and_flush_ordered_range()

    |- btrfs_start_ordered_extent() for range [16K, 24K)
       |- filemap_fdatawrite_range() for range [16K, 24K)
          |- extent_write_cache_pages()
             folio_lock() on folio 16K, deadlock.

But now we will have the following sequence:

btrfs_read_folio()
| Folio 16K is already locked
|- lock_extents_for_read()

    |- can_skip_ordered_extent() for range [16K, 24K)
    |  Returned true, the range [16K, 24K) will be skipped.
    |- can_skip_ordered_extent() for range [28K, 32K)
    |  Returned false.
    |- btrfs_start_ordered_extent() for range [28K, 32K) with

[16K, 32K) as no writeback range
No writeback for folio 16K will be triggered.

And there will be no more possible deadlock on the same folio.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/defrag.c       |   2 +-
 fs/btrfs/direct-io.c    |   2 +-
 fs/btrfs/extent_io.c    | 183 +++++++++++++++++++++++++++++++++++++++-
 fs/btrfs/file.c         |   4 +-
 fs/btrfs/inode.c        |   4 +-
 fs/btrfs/ordered-data.c |  29 +++++--
 fs/btrfs/ordered-data.h |   3 +-
 7 files changed, 210 insertions(+), 17 deletions(-)

fs/btrfs/defrag.c +1 -1

index 18f0704263f3..4b89094da3de 100644
@@ -902,7 +902,7 @@ static struct folio *defrag_prepare_one_folio(struct btrfs_inode *inode, pgoff_t
                        break;
 
                folio_unlock(folio);
-               btrfs_start_ordered_extent(ordered);
+               btrfs_start_ordered_extent(ordered, 0, 0);
                btrfs_put_ordered_extent(ordered);
                folio_lock(folio);
                /*

fs/btrfs/direct-io.c +1 -1

index e8548de319e7..fb6df17fb55c 100644
@@ -103,7 +103,7 @@ static int lock_extent_direct(struct inode *inode, u64 lockstart, u64 lockend,
                         */
                        if (writing ||
                            test_bit(BTRFS_ORDERED_DIRECT, &ordered->flags))
-                               btrfs_start_ordered_extent(ordered);
+                               btrfs_start_ordered_extent(ordered, 0, 0);
                        else
                                ret = nowait ? -EAGAIN : -ENOTBLK;
                        btrfs_put_ordered_extent(ordered);

fs/btrfs/extent_io.c +0 -0

index 7b0aa332aedc..a6ffee6f6fd9 100644

fs/btrfs/extent_io.c +13 -0

+                                      struct btrfs_ordered_extent *ordered,
+                                      u64 cur, u64 *next_ret)
+{
+       const struct btrfs_fs_info *fs_info = binode->root->fs_info;
+       struct folio *folio;
+       const u32 blocksize = fs_info->sectorsize;
+       u64 range_len;
+       bool ret;
+
+       folio = filemap_get_folio(binode->vfs_inode.i_mapping,
+                                 cur >> PAGE_SHIFT);
+
+       /*

fs/btrfs/extent_io.c +21 -0

+
+       /*
+        * 1) The first block is DIRTY.
+        *
+        * This means the OE is created by some folio before us, but writeback
+        * has not started.
+        * We can and must skip the whole OE, because it will never start until
+        * we finished our folio read and unlocked the folio.
+        */
+       if (btrfs_folio_test_dirty(fs_info, folio, cur, blocksize)) {
+               ret = true;
+               /*
+                * At least inside the folio, all the remaining blocks should
+                * also be dirty.
+                */
+               ASSERT(btrfs_folio_test_dirty(fs_info, folio, cur, range_len));
+               *next_ret = ordered->file_offset + ordered->num_bytes;
+               goto out;
+       }
+
+       /*

fs/btrfs/extent_io.c +14 -0

+
+               /*
+                * The whole range to the OE end or folio boundary should also
+                * be uptodate.
+                */
+               ASSERT(btrfs_folio_test_uptodate(fs_info, folio, cur, range_len));
+               ret = true;
+               *next_ret = cur + range_len;
+               goto out;
+       }
+
+       /*
+        * 3) The first block is not uptodate.
+        *

fs/btrfs/extent_io.c +35 -2

+
+again:
+       lock_extent(&binode->io_tree, start, end, cached_state);
+       cur_pos = start;
+       while (cur_pos < end) {
+               ordered = btrfs_lookup_ordered_range(binode, cur_pos,
+                                                    end - cur_pos + 1);
+               /*
+                * No ordered extents in the range, and we hold the
+                * extent lock, no one can modify the extent maps
+                * in the range, we're safe to return.
+                */
+               if (!ordered)
+                       break;
+
+               /* Check if we can skip waiting for the whole OE. */
+               if (can_skip_ordered_extent(binode, ordered, start, end)) {
+                       cur_pos = min(ordered->file_offset + ordered->num_bytes,
+                                     end + 1);
+                       btrfs_put_ordered_extent(ordered);
+                       continue;
+               }
+
+               /* Now wait for the OE to finish. */
+               unlock_extent(&binode->io_tree, start, end,
+                             cached_state);
+               btrfs_start_ordered_extent(ordered, start, end + 1 - start);
+               btrfs_put_ordered_extent(ordered);
+               /* We have unlocked the whole range, restart from the beginning. */
+               goto again;
+       }
+}
+
 int btrfs_read_folio(struct file *file, struct folio *folio)
 {
        struct btrfs_inode *inode = folio_to_inode(folio);

@@ -1091,7 +1270,7 @@ int btrfs_read_folio(struct file *file, struct folio *folio)
        struct extent_map *em_cached = NULL;
        int ret;
 
-       btrfs_lock_and_flush_ordered_range(inode, start, end, &cached_state);
+       lock_extents_for_read(inode, start, end, &cached_state);
        ret = btrfs_do_readpage(folio, &em_cached, &bio_ctrl, NULL);
        unlock_extent(&inode->io_tree, start, end, &cached_state);
 

@@ -2380,7 +2559,7 @@ void btrfs_readahead(struct readahead_control *rac)
        struct extent_map *em_cached = NULL;
        u64 prev_em_start = (u64)-1;
 
-       btrfs_lock_and_flush_ordered_range(inode, start, end, &cached_state);
+       lock_extents_for_read(inode, start, end, &cached_state);
 
        while ((folio = readahead_folio(rac)) != NULL)
                btrfs_do_readpage(folio, &em_cached, &bio_ctrl, &prev_em_start);

fs/btrfs/file.c +2 -2

index e87d4a37c929..00c68b7b2206 100644
@@ -941,7 +941,7 @@ lock_and_cleanup_extent_if_need(struct btrfs_inode *inode, struct folio *folio,
                                      cached_state);
                        folio_unlock(folio);
                        folio_put(folio);
-                       btrfs_start_ordered_extent(ordered);
+                       btrfs_start_ordered_extent(ordered, 0, 0);
                        btrfs_put_ordered_extent(ordered);
                        return -EAGAIN;
                }

@@ -1855,7 +1855,7 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
                unlock_extent(io_tree, page_start, page_end, &cached_state);
                folio_unlock(folio);
                up_read(&BTRFS_I(inode)->i_mmap_lock);
-               btrfs_start_ordered_extent(ordered);
+               btrfs_start_ordered_extent(ordered, 0, 0);
                btrfs_put_ordered_extent(ordered);
                goto again;
        }

fs/btrfs/inode.c +2 -2

index 0efe9f005149..e99cb5109967 100644
@@ -2821,7 +2821,7 @@ static void btrfs_writepage_fixup_worker(struct btrfs_work *work)
                unlock_extent(&inode->io_tree, page_start, page_end,
                              &cached_state);
                folio_unlock(folio);
-               btrfs_start_ordered_extent(ordered);
+               btrfs_start_ordered_extent(ordered, 0, 0);
                btrfs_put_ordered_extent(ordered);
                goto again;
        }

@@ -4873,7 +4873,7 @@ int btrfs_truncate_block(struct btrfs_inode *inode, loff_t from, loff_t len,
                unlock_extent(io_tree, block_start, block_end, &cached_state);
                folio_unlock(folio);
                folio_put(folio);
-               btrfs_start_ordered_extent(ordered);
+               btrfs_start_ordered_extent(ordered, 0, 0);
                btrfs_put_ordered_extent(ordered);
                goto again;
        }

fs/btrfs/ordered-data.c +21 -8

index 4aca7475fd82..6075a6fa4817 100644
@@ -729,7 +729,7 @@ static void btrfs_run_ordered_extent_work(struct btrfs_work *work)
        struct btrfs_ordered_extent *ordered;
 
        ordered = container_of(work, struct btrfs_ordered_extent, flush_work);
-       btrfs_start_ordered_extent(ordered);
+       btrfs_start_ordered_extent(ordered, 0, 0);
        complete(&ordered->completion);
 }
 

@@ -842,10 +842,12 @@ void btrfs_wait_ordered_roots(struct btrfs_fs_info *fs_info, u64 nr,
 /*
  * Start IO and wait for a given ordered extent to finish.
  *
- * Wait on page writeback for all the pages in the extent and the IO completion
- * code to insert metadata into the btree corresponding to the extent.
+ * Wait on page writeback for all the pages in the extent but not in
+ * [@nowriteback_start, @nowriteback_start + @nowriteback_len) and the
+ * IO completion code to insert metadata into the btree corresponding to the extent.
  */
-void btrfs_start_ordered_extent(struct btrfs_ordered_extent *entry)
+void btrfs_start_ordered_extent(struct btrfs_ordered_extent *entry,
+                               u64 nowriteback_start, u32 nowriteback_len)
 {
        u64 start = entry->file_offset;
        u64 end = start + entry->num_bytes - 1;

@@ -865,8 +867,19 @@ void btrfs_start_ordered_extent(struct btrfs_ordered_extent *entry)
         * start IO on any dirty ones so the wait doesn't stall waiting
         * for the flusher thread to find them
         */
-       if (!test_bit(BTRFS_ORDERED_DIRECT, &entry->flags))
-               filemap_fdatawrite_range(inode->vfs_inode.i_mapping, start, end);
+       if (!test_bit(BTRFS_ORDERED_DIRECT, &entry->flags)) {
+               if (!nowriteback_len) {
+                       filemap_fdatawrite_range(inode->vfs_inode.i_mapping, start, end);
+               } else {
+                       if (start < nowriteback_start)
+                               filemap_fdatawrite_range(inode->vfs_inode.i_mapping, start,
+                                               nowriteback_start - 1);
+                       if (nowriteback_start + nowriteback_len < end)
+                               filemap_fdatawrite_range(inode->vfs_inode.i_mapping,
+                                               nowriteback_start + nowriteback_len,
+                                               end);
+               }
+       }
 
        if (!freespace_inode)
                btrfs_might_wait_for_event(inode->root->fs_info, btrfs_ordered_extent);

@@ -921,7 +934,7 @@ int btrfs_wait_ordered_range(struct btrfs_inode *inode, u64 start, u64 len)
                        btrfs_put_ordered_extent(ordered);
                        break;
                }
-               btrfs_start_ordered_extent(ordered);
+               btrfs_start_ordered_extent(ordered, 0, 0);
                end = ordered->file_offset;
                /*
                 * If the ordered extent had an error save the error but don't

@@ -1174,7 +1187,7 @@ void btrfs_lock_and_flush_ordered_range(struct btrfs_inode *inode, u64 start,
                        break;
                }
                unlock_extent(&inode->io_tree, start, end, cachedp);
-               btrfs_start_ordered_extent(ordered);
+               btrfs_start_ordered_extent(ordered, 0, 0);
                btrfs_put_ordered_extent(ordered);
        }
 }

fs/btrfs/ordered-data.h +2 -2

index be36083297a7..5e17f03d43c9 100644
@@ -192,7 +192,8 @@ void btrfs_add_ordered_sum(struct btrfs_ordered_extent *entry,
                           struct btrfs_ordered_sum *sum);
 struct btrfs_ordered_extent *btrfs_lookup_ordered_extent(struct btrfs_inode *inode,
                                                         u64 file_offset);
-void btrfs_start_ordered_extent(struct btrfs_ordered_extent *entry);
+void btrfs_start_ordered_extent(struct btrfs_ordered_extent *entry,
+                               u64 nowriteback_start, u32 nowriteback_len);
 int btrfs_wait_ordered_range(struct btrfs_inode *inode, u64 start, u64 len);
 struct btrfs_ordered_extent *
 btrfs_lookup_first_ordered_extent(struct btrfs_inode *inode, u64 file_offset);
--

2.48.1

Currently if a btrfs has its block size (the older sector size) smaller than the page size, btrfs_do_readpage() will handle the range extent by extent, this is good for performance as it doesn't need to re-lookup the same extent map again and again.
(Although __get_extent_map() already does extra cached em check, thus the optimization is not that obvious)

This is totally fine and is a valid optimization, but it has an assumption that, there is no partial uptodate range in the page.

Meanwhile there is an incoming feature, requiring btrfs to skip the full page read if a buffered write range covers a full block but not a full page.

In that case, we can have a page that is partially uptodate, and the current per-extent lookup can not handle such case.

So here we change btrfs_do_readpage() to do block-by-block read, this simplifies the following things:

- Remove the need for @iosize variable Because we just use sectorsize as our increment.

- Remove @pg_offset, and calculate it inside the loop when needed It's just offset_in_folio().

- Use a for() loop instead of a while() loop

This will slightly reduce the read performance for subpage cases, but for the future where we need to skip already uptodate blocks, it should still be worthy.

For block size == page size, this brings no performance change.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/extent_io.c | 37 ++++++++++++-------------------------
 1 file changed, 12 insertions(+), 25 deletions(-)

fs/btrfs/extent_io.c +1 -5

index a6ffee6f6fd9..b3a4a94212c9 100644
@@ -947,9 +947,7 @@ static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached,
        u64 last_byte = i_size_read(inode);
        struct extent_map *em;
        int ret = 0;
-       size_t pg_offset = 0;
-       size_t iosize;
-       size_t blocksize = fs_info->sectorsize;
+       const size_t blocksize = fs_info->sectorsize;
 
        ret = set_folio_extent_mapped(folio);
        if (ret < 0) {

@@ -960,24 +958,23 @@ static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached,
        if (folio_contains(folio, last_byte >> PAGE_SHIFT)) {
                size_t zero_offset = offset_in_folio(folio, last_byte);
 
-               if (zero_offset) {
-                       iosize = folio_size(folio) - zero_offset;

fs/btrfs/extent_io.c +7 -18

                enum btrfs_compression_type compress_type = BTRFS_COMPRESS_NONE;
+               unsigned long pg_offset = offset_in_folio(folio, cur);
                bool force_bio_submit = false;
                u64 disk_bytenr;
                u64 block_start;
 
                ASSERT(IS_ALIGNED(cur, fs_info->sectorsize));
                if (cur >= last_byte) {
-                       iosize = folio_size(folio) - pg_offset;
-                       folio_zero_range(folio, pg_offset, iosize);
-                       end_folio_read(folio, true, cur, iosize);
+                       folio_zero_range(folio, pg_offset, end - cur + 1);
+                       end_folio_read(folio, true, cur, end - cur + 1);
                        break;
                }
                em = get_extent_map(BTRFS_I(inode), folio, cur, end - cur + 1, em_cached);

@@ -991,8 +988,6 @@ static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached,
 
                compress_type = extent_map_compression(em);
 
-               iosize = min(extent_map_end(em) - cur, end - cur + 1);
-               iosize = ALIGN(iosize, blocksize);
                if (compress_type != BTRFS_COMPRESS_NONE)
                        disk_bytenr = em->disk_bytenr;
                else

@@ -1050,18 +1045,13 @@ static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached,
 
                /* we've found a hole, just zero and go on */
                if (block_start == EXTENT_MAP_HOLE) {
-                       folio_zero_range(folio, pg_offset, iosize);
-
-                       end_folio_read(folio, true, cur, iosize);
-                       cur = cur + iosize;
-                       pg_offset += iosize;
+                       folio_zero_range(folio, pg_offset, blocksize);
+                       end_folio_read(folio, true, cur, blocksize);
                        continue;
                }
                /* the get_extent function already copied into the folio */
                if (block_start == EXTENT_MAP_INLINE) {
-                       end_folio_read(folio, true, cur, iosize);
-                       cur = cur + iosize;
-                       pg_offset += iosize;
+                       end_folio_read(folio, true, cur, blocksize);
                        continue;
                }
 

@@ -1072,12 +1062,9 @@ static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached,
 
                if (force_bio_submit)
                        submit_one_bio(bio_ctrl);
-               submit_extent_folio(bio_ctrl, disk_bytenr, folio, iosize,
+               submit_extent_folio(bio_ctrl, disk_bytenr, folio, blocksize,
                                    pg_offset);
-               cur = cur + iosize;
-               pg_offset += iosize;
        }
-
        return 0;
 }
 
--

2.48.1

[BUG]
Since the support of block size (sector size) < page size for btrfs, test case generic/563 fails with 4K block size and 64K page size:

    --- tests/generic/563.out   2024-04-25 18:13:45.178550333 +0930
    +++ /home/adam/xfstests-dev/results//generic/563.out.bad    2024-09-30 09:09:16.155312379 +0930
    @@ -3,7 +3,8 @@
     read is in range
     write is in range
     write -> read/write
    -read is in range
    +read has value of 8388608
    +read is NOT in range -33792 .. 33792
     write is in range
    ...

[CAUSE]
The test case creates a 8MiB file, then buffered write into the 8MiB using 4K block size, to overwrite the whole file.

On 4K page sized systems, since the write range covers the full block and page, btrfs will no bother reading the page, just like what XFS and EXT4 do.

But 64K page sized systems, although the 4K sized write is still block aligned, it's not page aligned any more, thus btrfs will read the full page, causing more read than expected and fail the test case.

[FIX]
To skip the full page read, we need to do the following modification:

- Do not trigger full page read as long as the buffered write is block aligned
This is pretty simple by modifying the check inside prepare_uptodate_page().

- Skip already uptodate blocks during full page read Or we can lead to the following data corruption:

  0       32K        64K
  |///////|          |

Where the file range [0, 32K) is dirtied by buffered write, the remaining range [32K, 64K) is not.

When reading the full page, since [0,32K) is only dirtied but not written back, there is no data extent map for it, but a hole covering [0, 64k).

If we continue reading the full page range [0, 64K), the dirtied range will be filled with 0 (since there is only a hole covering the whole range).
This causes the dirtied range to get lost.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/extent_io.c | 4 ++++
 fs/btrfs/file.c      | 5 +++--
 2 files changed, 7 insertions(+), 2 deletions(-)

fs/btrfs/extent_io.c +4 -0

index b3a4a94212c9..d7240e295bfc 100644
@@ -977,6 +977,10 @@ static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached,
                        end_folio_read(folio, true, cur, end - cur + 1);
                        break;
                }
+               if (btrfs_folio_test_uptodate(fs_info, folio, cur, blocksize)) {
+                       end_folio_read(folio, true, cur, blocksize);
+                       continue;
+               }
                em = get_extent_map(BTRFS_I(inode), folio, cur, end - cur + 1, em_cached);
                if (IS_ERR(em)) {
                        end_folio_read(folio, false, cur, end + 1 - cur);

fs/btrfs/file.c +3 -3

index 00c68b7b2206..e3d63192281d 100644
@@ -804,14 +804,15 @@ static int prepare_uptodate_folio(struct inode *inode, struct folio *folio, u64
 {
        u64 clamp_start = max_t(u64, pos, folio_pos(folio));
        u64 clamp_end = min_t(u64, pos + len, folio_pos(folio) + folio_size(folio));
+       const u32 sectorsize = inode_to_fs_info(inode)->sectorsize;
        int ret = 0;
 
        if (folio_test_uptodate(folio))
                return 0;
 
        if (!force_uptodate &&
-           IS_ALIGNED(clamp_start, PAGE_SIZE) &&
-           IS_ALIGNED(clamp_end, PAGE_SIZE))
+           IS_ALIGNED(clamp_start, sectorsize) &&
+           IS_ALIGNED(clamp_end, sectorsize))
                return 0;
 
        ret = btrfs_read_folio(NULL, folio);
--

2.48.1

Previously inline data extents creation is disable if the block size (previously called sector size) is smaller than the page size, for the following reasons:

- Possible mixed inline and regular data extents
However this is also the same if the block size matches the page size, thus we do not treat mixed inline and regular extents as an error.

And the chance to cause mixed inline and regular data extents are not even increased, it has the same requirement (compressed inline data extent covering the whole first block, followed by regular extents).

- Unable to handle async/inline delalloc range for block size < page size cases
This is already fixed since commit 1d2fbb7f1f9e ("btrfs: allow compression even if the range is not page aligned").

This was the major technical blockage, but it's no longer a blockage anymore.

With the major technical blockage already removed, we can enable inline data extents creation no matter the block size nor the page size, allowing the btrfs to have the same capacity for all block sizes.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/inode.c | 13 -------------
 1 file changed, 13 deletions(-)

fs/btrfs/inode.c +0 -14

index e99cb5109967..a1ea93bad80e 100644
@@ -566,19 +566,6 @@ static bool can_cow_file_range_inline(struct btrfs_inode *inode,
        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;
--

2.48.1

Since the initial enablement of block size < page size support for btrfs in v5.15, we have hit several milestones for block size < page size (subpage) support:

- RAID56 subpage support
In v5.19

- Refactored scrub support to support subpage better In v6.4

- Block perfect (previously requires page aligned ranges) compressed write In v6.13

- Various error handling fixes involving subpage
In v6.14

Finally the only missing feature is the pretty simple and harmless inlined data extent creation, just done in previous patches.

Now btrfs has all of its features ready for both regular and subpage cases, there is no reason to output a warning about the experimental subpage support, and we can finally remove it now.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/disk-io.c | 5 -----
 1 file changed, 5 deletions(-)

fs/btrfs/disk-io.c +0 -6

index a799216aa264..c0b40dedceb5 100644
@@ -3414,11 +3414,6 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
         */
        fs_info->max_inline = min_t(u64, fs_info->max_inline, fs_info->sectorsize);
 
-       if (sectorsize < PAGE_SIZE)
-               btrfs_warn(fs_info,
-               "read-write for sector size %u with page size %lu is experimental",
-                          sectorsize, PAGE_SIZE);
-
        ret = btrfs_init_workqueues(fs_info);
        if (ret)
                goto fail_sb_buffer;
--

2.48.1

Quoted 14 lines · Qu Wenruo

Currently we're using btrfs_lock_and_flush_ordered_range() for both btrfs_read_folio() and btrfs_readahead(), but it has one critical problem for future subpage enhancements:

- It will call btrfs_start_ordered_extent() to writeback the involved folios

But remember we're calling btrfs_lock_and_flush_ordered_range() at read paths, meaning the folio is already locked by read path.

If we really trigger writeback, this will lead to a deadlock and writeback can not hold the folio lock.

Quoted 7 lines

Such dead lock is prevented by the fact that btrfs always keeps a dirty folio also uptodate, by either dirtying all blocks of the folio, or read the folio before dirtying.

But it's not the common behavior, as XFS/EXT4 both allow the folio to be dirty but not uptodate, by allowing buffered write to dirty a full block without reading the full folio.

Ok, but what happens in other filesystems doesn't affect us.
Or did you forget to mention that some other subsequent patch or planned changes will introduce that behaviour?

If so, it would be best to say that, otherwise that paragraph isn't relevant.

Quoted 8 lines

Instead of blindly calling btrfs_start_ordered_extent(), introduce a newer helper, which is smarter in the following ways:

- Only wait and flush the ordered extent if
* The folio doesn't even have private set
* Part of the blocks of the ordered extent is not uptodate

is -> are

Quoted 4 lines

This can happen by:
* Folio writeback finished, then get invalidated. But OE not yet finished

Ok, this one I understand.

Quoted 1 line

* Direct IO

This one I don't because direct IO doesn't use the page cache. Can you elaborate here and explain why it can happen with direct IO?

Quoted 9 lines

We have to wait for the ordered extent, as it may contain to-be-inserted data checksum.
Without waiting, our read can fail due to the missing csum.

But either way, the OE should not need any extra flush inside the locked folio range.

Quoted 3 lines

- Skip the ordered extent completely if
* All the blocks are dirty
This happens when OE creation is caused by previous folio.

"OE creation is caused by previous folio" - what does this mean? You mean by a write that happened before the read?

Quoted 38 lines

The writeback will never happen (we're holding the folio lock for read), nor will the OE finish.

Thus we must skip the range.

* All the blocks are uptodate
This happens when the writeback finished, but OE not yet finished.

Since the blocks are already uptodate, we can skip the OE range.

The newer helper, lock_extents_for_read() will do a loop for the target range by:

1) Lock the full range

2) If there is no ordered extent in the remaining range, exit

3) If there is an ordered extent that we can skip Skip to the end of the OE, and continue checking We do not trigger writeback nor wait for the OE.

4) If there is an ordered extent that we can not skip Unlock the whole extent range and start the ordered extent.

And also update btrfs_start_ordered_extent() to add two more parameters:
@nowriteback_start and @nowriteback_len, to prevent triggering flush for a certain range.

This will allow us to handle the following case properly in the future:

16K page size, 4K btrfs block size:

  16K           20K             24K              28K            32K
  |/////////////////////////////|                |              |
  |<----------- OE 2----------->|                |<--- OE 1 --->|

Quoted 5 lines

The folio has been written back before, thus we have an OE at [28K, 32K).
Although the OE 1 finished its IO, the OE is not yet removed from IO tree.
Later the folio got invalidated, but OE still exists.

This last sentence to be less confusing: "The folio got invalited after writeback completed and before the ordered extent finished."

Quoted 5 lines

And [16K, 24K) range is dirty and uptodate, caused by a block aligned buffered write (and future enhancements allowing btrfs to skip full folio read for such case).

Quoted 2 lines

Furthermore, OE 2 is created covering range [16K, 24K) by the writeback of previous folio.

What previous folio? There's no other one in the example. Do you mean there's a folio for range [0, 16K) and running delalloc resulted in the creation of an OE for the range [0, 24K)? So that OE 2 doesn't really start at 16K but at 0 instead?

Normally the parameter list/description comes before describing the return values.

Quoted 2 lines

+ */
+static bool can_skip_one_ordered_range(struct btrfs_inode *binode,

Why binode and not just inode? There's no vfs inode in the function to make any confusion.

Quoted 20 lines

fs/btrfs/extent_io.c +18 -0

+                                      struct btrfs_ordered_extent *ordered,
+                                      u64 cur, u64 *next_ret)
+{
+       const struct btrfs_fs_info *fs_info = binode->root->fs_info;
+       struct folio *folio;
+       const u32 blocksize = fs_info->sectorsize;
+       u64 range_len;
+       bool ret;
+
+       folio = filemap_get_folio(binode->vfs_inode.i_mapping,
+                                 cur >> PAGE_SHIFT);
+
+       /*
+        * We should have locked the folio(s) for range [start, end], thus
+        * there must be a folio and it must be locked.
+        */
+       ASSERT(!IS_ERR(folio));
+       ASSERT(folio_test_locked(folio));

fs/btrfs/extent_io.c static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached, · line 1121

Quoted 5 lines

fs/btrfs/extent_io.c +5 -0

+
+       /*
+        * We several cases for the folio and OE combination:
+        *
+        * 0) Folio has no private flag

Why does the numbering start from 0? It's not code and it's not related to array indexes or offsets :)

fs/btrfs/extent_io.c static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached, · line 1123

Quoted 3 lines

fs/btrfs/extent_io.c +2 -0

+        *    The OE has all its IO done but not yet finished, and folio got
+        *    invalidated. Or direct IO.

Ok so I still don't understand the direct IO case, because direct IO doesn't use the page cache.
Just like the change log, some explanation here would be desired.

fs/btrfs/extent_io.c static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached, · line 1128

Quoted 5 lines

fs/btrfs/extent_io.c +5 -0

+        *
+        * Have to wait for the OE to finish, as it may contain the
+        * to-be-inserted data checksum.
+        * Without the data checksum inserted into csum tree, read
+        * will just fail with missing csum.

Well we don't need to wait if it's a NOCOW / NODATASUM write.

Quoted 25 lines · Qu Wenruo

[BUG]
Since the support of block size (sector size) < page size for btrfs, test case generic/563 fails with 4K block size and 64K page size:

     --- tests/generic/563.out   2024-04-25 18:13:45.178550333 +0930
     +++ /home/adam/xfstests-dev/results//generic/563.out.bad    2024-09-30 09:09:16.155312379 +0930
     @@ -3,7 +3,8 @@
      read is in range
      write is in range
      write -> read/write
     -read is in range
     +read has value of 8388608
     +read is NOT in range -33792 .. 33792
      write is in range
     ...

[CAUSE]
The test case creates a 8MiB file, then buffered write into the 8MiB using 4K block size, to overwrite the whole file.

On 4K page sized systems, since the write range covers the full block and page, btrfs will no bother reading the page, just like what XFS and EXT4 do.

Quoted 3 lines

But 64K page sized systems, although the 4K sized write is still block aligned, it's not page aligned any more, thus btrfs will read the full page, causing more read than expected and fail the test case.

This part "causing more read than expected" got me puzzled, but it's explained in the "Fix" section below. It's more like "causing previously dirty blocks within the page to be zeroed out".

Quoted 23 lines

[FIX]
To skip the full page read, we need to do the following modification:

- Do not trigger full page read as long as the buffered write is block aligned
This is pretty simple by modifying the check inside prepare_uptodate_page().

- Skip already uptodate blocks during full page read Or we can lead to the following data corruption:

   0       32K        64K
   |///////|          |

Where the file range [0, 32K) is dirtied by buffered write, the remaining range [32K, 64K) is not.

When reading the full page, since [0,32K) is only dirtied but not written back, there is no data extent map for it, but a hole covering [0, 64k).

Quoted 6 lines

If we continue reading the full page range [0, 64K), the dirtied range will be filled with 0 (since there is only a hole covering the whole range).
This causes the dirtied range to get lost.

Signed-off-by Qu Wenruo <[email protected]>

Reviewed-by Filipe Manana <[email protected]>

Looks good, thanks.

Quoted 46 lines

---
  fs/btrfs/extent_io.c | 4 ++++
  fs/btrfs/file.c      | 5 +++--
  2 files changed, 7 insertions(+), 2 deletions(-)

fs/btrfs/extent_io.c +4 -0

index b3a4a94212c9..d7240e295bfc 100644
@@ -977,6 +977,10 @@ static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached,
                         end_folio_read(folio, true, cur, end - cur + 1);
                         break;
                 }
+               if (btrfs_folio_test_uptodate(fs_info, folio, cur, blocksize)) {
+                       end_folio_read(folio, true, cur, blocksize);
+                       continue;
+               }
                 em = get_extent_map(BTRFS_I(inode), folio, cur, end - cur + 1, em_cached);
                 if (IS_ERR(em)) {
                         end_folio_read(folio, false, cur, end + 1 - cur);

fs/btrfs/file.c +3 -3

index 00c68b7b2206..e3d63192281d 100644
@@ -804,14 +804,15 @@ static int prepare_uptodate_folio(struct inode *inode, struct folio *folio, u64
  {
         u64 clamp_start = max_t(u64, pos, folio_pos(folio));
         u64 clamp_end = min_t(u64, pos + len, folio_pos(folio) + folio_size(folio));
+       const u32 sectorsize = inode_to_fs_info(inode)->sectorsize;
         int ret = 0;
 
         if (folio_test_uptodate(folio))
                 return 0;
 
         if (!force_uptodate &&
-           IS_ALIGNED(clamp_start, PAGE_SIZE) &&
-           IS_ALIGNED(clamp_end, PAGE_SIZE))
+           IS_ALIGNED(clamp_start, sectorsize) &&
+           IS_ALIGNED(clamp_end, sectorsize))
                 return 0;
 
         ret = btrfs_read_folio(NULL, folio);
--

2.48.1

Quoted 24 lines · Qu Wenruo

Since the initial enablement of block size < page size support for btrfs in v5.15, we have hit several milestones for block size < page size (subpage) support:

- RAID56 subpage support
In v5.19

- Refactored scrub support to support subpage better In v6.4

- Block perfect (previously requires page aligned ranges) compressed write In v6.13

- Various error handling fixes involving subpage
In v6.14

Finally the only missing feature is the pretty simple and harmless inlined data extent creation, just done in previous patches.

Now btrfs has all of its features ready for both regular and subpage cases, there is no reason to output a warning about the experimental subpage support, and we can finally remove it now.

Quoted 1 line

Signed-off-by Qu Wenruo <[email protected]>

I don't have a machine to test block size < page size, but I trust you did all the testing besides being the expert on the code base for the feature, so:

Acked-by Filipe Manana <[email protected]>

Thanks.

Quoted 24 lines

---
  fs/btrfs/disk-io.c | 5 -----
  1 file changed, 5 deletions(-)

fs/btrfs/disk-io.c +0 -6

index a799216aa264..c0b40dedceb5 100644
@@ -3414,11 +3414,6 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
          */
         fs_info->max_inline = min_t(u64, fs_info->max_inline, fs_info->sectorsize);
 
-       if (sectorsize < PAGE_SIZE)
-               btrfs_warn(fs_info,
-               "read-write for sector size %u with page size %lu is experimental",
-                          sectorsize, PAGE_SIZE);
-
         ret = btrfs_init_workqueues(fs_info);
         if (ret)
                 goto fail_sb_buffer;
--

2.48.1

Quoted 7 lines

Quoted 5 lines

+        *
+        * Have to wait for the OE to finish, as it may contain the +        * to-be-inserted data checksum.
+        * Without the data checksum inserted into csum tree, read +        * will just fail with missing csum.

Well we don't need to wait if it's a NOCOW / NODATASUM write.

That is also a valid optimization, I can enhance the check to skip NODATASUM OEs.

Thanks a lot for reviewing the core mechanism of the patchset.

Thanks,
Qu

Quoted 23 lines · Filipe Manana

Quoted 14 lines · Qu Wenruo

Currently we're using btrfs_lock_and_flush_ordered_range() for both btrfs_read_folio() and btrfs_readahead(), but it has one critical problem for future subpage enhancements:

- It will call btrfs_start_ordered_extent() to writeback the involved folios

But remember we're calling btrfs_lock_and_flush_ordered_range() at read paths, meaning the folio is already locked by read path.

If we really trigger writeback, this will lead to a deadlock and writeback can not hold the folio lock.

Quoted 7 lines

Such dead lock is prevented by the fact that btrfs always keeps a dirty folio also uptodate, by either dirtying all blocks of the folio, or read the folio before dirtying.

But it's not the common behavior, as XFS/EXT4 both allow the folio to be dirty but not uptodate, by allowing buffered write to dirty a full block without reading the full folio.

Quoted 5 lines

Ok, but what happens in other filesystems doesn't affect us.
Or did you forget to mention that some other subsequent patch or planned changes will introduce that behaviour?

If so, it would be best to say that, otherwise that paragraph isn't relevant.

Got it, will add the mention of the incoming feature of partial uptodate folio support.

But still, all the work done here is mostly to pass generic/563, which I have to say the EXT4/XFS behavior is still a pretty good optimizatioin.

I'll change the paraph to focus on the incoming optimization.

Quoted 18 lines

Quoted 8 lines

Instead of blindly calling btrfs_start_ordered_extent(), introduce a newer helper, which is smarter in the following ways:

- Only wait and flush the ordered extent if
* The folio doesn't even have private set
* Part of the blocks of the ordered extent is not uptodate

is -> are

Quoted 4 lines

This can happen by:
* Folio writeback finished, then get invalidated. But OE not yet finished

Ok, this one I understand.

Quoted 4 lines

Quoted 1 line

* Direct IO

This one I don't because direct IO doesn't use the page cache. Can you elaborate here and explain why it can happen with direct IO?

Direct IO always drops the cache first, so the folio in the direct IO range should have no private attached just like it was invalidated.

And direct IO doesn't wait for the OE to finish, we will have the same case just like the previous case.

Quoted 9 lines

Quoted 9 lines

We have to wait for the ordered extent, as it may contain to-be-inserted data checksum.
Without waiting, our read can fail due to the missing csum.

But either way, the OE should not need any extra flush inside the locked folio range.

Quoted 6 lines

Quoted 3 lines

- Skip the ordered extent completely if
* All the blocks are dirty
This happens when OE creation is caused by previous folio.

"OE creation is caused by previous folio" - what does this mean? You mean by a write that happened before the read?

I mean the following case, 16K page size 4K block size:

    0      8K     16K    24K     32K
    |/////////////|//////|       |

The writeback for folio 0 started, which ran delalloc range for [0, 24K), thus we have an OE at [0, 24K).

But at the same time, since range [24K, 32K) is not uptodate, a read can be triggered on folio 16K.

Quoted 52 lines

Quoted 38 lines

The writeback will never happen (we're holding the folio lock for read), nor will the OE finish.

Thus we must skip the range.

* All the blocks are uptodate
This happens when the writeback finished, but OE not yet finished.

Since the blocks are already uptodate, we can skip the OE range.

The newer helper, lock_extents_for_read() will do a loop for the target range by:

1) Lock the full range

2) If there is no ordered extent in the remaining range, exit

3) If there is an ordered extent that we can skip Skip to the end of the OE, and continue checking We do not trigger writeback nor wait for the OE.

4) If there is an ordered extent that we can not skip Unlock the whole extent range and start the ordered extent.

And also update btrfs_start_ordered_extent() to add two more parameters:
@nowriteback_start and @nowriteback_len, to prevent triggering flush for a certain range.

This will allow us to handle the following case properly in the future:

16K page size, 4K btrfs block size:

  16K           20K             24K              28K            32K
  |/////////////////////////////|                |              |
  |<----------- OE 2----------->|                |<--- OE 1 --->|

Quoted 5 lines

The folio has been written back before, thus we have an OE at [28K, 32K).
Although the OE 1 finished its IO, the OE is not yet removed from IO tree.
Later the folio got invalidated, but OE still exists.

This last sentence to be less confusing: "The folio got invalited after writeback completed and before the ordered extent finished."

Quoted 5 lines

And [16K, 24K) range is dirty and uptodate, caused by a block aligned buffered write (and future enhancements allowing btrfs to skip full folio read for such case).

Quoted 7 lines

Quoted 2 lines

Furthermore, OE 2 is created covering range [16K, 24K) by the writeback of previous folio.

What previous folio? There's no other one in the example. Do you mean there's a folio for range [0, 16K) and running delalloc resulted in the creation of an OE for the range [0, 24K)? So that OE 2 doesn't really start at 16K but at 0 instead?

Yep, exactly.

[...]

Quoted 8 lines

Normally the parameter list/description comes before describing the return values.

Quoted 2 lines

+ */
+static bool can_skip_one_ordered_range(struct btrfs_inode *binode,

Why binode and not just inode? There's no vfs inode in the function to make any confusion.

OK, will go the regular inode naming.

Quoted 20 lines

Quoted 20 lines

fs/btrfs/extent_io.c +18 -0

+                                      struct btrfs_ordered_extent *ordered,
+                                      u64 cur, u64 *next_ret)
+{
+       const struct btrfs_fs_info *fs_info = binode->root->fs_info;
+       struct folio *folio;
+       const u32 blocksize = fs_info->sectorsize;
+       u64 range_len;
+       bool ret;
+
+       folio = filemap_get_folio(binode->vfs_inode.i_mapping,
+                                 cur >> PAGE_SHIFT);
+
+       /*
+        * We should have locked the folio(s) for range [start, end], thus
+        * there must be a folio and it must be locked.
+        */
+       ASSERT(!IS_ERR(folio));
+       ASSERT(folio_test_locked(folio));

Quoted 8 lines

fs/btrfs/extent_io.c static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached, · line 1121

Quoted 5 lines

fs/btrfs/extent_io.c +5 -0

+
+       /*
+        * We several cases for the folio and OE combination:
+        *
+        * 0) Folio has no private flag

Why does the numbering start from 0? It's not code and it's not related to array indexes or offsets :)

Because I forgot the folio to-be-read may not even be managed by btrfs, and hit ASSERT()s related to that.
Thus I think the private flag should be an important prerequisite, and give it 0.

But you're right, in fact it's very common and should not be treated specially.

Quoted 8 lines

fs/btrfs/extent_io.c static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached, · line 1123

Quoted 3 lines

fs/btrfs/extent_io.c +2 -0

+        *    The OE has all its IO done but not yet finished, and folio got
+        *    invalidated. Or direct IO.

Ok so I still don't understand the direct IO case, because direct IO doesn't use the page cache.
Just like the change log, some explanation here would be desired.

Quoted 7 lines

fs/btrfs/extent_io.c static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached, · line 1128

Quoted 5 lines

fs/btrfs/extent_io.c +5 -0

+        *
+        * Have to wait for the OE to finish, as it may contain the
+        * to-be-inserted data checksum.
+        * Without the data checksum inserted into csum tree, read
+        * will just fail with missing csum.

Well we don't need to wait if it's a NOCOW / NODATASUM write.

That is also a valid optimization, I can enhance the check to skip NODATASUM OEs.

Thanks a lot for reviewing the core mechanism of the patchset.

Thanks,
Qu

Quoted 1 line · Qu Wenruo

[...]

Quoted 8 lines

Quoted 7 lines

fs/btrfs/extent_io.c static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached, · line 1128

Quoted 5 lines

fs/btrfs/extent_io.c +5 -0

+        *
+        * Have to wait for the OE to finish, as it may contain the
+        * to-be-inserted data checksum.
+        * Without the data checksum inserted into csum tree, read
+        * will just fail with missing csum.

Well we don't need to wait if it's a NOCOW / NODATASUM write.

Quoted 2 lines

That is also a valid optimization, I can enhance the check to skip NODATASUM OEs.

BTW, the enhancement will not be in this series.

As it will introduce extra skip cases, which may further makes things more complex.

I'll make it a dedicated patch in the future.
Currently there are quite some patches tested based on the behavior without NODATASUM skip.

Thanks,
Qu

Quoted 7 lines

Thanks a lot for reviewing the core mechanism of the patchset.

Thanks,
Qu