Re: ubifs: fix page_count in ->ubifs_migrate_page()

Re: ubifs: fix page_count in ->ubifs_migrate_page()

[Richard Weinberger]

Hello zhangjun,

thanks a lot for bringing this up!

Am Mittwoch, 12. Dezember 2018, 15:13:57 CET schrieb zhangjun:
> Because the PagePrivate() in UBIFS is different meanings,
> alloc_cma() will fail when one dirty page cache located in
> the type of MIGRATE_CMA
> 
> If not adjust the 'extra_count' for dirty page,
> ubifs_migrate_page() -> migrate_page_move_mapping() will
> always return -EAGAIN for:
> 	expected_count += page_has_private(page)
> This causes the migration to fail until the page cache is cleaned
> 
> In general, PagePrivate() indicates that buff_head is already bound
> to this page, and at the same time page_count() will also increase.
> But UBIFS set private flag when the cache is dirty, and page_count()
> not increase.
> Therefore, the expected_count of UBIFS is different from the general
> case.

As you noted, UBIFS uses PG_private a little different.
It uses it as marker and when set, the page counter is not incremented,
since no private data is attached.
The migration code assumes that PG_private indicates a counter of +1.
So, we have to pass a extra count of -1 to migrate_page_move_mapping() if
the flag is set.
Just like F2FS does. Not really nice but hey...

> Signed-off-by: zhangjun <openzhangj@gmail.com>

Fixes: 4ac1c17b2044 ("UBIFS: Implement ->migratepage()")

