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@xxxxxxx>
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