[PATCH] mm: change may_enter_fs check condition

[PATCH] mm: change may_enter_fs check condition

[yalin wang]

Add page_is_file_cache() for __GFP_FS check,
otherwise, a Pageswapcache() && PageDirty() page can always be write
back if the gfp flag is __GFP_FS, this is not the expected behavior.

Signed-off-by: yalin wang <yalin.wang2010@gmail.com>
---
 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index bd2918e..f8fc8c1 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -930,7 +930,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 		if (page_mapped(page) || PageSwapCache(page))
 			sc->nr_scanned++;
 
-		may_enter_fs = (sc->gfp_mask & __GFP_FS) ||
+		may_enter_fs = (page_is_file_cache(page) && (sc->gfp_mask & __GFP_FS)) ||
 			(PageSwapCache(page) && (sc->gfp_mask & __GFP_IO));
 
 		/*
-- 
1.9.1

[PATCH] mm: change may_enter_fs check condition

[yalin wang]

Add page_is_file_cache() for __GFP_FS check,
otherwise, a Pageswapcache() && PageDirty() page can always be write
back if the gfp flag is __GFP_FS, this is not the expected behavior.

Signed-off-by: yalin wang <yalin.wang2010@gmail.com>
---
 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index bd2918e..f8fc8c1 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -930,7 +930,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 		if (page_mapped(page) || PageSwapCache(page))
 			sc->nr_scanned++;
 
-		may_enter_fs = (sc->gfp_mask & __GFP_FS) ||
+		may_enter_fs = (page_is_file_cache(page) && (sc->gfp_mask & __GFP_FS)) ||
 			(PageSwapCache(page) && (sc->gfp_mask & __GFP_IO));
 
 		/*
-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: change may_enter_fs check condition

[Vlastimil Babka]

On 11/13/2015 12:47 PM, yalin wang wrote:
> Add page_is_file_cache() for __GFP_FS check,
> otherwise, a Pageswapcache() && PageDirty() page can always be write
> back if the gfp flag is __GFP_FS, this is not the expected behavior.

I'm not sure I understand your point correctly *), but you seem to imply 
that there would be an allocation that has __GFP_FS but doesn't have 
__GFP_IO? Are there such allocations and does it make sense?

*) It helps to state which problem you actually observed and are trying 
to fix. Or was this found by code inspection? In that case describe the 
theoretical problem, as "expected behavior" isn't always understood by 
everyone the same.

> Signed-off-by: yalin wang <yalin.wang2010@gmail.com>
> ---
>   mm/vmscan.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index bd2918e..f8fc8c1 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -930,7 +930,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>   		if (page_mapped(page) || PageSwapCache(page))
>   			sc->nr_scanned++;
>
> -		may_enter_fs = (sc->gfp_mask & __GFP_FS) ||
> +		may_enter_fs = (page_is_file_cache(page) && (sc->gfp_mask & __GFP_FS)) ||
>   			(PageSwapCache(page) && (sc->gfp_mask & __GFP_IO));
>
>   		/*
>

Re: [PATCH] mm: change may_enter_fs check condition

[Vlastimil Babka]

On 11/13/2015 12:47 PM, yalin wang wrote:
> Add page_is_file_cache() for __GFP_FS check,
> otherwise, a Pageswapcache() && PageDirty() page can always be write
> back if the gfp flag is __GFP_FS, this is not the expected behavior.

I'm not sure I understand your point correctly *), but you seem to imply 
that there would be an allocation that has __GFP_FS but doesn't have 
__GFP_IO? Are there such allocations and does it make sense?

*) It helps to state which problem you actually observed and are trying 
to fix. Or was this found by code inspection? In that case describe the 
theoretical problem, as "expected behavior" isn't always understood by 
everyone the same.

> Signed-off-by: yalin wang <yalin.wang2010@gmail.com>
> ---
>   mm/vmscan.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index bd2918e..f8fc8c1 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -930,7 +930,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>   		if (page_mapped(page) || PageSwapCache(page))
>   			sc->nr_scanned++;
>
> -		may_enter_fs = (sc->gfp_mask & __GFP_FS) ||
> +		may_enter_fs = (page_is_file_cache(page) && (sc->gfp_mask & __GFP_FS)) ||
>   			(PageSwapCache(page) && (sc->gfp_mask & __GFP_IO));
>
>   		/*
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: change may_enter_fs check condition

[Michal Hocko]

On Fri 13-11-15 13:01:16, Vlastimil Babka wrote:
> On 11/13/2015 12:47 PM, yalin wang wrote:
> >Add page_is_file_cache() for __GFP_FS check,
> >otherwise, a Pageswapcache() && PageDirty() page can always be write
> >back if the gfp flag is __GFP_FS, this is not the expected behavior.
> 
> I'm not sure I understand your point correctly *), but you seem to imply
> that there would be an allocation that has __GFP_FS but doesn't have
> __GFP_IO? Are there such allocations and does it make sense?

No it doesn't. There is a natural layering here and __GFP_FS allocations
should contain __GFP_IO.

The patch as is makes only little sense to me. Are you seeing any issue
which this is trying to fix?

> *) It helps to state which problem you actually observed and are trying to
> fix. Or was this found by code inspection? In that case describe the
> theoretical problem, as "expected behavior" isn't always understood by
> everyone the same.
> 
> >Signed-off-by: yalin wang <yalin.wang2010@gmail.com>
> >---
> >  mm/vmscan.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/mm/vmscan.c b/mm/vmscan.c
> >index bd2918e..f8fc8c1 100644
> >--- a/mm/vmscan.c
> >+++ b/mm/vmscan.c
> >@@ -930,7 +930,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> >  		if (page_mapped(page) || PageSwapCache(page))
> >  			sc->nr_scanned++;
> >
> >-		may_enter_fs = (sc->gfp_mask & __GFP_FS) ||
> >+		may_enter_fs = (page_is_file_cache(page) && (sc->gfp_mask & __GFP_FS)) ||
> >  			(PageSwapCache(page) && (sc->gfp_mask & __GFP_IO));
> >
> >  		/*
> >

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: change may_enter_fs check condition

[Michal Hocko]

On Fri 13-11-15 13:01:16, Vlastimil Babka wrote:
> On 11/13/2015 12:47 PM, yalin wang wrote:
> >Add page_is_file_cache() for __GFP_FS check,
> >otherwise, a Pageswapcache() && PageDirty() page can always be write
> >back if the gfp flag is __GFP_FS, this is not the expected behavior.
> 
> I'm not sure I understand your point correctly *), but you seem to imply
> that there would be an allocation that has __GFP_FS but doesn't have
> __GFP_IO? Are there such allocations and does it make sense?

No it doesn't. There is a natural layering here and __GFP_FS allocations
should contain __GFP_IO.

The patch as is makes only little sense to me. Are you seeing any issue
which this is trying to fix?

> *) It helps to state which problem you actually observed and are trying to
> fix. Or was this found by code inspection? In that case describe the
> theoretical problem, as "expected behavior" isn't always understood by
> everyone the same.
> 
> >Signed-off-by: yalin wang <yalin.wang2010@gmail.com>
> >---
> >  mm/vmscan.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/mm/vmscan.c b/mm/vmscan.c
> >index bd2918e..f8fc8c1 100644
> >--- a/mm/vmscan.c
> >+++ b/mm/vmscan.c
> >@@ -930,7 +930,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> >  		if (page_mapped(page) || PageSwapCache(page))
> >  			sc->nr_scanned++;
> >
> >-		may_enter_fs = (sc->gfp_mask & __GFP_FS) ||
> >+		may_enter_fs = (page_is_file_cache(page) && (sc->gfp_mask & __GFP_FS)) ||
> >  			(PageSwapCache(page) && (sc->gfp_mask & __GFP_IO));
> >
> >  		/*
> >

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: change may_enter_fs check condition

[yalin wang]

> On Nov 13, 2015, at 23:36, Michal Hocko <mhocko@kernel.org> wrote:
> 
> On Fri 13-11-15 13:01:16, Vlastimil Babka wrote:
>> On 11/13/2015 12:47 PM, yalin wang wrote:
>>> Add page_is_file_cache() for __GFP_FS check,
>>> otherwise, a Pageswapcache() && PageDirty() page can always be write
>>> back if the gfp flag is __GFP_FS, this is not the expected behavior.
>> 
>> I'm not sure I understand your point correctly *), but you seem to imply
>> that there would be an allocation that has __GFP_FS but doesn't have
>> __GFP_IO? Are there such allocations and does it make sense?
> 
> No it doesn't. There is a natural layering here and __GFP_FS allocations
> should contain __GFP_IO.
> 
> The patch as is makes only little sense to me. Are you seeing any issue
> which this is trying to fix?
mm..
i don’t see issue for this part ,
just feel confuse when i see code about this part ,
then i make a patch for this .
i am not sure if __GFP_FS will make sure __GFP_IO flag must be always set.
if it is ,  i think can add comment here to make people clear . :)

Thanks

Re: [PATCH] mm: change may_enter_fs check condition

[yalin wang]

> On Nov 13, 2015, at 23:36, Michal Hocko <mhocko@kernel.org> wrote:
> 
> On Fri 13-11-15 13:01:16, Vlastimil Babka wrote:
>> On 11/13/2015 12:47 PM, yalin wang wrote:
>>> Add page_is_file_cache() for __GFP_FS check,
>>> otherwise, a Pageswapcache() && PageDirty() page can always be write
>>> back if the gfp flag is __GFP_FS, this is not the expected behavior.
>> 
>> I'm not sure I understand your point correctly *), but you seem to imply
>> that there would be an allocation that has __GFP_FS but doesn't have
>> __GFP_IO? Are there such allocations and does it make sense?
> 
> No it doesn't. There is a natural layering here and __GFP_FS allocations
> should contain __GFP_IO.
> 
> The patch as is makes only little sense to me. Are you seeing any issue
> which this is trying to fix?
mm..
i don’t see issue for this part ,
just feel confuse when i see code about this part ,
then i make a patch for this .
i am not sure if __GFP_FS will make sure __GFP_IO flag must be always set.
if it is ,  i think can add comment here to make people clear . :)

Thanks

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>