Skip to content

Commit b595d25

Browse files
josefbacikkdave
authored andcommitted
btrfs: don't clear uptodate on write errors
We have been consistently seeing hangs with generic/648 in our subpage GitHub CI setup. This is a classic deadlock, we are calling btrfs_read_folio() on a folio, which requires holding the folio lock on the folio, and then finding a ordered extent that overlaps that range and calling btrfs_start_ordered_extent(), which then tries to write out the dirty page, which requires taking the folio lock and then we deadlock. The hang happens because we're writing to range [1271750656, 1271767040), page index [77621, 77622], and page 77621 is !Uptodate. It is also Dirty, so we call btrfs_read_folio() for 77621 and which does btrfs_lock_and_flush_ordered_range() for that range, and we find an ordered extent which is [1271644160, 1271746560), page index [77615, 77621]. The page indexes overlap, but the actual bytes don't overlap. We're holding the page lock for 77621, then call btrfs_lock_and_flush_ordered_range() which tries to flush the dirty page, and tries to lock 77621 again and then we deadlock. The byte ranges do not overlap, but with subpage support if we clear uptodate on any portion of the page we mark the entire thing as not uptodate. We have been clearing page uptodate on write errors, but no other file system does this, and is in fact incorrect. This doesn't hurt us in the !subpage case because we can't end up with overlapped ranges that don't also overlap on the page. Fix this by not clearing uptodate when we have a write error. The only thing we should be doing in this case is setting the mapping error and carrying on. This makes it so we would no longer call btrfs_read_folio() on the page as it's uptodate and eliminates the deadlock. With this patch we're now able to make it through a full fstests run on our subpage blocksize VMs. Note for stable backports: this probably goes beyond 6.1 but the code has been cleaned up and clearing the uptodate bit must be verified on each version independently. CC: stable@vger.kernel.org # 6.1+ Reviewed-by: Qu Wenruo <wqu@suse.com> Signed-off-by: Josef Bacik <josef@toxicpanda.com> Signed-off-by: David Sterba <dsterba@suse.com>
1 parent 9af8669 commit b595d25

2 files changed

Lines changed: 1 addition & 12 deletions

File tree

fs/btrfs/extent_io.c

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -484,10 +484,8 @@ static void end_bio_extent_writepage(struct btrfs_bio *bbio)
484484
bvec->bv_offset, bvec->bv_len);
485485

486486
btrfs_finish_ordered_extent(bbio->ordered, page, start, len, !error);
487-
if (error) {
488-
btrfs_page_clear_uptodate(fs_info, page, start, len);
487+
if (error)
489488
mapping_set_error(page->mapping, error);
490-
}
491489
btrfs_page_clear_writeback(fs_info, page, start, len);
492490
}
493491

@@ -1456,8 +1454,6 @@ static int __extent_writepage(struct page *page, struct btrfs_bio_ctrl *bio_ctrl
14561454
if (ret) {
14571455
btrfs_mark_ordered_io_finished(BTRFS_I(inode), page, page_start,
14581456
PAGE_SIZE, !ret);
1459-
btrfs_page_clear_uptodate(btrfs_sb(inode->i_sb), page,
1460-
page_start, PAGE_SIZE);
14611457
mapping_set_error(page->mapping, ret);
14621458
}
14631459
unlock_page(page);
@@ -1624,8 +1620,6 @@ static void extent_buffer_write_end_io(struct btrfs_bio *bbio)
16241620
struct page *page = bvec->bv_page;
16251621
u32 len = bvec->bv_len;
16261622

1627-
if (!uptodate)
1628-
btrfs_page_clear_uptodate(fs_info, page, start, len);
16291623
btrfs_page_clear_writeback(fs_info, page, start, len);
16301624
bio_offset += len;
16311625
}
@@ -2201,7 +2195,6 @@ void extent_write_locked_range(struct inode *inode, struct page *locked_page,
22012195
if (ret) {
22022196
btrfs_mark_ordered_io_finished(BTRFS_I(inode), page,
22032197
cur, cur_len, !ret);
2204-
btrfs_page_clear_uptodate(fs_info, page, cur, cur_len);
22052198
mapping_set_error(page->mapping, ret);
22062199
}
22072200
btrfs_page_unlock_writer(fs_info, page, cur, cur_len);

fs/btrfs/inode.c

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1085,9 +1085,6 @@ static void submit_uncompressed_range(struct btrfs_inode *inode,
10851085
btrfs_mark_ordered_io_finished(inode, locked_page,
10861086
page_start, PAGE_SIZE,
10871087
!ret);
1088-
btrfs_page_clear_uptodate(inode->root->fs_info,
1089-
locked_page, page_start,
1090-
PAGE_SIZE);
10911088
mapping_set_error(locked_page->mapping, ret);
10921089
unlock_page(locked_page);
10931090
}
@@ -2791,7 +2788,6 @@ static void btrfs_writepage_fixup_worker(struct btrfs_work *work)
27912788
mapping_set_error(page->mapping, ret);
27922789
btrfs_mark_ordered_io_finished(inode, page, page_start,
27932790
PAGE_SIZE, !ret);
2794-
btrfs_page_clear_uptodate(fs_info, page, page_start, PAGE_SIZE);
27952791
clear_page_dirty_for_io(page);
27962792
}
27972793
btrfs_page_clear_checked(fs_info, page, page_start, PAGE_SIZE);

0 commit comments

Comments
 (0)