Skip to content

Commit c992fa1

Browse files
adam900710kdave
authored andcommitted
btrfs: subpage: fix a wrong check on subpage->writers
[BUG] When looping btrfs/074 with 64K page size and 4K sectorsize, there is a low chance (1/50~1/100) to crash with the following ASSERT() triggered in btrfs_subpage_start_writer(): ret = atomic_add_return(nbits, &subpage->writers); ASSERT(ret == nbits); <<< This one <<< [CAUSE] With more debugging output on the parameters of btrfs_subpage_start_writer(), it shows a very concerning error: ret=29 nbits=13 start=393216 len=53248 For @nbits it's correct, but @ret which is the returned value from atomic_add_return(), it's not only larger than nbits, but also larger than max sectors per page value (for 64K page size and 4K sector size, it's 16). This indicates that some call sites are not properly decreasing the value. And that's exactly the case, in btrfs_page_unlock_writer(), due to the fact that we can have page locked either by lock_page() or process_one_page(), we have to check if the subpage has any writer. If no writers, it's locked by lock_page() and we only need to unlock it. But unfortunately the check for the writers are completely opposite: if (atomic_read(&subpage->writers)) /* No writers, locked by plain lock_page() */ return unlock_page(page); We directly unlock the page if it has writers, which is the completely opposite what we want. Thankfully the affected call site is only limited to extent_write_locked_range(), so it's mostly affecting compressed write. [FIX] Just fix the wrong check condition to fix the bug. Fixes: e55a0de ("btrfs: rework page locking in __extent_writepage()") CC: [email protected] # 5.16 Signed-off-by: Qu Wenruo <[email protected]> Signed-off-by: David Sterba <[email protected]>
1 parent 558732d commit c992fa1

File tree

1 file changed

+1
-1
lines changed

1 file changed

+1
-1
lines changed

fs/btrfs/subpage.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -736,7 +736,7 @@ void btrfs_page_unlock_writer(struct btrfs_fs_info *fs_info, struct page *page,
736736
* Since we own the page lock, no one else could touch subpage::writers
737737
* and we are safe to do several atomic operations without spinlock.
738738
*/
739-
if (atomic_read(&subpage->writers))
739+
if (atomic_read(&subpage->writers) == 0)
740740
/* No writers, locked by plain lock_page() */
741741
return unlock_page(page);
742742

0 commit comments

Comments
 (0)