Re: [PATCH v2 5/9] btrfs: do proper folio cleanup when run_delalloc_nocow() failed

在 2025/1/10 09:56, Boris Burkov 写道: [...]

I like this function. Can you add a simple doc with pre and post conditions please?

Sure, no problem.

Would be something like this:

/* * To cleanup dirty folios when failed to run a delalloc range. * * When running a delalloc range, we may need to split into * different extents (fragmentation or NOCOW limits), and if * we hit error, previous successfully executed ranges also need * to have their dirty flags cleared, with the address space marked * as error. */

...

+static void cleanup_dirty_folios(struct btrfs_inode *inode,

• 		 struct folio *locked_folio,
  

• 		 u64 start, u64 end, int error)
  

+{

• struct btrfs_fs_info *fs_info = inode->root->fs_info;
• struct address_space *mapping = inode->vfs_inode.i_mapping;
• pgoff_t start_index = start >> PAGE_SHIFT;
• pgoff_t end_index = end >> PAGE_SHIFT;
• u32 len;
• ASSERT(end + 1 - start < U32_MAX);
• ASSERT(IS_ALIGNED(start, fs_info->sectorsize) &&

•       IS_ALIGNED(end + 1, fs_info->sectorsize));
  

• len = end + 1 - start;
• /*

• * Handle the locked folio first.
  

• * btrfs_folio_clamp_*() helpers can handle range out of the folio case.
  

• */
  

• btrfs_folio_clamp_clear_dirty(fs_info, locked_folio, start, len);
• btrfs_folio_clamp_set_writeback(fs_info, locked_folio, start, len);
• btrfs_folio_clamp_clear_writeback(fs_info, locked_folio, start, len);

Could this clear dirty; set writeback; clear writeback sequence benefit from a good name and a helper function too?

Sure, what about btrfs_folio_clamp_finish_io()?

...

• for (pgoff_t index = start_index; index <= end_index; index++) {

• struct folio *folio;
  

• /* Already handled at the beginning. */
  

• if (index == locked_folio->index)
  

• 	continue;
  

• folio = __filemap_get_folio(mapping, index, FGP_LOCK, GFP_NOFS);
  

• /* Cache already dropped, no need to do any cleanup. */
  

• if (IS_ERR(folio))
  

• 	continue;
  

• btrfs_folio_clamp_clear_dirty(fs_info, folio, start, len);
  

• btrfs_folio_clamp_set_writeback(fs_info, folio, start, len);
  

• btrfs_folio_clamp_clear_writeback(fs_info, folio, start, len);
  

• folio_unlock(folio);
  

• folio_put(folio);
  

• }
• mapping_set_error(mapping, error);

+}

• /*
  • when nowcow writeback call back. This checks for snapshots or COW copies
  • of the extents that exist in the file, and COWs the file as required.

@@ -1976,6 +2018,11 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode, struct btrfs_root *root = inode->root; struct btrfs_path *path; u64 cow_start = (u64)-1;

• /*

• * If not 0, represents the inclusive end of the last fallback_to_cow()
  

• * range. Only for error handling.
  

• */
  

• u64 cow_end = 0; u64 cur_offset = start; int ret; bool check_prev = true;

@@ -2136,6 +2183,7 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode, found_key.offset - 1); cow_start = (u64)-1; if (ret) {

• 		cow_end = found_key.offset - 1;
  	btrfs_dec_nocow_writers(nocow_bg);
  	goto error;
  }
  

@@ -2209,11 +2257,12 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode, cow_start = cur_offset;

if (cow_start != (u64)-1) {

• cur_offset = end;
  

  ret = fallback_to_cow(inode, locked_folio, cow_start, end); cow_start = (u64)-1;

• if (ret)
  

• if (ret) {
  

• 	cow_end = end;
  goto error;
  

• }
  

  }

  btrfs_free_path(path);

@@ -2221,12 +2270,42 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,

error: /*

• * If an error happened while a COW region is outstanding, cur_offset
  

• * needs to be reset to cow_start to ensure the COW region is unlocked
  

• * as well.
  

• * There are several error cases:
  

• *
  

• * 1) Failed without falling back to COW
  

• *    start         cur_start              end
  

• *    |/////////////|                      |
  

• *
  

• *    For range [start, cur_start) the folios are already unlocked (except
  

• *    @locked_folio), EXTENT_DELALLOC already removed.
  

• *    Only need to clear the dirty flag as they will never be submitted.
  

• *    Ordered extent and extent maps are handled by
  

• *    btrfs_mark_ordered_io_finished() inside run_delalloc_range().
  

• *
  

• * 2) Failed with error from fallback_to_cow()
  

• *    start         cur_start   cow_end    end
  

• *    |/////////////|-----------|          |
  

• *
  

• *    For range [start, cur_start) it's the same as case 1).
  

• *    But for range [cur_start, cow_end), the folios have dirty flag
  

• *    cleared and unlocked, EXTENT_DEALLLOC cleared.
  

• *    There may or may not be any ordered extents/extent maps allocated.
  

• *
  

• *    We should not call extent_clear_unlock_delalloc() on range [cur_start,
  

• *    cow_end), as the folios are already unlocked.
  

• *
  

I think it would be helpful to include cur_offset in your drawings.

I noticed this when crafting a new patch too, there is no @cur_start at all, but only @cur_offset.

Will fix it in the next update.

Thanks again for the detailed review, Qu

...

• * So clear the folio dirty flags for [start, cur_offset) first.
  

  */

• if (cow_start != (u64)-1)

• cur_offset = cow_start;
  

• if (cur_offset > start)

• cleanup_dirty_folios(inode, locked_folio, start, cur_offset - 1, ret);
  

• /*

• * If an error happened while a COW region is outstanding, cur_offset
  

• * needs to be reset to @cow_end + 1 to skip the COW range, as
  

• * cow_file_range() will do the proper cleanup at error.
  

• */
  

• if (cow_end)

• cur_offset = cow_end + 1;
  
  

  /*

  • We need to lock the extent here because we're clearing DELALLOC and

-- 2.47.1