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

[Qu Wenruo <wqu@suse.com>]

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.
>