> ---
>  fs/ubifs/file.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index 1b78f2e..2136a5c 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -1480,8 +1480,15 @@ static int ubifs_migrate_page(struct address_space *mapping,
>  		struct page *newpage, struct page *page, enum migrate_mode mode)
>  {
>  	int rc;
> +	int extra_count;
>  
> -	rc = migrate_page_move_mapping(mapping, newpage, page, NULL, mode, 0);
> +	/*
> +	 * UBIFS is using PagePrivate() which can have different meanings across
> +	 * filesystems. So here adjusting the 'extra_count' make it work.
> +	 */

Please rewrite that comment.
/*
 * UBIFS uses PG_private as marker and does not raise the page counter.
 * migrate_page_move_mapping() expects a incremented counter if PG_private
 * is set. Therefore pass -1 as extra_count for this case.
 */

> +	extra_count = 0 - page_has_private(page);

if (page_has_private(page))
	extra_count = -1;

That way this corner is much more obvious.

Thanks,
//richard

Re: ubifs: fix page_count in ->ubifs_migrate_page()

[zhangjun]

On 2018/12/13 下午10:23, Richard Weinberger wrote:
> Hello zhangjun,
>
> thanks a lot for bringing this up!
>
> Am Mittwoch, 12. Dezember 2018, 15:13:57 CET schrieb zhangjun:
>> Because the PagePrivate() in UBIFS is different meanings,
>> alloc_cma() will fail when one dirty page cache located in
>> the type of MIGRATE_CMA
>>
>> If not adjust the 'extra_count' for dirty page,
>> ubifs_migrate_page() -> migrate_page_move_mapping() will
>> always return -EAGAIN for:
>> 	expected_count += page_has_private(page)
>> This causes the migration to fail until the page cache is cleaned
>>
>> In general, PagePrivate() indicates that buff_head is already bound
>> to this page, and at the same time page_count() will also increase.
>> But UBIFS set private flag when the cache is dirty, and page_count()
>> not increase.
>> Therefore, the expected_count of UBIFS is different from the general
>> case.
> As you noted, UBIFS uses PG_private a little different.
> It uses it as marker and when set, the page counter is not incremented,
> since no private data is attached.
> The migration code assumes that PG_private indicates a counter of +1.
> So, we have to pass a extra count of -1 to migrate_page_move_mapping() if
> the flag is set.
> Just like F2FS does. Not really nice but hey...
>
>> Signed-off-by: zhangjun <openzhangj@gmail.com>
> Fixes: 4ac1c17b2044 ("UBIFS: Implement ->migratepage()")
>
>> ---
>>   fs/ubifs/file.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
>> index 1b78f2e..2136a5c 100644
>> --- a/fs/ubifs/file.c
>> +++ b/fs/ubifs/file.c
>> @@ -1480,8 +1480,15 @@ static int ubifs_migrate_page(struct address_space *mapping,
>>   		struct page *newpage, struct page *page, enum migrate_mode mode)
>>   {
>>   	int rc;
>> +	int extra_count;
>>   
>> -	rc = migrate_page_move_mapping(mapping, newpage, page, NULL, mode, 0);
>> +	/*
>> +	 * UBIFS is using PagePrivate() which can have different meanings across
>> +	 * filesystems. So here adjusting the 'extra_count' make it work.
>> +	 */
> Please rewrite that comment.
> /*
>   * UBIFS uses PG_private as marker and does not raise the page counter.
>   * migrate_page_move_mapping() expects a incremented counter if PG_private
>   * is set. Therefore pass -1 as extra_count for this case.
>   */
>
>> +	extra_count = 0 - page_has_private(page);
> if (page_has_private(page))
> 	extra_count = -1;
>
> That way this corner is much more obvious.
>
> Thanks,
> //richard
>
>
Hello Richard.

Thank you very much for your help.

1. Can i use your description for comment? like this:

/*
  * UBIFS uses PG_private a little different.
  * It uses it as marker and when set, the page counter is not incremented,
  * since no private data is attached.
  * The migration code assumes that PG_private indicates a counter of +1.
  * So, we have to pass a extra count of -1 to migrate_page_move_mapping() if
  * the flag is set.
  */

2. It's more obvious, but the branch may break the cpu pipeline?

Re: ubifs: fix page_count in ->ubifs_migrate_page()

[Dave Chinner]

On Thu, Dec 13, 2018 at 03:23:37PM +0100, Richard Weinberger wrote:
> Hello zhangjun,
> 
> thanks a lot for bringing this up!
> 
> Am Mittwoch, 12. Dezember 2018, 15:13:57 CET schrieb zhangjun:
> > Because the PagePrivate() in UBIFS is different meanings,
> > alloc_cma() will fail when one dirty page cache located in
> > the type of MIGRATE_CMA
> > 
> > If not adjust the 'extra_count' for dirty page,
> > ubifs_migrate_page() -> migrate_page_move_mapping() will
> > always return -EAGAIN for:
> > 	expected_count += page_has_private(page)
> > This causes the migration to fail until the page cache is cleaned
> > 
> > In general, PagePrivate() indicates that buff_head is already bound
> > to this page, and at the same time page_count() will also increase.


That's an invalid assumption.

We should not be trying to infer what PagePrivate() means in code
that has no business using looking at it i.e. page->private is private
information for the owner of the page, and it's life cycle and
intent are unknown to anyone other than the page owner.

e.g. on XFS, a page cache page's page->private /might/ contain a
struct iomap_page, or it might be NULL. Assigning a struct
iomap_page to the page does not change the reference count on the
page.  IOWs, the page needs to be handled exactly the same
way by external code regardless of whether there is somethign
attached to page->private or not.

Hence it looks to me like the migration code is making invalid
assumptions about PagePrivate inferring reference counts and so the
migration code needs to be fixed. Requiring filesystems to work
around invalid assumptions in the migration code is a sure recipe
for problems with random filesystems using page->private for their
own internal purposes....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: ubifs: fix page_count in ->ubifs_migrate_page()

[zhangjun]

On 2018/12/14 上午6:57, Dave Chinner wrote:
> On Thu, Dec 13, 2018 at 03:23:37PM +0100, Richard Weinberger wrote:
>> Hello zhangjun,
>>
>> thanks a lot for bringing this up!
>>
>> Am Mittwoch, 12. Dezember 2018, 15:13:57 CET schrieb zhangjun:
>>> Because the PagePrivate() in UBIFS is different meanings,
>>> alloc_cma() will fail when one dirty page cache located in
>>> the type of MIGRATE_CMA
>>>
>>> If not adjust the 'extra_count' for dirty page,
>>> ubifs_migrate_page() -> migrate_page_move_mapping() will
>>> always return -EAGAIN for:
>>> 	expected_count += page_has_private(page)
>>> This causes the migration to fail until the page cache is cleaned
>>>
>>> In general, PagePrivate() indicates that buff_head is already bound
>>> to this page, and at the same time page_count() will also increase.
>
> That's an invalid assumption.
>
> We should not be trying to infer what PagePrivate() means in code
> that has no business using looking at it i.e. page->private is private
> information for the owner of the page, and it's life cycle and
> intent are unknown to anyone other than the page owner.
>
> e.g. on XFS, a page cache page's page->private /might/ contain a
> struct iomap_page, or it might be NULL. Assigning a struct
> iomap_page to the page does not change the reference count on the
> page.  IOWs, the page needs to be handled exactly the same
> way by external code regardless of whether there is somethign
> attached to page->private or not.
>
> Hence it looks to me like the migration code is making invalid
> assumptions about PagePrivate inferring reference counts and so the
> migration code needs to be fixed. Requiring filesystems to work
> around invalid assumptions in the migration code is a sure recipe
> for problems with random filesystems using page->private for their
> own internal purposes....
>
> Cheers,
>
> Dave.
I agree with your main point of view, but for the buffer_head based file 
system this assumption is no problem,
and the parameters and comments from the migrate_page_move_mapping() 
function:
   * 3 for pages with a mapping and PagePrivate/PagePrivate2 set.
This assumption has been explained.
Or to accurately say that the migrate system does not currently have a 
generic function for this case.
Since you call the function implemented for buffer_head, you should 
follow its rules.

Re: ubifs: fix page_count in ->ubifs_migrate_page()

[Gao Xiang]

Hi,

On 2018/12/14 14:15, zhangjun wrote:
> On 2018/12/14 上午6:57, Dave Chinner wrote:
>> On Thu, Dec 13, 2018 at 03:23:37PM +0100, Richard Weinberger wrote:
>>> Hello zhangjun,
>>>
>>> thanks a lot for bringing this up!
>>>
>>> Am Mittwoch, 12. Dezember 2018, 15:13:57 CET schrieb zhangjun:
>>>> Because the PagePrivate() in UBIFS is different meanings,
>>>> alloc_cma() will fail when one dirty page cache located in
>>>> the type of MIGRATE_CMA
>>>>
>>>> If not adjust the 'extra_count' for dirty page,
>>>> ubifs_migrate_page() -> migrate_page_move_mapping() will
>>>> always return -EAGAIN for:
>>>>     expected_count += page_has_private(page)
>>>> This causes the migration to fail until the page cache is cleaned
>>>>
>>>> In general, PagePrivate() indicates that buff_head is already bound
>>>> to this page, and at the same time page_count() will also increase.
>>
>> That's an invalid assumption.
>>
>> We should not be trying to infer what PagePrivate() means in code
>> that has no business using looking at it i.e. page->private is private
>> information for the owner of the page, and it's life cycle and
>> intent are unknown to anyone other than the page owner.
>>
>> e.g. on XFS, a page cache page's page->private /might/ contain a
>> struct iomap_page, or it might be NULL. Assigning a struct
>> iomap_page to the page does not change the reference count on the
>> page.  IOWs, the page needs to be handled exactly the same
>> way by external code regardless of whether there is somethign
>> attached to page->private or not.
>>
>> Hence it looks to me like the migration code is making invalid
>> assumptions about PagePrivate inferring reference counts and so the
>> migration code needs to be fixed. Requiring filesystems to work
>> around invalid assumptions in the migration code is a sure recipe
>> for problems with random filesystems using page->private for their
>> own internal purposes....
>>
>> Cheers,
>>
>> Dave.
> I agree with your main point of view, but for the buffer_head based file system this assumption is no problem,
> and the parameters and comments from the migrate_page_move_mapping() function:
>   * 3 for pages with a mapping and PagePrivate/PagePrivate2 set.
> This assumption has been explained.
> Or to accurately say that the migrate system does not currently have a generic function for this case.
> Since you call the function implemented for buffer_head, you should follow its rules.
> 

This restriction also exists in the reclaim code. If PagePrivate is set, this page should be with an extra reference (==3) to be freeable
at the moment.

 571 static inline int is_page_cache_freeable(struct page *page)
 572 {
 573         /*
 574          * A freeable page cache page is referenced only by the caller
 575          * that isolated the page, the page cache radix tree and
 576          * optional buffer heads at page->private.
 577          */
 578         return page_count(page) - page_has_private(page) == 2;
 579 }

And I personally think this restriction is good for race-free code (at least erofs doesn't use buffer_head, but it also follows
the rule to avoid race because an extra pointer to the PagePrivate page) since it indicates that some pointer also points to the page
but the page can be freeable at the moment, an extra reference is preferred to keep the relationship.

I think it is what PagePrivate implys (some other pointer to the page) but not the page->private. Those are two slightly different fields...

My personal thought...

Thanks,
Gao Xiang