[PATCH] btrfs: remove the dead copied check in btrfs_copy_from_user()

[PATCH] btrfs: remove the dead copied check in btrfs_copy_from_user()

[Qu Wenruo]

There is btrfs specific check in btrfs_copy_from_user(), after
iov_iter_copy_from_user_atomic() call, we check if the page is uptodate
and if the copied bytes is smaller than what we expect.

However that check will never be triggered due to the following reasons:
- PageUptodate() check conflicts with current behavior
  Currently we ensure all pages that will go through a partial write
  (some bytes are not covered by the write range) will be forced
  uptodate.

  This is the common behavior to ensure we get the correct content.
  This behavior is always true, no matter if my previous patch "btrfs:
  refactor how we prepare pages for btrfs_buffered_write()" is applied.

- iov_iter_copy_from_user_atomic() only returns 0 or @bytes
  It won't return a short write.

So we're completely fine to remove the (PageUptodate() && copied <
count) check, as we either get copied == 0, and break the loop anyway,
or do a proper copy.

This will revert commit 31339acd07b4 ("Btrfs: deal with short returns from
copy_from_user").

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/file.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 705ebe709e8d..2f96f083eb8c 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -403,18 +403,6 @@ static noinline int btrfs_copy_from_user(loff_t pos, size_t write_bytes,
 		/* Flush processor's dcache for this page */
 		flush_dcache_page(page);
 
-		/*
-		 * if we get a partial write, we can end up with
-		 * partially up to date pages.  These add
-		 * a lot of complexity, so make sure they don't
-		 * happen by forcing this copy to be retried.
-		 *
-		 * The rest of the btrfs_file_write code will fall
-		 * back to page at a time copies after we return 0.
-		 */
-		if (!PageUptodate(page) && copied < count)
-			copied = 0;
-
 		iov_iter_advance(i, copied);
 		write_bytes -= copied;
 		total_copied += copied;
-- 
2.28.0

Re: [PATCH] btrfs: remove the dead copied check in btrfs_copy_from_user()

[David Sterba]

On Thu, Aug 13, 2020 at 02:15:33PM +0800, Qu Wenruo wrote:
> There is btrfs specific check in btrfs_copy_from_user(), after
> iov_iter_copy_from_user_atomic() call, we check if the page is uptodate
> and if the copied bytes is smaller than what we expect.
> 
> However that check will never be triggered due to the following reasons:
> - PageUptodate() check conflicts with current behavior
>   Currently we ensure all pages that will go through a partial write
>   (some bytes are not covered by the write range) will be forced
>   uptodate.
> 
>   This is the common behavior to ensure we get the correct content.
>   This behavior is always true, no matter if my previous patch "btrfs:
>   refactor how we prepare pages for btrfs_buffered_write()" is applied.

Would it make sense to add an assert here? Checking for the page
up-to-date status.

> - iov_iter_copy_from_user_atomic() only returns 0 or @bytes
>   It won't return a short write.

And maybe for that one too, I'm not able to navigate through the maze of
the iov_iter_* macros.

> So we're completely fine to remove the (PageUptodate() && copied <
> count) check, as we either get copied == 0, and break the loop anyway,
> or do a proper copy.
> 
> This will revert commit 31339acd07b4 ("Btrfs: deal with short returns from
> copy_from_user").

As this is a very old patch, the changes outside of btrfs are likely to
make the piece of code redundant.

Re: [PATCH] btrfs: remove the dead copied check in btrfs_copy_from_user()

[Qu Wenruo]

On 2020/8/13 下午6:48, David Sterba wrote:
> On Thu, Aug 13, 2020 at 02:15:33PM +0800, Qu Wenruo wrote:
>> There is btrfs specific check in btrfs_copy_from_user(), after
>> iov_iter_copy_from_user_atomic() call, we check if the page is uptodate
>> and if the copied bytes is smaller than what we expect.
>>
>> However that check will never be triggered due to the following reasons:
>> - PageUptodate() check conflicts with current behavior
>>   Currently we ensure all pages that will go through a partial write
>>   (some bytes are not covered by the write range) will be forced
>>   uptodate.
>>
>>   This is the common behavior to ensure we get the correct content.
>>   This behavior is always true, no matter if my previous patch "btrfs:
>>   refactor how we prepare pages for btrfs_buffered_write()" is applied.
> 
> Would it make sense to add an assert here? Checking for the page
> up-to-date status.

We can have page without uptodate bit.

Uptodate is only forced for partial written pages.
For pages that would be completely covered by write range, it doesn't
need to be uptodate.

Thus the ASSERT() may be more complex than what we initially thought,
but may still be feasible.

> 
>> - iov_iter_copy_from_user_atomic() only returns 0 or @bytes
>>   It won't return a short write.
> 
> And maybe for that one too, I'm not able to navigate through the maze of
> the iov_iter_* macros.

Well, after more digging, we have similar check hidden in other location.
In fact fs/buffer.c has its block_write_end() which do the same check there.

So I'm afraid unless we go through the whole maze, it may still be a
problem.

The current theory would be a write in a page, but split into several
different iov, thus we can still write some iov, but fails the
remaining, thus lead the problem.

Please discard this patch.

Thanks,
Qu

> 
>> So we're completely fine to remove the (PageUptodate() && copied <
>> count) check, as we either get copied == 0, and break the loop anyway,
>> or do a proper copy.
>>
>> This will revert commit 31339acd07b4 ("Btrfs: deal with short returns from
>> copy_from_user").
> 
> As this is a very old patch, the changes outside of btrfs are likely to
> make the piece of code redundant.
>