* Re: [PATCH v3 16/22] btrfs: extent_io: implement try_release_extent_buffer() for subpage metadata support
@ 2021-01-06 3:59 kernel test robot
0 siblings, 0 replies; 4+ messages in thread
From: kernel test robot @ 2021-01-06 3:59 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 7713 bytes --]
CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210106010201.37864-17-wqu@suse.com>
References: <20210106010201.37864-17-wqu@suse.com>
TO: Qu Wenruo <wqu@suse.com>
TO: linux-btrfs(a)vger.kernel.org
Hi Qu,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on kdave/for-next]
[also build test WARNING on v5.11-rc2 next-20210104]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Qu-Wenruo/btrfs-add-read-only-support-for-subpage-sector-size/20210106-090847
base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
:::::: branch date: 3 hours ago
:::::: commit date: 3 hours ago
config: m68k-randconfig-s032-20210106 (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.3-208-g46a52ca4-dirty
# https://github.com/0day-ci/linux/commit/c0f229c17f975112f926038d70649bb2e479bfe3
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Qu-Wenruo/btrfs-add-read-only-support-for-subpage-sector-size/20210106-090847
git checkout c0f229c17f975112f926038d70649bb2e479bfe3
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
"sparse warnings: (new ones prefixed by >>)"
fs/btrfs/extent_io.c:5061:25: sparse: sparse: context imbalance in 'btrfs_release_extent_buffer_pages' - different lock contexts for basic block
>> fs/btrfs/extent_io.c:6271:9: sparse: sparse: context imbalance in 'try_release_subpage_extent_buffer' - different lock contexts for basic block
vim +/try_release_subpage_extent_buffer +6271 fs/btrfs/extent_io.c
6af118ce51b52ce Chris Mason 2008-07-22 6262
c0f229c17f97511 Qu Wenruo 2021-01-06 6263 static int try_release_subpage_extent_buffer(struct page *page)
c0f229c17f97511 Qu Wenruo 2021-01-06 6264 {
c0f229c17f97511 Qu Wenruo 2021-01-06 6265 struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
c0f229c17f97511 Qu Wenruo 2021-01-06 6266 u64 page_start = page_offset(page);
c0f229c17f97511 Qu Wenruo 2021-01-06 6267 int bitmap_size = BTRFS_SUBPAGE_BITMAP_SIZE;
c0f229c17f97511 Qu Wenruo 2021-01-06 6268 int bit_start = 0;
c0f229c17f97511 Qu Wenruo 2021-01-06 6269 int ret;
c0f229c17f97511 Qu Wenruo 2021-01-06 6270
c0f229c17f97511 Qu Wenruo 2021-01-06 @6271 while (bit_start < bitmap_size) {
c0f229c17f97511 Qu Wenruo 2021-01-06 6272 struct btrfs_subpage *subpage;
c0f229c17f97511 Qu Wenruo 2021-01-06 6273 struct extent_buffer *eb;
c0f229c17f97511 Qu Wenruo 2021-01-06 6274 unsigned long flags;
c0f229c17f97511 Qu Wenruo 2021-01-06 6275 u16 tmp = 1 << bit_start;
c0f229c17f97511 Qu Wenruo 2021-01-06 6276 u64 start;
c0f229c17f97511 Qu Wenruo 2021-01-06 6277
c0f229c17f97511 Qu Wenruo 2021-01-06 6278 /*
c0f229c17f97511 Qu Wenruo 2021-01-06 6279 * Make sure the page still has private, as previous iteration
c0f229c17f97511 Qu Wenruo 2021-01-06 6280 * can detach page private.
c0f229c17f97511 Qu Wenruo 2021-01-06 6281 */
c0f229c17f97511 Qu Wenruo 2021-01-06 6282 spin_lock(&page->mapping->private_lock);
c0f229c17f97511 Qu Wenruo 2021-01-06 6283 if (!PagePrivate(page)) {
c0f229c17f97511 Qu Wenruo 2021-01-06 6284 spin_unlock(&page->mapping->private_lock);
c0f229c17f97511 Qu Wenruo 2021-01-06 6285 break;
c0f229c17f97511 Qu Wenruo 2021-01-06 6286 }
c0f229c17f97511 Qu Wenruo 2021-01-06 6287
c0f229c17f97511 Qu Wenruo 2021-01-06 6288 subpage = (struct btrfs_subpage *)page->private;
c0f229c17f97511 Qu Wenruo 2021-01-06 6289
c0f229c17f97511 Qu Wenruo 2021-01-06 6290 spin_lock_irqsave(&subpage->lock, flags);
c0f229c17f97511 Qu Wenruo 2021-01-06 6291 spin_unlock(&page->mapping->private_lock);
c0f229c17f97511 Qu Wenruo 2021-01-06 6292
c0f229c17f97511 Qu Wenruo 2021-01-06 6293 if (!(tmp & subpage->tree_block_bitmap)) {
c0f229c17f97511 Qu Wenruo 2021-01-06 6294 spin_unlock_irqrestore(&subpage->lock, flags);
c0f229c17f97511 Qu Wenruo 2021-01-06 6295 bit_start++;
c0f229c17f97511 Qu Wenruo 2021-01-06 6296 continue;
c0f229c17f97511 Qu Wenruo 2021-01-06 6297 }
c0f229c17f97511 Qu Wenruo 2021-01-06 6298
c0f229c17f97511 Qu Wenruo 2021-01-06 6299 start = bit_start * fs_info->sectorsize + page_start;
c0f229c17f97511 Qu Wenruo 2021-01-06 6300 bit_start += fs_info->nodesize >> fs_info->sectorsize_bits;
c0f229c17f97511 Qu Wenruo 2021-01-06 6301 /*
c0f229c17f97511 Qu Wenruo 2021-01-06 6302 * Here we can't call find_extent_buffer() which will increase
c0f229c17f97511 Qu Wenruo 2021-01-06 6303 * eb->refs.
c0f229c17f97511 Qu Wenruo 2021-01-06 6304 */
c0f229c17f97511 Qu Wenruo 2021-01-06 6305 rcu_read_lock();
c0f229c17f97511 Qu Wenruo 2021-01-06 6306 eb = radix_tree_lookup(&fs_info->buffer_radix,
c0f229c17f97511 Qu Wenruo 2021-01-06 6307 start >> fs_info->sectorsize_bits);
c0f229c17f97511 Qu Wenruo 2021-01-06 6308 ASSERT(eb);
c0f229c17f97511 Qu Wenruo 2021-01-06 6309 spin_lock(&eb->refs_lock);
c0f229c17f97511 Qu Wenruo 2021-01-06 6310 if (atomic_read(&eb->refs) != 1 || extent_buffer_under_io(eb) ||
c0f229c17f97511 Qu Wenruo 2021-01-06 6311 !test_and_clear_bit(EXTENT_BUFFER_TREE_REF, &eb->bflags)) {
c0f229c17f97511 Qu Wenruo 2021-01-06 6312 spin_unlock(&eb->refs_lock);
c0f229c17f97511 Qu Wenruo 2021-01-06 6313 rcu_read_unlock();
c0f229c17f97511 Qu Wenruo 2021-01-06 6314 continue;
c0f229c17f97511 Qu Wenruo 2021-01-06 6315 }
c0f229c17f97511 Qu Wenruo 2021-01-06 6316 rcu_read_unlock();
c0f229c17f97511 Qu Wenruo 2021-01-06 6317 spin_unlock_irqrestore(&subpage->lock, flags);
c0f229c17f97511 Qu Wenruo 2021-01-06 6318 /*
c0f229c17f97511 Qu Wenruo 2021-01-06 6319 * Here we don't care the return value, we will always check
c0f229c17f97511 Qu Wenruo 2021-01-06 6320 * the page private at the end.
c0f229c17f97511 Qu Wenruo 2021-01-06 6321 * And release_extent_buffer() will release the refs_lock.
c0f229c17f97511 Qu Wenruo 2021-01-06 6322 */
c0f229c17f97511 Qu Wenruo 2021-01-06 6323 release_extent_buffer(eb);
c0f229c17f97511 Qu Wenruo 2021-01-06 6324 }
c0f229c17f97511 Qu Wenruo 2021-01-06 6325 /* Finally to check if we have cleared page private */
c0f229c17f97511 Qu Wenruo 2021-01-06 6326 spin_lock(&page->mapping->private_lock);
c0f229c17f97511 Qu Wenruo 2021-01-06 6327 if (!PagePrivate(page))
c0f229c17f97511 Qu Wenruo 2021-01-06 6328 ret = 1;
c0f229c17f97511 Qu Wenruo 2021-01-06 6329 else
c0f229c17f97511 Qu Wenruo 2021-01-06 6330 ret = 0;
c0f229c17f97511 Qu Wenruo 2021-01-06 6331 spin_unlock(&page->mapping->private_lock);
c0f229c17f97511 Qu Wenruo 2021-01-06 6332 return ret;
c0f229c17f97511 Qu Wenruo 2021-01-06 6333
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 33611 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v3 00/22] btrfs: add read-only support for subpage sector size
@ 2021-01-06 1:01 Qu Wenruo
2021-01-06 1:01 ` [PATCH v3 16/22] btrfs: extent_io: implement try_release_extent_buffer() for subpage metadata support Qu Wenruo
0 siblings, 1 reply; 4+ messages in thread
From: Qu Wenruo @ 2021-01-06 1:01 UTC (permalink / raw)
To: linux-btrfs
Patches can be fetched from github:
https://github.com/adam900710/linux/tree/subpage
Currently the branch also contains partial RW data support (still some
out-of-sync subpage data page status).
Great thanks to David for his effort reviewing and merging the
preparation patches into misc-next.
Now all previously submitted preparation patches are already in
misc-next.
=== What works ===
Just from the patchset:
- Data read
Both regular and compressed data, with csum check.
- Metadata read
This means, with these patchset, 64K page systems can at least mount
btrfs with 4K sector size.
In the subpage branch
- Metadata read write and balance
Not yet full tested due to data write still has bugs need to be
solved.
But considering that metadata operations from previous iteration
is mostly untouched, metadata read write should be pretty stable.
- Baisc data balance
This is new.
- Data read write
Only uncompressed data writes. Fsstress can survive.
But still very rare data csum error, which only happens in bookend data
extents (part of the data extent which is not referred by any file).
This looks like something related to reflink/invalidate page and cow
fixup.
Still invetigating.
=== Needs feedback ===
The following design needs extra comments:
- u16 bitmap
As David mentioned, using u16 as bit map is not the fastest way.
That's also why current bitmap code requires unsigned long (u32) as
minimal unit.
But using bitmap directly would double the memory usage.
Thus the best way is to pack two u16 bitmap into one u32 bitmap, but
that still needs extra investigation to find better practice.
Anyway the skeleton should be pretty simple to expand.
- Separate handling for subpage metadata
Currently the metadata read and (later write path) handles subpage
metadata differently. Mostly due to the page locking must be skipped
for subpage metadata.
I tried several times to use as many common code as possible, but
every time I ended up reverting back to current code.
Thankfully, for data handling we will use the same common code.
- Incompatible subpage strcuture against iomap_page
In btrfs we need extra bits than iomap_page.
This is due to we need sector perfect write for data balance.
E.g. if only one 4K sector is dirty in a 64K page, we should only
write that dirty 4K back to disk, not the full 64K page.
As data balance requires the new data extents to have exactly the
same size as the original ones.
This means, unless iomap_page get extra bits like what we're doing in
btrfs for dirty, we can't merge the btrfs_subpage with iomap_page.
=== Patchset structure ===
Patch 01~03: Existing preparation patches.
Mostly readability related patches found during RW
development
Patch 04~05: New preparation patches.
Mostly related to __process_pages_contig().
Patch 06~10: Subpage handling for extent buffer allocation and
freeing
Patch 11~22: Subpage handling for extent buffer read path
=== Changelog ===
v1:
- Separate the main implementation from previous huge patchset
Huge patchset doesn't make much sense.
- Use bitmap implementation
Now page::private will be a pointer to btrfs_subpage structure, which
contains bitmaps for various page status.
v2:
- Use page::private as btrfs_subpage for extra info
This replace old extent io tree based solution, which reduces latency
and don't require memory allocation for its operations.
- Cherry-pick new preparation patches from RW development
Those new preparation patches improves the readability by their own.
v3:
- Make dummy extent buffer to follow the same subpage accessors
Fsstress exposed several ASSERT() for dummy extent buffers.
It turns out we need to make dummy extent buffer to own the same
btrfs_subpage structure to make eb accessors to work properly
- Two new small __process_pages_contig() related preparation patches
One to make __process_pages_contig() to enhance the error handling
path for locked_page, one to merge one macro.
- Extent buffers refs count update
Except try_release_extent_buffer(), all other eb uses will try to
increase the ref count of the eb.
For try_release_extent_buffer(), the eb refs check will happen inside
the rcu critical section to avoid eb being freed.
- Comment updates
Addressing the comments from the mail list.
Qu Wenruo (22):
btrfs: extent_io: rename @offset parameter to @disk_bytenr for
submit_extent_page()
btrfs: extent_io: refactor __extent_writepage_io() to improve
readability
btrfs: file: update comment for btrfs_dirty_pages()
btrfs: extent_io: update locked page dirty/writeback/error bits in
__process_pages_contig()
btrfs: extent_io: merge PAGE_CLEAR_DIRTY and PAGE_SET_WRITEBACK into
PAGE_START_WRITEBACK
btrfs: extent_io: introduce a helper to grab an existing extent buffer
from a page
btrfs: extent_io: introduce the skeleton of btrfs_subpage structure
btrfs: extent_io: make attach_extent_buffer_page() to handle subpage
case
btrfs: extent_io: make grab_extent_buffer_from_page() to handle
subpage case
btrfs: extent_io: support subpage for extent buffer page release
btrfs: extent_io: attach private to dummy extent buffer pages
btrfs: subpage: introduce helper for subpage uptodate status
btrfs: subpage: introduce helper for subpage error status
btrfs: extent_io: make set/clear_extent_buffer_uptodate() to support
subpage size
btrfs: extent_io: make btrfs_clone_extent_buffer() to be subpage
compatible
btrfs: extent_io: implement try_release_extent_buffer() for subpage
metadata support
btrfs: extent_io: introduce read_extent_buffer_subpage()
btrfs: extent_io: make endio_readpage_update_page_status() to handle
subpage case
btrfs: disk-io: introduce subpage metadata validation check
btrfs: introduce btrfs_subpage for data inodes
btrfs: integrate page status update for read path into
begin/end_page_read()
btrfs: allow RO mount of 4K sector size fs on 64K page system
fs/btrfs/Makefile | 3 +-
fs/btrfs/compression.c | 10 +-
fs/btrfs/disk-io.c | 82 +++++-
fs/btrfs/extent_io.c | 534 ++++++++++++++++++++++++++++--------
fs/btrfs/extent_io.h | 15 +-
fs/btrfs/file.c | 35 +--
fs/btrfs/free-space-cache.c | 15 +-
fs/btrfs/inode.c | 40 ++-
fs/btrfs/ioctl.c | 5 +-
fs/btrfs/reflink.c | 5 +-
fs/btrfs/relocation.c | 12 +-
fs/btrfs/subpage.c | 39 +++
fs/btrfs/subpage.h | 256 +++++++++++++++++
fs/btrfs/super.c | 7 +
14 files changed, 876 insertions(+), 182 deletions(-)
create mode 100644 fs/btrfs/subpage.c
create mode 100644 fs/btrfs/subpage.h
--
2.29.2
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v3 16/22] btrfs: extent_io: implement try_release_extent_buffer() for subpage metadata support
2021-01-06 1:01 [PATCH v3 00/22] btrfs: add read-only support for subpage sector size Qu Wenruo
@ 2021-01-06 1:01 ` Qu Wenruo
2021-01-06 8:24 ` Qu Wenruo
0 siblings, 1 reply; 4+ messages in thread
From: Qu Wenruo @ 2021-01-06 1:01 UTC (permalink / raw)
To: linux-btrfs
Unlike the original try_release_extent_buffer,
try_release_subpage_extent_buffer() will iterate through
btrfs_subpage::tree_block_bitmap, and try to release each extent buffer.
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
fs/btrfs/extent_io.c | 76 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 76 insertions(+)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 194cb8b63216..792264f5c3c2 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -6258,10 +6258,86 @@ void memmove_extent_buffer(const struct extent_buffer *dst,
}
}
+static int try_release_subpage_extent_buffer(struct page *page)
+{
+ struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
+ u64 page_start = page_offset(page);
+ int bitmap_size = BTRFS_SUBPAGE_BITMAP_SIZE;
+ int bit_start = 0;
+ int ret;
+
+ while (bit_start < bitmap_size) {
+ struct btrfs_subpage *subpage;
+ struct extent_buffer *eb;
+ unsigned long flags;
+ u16 tmp = 1 << bit_start;
+ u64 start;
+
+ /*
+ * Make sure the page still has private, as previous iteration
+ * can detach page private.
+ */
+ spin_lock(&page->mapping->private_lock);
+ if (!PagePrivate(page)) {
+ spin_unlock(&page->mapping->private_lock);
+ break;
+ }
+
+ subpage = (struct btrfs_subpage *)page->private;
+
+ spin_lock_irqsave(&subpage->lock, flags);
+ spin_unlock(&page->mapping->private_lock);
+
+ if (!(tmp & subpage->tree_block_bitmap)) {
+ spin_unlock_irqrestore(&subpage->lock, flags);
+ bit_start++;
+ continue;
+ }
+
+ start = bit_start * fs_info->sectorsize + page_start;
+ bit_start += fs_info->nodesize >> fs_info->sectorsize_bits;
+ /*
+ * Here we can't call find_extent_buffer() which will increase
+ * eb->refs.
+ */
+ rcu_read_lock();
+ eb = radix_tree_lookup(&fs_info->buffer_radix,
+ start >> fs_info->sectorsize_bits);
+ ASSERT(eb);
+ spin_lock(&eb->refs_lock);
+ if (atomic_read(&eb->refs) != 1 || extent_buffer_under_io(eb) ||
+ !test_and_clear_bit(EXTENT_BUFFER_TREE_REF, &eb->bflags)) {
+ spin_unlock(&eb->refs_lock);
+ rcu_read_unlock();
+ continue;
+ }
+ rcu_read_unlock();
+ spin_unlock_irqrestore(&subpage->lock, flags);
+ /*
+ * Here we don't care the return value, we will always check
+ * the page private at the end.
+ * And release_extent_buffer() will release the refs_lock.
+ */
+ release_extent_buffer(eb);
+ }
+ /* Finally to check if we have cleared page private */
+ spin_lock(&page->mapping->private_lock);
+ if (!PagePrivate(page))
+ ret = 1;
+ else
+ ret = 0;
+ spin_unlock(&page->mapping->private_lock);
+ return ret;
+
+}
+
int try_release_extent_buffer(struct page *page)
{
struct extent_buffer *eb;
+ if (btrfs_sb(page->mapping->host->i_sb)->sectorsize < PAGE_SIZE)
+ return try_release_subpage_extent_buffer(page);
+
/*
* We need to make sure nobody is attaching this page to an eb right
* now.
--
2.29.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3 16/22] btrfs: extent_io: implement try_release_extent_buffer() for subpage metadata support
2021-01-06 1:01 ` [PATCH v3 16/22] btrfs: extent_io: implement try_release_extent_buffer() for subpage metadata support Qu Wenruo
@ 2021-01-06 8:24 ` Qu Wenruo
2021-01-06 8:43 ` Qu Wenruo
0 siblings, 1 reply; 4+ messages in thread
From: Qu Wenruo @ 2021-01-06 8:24 UTC (permalink / raw)
To: linux-btrfs
On 2021/1/6 上午9:01, Qu Wenruo wrote:
> Unlike the original try_release_extent_buffer,
> try_release_subpage_extent_buffer() will iterate through
> btrfs_subpage::tree_block_bitmap, and try to release each extent buffer.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> fs/btrfs/extent_io.c | 76 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 76 insertions(+)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 194cb8b63216..792264f5c3c2 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -6258,10 +6258,86 @@ void memmove_extent_buffer(const struct extent_buffer *dst,
> }
> }
>
> +static int try_release_subpage_extent_buffer(struct page *page)
> +{
> + struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
> + u64 page_start = page_offset(page);
> + int bitmap_size = BTRFS_SUBPAGE_BITMAP_SIZE;
> + int bit_start = 0;
> + int ret;
> +
> + while (bit_start < bitmap_size) {
> + struct btrfs_subpage *subpage;
> + struct extent_buffer *eb;
> + unsigned long flags;
> + u16 tmp = 1 << bit_start;
> + u64 start;
> +
> + /*
> + * Make sure the page still has private, as previous iteration
> + * can detach page private.
> + */
> + spin_lock(&page->mapping->private_lock);
> + if (!PagePrivate(page)) {
> + spin_unlock(&page->mapping->private_lock);
> + break;
> + }
> +
> + subpage = (struct btrfs_subpage *)page->private;
> +
> + spin_lock_irqsave(&subpage->lock, flags);
> + spin_unlock(&page->mapping->private_lock);
> +
> + if (!(tmp & subpage->tree_block_bitmap)) {
> + spin_unlock_irqrestore(&subpage->lock, flags);
> + bit_start++;
> + continue;
> + }
> +
> + start = bit_start * fs_info->sectorsize + page_start;
> + bit_start += fs_info->nodesize >> fs_info->sectorsize_bits;
> + /*
> + * Here we can't call find_extent_buffer() which will increase
> + * eb->refs.
> + */
> + rcu_read_lock();
> + eb = radix_tree_lookup(&fs_info->buffer_radix,
> + start >> fs_info->sectorsize_bits);
> + ASSERT(eb);
Another ASSERT() hit here. Surprised that I have never hit such case before.
Since in releasse_extent_buffer(), radix tree is removed first, then
subpage tree_block_bitmap update, we could have cases where subpage
tree_block_bitmap is set but no eb in radix tree.
In that case, we can safely go next bit and re-check.
The function return value is only bounded to if the page has private bit
or not, so here we can safely continue.
Nik is right on this, we need better eb refs handling refactor, I'll
investigate more time to make the eb refs handling better.
Thanks,
Qu
> + spin_lock(&eb->refs_lock);
> + if (atomic_read(&eb->refs) != 1 || extent_buffer_under_io(eb) ||
> + !test_and_clear_bit(EXTENT_BUFFER_TREE_REF, &eb->bflags)) {
> + spin_unlock(&eb->refs_lock);
> + rcu_read_unlock();
> + continue;
> + }
> + rcu_read_unlock();
> + spin_unlock_irqrestore(&subpage->lock, flags);
> + /*
> + * Here we don't care the return value, we will always check
> + * the page private at the end.
> + * And release_extent_buffer() will release the refs_lock.
> + */
> + release_extent_buffer(eb);
> + }
> + /* Finally to check if we have cleared page private */
> + spin_lock(&page->mapping->private_lock);
> + if (!PagePrivate(page))
> + ret = 1;
> + else
> + ret = 0;
> + spin_unlock(&page->mapping->private_lock);
> + return ret;
> +
> +}
> +
> int try_release_extent_buffer(struct page *page)
> {
> struct extent_buffer *eb;
>
> + if (btrfs_sb(page->mapping->host->i_sb)->sectorsize < PAGE_SIZE)
> + return try_release_subpage_extent_buffer(page);
> +
> /*
> * We need to make sure nobody is attaching this page to an eb right
> * now.
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3 16/22] btrfs: extent_io: implement try_release_extent_buffer() for subpage metadata support
2021-01-06 8:24 ` Qu Wenruo
@ 2021-01-06 8:43 ` Qu Wenruo
0 siblings, 0 replies; 4+ messages in thread
From: Qu Wenruo @ 2021-01-06 8:43 UTC (permalink / raw)
To: linux-btrfs
On 2021/1/6 下午4:24, Qu Wenruo wrote:
>
>
> On 2021/1/6 上午9:01, Qu Wenruo wrote:
>> Unlike the original try_release_extent_buffer,
>> try_release_subpage_extent_buffer() will iterate through
>> btrfs_subpage::tree_block_bitmap, and try to release each extent buffer.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> fs/btrfs/extent_io.c | 76 ++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 76 insertions(+)
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index 194cb8b63216..792264f5c3c2 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -6258,10 +6258,86 @@ void memmove_extent_buffer(const struct
>> extent_buffer *dst,
>> }
>> }
>> +static int try_release_subpage_extent_buffer(struct page *page)
>> +{
>> + struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
>> + u64 page_start = page_offset(page);
>> + int bitmap_size = BTRFS_SUBPAGE_BITMAP_SIZE;
>> + int bit_start = 0;
>> + int ret;
>> +
>> + while (bit_start < bitmap_size) {
>> + struct btrfs_subpage *subpage;
>> + struct extent_buffer *eb;
>> + unsigned long flags;
>> + u16 tmp = 1 << bit_start;
>> + u64 start;
>> +
>> + /*
>> + * Make sure the page still has private, as previous iteration
>> + * can detach page private.
>> + */
>> + spin_lock(&page->mapping->private_lock);
>> + if (!PagePrivate(page)) {
>> + spin_unlock(&page->mapping->private_lock);
>> + break;
>> + }
>> +
>> + subpage = (struct btrfs_subpage *)page->private;
>> +
>> + spin_lock_irqsave(&subpage->lock, flags);
>> + spin_unlock(&page->mapping->private_lock);
>> +
>> + if (!(tmp & subpage->tree_block_bitmap)) {
>> + spin_unlock_irqrestore(&subpage->lock, flags);
>> + bit_start++;
>> + continue;
>> + }
>> +
>> + start = bit_start * fs_info->sectorsize + page_start;
>> + bit_start += fs_info->nodesize >> fs_info->sectorsize_bits;
>> + /*
>> + * Here we can't call find_extent_buffer() which will increase
>> + * eb->refs.
>> + */
>> + rcu_read_lock();
>> + eb = radix_tree_lookup(&fs_info->buffer_radix,
>> + start >> fs_info->sectorsize_bits);
>> + ASSERT(eb);
>
> Another ASSERT() hit here. Surprised that I have never hit such case
> before.
>
> Since in releasse_extent_buffer(), radix tree is removed first, then
> subpage tree_block_bitmap update, we could have cases where subpage
> tree_block_bitmap is set but no eb in radix tree.
>
> In that case, we can safely go next bit and re-check.
>
> The function return value is only bounded to if the page has private bit
> or not, so here we can safely continue.
>
> Nik is right on this, we need better eb refs handling refactor, I'll
> investigate more time to make the eb refs handling better.
The root problem here is, we have too many things to be synchronized for
extent buffer.
We have eb::refs, eb::bflags (IN_TREE), buffer_radix, and the new
subpage::tree_block_bitmap, they all need to be in sync with each other.
I'm wondering if we could find a good and clear enough way to handle
extent buffers.
IMHO, we need to sacrifice some metadata performance (which is already
poor enough), or there is really no better way to solve the mess...
Thanks,
Qu
>
> Thanks,
> Qu
>> + spin_lock(&eb->refs_lock);
>> + if (atomic_read(&eb->refs) != 1 || extent_buffer_under_io(eb) ||
>> + !test_and_clear_bit(EXTENT_BUFFER_TREE_REF, &eb->bflags)) {
>> + spin_unlock(&eb->refs_lock);
>> + rcu_read_unlock();
>> + continue;
>> + }
>> + rcu_read_unlock();
>> + spin_unlock_irqrestore(&subpage->lock, flags);
>> + /*
>> + * Here we don't care the return value, we will always check
>> + * the page private at the end.
>> + * And release_extent_buffer() will release the refs_lock.
>> + */
>> + release_extent_buffer(eb);
>> + }
>> + /* Finally to check if we have cleared page private */
>> + spin_lock(&page->mapping->private_lock);
>> + if (!PagePrivate(page))
>> + ret = 1;
>> + else
>> + ret = 0;
>> + spin_unlock(&page->mapping->private_lock);
>> + return ret;
>> +
>> +}
>> +
>> int try_release_extent_buffer(struct page *page)
>> {
>> struct extent_buffer *eb;
>> + if (btrfs_sb(page->mapping->host->i_sb)->sectorsize < PAGE_SIZE)
>> + return try_release_subpage_extent_buffer(page);
>> +
>> /*
>> * We need to make sure nobody is attaching this page to an eb
>> right
>> * now.
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-01-06 8:45 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-06 3:59 [PATCH v3 16/22] btrfs: extent_io: implement try_release_extent_buffer() for subpage metadata support kernel test robot
-- strict thread matches above, loose matches on Subject: below --
2021-01-06 1:01 [PATCH v3 00/22] btrfs: add read-only support for subpage sector size Qu Wenruo
2021-01-06 1:01 ` [PATCH v3 16/22] btrfs: extent_io: implement try_release_extent_buffer() for subpage metadata support Qu Wenruo
2021-01-06 8:24 ` Qu Wenruo
2021-01-06 8:43 ` Qu Wenruo
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.