Re: [PATCH 10/10] btrfs: add and use simple page/bio to inode/fs_info helpers

[Qu Wenruo <wqu@suse.com>]

On 2021/7/27 下午4:45, David Sterba wrote:
> On Tue, Jul 27, 2021 at 06:26:47AM +0800, Qu Wenruo wrote:
>> On 2021/7/26 下午11:09, David Sterba wrote:
>>> On Mon, Jul 26, 2021 at 08:41:57PM +0800, Qu Wenruo wrote:
>>>>
>>>>
>>>> On 2021/7/26 下午8:15, David Sterba wrote:
>>>>> We have lots of places where we want to obtain inode from page, fs_info
>>>>> from page and open code the pointer chains.
>>>>
>>>> All those inode/fs_info grabbing from just a page is dangerous.
>>>>
>>>> If an anonymous page is passed in unintentionally, it can easily crash
>>>> the system.
>>>>
>>>> Thus at least some ASSERT() here is a must to me.
>>>
>>> But we can only check if the pointer is valid, any page can have a valid
>>> pointer but not our fs_info. If it crashes on an unexpected page than
>>> what can we do in the code anyway?
>>
>> What I mean is to check page->mapping for the page passed in.
>>
>> Indeed we can't do anything when we hit a page with NULL mapping
>> pointer, but that's a code bug.
>> An ASSERT() would make us developer aware what's going wrong and to fix
>> the bug.
> 
> The assert is a more verbose crash, so that's slightly more developer
> friendly but I'm still not convinced it's worth the assert. Right now
> the macros are not static inlines so they don't need full definitions of
> page and mapping and the other types. Embedding the asserts into macros
> would look like
> 
>    ({ ASSERT(page); ASSERT(page->mapping); page->mapping->host; })

ASSERT(page) is not needed.

ASSERT(page->mapping) is what I think is necessary.

With something like ({ ASSERT(page); page->mapping->host; }), it still 
looks fine to me.

> 
> Or perhaps also page with a temporary variable to avoid multiple
> evaluations.
> 
> The helpers are used in a handful of places, if we really care about
> consistency of the assertions, something like assert_page_ok(page) would
> have to be in each function that gets the page from other subsystems.
> 

The call site that should really be concerned is compression, which 
utilize anonymous pages, along with cached pages.
(Scrub is another case, but scrub never mix cached pages with anonymous, 
thus it's less a concern)

In fact, during subpage compression development, I have already exposed 
several locations where we are trying to pass anonymous page with 
manually populated page->i_mapping.

Thus I'm sensitive to this problem.

So far in your code, it's not touching compression code, thus it's fine 
for now.

But it's just a problem of time before next refactor to use this macros 
for compression code.

Without proper ASSERT(), it's super easy to cause problems IMHO.

Thanks,
Qu