From: Qu Wenruo <wqu@suse.com>
To: linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 07/27] btrfs: add subpage checked_bitmap to make PageChecked flag to be subpage compatible
Date: Fri, 16 Jul 2021 15:54:32 +0800 [thread overview]
Message-ID: <ea8930f6-037f-0076-0de5-67b82e70c338@suse.com> (raw)
In-Reply-To: <20210713061516.163318-8-wqu@suse.com>
On 2021/7/13 下午2:14, Qu Wenruo wrote:
> Although in btrfs we have very limited user of PageChecked flag, it's
> still some page flag not yet subpage compatible.
>
> Fix it by introducing btrfs_subpage::checked_bitmap to do the covert.
>
> For most call sites, especially for free-space cache, COW fixup and
> btrfs_invalidatepage(), they all work in full page mode anyway.
>
> For other call sites, they work as fully subpage mode.
>
> Some call sites need extra modification:
>
> - btrfs_drop_pages()
> Needs extra parameter to get the real range we need to clear checked
> flag.
>
> - btrfs_invalidatepage()
> We need to call subpage helper before calling __btrfs_releasepage(),
> or it will trigger ASSERT() as page->private will be cleared.
>
> - btrfs_verify_data_csum()
> In theory we don't need the io_bio->csum check anymore, but it's
> won't hurt.
> Just change the comment.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> fs/btrfs/compression.c | 11 +++++++++--
> fs/btrfs/file.c | 20 +++++++++++++++-----
> fs/btrfs/free-space-cache.c | 6 +++++-
> fs/btrfs/inode.c | 30 ++++++++++++++----------------
> fs/btrfs/reflink.c | 2 +-
> fs/btrfs/subpage.c | 31 +++++++++++++++++++++++++++++++
> fs/btrfs/subpage.h | 8 ++++++++
> 7 files changed, 83 insertions(+), 25 deletions(-)
>
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index f08522e8b4cd..be81e34fbd56 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -29,6 +29,7 @@
> #include "extent_io.h"
> #include "extent_map.h"
> #include "zoned.h"
> +#include "subpage.h"
>
> static const char* const btrfs_compress_types[] = { "", "zlib", "lzo", "zstd" };
>
> @@ -296,8 +297,14 @@ static void end_compressed_bio_read(struct bio *bio)
> * checked so the end_io handlers know about it
> */
> ASSERT(!bio_flagged(bio, BIO_CLONED));
> - bio_for_each_segment_all(bvec, cb->orig_bio, iter_all)
> - SetPageChecked(bvec->bv_page);
> + bio_for_each_segment_all(bvec, cb->orig_bio, iter_all) {
> + u64 bvec_start = page_offset(bvec->bv_page) +
> + bvec->bv_offset;
> +
> + btrfs_page_set_checked(btrfs_sb(cb->inode->i_sb),
> + bvec->bv_page, bvec_start,
> + bvec->bv_len);
> + }
>
> bio_endio(cb->orig_bio);
> }
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 0831ca08376f..ccbbf2732685 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -437,9 +437,16 @@ static noinline int btrfs_copy_from_user(loff_t pos, size_t write_bytes,
> /*
> * unlocks pages after btrfs_file_write is done with them
> */
> -static void btrfs_drop_pages(struct page **pages, size_t num_pages)
> +static void btrfs_drop_pages(struct btrfs_fs_info *fs_info,
> + struct page **pages, size_t num_pages,
> + u64 pos, u64 copied)
> {
> size_t i;
> + u64 block_start = round_down(pos, fs_info->sectorsize);
> + u64 block_len = round_up(pos + copied, fs_info->sectorsize) -
> + block_start;
> +
> + ASSERT(block_len <= U32_MAX);
> for (i = 0; i < num_pages; i++) {
> /* page checked is some magic around finding pages that
> * have been modified without going through btrfs_set_page_dirty
> @@ -447,7 +454,8 @@ static void btrfs_drop_pages(struct page **pages, size_t num_pages)
> * accessed as prepare_pages should have marked them accessed
> * in prepare_pages via find_or_create_page()
> */
> - ClearPageChecked(pages[i]);
> + btrfs_page_clamp_clear_checked(fs_info, pages[i], block_start,
> + block_len);
Currently btrfs_subpage_clamp() can't handle any page whose
page_offset() is beyond @block_start + @block_len.
This leads to rare crash in generic/269 and generic/102.
Will also update btrfs_subpage_clamp() to handle such case in next update.
Thanks,
Q
> unlock_page(pages[i]);
> put_page(pages[i]);
> }
> @@ -504,7 +512,8 @@ int btrfs_dirty_pages(struct btrfs_inode *inode, struct page **pages,
> struct page *p = pages[i];
>
> btrfs_page_clamp_set_uptodate(fs_info, p, start_pos, num_bytes);
> - ClearPageChecked(p);
> + btrfs_page_clamp_clear_checked(fs_info, p, start_pos,
> + num_bytes);
> btrfs_page_clamp_set_dirty(fs_info, p, start_pos, num_bytes);
> }
>
> @@ -1845,7 +1854,8 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
>
> btrfs_delalloc_release_extents(BTRFS_I(inode), reserve_bytes);
> if (ret) {
> - btrfs_drop_pages(pages, num_pages);
> + btrfs_drop_pages(fs_info, pages, num_pages, pos,
> + copied);
> break;
> }
>
> @@ -1853,7 +1863,7 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
> if (only_release_metadata)
> btrfs_check_nocow_unlock(BTRFS_I(inode));
>
> - btrfs_drop_pages(pages, num_pages);
> + btrfs_drop_pages(fs_info, pages, num_pages, pos, copied);
>
> cond_resched();
>
> diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
> index 2131ae5b9ed7..f0a84fe7dc80 100644
> --- a/fs/btrfs/free-space-cache.c
> +++ b/fs/btrfs/free-space-cache.c
> @@ -22,6 +22,7 @@
> #include "delalloc-space.h"
> #include "block-group.h"
> #include "discard.h"
> +#include "subpage.h"
>
> #define BITS_PER_BITMAP (PAGE_SIZE * 8UL)
> #define MAX_CACHE_BYTES_PER_GIG SZ_64K
> @@ -417,7 +418,10 @@ static void io_ctl_drop_pages(struct btrfs_io_ctl *io_ctl)
>
> for (i = 0; i < io_ctl->num_pages; i++) {
> if (io_ctl->pages[i]) {
> - ClearPageChecked(io_ctl->pages[i]);
> + btrfs_page_clear_checked(io_ctl->fs_info,
> + io_ctl->pages[i],
> + page_offset(io_ctl->pages[i]),
> + PAGE_SIZE);
> unlock_page(io_ctl->pages[i]);
> put_page(io_ctl->pages[i]);
> }
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index b524deadb5c6..5c29d131a574 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2757,7 +2757,8 @@ static void btrfs_writepage_fixup_worker(struct btrfs_work *work)
> clear_page_dirty_for_io(page);
> SetPageError(page);
> }
> - ClearPageChecked(page);
> + btrfs_page_clear_checked(inode->root->fs_info, page,
> + page_start, PAGE_SIZE);
> unlock_page(page);
> put_page(page);
> kfree(fixup);
> @@ -2812,7 +2813,7 @@ int btrfs_writepage_cow_fixup(struct page *page, u64 start, u64 end)
> * page->mapping outside of the page lock.
> */
> ihold(inode);
> - SetPageChecked(page);
> + btrfs_page_set_checked(fs_info, page, page_offset(page), PAGE_SIZE);
> get_page(page);
> btrfs_init_work(&fixup->work, btrfs_writepage_fixup_worker, NULL, NULL);
> fixup->page = page;
> @@ -3257,27 +3258,23 @@ unsigned int btrfs_verify_data_csum(struct btrfs_io_bio *io_bio, u32 bio_offset,
> struct page *page, u64 start, u64 end)
> {
> struct inode *inode = page->mapping->host;
> + struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> struct extent_io_tree *io_tree = &BTRFS_I(inode)->io_tree;
> struct btrfs_root *root = BTRFS_I(inode)->root;
> const u32 sectorsize = root->fs_info->sectorsize;
> u32 pg_off;
> unsigned int result = 0;
>
> - if (PageChecked(page)) {
> - ClearPageChecked(page);
> + if (btrfs_page_test_checked(fs_info, page, start, end + 1 - start)) {
> + btrfs_page_clear_checked(fs_info, page, start, end + 1 - start);
> return 0;
> }
>
> /*
> - * For subpage case, above PageChecked is not safe as it's not subpage
> - * compatible.
> - * But for now only cow fixup and compressed read utilize PageChecked
> - * flag, while in this context we can easily use io_bio->csum to
> - * determine if we really need to do csum verification.
> - *
> - * So for now, just exit if io_bio->csum is NULL, as it means it's
> - * compressed read, and its compressed data csum has already been
> - * verified.
> + * This only happens for NODATASUM or compressed read.
> + * Normally this should be covered by above check for compressed read
> + * or the next check for NODATASUM.
> + * Just do a quicker exit here.
> */
> if (io_bio->csum == NULL)
> return 0;
> @@ -5083,7 +5080,8 @@ int btrfs_truncate_block(struct btrfs_inode *inode, loff_t from, loff_t len,
> len);
> flush_dcache_page(page);
> }
> - ClearPageChecked(page);
> + btrfs_page_clear_checked(fs_info, page, block_start,
> + block_end + 1 - block_start);
> btrfs_page_set_dirty(fs_info, page, block_start, block_end + 1 - block_start);
> unlock_extent_cached(io_tree, block_start, block_end, &cached_state);
>
> @@ -8673,9 +8671,9 @@ static void btrfs_invalidatepage(struct page *page, unsigned int offset,
> * did something wrong.
> */
> ASSERT(!PageOrdered(page));
> + btrfs_page_clear_checked(fs_info, page, page_offset(page), PAGE_SIZE);
> if (!inode_evicting)
> __btrfs_releasepage(page, GFP_NOFS);
> - ClearPageChecked(page);
> clear_page_extent_mapped(page);
> }
>
> @@ -8819,7 +8817,7 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
> memzero_page(page, zero_start, PAGE_SIZE - zero_start);
> flush_dcache_page(page);
> }
> - ClearPageChecked(page);
> + btrfs_page_clear_checked(fs_info, page, page_start, PAGE_SIZE);
> btrfs_page_set_dirty(fs_info, page, page_start, end + 1 - page_start);
> btrfs_page_set_uptodate(fs_info, page, page_start, end + 1 - page_start);
>
> diff --git a/fs/btrfs/reflink.c b/fs/btrfs/reflink.c
> index 9b0814318e72..a7de8cfdcac0 100644
> --- a/fs/btrfs/reflink.c
> +++ b/fs/btrfs/reflink.c
> @@ -138,7 +138,7 @@ static int copy_inline_to_page(struct btrfs_inode *inode,
> }
>
> btrfs_page_set_uptodate(fs_info, page, file_offset, block_size);
> - ClearPageChecked(page);
> + btrfs_page_clear_checked(fs_info, page, file_offset, block_size);
> btrfs_page_set_dirty(fs_info, page, file_offset, block_size);
> out_unlock:
> if (page) {
> diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c
> index a61aa33aeeee..b8a420cb1683 100644
> --- a/fs/btrfs/subpage.c
> +++ b/fs/btrfs/subpage.c
> @@ -468,6 +468,34 @@ void btrfs_subpage_clear_ordered(const struct btrfs_fs_info *fs_info,
> ClearPageOrdered(page);
> spin_unlock_irqrestore(&subpage->lock, flags);
> }
> +
> +void btrfs_subpage_set_checked(const struct btrfs_fs_info *fs_info,
> + struct page *page, u64 start, u32 len)
> +{
> + struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
> + const u16 tmp = btrfs_subpage_calc_bitmap(fs_info, page, start, len);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&subpage->lock, flags);
> + subpage->checked_bitmap |= tmp;
> + if (subpage->checked_bitmap == U16_MAX)
> + SetPageChecked(page);
> + spin_unlock_irqrestore(&subpage->lock, flags);
> +}
> +
> +void btrfs_subpage_clear_checked(const struct btrfs_fs_info *fs_info,
> + struct page *page, u64 start, u32 len)
> +{
> + struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
> + const u16 tmp = btrfs_subpage_calc_bitmap(fs_info, page, start, len);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&subpage->lock, flags);
> + subpage->checked_bitmap &= ~tmp;
> + ClearPageChecked(page);
> + spin_unlock_irqrestore(&subpage->lock, flags);
> +}
> +
> /*
> * Unlike set/clear which is dependent on each page status, for test all bits
> * are tested in the same way.
> @@ -491,6 +519,7 @@ IMPLEMENT_BTRFS_SUBPAGE_TEST_OP(error);
> IMPLEMENT_BTRFS_SUBPAGE_TEST_OP(dirty);
> IMPLEMENT_BTRFS_SUBPAGE_TEST_OP(writeback);
> IMPLEMENT_BTRFS_SUBPAGE_TEST_OP(ordered);
> +IMPLEMENT_BTRFS_SUBPAGE_TEST_OP(checked);
>
> /*
> * Note that, in selftests (extent-io-tests), we can have empty fs_info passed
> @@ -561,6 +590,8 @@ IMPLEMENT_BTRFS_PAGE_OPS(writeback, set_page_writeback, end_page_writeback,
> PageWriteback);
> IMPLEMENT_BTRFS_PAGE_OPS(ordered, SetPageOrdered, ClearPageOrdered,
> PageOrdered);
> +IMPLEMENT_BTRFS_PAGE_OPS(checked, SetPageChecked, ClearPageChecked,
> + PageChecked);
>
> void btrfs_page_assert_not_dirty(const struct btrfs_fs_info *fs_info,
> struct page *page)
> diff --git a/fs/btrfs/subpage.h b/fs/btrfs/subpage.h
> index 9aa40d795ba9..6fb54b22b295 100644
> --- a/fs/btrfs/subpage.h
> +++ b/fs/btrfs/subpage.h
> @@ -44,6 +44,13 @@ struct btrfs_subpage {
>
> /* Tracke pending ordered extent in this sector */
> u16 ordered_bitmap;
> +
> + /*
> + * If the sector is already checked, thus no need to go
> + * through csum verification.
> + * Used by both compression and cow fixup.
> + */
> + u16 checked_bitmap;
> };
> };
> };
> @@ -122,6 +129,7 @@ DECLARE_BTRFS_SUBPAGE_OPS(error);
> DECLARE_BTRFS_SUBPAGE_OPS(dirty);
> DECLARE_BTRFS_SUBPAGE_OPS(writeback);
> DECLARE_BTRFS_SUBPAGE_OPS(ordered);
> +DECLARE_BTRFS_SUBPAGE_OPS(checked);
>
> bool btrfs_subpage_clear_and_test_dirty(const struct btrfs_fs_info *fs_info,
> struct page *page, u64 start, u32 len);
>
next prev parent reply other threads:[~2021-07-16 7:54 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-13 6:14 [PATCH 00/27] btrfs: limited subpage compressed write support Qu Wenruo
2021-07-13 6:14 ` [PATCH 01/27] btrfs: remove unused parameter @nr_pages in add_ra_bio_pages() Qu Wenruo
2021-07-13 6:14 ` [PATCH 02/27] btrfs: remove unnecessary parameter @delalloc_start for writepage_delalloc() Qu Wenruo
2021-07-13 6:14 ` [PATCH 03/27] btrfs: use async_chunk::async_cow to replace the confusing pending pointer Qu Wenruo
2021-07-13 7:36 ` Nikolay Borisov
2021-07-13 6:14 ` [PATCH 04/27] btrfs: don't pass compressed pages to btrfs_writepage_endio_finish_ordered() Qu Wenruo
2021-07-13 6:14 ` [PATCH 05/27] btrfs: make add_ra_bio_pages() to be subpage compatible Qu Wenruo
2021-07-13 6:14 ` [PATCH 06/27] btrfs: introduce compressed_bio::pending_sectors to trace compressed bio more elegantly Qu Wenruo
2021-07-13 6:14 ` [PATCH 07/27] btrfs: add subpage checked_bitmap to make PageChecked flag to be subpage compatible Qu Wenruo
2021-07-16 7:54 ` Qu Wenruo [this message]
2021-07-13 6:14 ` [PATCH 08/27] btrfs: handle errors properly inside btrfs_submit_compressed_read() Qu Wenruo
2021-07-13 6:14 ` [PATCH 09/27] btrfs: handle errors properly inside btrfs_submit_compressed_write() Qu Wenruo
2021-07-13 6:14 ` [PATCH 10/27] btrfs: introduce submit_compressed_bio() for compression Qu Wenruo
2021-07-13 6:15 ` [PATCH 11/27] btrfs: introduce alloc_compressed_bio() " Qu Wenruo
2021-07-13 6:15 ` [PATCH 12/27] btrfs: make btrfs_submit_compressed_read() to determine stripe boundary at bio allocation time Qu Wenruo
2021-07-13 6:15 ` [PATCH 13/27] btrfs: make btrfs_submit_compressed_write() " Qu Wenruo
2021-07-13 6:15 ` [PATCH 14/27] btrfs: remove unused function btrfs_bio_fits_in_stripe() Qu Wenruo
2021-07-13 6:15 ` [PATCH 15/27] btrfs: refactor submit_compressed_extents() Qu Wenruo
2021-07-13 6:15 ` [PATCH 16/27] btrfs: cleanup for extent_write_locked_range() Qu Wenruo
2021-07-13 6:15 ` [PATCH 17/27] btrfs: make compress_file_range() to be subpage compatible Qu Wenruo
2021-07-13 6:15 ` [PATCH 18/27] btrfs: make btrfs_submit_compressed_write() " Qu Wenruo
2021-07-13 6:15 ` [PATCH 19/27] btrfs: make end_compressed_bio_writeback() to be subpage compatble Qu Wenruo
2021-07-13 6:15 ` [PATCH 20/27] btrfs: make extent_write_locked_range() to be subpage compatible Qu Wenruo
2021-07-13 6:15 ` [PATCH 21/27] btrfs: extract uncompressed async extent submission code into a new helper Qu Wenruo
2021-07-13 6:15 ` [PATCH 22/27] btrfs: rework lzo_compress_pages() to make it subpage compatible Qu Wenruo
2021-07-13 6:15 ` [PATCH 23/27] btrfs: teach __extent_writepage() to handle locked page differently Qu Wenruo
2021-07-13 6:15 ` [PATCH 24/27] btrfs: allow page to be unlocked by btrfs_page_end_writer_lock() even if it's locked by plain page_lock() Qu Wenruo
2021-07-13 6:15 ` [PATCH 25/27] btrfs: allow subpage to compress a range which only covers one page Qu Wenruo
2021-07-13 6:15 ` [PATCH 26/27] btrfs: don't run delalloc range which is beyond the locked_page to prevent deadlock for subpage compression Qu Wenruo
2021-07-13 6:15 ` [PATCH 27/27] btrfs: only allow subpage compression if the range is fully page aligned Qu Wenruo
2021-07-16 9:11 ` [PATCH 00/27] btrfs: limited subpage compressed write support Qu Wenruo
2021-07-20 5:00 ` Qu Wenruo
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=ea8930f6-037f-0076-0de5-67b82e70c338@suse.com \
--to=wqu@suse.com \
--cc=linux-btrfs@vger.kernel.org \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.