mbox series

[RFC,V2,0/9] Introduce attach/clear_page_private to cleanup code

Message ID 20200430214450.10662-1-guoqing.jiang@cloud.ionos.com (mailing list archive)
Headers show
Series Introduce attach/clear_page_private to cleanup code | expand

Message

Guoqing Jiang April 30, 2020, 9:44 p.m. UTC 
Hi,

Based on the previous thread [1], this patchset introduces attach_page_private
and clear_page_private to replace attach_page_buffers and __clear_page_buffers.
Thanks a lot for the constructive suggestions and comments from Christoph,
Matthew and Dave.

And sorry for cross post to different lists since it modifies different subsystems.

RFC -> RFC V2:

1. rename the new functions and add comments for them.
2. change the return type of attach_page_private.
3. call attach_page_private(newpage, clear_page_private(page)) to cleanup code further.
4. avoid potential use-after-free in orangefs.

[1]. https://lore.kernel.org/linux-fsdevel/20200420221424.GH5820@bombadil.infradead.org/

Thanks,
Guoqing

Guoqing Jiang (9):
  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

 drivers/md/md-bitmap.c      | 12 ++----------
 fs/btrfs/disk-io.c          |  4 +---
 fs/btrfs/extent_io.c        | 21 ++++++---------------
 fs/btrfs/inode.c            | 23 +++++------------------
 fs/buffer.c                 | 16 ++++------------
 fs/f2fs/f2fs.h              | 11 ++---------
 fs/iomap/buffered-io.c      | 19 ++++---------------
 fs/ntfs/aops.c              |  2 +-
 fs/ntfs/mft.c               |  2 +-
 fs/orangefs/inode.c         | 32 ++++++--------------------------
 include/linux/buffer_head.h |  8 --------
 include/linux/pagemap.h     | 35 +++++++++++++++++++++++++++++++++++
 12 files changed, 67 insertions(+), 118 deletions(-)

Comments

Matthew Wilcox May 1, 2020, 10:16 p.m. UTC | #1 
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);
Guoqing Jiang May 1, 2020, 10:42 p.m. UTC | #2 
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);

         bh = head;


Cheers,
Guoqing
Matthew Wilcox May 2, 2020, 12:41 a.m. UTC | #3 
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.
Anybody know?
Guoqing Jiang May 2, 2020, 8:56 a.m. UTC | #4 
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