Re: [PATCH] btrfs: remove btrfs_writepage_cow_fixup

[Christoph Hellwig <hch@lst.de>]

On Fri, Jun 24, 2022 at 08:30:00PM +0800, Qu Wenruo wrote:
> But from my previous feedback on subpage code, it looks like it's some
> hardware archs (S390?) that can not do page flags update atomically.
>
> I have tested similar thing, with extra ASSERT() to make sure the cow
> fixup code never get triggered.
>
> At least for x86_64 and aarch64 it's OK here.
>
> So I hope this time we can get a concrete reason on why we need the
> extra page Private2 bit in the first place.

I don't think atomic page flags are a thing here.  I remember Jan
had chased a bug where we'd get into trouble into this area in
ext4 due to the way pages are locked down for direct I/O, but I
don't even remember seeing that on XFS.  Either way the PageOrdered
check prevents a crash in that case and we really can't expect
data to properly be written back in that case.

>
> Thanks,
> Qu
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> ---
>>   fs/btrfs/ctree.h     |   8 --
>>   fs/btrfs/disk-io.c   |   6 +-
>>   fs/btrfs/extent_io.c |   9 +--
>>   fs/btrfs/inode.c     | 188 -------------------------------------------
>>   4 files changed, 3 insertions(+), 208 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>> index 12f59e35755fa..39a1235eda48b 100644
>> --- a/fs/btrfs/ctree.h
>> +++ b/fs/btrfs/ctree.h
>> @@ -882,13 +882,6 @@ struct btrfs_fs_info {
>>   	struct btrfs_workqueue *endio_write_workers;
>>   	struct btrfs_workqueue *endio_freespace_worker;
>>   	struct btrfs_workqueue *caching_workers;
>> -
>> -	/*
>> -	 * fixup workers take dirty pages that didn't properly go through
>> -	 * the cow mechanism and make them safe to write.  It happens
>> -	 * for the sys_munmap function call path
>> -	 */
>> -	struct btrfs_workqueue *fixup_workers;
>>   	struct btrfs_workqueue *delayed_workers;
>>
>>   	struct task_struct *transaction_kthread;
>> @@ -3390,7 +3383,6 @@ int btrfs_prealloc_file_range_trans(struct inode *inode,
>>   int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page,
>>   		u64 start, u64 end, int *page_started, unsigned long *nr_written,
>>   		struct writeback_control *wbc);
>> -int btrfs_writepage_cow_fixup(struct page *page);
>>   void btrfs_writepage_endio_finish_ordered(struct btrfs_inode *inode,
>>   					  struct page *page, u64 start,
>>   					  u64 end, bool uptodate);
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 71d92f5dfcb2e..bf908e178ee59 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -2163,7 +2163,6 @@ static int read_backup_root(struct btrfs_fs_info *fs_info, u8 priority)
>>   /* helper to cleanup workers */
>>   static void btrfs_stop_all_workers(struct btrfs_fs_info *fs_info)
>>   {
>> -	btrfs_destroy_workqueue(fs_info->fixup_workers);
>>   	btrfs_destroy_workqueue(fs_info->delalloc_workers);
>>   	btrfs_destroy_workqueue(fs_info->hipri_workers);
>>   	btrfs_destroy_workqueue(fs_info->workers);
>> @@ -2358,9 +2357,6 @@ static int btrfs_init_workqueues(struct btrfs_fs_info *fs_info)
>>   	fs_info->caching_workers =
>>   		btrfs_alloc_workqueue(fs_info, "cache", flags, max_active, 0);
>>
>> -	fs_info->fixup_workers =
>> -		btrfs_alloc_workqueue(fs_info, "fixup", flags, 1, 0);
>> -
>>   	fs_info->endio_workers =
>>   		alloc_workqueue("btrfs-endio", flags, max_active);
>>   	fs_info->endio_meta_workers =
>> @@ -2390,7 +2386,7 @@ static int btrfs_init_workqueues(struct btrfs_fs_info *fs_info)
>>   	      fs_info->compressed_write_workers &&
>>   	      fs_info->endio_write_workers && fs_info->endio_raid56_workers &&
>>   	      fs_info->endio_freespace_worker && fs_info->rmw_workers &&
>> -	      fs_info->caching_workers && fs_info->fixup_workers &&
>> +	      fs_info->caching_workers &&
>>   	      fs_info->delayed_workers && fs_info->qgroup_rescan_workers &&
>>   	      fs_info->discard_ctl.discard_workers)) {
>>   		return -ENOMEM;
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index 587d2ba20b53b..232a858b99a0a 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -3944,13 +3944,8 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
>>   	bool has_error = false;
>>   	bool compressed;
>>
>> -	ret = btrfs_writepage_cow_fixup(page);
>> -	if (ret) {
>> -		/* Fixup worker will requeue */
>> -		redirty_page_for_writepage(wbc, page);
>> -		unlock_page(page);
>> -		return 1;
>> -	}
>> +	if (WARN_ON_ONCE(!PageOrdered(page)))
>> +		return -EIO;
>>
>>   	/*
>>   	 * we don't want to touch the inode after unlocking the page,
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index eea351216db33..5cf314a1b5d17 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -2815,194 +2815,6 @@ int btrfs_set_extent_delalloc(struct btrfs_inode *inode, u64 start, u64 end,
>>   				   cached_state);
>>   }
>>
>> -/* see btrfs_writepage_start_hook for details on why this is required */
>> -struct btrfs_writepage_fixup {
>> -	struct page *page;
>> -	struct inode *inode;
>> -	struct btrfs_work work;
>> -};
>> -
>> -static void btrfs_writepage_fixup_worker(struct btrfs_work *work)
>> -{
>> -	struct btrfs_writepage_fixup *fixup;
>> -	struct btrfs_ordered_extent *ordered;
>> -	struct extent_state *cached_state = NULL;
>> -	struct extent_changeset *data_reserved = NULL;
>> -	struct page *page;
>> -	struct btrfs_inode *inode;
>> -	u64 page_start;
>> -	u64 page_end;
>> -	int ret = 0;
>> -	bool free_delalloc_space = true;
>> -
>> -	fixup = container_of(work, struct btrfs_writepage_fixup, work);
>> -	page = fixup->page;
>> -	inode = BTRFS_I(fixup->inode);
>> -	page_start = page_offset(page);
>> -	page_end = page_offset(page) + PAGE_SIZE - 1;
>> -
>> -	/*
>> -	 * This is similar to page_mkwrite, we need to reserve the space before
>> -	 * we take the page lock.
>> -	 */
>> -	ret = btrfs_delalloc_reserve_space(inode, &data_reserved, page_start,
>> -					   PAGE_SIZE);
>> -again:
>> -	lock_page(page);
>> -
>> -	/*
>> -	 * Before we queued this fixup, we took a reference on the page.
>> -	 * page->mapping may go NULL, but it shouldn't be moved to a different
>> -	 * address space.
>> -	 */
>> -	if (!page->mapping || !PageDirty(page) || !PageChecked(page)) {
>> -		/*
>> -		 * Unfortunately this is a little tricky, either
>> -		 *
>> -		 * 1) We got here and our page had already been dealt with and
>> -		 *    we reserved our space, thus ret == 0, so we need to just
>> -		 *    drop our space reservation and bail.  This can happen the
>> -		 *    first time we come into the fixup worker, or could happen
>> -		 *    while waiting for the ordered extent.
>> -		 * 2) Our page was already dealt with, but we happened to get an
>> -		 *    ENOSPC above from the btrfs_delalloc_reserve_space.  In
>> -		 *    this case we obviously don't have anything to release, but
>> -		 *    because the page was already dealt with we don't want to
>> -		 *    mark the page with an error, so make sure we're resetting
>> -		 *    ret to 0.  This is why we have this check _before_ the ret
>> -		 *    check, because we do not want to have a surprise ENOSPC
>> -		 *    when the page was already properly dealt with.
>> -		 */
>> -		if (!ret) {
>> -			btrfs_delalloc_release_extents(inode, PAGE_SIZE);
>> -			btrfs_delalloc_release_space(inode, data_reserved,
>> -						     page_start, PAGE_SIZE,
>> -						     true);
>> -		}
>> -		ret = 0;
>> -		goto out_page;
>> -	}
>> -
>> -	/*
>> -	 * We can't mess with the page state unless it is locked, so now that
>> -	 * it is locked bail if we failed to make our space reservation.
>> -	 */
>> -	if (ret)
>> -		goto out_page;
>> -
>> -	lock_extent_bits(&inode->io_tree, page_start, page_end, &cached_state);
>> -
>> -	/* already ordered? We're done */
>> -	if (PageOrdered(page))
>> -		goto out_reserved;
>> -
>> -	ordered = btrfs_lookup_ordered_range(inode, page_start, PAGE_SIZE);
>> -	if (ordered) {
>> -		unlock_extent_cached(&inode->io_tree, page_start, page_end,
>> -				     &cached_state);
>> -		unlock_page(page);
>> -		btrfs_start_ordered_extent(ordered, 1);
>> -		btrfs_put_ordered_extent(ordered);
>> -		goto again;
>> -	}
>> -
>> -	ret = btrfs_set_extent_delalloc(inode, page_start, page_end, 0,
>> -					&cached_state);
>> -	if (ret)
>> -		goto out_reserved;
>> -
>> -	/*
>> -	 * Everything went as planned, we're now the owner of a dirty page with
>> -	 * delayed allocation bits set and space reserved for our COW
>> -	 * destination.
>> -	 *
>> -	 * The page was dirty when we started, nothing should have cleaned it.
>> -	 */
>> -	BUG_ON(!PageDirty(page));
>> -	free_delalloc_space = false;
>> -out_reserved:
>> -	btrfs_delalloc_release_extents(inode, PAGE_SIZE);
>> -	if (free_delalloc_space)
>> -		btrfs_delalloc_release_space(inode, data_reserved, page_start,
>> -					     PAGE_SIZE, true);
>> -	unlock_extent_cached(&inode->io_tree, page_start, page_end,
>> -			     &cached_state);
>> -out_page:
>> -	if (ret) {
>> -		/*
>> -		 * We hit ENOSPC or other errors.  Update the mapping and page
>> -		 * to reflect the errors and clean the page.
>> -		 */
>> -		mapping_set_error(page->mapping, ret);
>> -		end_extent_writepage(page, ret, page_start, page_end);
>> -		clear_page_dirty_for_io(page);
>> -		SetPageError(page);
>> -	}
>> -	btrfs_page_clear_checked(inode->root->fs_info, page, page_start, PAGE_SIZE);
>> -	unlock_page(page);
>> -	put_page(page);
>> -	kfree(fixup);
>> -	extent_changeset_free(data_reserved);
>> -	/*
>> -	 * As a precaution, do a delayed iput in case it would be the last iput
>> -	 * that could need flushing space. Recursing back to fixup worker would
>> -	 * deadlock.
>> -	 */
>> -	btrfs_add_delayed_iput(&inode->vfs_inode);
>> -}
>> -
>> -/*
>> - * There are a few paths in the higher layers of the kernel that directly
>> - * set the page dirty bit without asking the filesystem if it is a
>> - * good idea.  This causes problems because we want to make sure COW
>> - * properly happens and the data=ordered rules are followed.
>> - *
>> - * In our case any range that doesn't have the ORDERED bit set
>> - * hasn't been properly setup for IO.  We kick off an async process
>> - * to fix it up.  The async helper will wait for ordered extents, set
>> - * the delalloc bit and make it safe to write the page.
>> - */
>> -int btrfs_writepage_cow_fixup(struct page *page)
>> -{
>> -	struct inode *inode = page->mapping->host;
>> -	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>> -	struct btrfs_writepage_fixup *fixup;
>> -
>> -	/* This page has ordered extent covering it already */
>> -	if (PageOrdered(page))
>> -		return 0;
>> -
>> -	/*
>> -	 * PageChecked is set below when we create a fixup worker for this page,
>> -	 * don't try to create another one if we're already PageChecked()
>> -	 *
>> -	 * The extent_io writepage code will redirty the page if we send back
>> -	 * EAGAIN.
>> -	 */
>> -	if (PageChecked(page))
>> -		return -EAGAIN;
>> -
>> -	fixup = kzalloc(sizeof(*fixup), GFP_NOFS);
>> -	if (!fixup)
>> -		return -EAGAIN;
>> -
>> -	/*
>> -	 * We are already holding a reference to this inode from
>> -	 * write_cache_pages.  We need to hold it because the space reservation
>> -	 * takes place outside of the page lock, and we can't trust
>> -	 * page->mapping outside of the page lock.
>> -	 */
>> -	ihold(inode);
>> -	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;
>> -	fixup->inode = inode;
>> -	btrfs_queue_work(fs_info->fixup_workers, &fixup->work);
>> -
>> -	return -EAGAIN;
>> -}
>> -
>>   static int insert_reserved_file_extent(struct btrfs_trans_handle *trans,
>>   				       struct btrfs_inode *inode, u64 file_pos,
>>   				       struct btrfs_file_extent_item *stack_fi,
---end quoted text---