mbox series

[v3,0/7] extent buffer dirty cleanups

Message ID cover.1674766637.git.josef@toxicpanda.com (mailing list archive)
Headers show
Series extent buffer dirty cleanups | expand

Message

Josef Bacik Jan. 26, 2023, 9 p.m. UTC
v2->v3:
- Reworked the logic around clearing dirty on extent buffers not in our
  transaction.  We do need this for update_ref_for_cow in some cases that I
  didn't account for.  I've expanded the logic for this make it more idiot
  proof, and adjusted all of the patches accordingly.

v1->v2:
- Added "btrfs: do not call btrfs_clean_tree_block in update_ref_for_cow", Qu
  noticed some corruption with the original patchset, this turned out to be
  because we were clearing the extent buffer dirty at cow time, which doesn't
  make sense as we're not free'ing the current block in our current transaction.

--- Original email ---
Hello,

While sync'ing ctree.c to btrfs-progs I noticed we have some oddities when it
comes to how we deal with the extent buffer being dirty.  We have
btrfs_clean_tree_block, which is sort of meant to be run against extent buffers
we've modified in this transaction.  However we have some other places where
we've open coded the same work without the generation check.  This makes it kind
of confusing, and is inconsistent with how we deal with the
fs_info->dirty_metadata_bytes.

So clean this stuff up so we have one helper we use for setting the extent
buffer dirty (btrfs_mark_buffer_dirty) and one for clearing dirty
(btrfs_clear_buffer_dirty).  This makes everything more consistent and clean
across the board.  I've additionally cleaned up a random writeback thing we had
in tree-log that I noticed while doing these cleanups.  Thanks,

Josef

Josef Bacik (7):
  btrfs: always lock the block before calling btrfs_clean_tree_block
  btrfs: add trans argument to btrfs_clean_tree_block
  btrfs: replace clearing extent buffer dirty bit with btrfs_clean_block
  btrfs: do not increment dirty_metadata_bytes in set_btree_ioerr
  btrfs: rename btrfs_clean_tree_block => btrfs_clear_buffer_dirty
  btrfs: combine btrfs_clear_buffer_dirty and clear_extent_buffer_dirty
  btrfs: remove btrfs_wait_tree_block_writeback

 fs/btrfs/ctree.c           | 31 +++++++++++++++--------------
 fs/btrfs/disk-io.c         | 25 +++++-------------------
 fs/btrfs/disk-io.h         |  3 ++-
 fs/btrfs/extent-tree.c     |  9 ++++-----
 fs/btrfs/extent_io.c       | 23 ++++++++++++++--------
 fs/btrfs/extent_io.h       |  5 ++++-
 fs/btrfs/free-space-tree.c |  2 +-
 fs/btrfs/ioctl.c           |  2 +-
 fs/btrfs/qgroup.c          |  2 +-
 fs/btrfs/tree-log.c        | 40 ++++++++++++++------------------------
 10 files changed, 64 insertions(+), 78 deletions(-)

Comments

Wang Yugui Jan. 27, 2023, 11:27 a.m. UTC | #1
Hi,

> v2->v3:
> - Reworked the logic around clearing dirty on extent buffers not in our
>   transaction.  We do need this for update_ref_for_cow in some cases that I
>   didn't account for.  I've expanded the logic for this make it more idiot
>   proof, and adjusted all of the patches accordingly.

The issue reported for v2 have been fixed. Thanks a lot.

no regression have been found for these v3 patches here.

Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2023/01/27


> 
> v1->v2:
> - Added "btrfs: do not call btrfs_clean_tree_block in update_ref_for_cow", Qu
>   noticed some corruption with the original patchset, this turned out to be
>   because we were clearing the extent buffer dirty at cow time, which doesn't
>   make sense as we're not free'ing the current block in our current transaction.
> 
> --- Original email ---
> Hello,
> 
> While sync'ing ctree.c to btrfs-progs I noticed we have some oddities when it
> comes to how we deal with the extent buffer being dirty.  We have
> btrfs_clean_tree_block, which is sort of meant to be run against extent buffers
> we've modified in this transaction.  However we have some other places where
> we've open coded the same work without the generation check.  This makes it kind
> of confusing, and is inconsistent with how we deal with the
> fs_info->dirty_metadata_bytes.
> 
> So clean this stuff up so we have one helper we use for setting the extent
> buffer dirty (btrfs_mark_buffer_dirty) and one for clearing dirty
> (btrfs_clear_buffer_dirty).  This makes everything more consistent and clean
> across the board.  I've additionally cleaned up a random writeback thing we had
> in tree-log that I noticed while doing these cleanups.  Thanks,
> 
> Josef
> 
> Josef Bacik (7):
>   btrfs: always lock the block before calling btrfs_clean_tree_block
>   btrfs: add trans argument to btrfs_clean_tree_block
>   btrfs: replace clearing extent buffer dirty bit with btrfs_clean_block
>   btrfs: do not increment dirty_metadata_bytes in set_btree_ioerr
>   btrfs: rename btrfs_clean_tree_block => btrfs_clear_buffer_dirty
>   btrfs: combine btrfs_clear_buffer_dirty and clear_extent_buffer_dirty
>   btrfs: remove btrfs_wait_tree_block_writeback
> 
>  fs/btrfs/ctree.c           | 31 +++++++++++++++--------------
>  fs/btrfs/disk-io.c         | 25 +++++-------------------
>  fs/btrfs/disk-io.h         |  3 ++-
>  fs/btrfs/extent-tree.c     |  9 ++++-----
>  fs/btrfs/extent_io.c       | 23 ++++++++++++++--------
>  fs/btrfs/extent_io.h       |  5 ++++-
>  fs/btrfs/free-space-tree.c |  2 +-
>  fs/btrfs/ioctl.c           |  2 +-
>  fs/btrfs/qgroup.c          |  2 +-
>  fs/btrfs/tree-log.c        | 40 ++++++++++++++------------------------
>  10 files changed, 64 insertions(+), 78 deletions(-)
> 
> -- 
> 2.26.3
David Sterba Feb. 6, 2023, 5:12 p.m. UTC | #2
On Thu, Jan 26, 2023 at 04:00:53PM -0500, Josef Bacik wrote:
> v2->v3:
> - Reworked the logic around clearing dirty on extent buffers not in our
>   transaction.  We do need this for update_ref_for_cow in some cases that I
>   didn't account for.  I've expanded the logic for this make it more idiot
>   proof, and adjusted all of the patches accordingly.

Added to misc-next, thanks.