From: Josef Bacik <josef@toxicpanda.com>
To: Goldwyn Rodrigues <rgoldwyn@suse.de>
Cc: linux-btrfs@vger.kernel.org, Goldwyn Rodrigues <rgoldwyn@suse.com>
Subject: Re: [PATCH 06/16] btrfs: Lock extents before pages in writepages
Date: Tue, 13 Dec 2022 13:39:17 -0500 [thread overview]
Message-ID: <Y5jG1Rdy+6QPjy0c@localhost.localdomain> (raw)
In-Reply-To: <28cb7dbc0216d2a5f55efd296113f9f9576dda41.1668530684.git.rgoldwyn@suse.com>
On Tue, Nov 15, 2022 at 12:00:24PM -0600, Goldwyn Rodrigues wrote:
> writepages() locks the extents in find_lock_delalloc_range() and unlocks
> using clear_bit EXTENT_LOCKED operations is cow/delalloc operations.
>
> Call extent locking/unlocking around writepages() sequence as opposed to
> while performing delayed allocation.
>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
> fs/btrfs/extent_io.c | 5 -----
> fs/btrfs/inode.c | 43 +++++++++++++++++++++++++++++++------------
> 2 files changed, 31 insertions(+), 17 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 92068e4ff9c3..42bae149f923 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -464,15 +464,10 @@ noinline_for_stack bool find_lock_delalloc_range(struct inode *inode,
> }
> }
>
> - /* step three, lock the state bits for the whole range */
> - lock_extent(tree, delalloc_start, delalloc_end, &cached_state);
> -
> /* then test to make sure it is all still delalloc */
> ret = test_range_bit(tree, delalloc_start, delalloc_end,
> EXTENT_DELALLOC, 1, 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();
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 4bfa51871ddc..92726831dd5d 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -992,7 +992,6 @@ static int submit_one_async_extent(struct btrfs_inode *inode,
> struct async_extent *async_extent,
> u64 *alloc_hint)
> {
> - struct extent_io_tree *io_tree = &inode->io_tree;
> struct btrfs_root *root = inode->root;
> struct btrfs_fs_info *fs_info = root->fs_info;
> struct btrfs_key ins;
> @@ -1013,7 +1012,6 @@ static int submit_one_async_extent(struct btrfs_inode *inode,
> if (!(start >= locked_page_end || end <= locked_page_start))
> locked_page = async_chunk->locked_page;
> }
> - lock_extent(io_tree, start, end, NULL);
>
> /* We have fall back to uncompressed write */
> if (!async_extent->pages)
> @@ -1067,7 +1065,7 @@ static int submit_one_async_extent(struct btrfs_inode *inode,
>
> /* Clear dirty, set writeback and unlock the pages. */
> extent_clear_unlock_delalloc(inode, start, end,
> - NULL, EXTENT_LOCKED | EXTENT_DELALLOC,
> + NULL, EXTENT_DELALLOC,
> PAGE_UNLOCK | PAGE_START_WRITEBACK);
> if (btrfs_submit_compressed_write(inode, start, /* file_offset */
> async_extent->ram_size, /* num_bytes */
> @@ -1095,7 +1093,7 @@ static int submit_one_async_extent(struct btrfs_inode *inode,
> btrfs_free_reserved_extent(fs_info, ins.objectid, ins.offset, 1);
> out_free:
> extent_clear_unlock_delalloc(inode, start, end,
> - NULL, EXTENT_LOCKED | EXTENT_DELALLOC |
> + NULL, EXTENT_DELALLOC |
> EXTENT_DELALLOC_NEW |
> EXTENT_DEFRAG | EXTENT_DO_ACCOUNTING,
> PAGE_UNLOCK | PAGE_START_WRITEBACK |
> @@ -1263,7 +1261,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> */
> extent_clear_unlock_delalloc(inode, start, end,
> locked_page,
> - EXTENT_LOCKED | EXTENT_DELALLOC |
> + EXTENT_DELALLOC |
> EXTENT_DELALLOC_NEW | EXTENT_DEFRAG |
> EXTENT_DO_ACCOUNTING, PAGE_UNLOCK |
> PAGE_START_WRITEBACK | PAGE_END_WRITEBACK);
> @@ -1374,7 +1372,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
>
> extent_clear_unlock_delalloc(inode, start, start + ram_size - 1,
> locked_page,
> - EXTENT_LOCKED | EXTENT_DELALLOC,
> + EXTENT_DELALLOC,
> page_ops);
> if (num_bytes < cur_alloc_size)
> num_bytes = 0;
> @@ -1425,7 +1423,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> * We process each region below.
> */
>
> - clear_bits = EXTENT_LOCKED | EXTENT_DELALLOC | EXTENT_DELALLOC_NEW |
> + clear_bits = EXTENT_DELALLOC | EXTENT_DELALLOC_NEW |
> EXTENT_DEFRAG | EXTENT_CLEAR_META_RESV;
> page_ops = PAGE_UNLOCK | PAGE_START_WRITEBACK | PAGE_END_WRITEBACK;
>
> @@ -1575,7 +1573,7 @@ static int cow_file_range_async(struct btrfs_inode *inode,
> memalloc_nofs_restore(nofs_flag);
>
> if (!ctx) {
> - unsigned clear_bits = EXTENT_LOCKED | EXTENT_DELALLOC |
> + unsigned clear_bits = EXTENT_DELALLOC |
> EXTENT_DELALLOC_NEW | EXTENT_DEFRAG |
> EXTENT_DO_ACCOUNTING;
> unsigned long page_ops = PAGE_UNLOCK | PAGE_START_WRITEBACK |
> @@ -1955,7 +1953,7 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
> path = btrfs_alloc_path();
> if (!path) {
> extent_clear_unlock_delalloc(inode, start, end, locked_page,
> - EXTENT_LOCKED | EXTENT_DELALLOC |
> + EXTENT_DELALLOC |
> EXTENT_DO_ACCOUNTING |
> EXTENT_DEFRAG, PAGE_UNLOCK |
> PAGE_START_WRITEBACK |
> @@ -2169,7 +2167,7 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
> nocow_args.num_bytes);
>
> extent_clear_unlock_delalloc(inode, cur_offset, nocow_end,
> - locked_page, EXTENT_LOCKED |
> + locked_page,
> EXTENT_DELALLOC |
> EXTENT_CLEAR_DATA_RESV,
> PAGE_UNLOCK | PAGE_SET_ORDERED);
> @@ -2205,7 +2203,7 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
>
> if (ret && cur_offset < end)
> extent_clear_unlock_delalloc(inode, cur_offset, end,
> - locked_page, EXTENT_LOCKED |
> + locked_page,
> EXTENT_DELALLOC | EXTENT_DEFRAG |
> EXTENT_DO_ACCOUNTING, PAGE_UNLOCK |
> PAGE_START_WRITEBACK |
> @@ -8223,7 +8221,28 @@ static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> static int btrfs_writepages(struct address_space *mapping,
> struct writeback_control *wbc)
> {
> - return extent_writepages(mapping, wbc);
> + u64 start, end;
> + struct inode *inode = mapping->host;
> + struct extent_state *cached = NULL;
> + int ret;
> + u64 isize = round_up(i_size_read(inode), PAGE_SIZE) - 1;
> +
> + if (wbc->range_cyclic) {
> + start = mapping->writeback_index << PAGE_SHIFT;
> + end = isize;
I had to go look, but ->range_cyclic can be used by the background flushing
thread, which of course will be single threaded. However we can also trigger
this with memcg flushing during balance_dirty_pages(), which means that
writeback_index can change. Additionally isize can change arbitrarily between
now and extent_writepages.
I think you're going to have to change extent_writepages to take the start and
end index in order to make sure we're not writing things we haven't locked.
Alternatively you could keep track of range_cyclic here, update the wbc with the
range_start and range_end that you calculate here, and reset it on the way out.
Personally I think it would be cleaner to pass in the start and end into
extent_writepages, and move the ->writeback_index update into this function, as
well as the logic for start and end. Thanks,
Josef
next prev parent reply other threads:[~2022-12-13 18:39 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <cover.1668530684.git.rgoldwyn@suse.com>
2022-11-15 18:00 ` [PATCH 01/16] btrfs: check for range correctness while locking or setting extent bits Goldwyn Rodrigues
2022-11-17 11:09 ` Johannes Thumshirn
2022-11-22 17:17 ` Goldwyn Rodrigues
2022-11-23 8:48 ` Johannes Thumshirn
2022-11-23 13:12 ` Filipe Manana
2022-11-23 14:35 ` Goldwyn Rodrigues
2022-12-13 16:25 ` Josef Bacik
2022-11-15 18:00 ` [PATCH 02/16] btrfs: qgroup flush responsibility of the caller Goldwyn Rodrigues
2022-12-13 16:30 ` Josef Bacik
2022-11-15 18:00 ` [PATCH 03/16] btrfs: wait ordered range before locking during truncate Goldwyn Rodrigues
2022-11-17 11:22 ` Johannes Thumshirn
2022-12-13 18:14 ` Josef Bacik
2022-11-15 18:00 ` [PATCH 04/16] btrfs: lock extents while truncating Goldwyn Rodrigues
2022-12-13 18:29 ` Josef Bacik
2022-11-15 18:00 ` [PATCH 05/16] btrfs: No need to lock extent while performing invalidate_folio() Goldwyn Rodrigues
2022-12-13 18:30 ` Josef Bacik
2022-11-15 18:00 ` [PATCH 06/16] btrfs: Lock extents before pages in writepages Goldwyn Rodrigues
2022-12-13 18:39 ` Josef Bacik [this message]
2022-11-15 18:00 ` [PATCH 07/16] btrfs: Lock extents before folio for read()s Goldwyn Rodrigues
2022-11-21 13:31 ` kernel test robot
2022-11-22 17:11 ` Goldwyn Rodrigues
2022-11-27 8:48 ` kernel test robot
2022-12-13 18:57 ` Josef Bacik
2022-11-15 18:00 ` [PATCH 08/16] btrfs: Lock extents before pages for buffered write() Goldwyn Rodrigues
2022-12-13 19:01 ` Josef Bacik
2022-11-15 18:00 ` [PATCH 09/16] btrfs: lock/unlock extents while creation/end of async_chunk Goldwyn Rodrigues
2022-12-13 19:05 ` Josef Bacik
2022-11-15 18:00 ` [PATCH 10/16] btrfs: decide early if range should be async Goldwyn Rodrigues
2022-12-13 19:07 ` Josef Bacik
2022-11-15 18:00 ` [PATCH 11/16] btrfs: lock extents before pages - defrag Goldwyn Rodrigues
2022-12-13 19:08 ` Josef Bacik
2022-11-15 18:00 ` [PATCH 12/16] btrfs: Perform memory faults under locked extent Goldwyn Rodrigues
2022-12-13 19:12 ` Josef Bacik
2022-11-15 18:00 ` [PATCH 13/16] btrfs: writepage fixup lock rearrangement Goldwyn Rodrigues
2022-12-13 19:13 ` Josef Bacik
2022-11-15 18:00 ` [PATCH 14/16] btrfs: lock extent before pages for encoded read ioctls Goldwyn Rodrigues
2022-12-13 19:14 ` Josef Bacik
2022-11-15 18:00 ` [PATCH 15/16] btrfs: lock extent before pages in encoded write Goldwyn Rodrigues
2022-12-13 19:19 ` Josef Bacik
2022-11-15 18:00 ` [PATCH 16/16] btrfs: btree_writepages lock extents before pages Goldwyn Rodrigues
2022-12-13 19:20 ` Josef Bacik
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y5jG1Rdy+6QPjy0c@localhost.localdomain \
--to=josef@toxicpanda.com \
--cc=linux-btrfs@vger.kernel.org \
--cc=rgoldwyn@suse.com \
--cc=rgoldwyn@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox