Re: [ISSUE] split_folio() and dirty IOMAP folios

[David Hildenbrand <david@redhat.com>]

On 11.11.24 16:19, David Hildenbrand wrote:
> On 08.11.24 10:11, David Hildenbrand wrote:
>> On 07.11.24 21:20, Matthew Wilcox wrote:
>>> On Thu, Nov 07, 2024 at 05:34:40PM +0100, David Hildenbrand wrote:
>>>> On 07.11.24 17:09, Matthew Wilcox wrote:
>>>>> On Thu, Nov 07, 2024 at 04:07:08PM +0100, David Hildenbrand wrote:
>>>>>> I'm debugging an interesting problem: split_folio() will fail on dirty
>>>>>> folios on XFS, and I am not sure who will trigger the writeback in a timely
>>>>>> manner so code relying on the split to work at some point (in sane setups
>>>>>> where page pinning is not applicable) can make progress.
>>>>>
>>>>> You could call something like filemap_write_and_wait_range()?
>>>>
>>>> Thanks, have to look into some details of that.
>>>>
>>>> Looks like the folio_clear_dirty_for_io() is buried in
>>>> folio_prepare_writeback(), so that part is taken care of.
>>>>
>>>> Guess I have to fo from folio to "mapping,lstart,lend" such that
>>>> __filemap_fdatawrite_range() would look up the folio again. Sounds doable.
>>>>
>>>> (I assume I have to drop the folio lock+reference before calling that)
>>>
>>> I was thinking you'd do it higher in the callchain than
>>> gmap_make_secure().  Presumably userspace says "I want to make this
>>> 256MB range secure" and we can start by writing back that entire
>>> 256MB chunk of address space.
>>>
>>> That doesn't prevent anybody from dirtying it in-between, of course,
>>> so you can still get -EBUSY and have to loop round again.
>>
>> I'm afraid that won't really work.
>>
>> On the one hand, we might be allocating these pages (+disk blocks)
>> during the unpack operation -- where we essentially trigger page faults
>> first using gmap_fault() -- so the pages might not even exist before the
>> gmap_make_secure() during unpack. One work around would be to
>> preallocate+writeback from user space, but it doesn't sound quite right.
>>
>> But the bigger problem I see is that the initial "unpack" operation is
>> not the only case where we trigger this conversion to "secure" state.
>> Once the VM is running, we can see calls on arbitrary guest memory even
>> during page faults, when gmap_make_secure() is called via
>> gmap_convert_to_secure().
>>
>>
>> I'm still not sure why we see essentially no progress being made, even
>> though we temporarily drop the PTL, mmap lock, folio lock, folio ref ...
>> maybe related to us triggering a write fault that somehow ends up
>> setting the folio dirty :/ Or because writeback is simply too slow /
>> backs off.
>>
>> I'll play with handling -EBUSY from split_folio() differently: if the
>> folio is under writeback, wait on that. If the folio is dirty, trigger
>> writeback. And I'll look into whether we really need a writable PTE, I
>> suspect not, because we are not actually "modifying" page content.
> 
> The following hack makes it fly:
> 
>           case -E2BIG:
>                   folio_lock(folio);
>                   rc = split_folio(folio);
> +               if (rc == -EBUSY) {
> +                       if (folio_test_dirty(folio) && !folio_test_anon(folio) &&
> +                           folio->mapping) {
> +                               struct address_space *mapping = folio->mapping;
> +                               loff_t lstart = folio_pos(folio);
> +                               loff_t lend = lstart + folio_size(folio);
> +
> +                               folio_unlock(folio);
> +                               /* Mapping can go away ... */
> +                               filemap_write_and_wait_range(mapping, lstart, lend);
> +                       } else {
> +                               folio_unlock(folio);
> +                       }
> +                       folio_wait_writeback(folio);
> +                       folio_lock(folio);
> +                       split_folio(folio);
> +                       folio_unlock(folio);
> +                       folio_put(folio);
> +                       return -EAGAIN;
> +               }
>                   folio_unlock(folio);
>                   folio_put(folio);
> 
> 
> I think the reason why we don't make any progress on s390x is that the writeback will
> mark the folio clean and turn the folio read-only in the page tables as well. So when we
> lookup the folio again in the page table, we see that the PTE is not writable and
> trigger a write fault ...
> 
> ... the write fault will mark the folio dirty again, so the split will never succeed.
> 
> In above diff, we really must try the split_folio() a second time after waiting, otherwise we
> run into the same endless loop.
> 
> 
> I'm still not 100% sure if we need a writable PTE; after all we are not modifying page content.
> But that's just a side effect of not being able to wait for the split_folio() to make progress
> in the writeback case so we can retry the split again.

After discussing this with Darrick and Willy yesterday, I think the 
reason we need a writable PTE is because we *might* modify page content:

"Requests the Ultravisor to make a page accessible to a guest. If it's 
brought in the first time, it will be cleared. If it has been exported 
before, it will be decrypted and integrity checked."

So we'll be effectively modifying the page content we will read when the 
(now secure) page is in the unprotected/exported state.

That makes things more complicated, unfortunately :)

-- 
Cheers,

David / dhildenb