Re: [PATCH 3/6] iomap: Check iblocksize before transforming page->private

[Nikolay Borisov <nborisov@suse.com>]

On 26.06.19 г. 6:03 ч., Goldwyn Rodrigues wrote:
> On 21:04 25/06, Filipe Manana wrote:
>> On Tue, Jun 25, 2019 at 8:58 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>>>
>>> On  9:05 24/06, Christoph Hellwig wrote:
>>>> On Fri, Jun 21, 2019 at 02:28:25PM -0500, Goldwyn Rodrigues wrote:
>>>>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>>>>
>>>>> btrfs uses page->private as well to store extent_buffer. Make
>>>>> the check stricter to make sure we are using page->private for iop by
>>>>> comparing iblocksize < PAGE_SIZE.
>>>>>
>>>>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>>>
>>>> If btrfs uses page->private itself and also uses functions that call
>>>> to_iomap_page we have a major problem, as we now have a usage conflict.
>>>>
>>>> How do you end up here?
>>>>
>>>
>>> Btrfs uses page->private to identify which extent_buffer it belongs to.
>>> So, if you read, it fills the page->private. Then you try to write to
>>> it, iomap will assume it to be iomap_page pointer.
>>>
>>> I don't think we can move extent_buffer out of page->private for btrfs.
>>> Any other ideas?
>>
>> The extent buffer is only for pages belonging to the btree inode (i.e.
>> pages that correspond to a btree node/lead).
>> Haven't looked in detail to this patchset, but you can't do buffered
>> writes or direct IO against the btree inode, can you?
>> So for file inodes, this problem doesn't exist.
> 
> Why do we call set_page_extent_mapped(page) in lock_and_cleanup_extent_if_needed() or __do_readpage()?
> 
> I must admit, the backtrace crashes that I saw had the page->private set to
> EXTENT_PAGE_PRIVATE rather than the extent_buffer pointer. Does that mean
> calling this function is not necessary in these codepaths?

On a quick look it seems PagePrivate is not used for ordinary data
pages. E.g. set_Page_extent_mapped seems to be a leftover from old code.
No one is actually checking the EXTENT_PAGE_PRIVATE flag. For data pages
what seems to be important is PagePrivate2 for the fixup worker. IMO
set_page_extent_mapped should be removed as well as references to
pageprivate in the context of ordinary data pages.

Chris, any ideas what set_page_extent_mapped was used for?

>