Re: [PATCH v2 03/19] fs: Add aops->migrate_folio

David Hildenbrand
2022-06-10T10:17:43Z

On 09.06.22 16:35, Matthew Wilcox wrote:
> On Thu, Jun 09, 2022 at 02:50:20PM +0200, David Hildenbrand wrote:
>> On 08.06.22 17:02, Matthew Wilcox (Oracle) wrote:
>>> diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
>>> index c0fe711f14d3..3d28b23676bd 100644
>>> --- a/Documentation/filesystems/locking.rst
>>> +++ b/Documentation/filesystems/locking.rst
>>> @@ -253,7 +253,8 @@ prototypes::
>>> void (*free_folio)(struct folio *);
>>> int (*direct_IO)(struct kiocb *, struct iov_iter *iter);
>>> bool (*isolate_page) (struct page *, isolate_mode_t);
>>> - int (*migratepage)(struct address_space *, struct page *, struct page *);
>>> + int (*migrate_folio)(struct address_space *, struct folio *dst,
>>> + struct folio *src, enum migrate_mode);
>>> void (*putback_page) (struct page *);
>>
>> isolate_page/putback_page are leftovers from the previous patch, no?
>
> Argh, right, I completely forgot I needed to update the documentation in
> that patch.
>
>>> +++ b/Documentation/vm/page_migration.rst
>>> @@ -181,22 +181,23 @@ which are function pointers of struct address_space_operations.
>>> Once page is successfully isolated, VM uses page.lru fields so driver
>>> shouldn't expect to preserve values in those fields.
>>>
>>> -2. ``int (*migratepage) (struct address_space *mapping,``
>>> -| ``struct page *newpage, struct page *oldpage, enum migrate_mode);``
>>> -
>>> - After isolation, VM calls migratepage() of driver with the isolated page.
>>> - The function of migratepage() is to move the contents of the old page to the
>>> - new page
>>> - and set up fields of struct page newpage. Keep in mind that you should
>>> - indicate to the VM the oldpage is no longer movable via __ClearPageMovable()
>>> - under page_lock if you migrated the oldpage successfully and returned
>>> - MIGRATEPAGE_SUCCESS. If driver cannot migrate the page at the moment, driver
>>> - can return -EAGAIN. On -EAGAIN, VM will retry page migration in a short time
>>> - because VM interprets -EAGAIN as "temporary migration failure". On returning
>>> - any error except -EAGAIN, VM will give up the page migration without
>>> - retrying.
>>> -
>>> - Driver shouldn't touch the page.lru field while in the migratepage() function.
>>> +2. ``int (*migrate_folio) (struct address_space *mapping,``
>>> +| ``struct folio *dst, struct folio *src, enum migrate_mode);``
>>> +
>>> + After isolation, VM calls the driver's migrate_folio() with the
>>> + isolated folio. The purpose of migrate_folio() is to move the contents
>>> + of the source folio to the destination folio and set up the fields
>>> + of destination folio. Keep in mind that you should indicate to the
>>> + VM the source folio is no longer movable via __ClearPageMovable()
>>> + under folio if you migrated the source successfully and returned
>>> + MIGRATEPAGE_SUCCESS. If driver cannot migrate the folio at the
>>> + moment, driver can return -EAGAIN. On -EAGAIN, VM will retry folio
>>> + migration in a short time because VM interprets -EAGAIN as "temporary
>>> + migration failure". On returning any error except -EAGAIN, VM will
>>> + give up the folio migration without retrying.
>>> +
>>> + Driver shouldn't touch the folio.lru field while in the migrate_folio()
>>> + function.
>>>
>>> 3. ``void (*putback_page)(struct page *);``
>>
>> Hmm, here it's a bit more complicated now, because we essentially have
>> two paths: LRU+migrate_folio or !LRU+movable_ops
>> (isolate/migrate/putback page)
>
> Oh ... actually, this is just documenting the driver side of things.
> I don't really like how it's written. Here, have some rewritten
> documentation (which is now part of the previous patch):
>

LGTM, thanks.

--
Thanks,

David / dhildenb