Re: [RFC PATCH V2 0/9] Introduce attach/clear_page_private to cleanup code

Re: [RFC PATCH V2 0/9] Introduce attach/clear_page_private to cleanup code

[Guoqing Jiang]

On 5/2/20 2:41 AM, Matthew Wilcox wrote:
> On Sat, May 02, 2020 at 12:42:15AM +0200, Guoqing Jiang wrote:
>> On 5/2/20 12:16 AM, Matthew Wilcox wrote:
>>> On Thu, Apr 30, 2020 at 11:44:41PM +0200, Guoqing Jiang wrote:
>>>>     include/linux/pagemap.h: introduce attach/clear_page_private
>>>>     md: remove __clear_page_buffers and use attach/clear_page_private
>>>>     btrfs: use attach/clear_page_private
>>>>     fs/buffer.c: use attach/clear_page_private
>>>>     f2fs: use attach/clear_page_private
>>>>     iomap: use attach/clear_page_private
>>>>     ntfs: replace attach_page_buffers with attach_page_private
>>>>     orangefs: use attach/clear_page_private
>>>>     buffer_head.h: remove attach_page_buffers
>>> I think mm/migrate.c could also use this:
>>>
>>>           ClearPagePrivate(page);
>>>           set_page_private(newpage, page_private(page));
>>>           set_page_private(page, 0);
>>>           put_page(page);
>>>           get_page(newpage);
>>>
>> Thanks for checking!  Assume the below change is appropriate.
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 7160c1556f79..f214adfb3fa4 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -797,10 +797,7 @@ static int __buffer_migrate_page(struct address_space
>> *mapping,
>>          if (rc != MIGRATEPAGE_SUCCESS)
>>                  goto unlock_buffers;
>>
>> -       ClearPagePrivate(page);
>> -       set_page_private(newpage, page_private(page));
>> -       set_page_private(page, 0);
>> -       put_page(page);
>> +       set_page_private(newpage, detach_page_private(page));
>>          get_page(newpage);
> I think you can do:
>
> @@ -797,11 +797,7 @@ static int __buffer_migrate_page(struct address_space *mapping,
>          if (rc != MIGRATEPAGE_SUCCESS)
>                  goto unlock_buffers;
>   
> -       ClearPagePrivate(page);
> -       set_page_private(newpage, page_private(page));
> -       set_page_private(page, 0);
> -       put_page(page);
> -       get_page(newpage);
> +       attach_page_private(newpage, detach_page_private(page));
>   
>          bh = head;
>          do {
> @@ -810,8 +806,6 @@ static int __buffer_migrate_page(struct address_space *mapping,
>   
>          } while (bh != head);
>   
> -       SetPagePrivate(newpage);
> -
>          if (mode != MIGRATE_SYNC_NO_COPY)
>
> ... but maybe there's a subtlety to the ordering of the setup of the bh
> and setting PagePrivate that means what you have there is a better patch.

Yes, it is better but not sure if the order can be changed here. And seems
the original commit is this one.

commit e965f9630c651fa4249039fd4b80c9392d07a856
Author: Christoph Lameter <clameter@sgi.com>
Date:   Wed Feb 1 03:05:41 2006 -0800

     [PATCH] Direct Migration V9: Avoid writeback / page_migrate() method

     Migrate a page with buffers without requiring writeback

     This introduces a new address space operation migratepage() that 
may be used
     by a filesystem to implement its own version of page migration.

     A version is provided that migrates buffers attached to pages. Some
     filesystems (ext2, ext3, xfs) are modified to utilize this feature.

     The swapper address space operation are modified so that a regular
     migrate_page() will occur for anonymous pages without writeback 
(migrate_pages
     forces every anonymous page to have a swap entry).

Hope mm experts could take a look, so CC more people and mm list. And
the question is that if we can setting PagePrivate before setup bh in the
__buffer_migrate_page, thanks for your any further input.

Thanks,
Guoqing