mm allocation failure and hang when running xfstests generic/269 on xfs

Hi,

It's reproduciable, not everytime though. Ext4 works fine.

Quoted 3 lines · Xiong Zhou

Hi,

It's reproduciable, not everytime though. Ext4 works fine.

On ext4 fsstress won't run bulkstat because it doesn't exist. Either way this smells like a MM issue to me as there were not XFS changes in that area recently.

Quoted 5 lines · Christoph Hellwig

Quoted 3 lines · Xiong Zhou

Hi,

It's reproduciable, not everytime though. Ext4 works fine.

Quoted 3 lines

On ext4 fsstress won't run bulkstat because it doesn't exist. Either way this smells like a MM issue to me as there were not XFS changes in that area recently.

Yap.

First bad commit:

From below commit, the allocation failure print was due to current process received SIGKILL signal.

commit 5d17a73a2ebeb8d1c6924b91e53ab2650fe86ffb

Author: Michal Hocko <[email protected]>
Date:   Fri Feb 24 14:58:53 2017 -0800

vmalloc: back off when the current task is killed

Reverting this commit on top of
e5d56ef Merge tag 'watchdog-for-linus-v4.11'
survives the tests.

Quoted 7 lines · Christoph Hellwig

Quoted 3 lines · Xiong Zhou

Hi,

It's reproduciable, not everytime though. Ext4 works fine.

On ext4 fsstress won't run bulkstat because it doesn't exist. Either way this smells like a MM issue to me as there were not XFS changes in that area recently.

Yap.

First bad commit:

commit 5d17a73a2ebeb8d1c6924b91e53ab2650fe86ffb

Author: Michal Hocko <[email protected]>
Date:   Fri Feb 24 14:58:53 2017 -0800

vmalloc: back off when the current task is killed

Reverting this commit on top of
e5d56ef Merge tag 'watchdog-for-linus-v4.11'
survives the tests.

Quoted 13 lines · Xiong Zhou

Quoted 7 lines · Christoph Hellwig

Quoted 3 lines · Xiong Zhou

Hi,

It's reproduciable, not everytime though. Ext4 works fine.

On ext4 fsstress won't run bulkstat because it doesn't exist. Either way this smells like a MM issue to me as there were not XFS changes in that area recently.

Yap.

First bad commit:

commit 5d17a73a2ebeb8d1c6924b91e53ab2650fe86ffb

Quoted 8 lines

Author: Michal Hocko <[email protected]>
Date:   Fri Feb 24 14:58:53 2017 -0800

vmalloc: back off when the current task is killed

Reverting this commit on top of
e5d56ef Merge tag 'watchdog-for-linus-v4.11'
survives the tests.

Does fsstress test or the system hang ? I am not familiar with this code but If it's the test which is getting hung and its hitting this new check introduced by the above commit that means the requester is currently being killed by OOM killer for some other memory allocation request.

Quoted 23 lines · Anshuman Khandual

Quoted 13 lines · Xiong Zhou

Quoted 7 lines · Christoph Hellwig

Quoted 3 lines · Xiong Zhou

Hi,

It's reproduciable, not everytime though. Ext4 works fine.

On ext4 fsstress won't run bulkstat because it doesn't exist. Either way this smells like a MM issue to me as there were not XFS changes in that area recently.

Yap.

First bad commit:

commit 5d17a73a2ebeb8d1c6924b91e53ab2650fe86ffb

Quoted 8 lines

Author: Michal Hocko <[email protected]>
Date:   Fri Feb 24 14:58:53 2017 -0800

vmalloc: back off when the current task is killed

Reverting this commit on top of
e5d56ef Merge tag 'watchdog-for-linus-v4.11'
survives the tests.

Quoted 5 lines

Does fsstress test or the system hang ? I am not familiar with this code but If it's the test which is getting hung and its hitting this new check introduced by the above commit that means the requester is currently being killed by OOM killer for some other memory allocation request.

Well, not exactly. It is sufficient for it to be _killed_ by SIGKILL. And for that it just needs to do a group_exit when one thread was still in the kernel (see zap_process). While I can change this check to actually do the oom specific check I believe a more generic fatal_signal_pending is the right thing to do here. I am still not sure what is the actual problem here, though. Could you be more specific please?

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 8 lines · Christoph Hellwig

Quoted 3 lines · Xiong Zhou

Hi,

It's reproduciable, not everytime though. Ext4 works fine.

On ext4 fsstress won't run bulkstat because it doesn't exist. Either way this smells like a MM issue to me as there were not XFS changes in that area recently.

Yap.

First bad commit:

commit 5d17a73a2ebeb8d1c6924b91e53ab2650fe86ffb

Author: Michal Hocko <[email protected]>
Date:   Fri Feb 24 14:58:53 2017 -0800

vmalloc: back off when the current task is killed

Reverting this commit on top of
e5d56ef Merge tag 'watchdog-for-linus-v4.11'
survives the tests.

So, commit 5d17a73a2ebeb8d1("vmalloc: back off when the current task is killed") implemented __GFP_KILLABLE flag and automatically applied that flag. As a result, those who are not ready to fail upon SIGKILL are confused. ;-)

From d02cb0285d8ce3344fd64dc7e2912e9a04bef80d Mon Sep 17 00:00:00 2001

On Thu 02-03-17 19:04:48, Tetsuo Handa wrote:
[...]

Quoted 4 lines

So, commit 5d17a73a2ebeb8d1("vmalloc: back off when the current task is killed") implemented __GFP_KILLABLE flag and automatically applied that flag. As a result, those who are not ready to fail upon SIGKILL are confused. ;-)

You are right! The function is documented it might fail but the code doesn't really allow that. This seems like a bug to me. What do you think about the following?

---
From d02cb0285d8ce3344fd64dc7e2912e9a04bef80d Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 2 Mar 2017 11:31:11 +0100
Subject: [PATCH] xfs: allow kmem_zalloc_greedy to fail

Even though kmem_zalloc_greedy is documented it might fail the current code doesn't really implement this properly and loops on the smallest allowed size for ever. This is a problem because vzalloc might fail permanently. Since 5d17a73a2ebe ("vmalloc: back off when the current task is killed") such a failure is much more probable than it used to be. Fix this by bailing out if the minimum size request failed.

This has been noticed by a hung generic/269 xfstest by Xiong Zhou.

Reported-by Xiong Zhou <[email protected]>

Analyzed-by Tetsuo Handa <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 fs/xfs/kmem.c | 2 ++
 1 file changed, 2 insertions(+)

fs/xfs/kmem.c +2 -0

index 339c696bbc01..ee95f5c6db45 100644
@@ -34,6 +34,8 @@ kmem_zalloc_greedy(size_t *size, size_t minsize, size_t maxsize)
 	size_t		kmsize = maxsize;
 
 	while (!(ptr = vzalloc(kmsize))) {
+		if (kmsize == minsize)
+			break;
 		if ((kmsize >>= 1) <= minsize)
 			kmsize = minsize;
 	}

Signature · 5 lines

2.11.0

-- 
Michal Hocko
SUSE Labs

On Thu 02-03-17 19:04:48, Tetsuo Handa wrote:
[...]

Quoted 4 lines

So, commit 5d17a73a2ebeb8d1("vmalloc: back off when the current task is killed") implemented __GFP_KILLABLE flag and automatically applied that flag. As a result, those who are not ready to fail upon SIGKILL are confused. ;-)

You are right! The function is documented it might fail but the code doesn't really allow that. This seems like a bug to me. What do you think about the following?
---
From d02cb0285d8ce3344fd64dc7e2912e9a04bef80d Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 2 Mar 2017 11:31:11 +0100
Subject: [PATCH] xfs: allow kmem_zalloc_greedy to fail

Even though kmem_zalloc_greedy is documented it might fail the current code doesn't really implement this properly and loops on the smallest allowed size for ever. This is a problem because vzalloc might fail permanently. Since 5d17a73a2ebe ("vmalloc: back off when the current task is killed") such a failure is much more probable than it used to be. Fix this by bailing out if the minimum size request failed.

This has been noticed by a hung generic/269 xfstest by Xiong Zhou.

Reported-by Xiong Zhou <[email protected]>

Analyzed-by Tetsuo Handa <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 fs/xfs/kmem.c | 2 ++
 1 file changed, 2 insertions(+)

fs/xfs/kmem.c +0 -0

index 339c696bbc01..ee95f5c6db45 100644

Signature · 11 lines

2.11.0

-- 
Michal Hocko
SUSE Labs

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

Quoted 39 lines · Michal Hocko

On Thu 02-03-17 19:04:48, Tetsuo Handa wrote:
[...]

Quoted 4 lines

So, commit 5d17a73a2ebeb8d1("vmalloc: back off when the current task is killed") implemented __GFP_KILLABLE flag and automatically applied that flag. As a result, those who are not ready to fail upon SIGKILL are confused. ;-)

You are right! The function is documented it might fail but the code doesn't really allow that. This seems like a bug to me. What do you think about the following?
---
From d02cb0285d8ce3344fd64dc7e2912e9a04bef80d Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 2 Mar 2017 11:31:11 +0100
Subject: [PATCH] xfs: allow kmem_zalloc_greedy to fail

Even though kmem_zalloc_greedy is documented it might fail the current code doesn't really implement this properly and loops on the smallest allowed size for ever. This is a problem because vzalloc might fail permanently. Since 5d17a73a2ebe ("vmalloc: back off when the current task is killed") such a failure is much more probable than it used to be. Fix this by bailing out if the minimum size request failed.

This has been noticed by a hung generic/269 xfstest by Xiong Zhou.

Reported-by Xiong Zhou <[email protected]>

Analyzed-by Tetsuo Handa <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 fs/xfs/kmem.c | 2 ++
 1 file changed, 2 insertions(+)

fs/xfs/kmem.c +0 -0

index 339c696bbc01..ee95f5c6db45 100644
@@ -34,6 +34,8 @@ kmem_zalloc_greedy(size_t *size, size_t minsize, size_t maxsize)
 	size_t		kmsize = maxsize;
 

fs/xfs/kmem.c kmem_zalloc_greedy(size_t *size, size_t minsize, size_t maxsize) · line 41

Quoted 6 lines

fs/xfs/kmem.c +2 -0

 	while (!(ptr = vzalloc(kmsize))) {
+		if (kmsize == minsize)
+			break;
 		if ((kmsize >>= 1) <= minsize)
 			kmsize = minsize;
 	}

More consistent with the rest of the kmem code might be to accept a flags argument and do something like this based on KM_MAYFAIL.

The one
current caller looks like it would pass it, but I suppose we'd still need a mechanism to break out should a new caller not pass that flag. Would a fatal_signal_pending() check in the loop as well allow us to break out in the scenario that is reproduced here?

Quoted 40 lines · Brian Foster

Quoted 39 lines · Michal Hocko

On Thu 02-03-17 19:04:48, Tetsuo Handa wrote:
[...]

Quoted 4 lines

So, commit 5d17a73a2ebeb8d1("vmalloc: back off when the current task is killed") implemented __GFP_KILLABLE flag and automatically applied that flag. As a result, those who are not ready to fail upon SIGKILL are confused. ;-)

You are right! The function is documented it might fail but the code doesn't really allow that. This seems like a bug to me. What do you think about the following?
---
From d02cb0285d8ce3344fd64dc7e2912e9a04bef80d Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 2 Mar 2017 11:31:11 +0100
Subject: [PATCH] xfs: allow kmem_zalloc_greedy to fail

Even though kmem_zalloc_greedy is documented it might fail the current code doesn't really implement this properly and loops on the smallest allowed size for ever. This is a problem because vzalloc might fail permanently. Since 5d17a73a2ebe ("vmalloc: back off when the current task is killed") such a failure is much more probable than it used to be. Fix this by bailing out if the minimum size request failed.

This has been noticed by a hung generic/269 xfstest by Xiong Zhou.

Reported-by Xiong Zhou <[email protected]>

Analyzed-by Tetsuo Handa <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 fs/xfs/kmem.c | 2 ++
 1 file changed, 2 insertions(+)

fs/xfs/kmem.c +0 -0

index 339c696bbc01..ee95f5c6db45 100644
@@ -34,6 +34,8 @@ kmem_zalloc_greedy(size_t *size, size_t minsize, size_t maxsize)
 	size_t		kmsize = maxsize;
 

Quoted 9 lines

fs/xfs/kmem.c kmem_zalloc_greedy(size_t *size, size_t minsize, size_t maxsize) · line 41

Quoted 6 lines

fs/xfs/kmem.c +2 -0

 	while (!(ptr = vzalloc(kmsize))) {
+		if (kmsize == minsize)
+			break;
 		if ((kmsize >>= 1) <= minsize)
 			kmsize = minsize;
 	}

More consistent with the rest of the kmem code might be to accept a flags argument and do something like this based on KM_MAYFAIL.

Well, vmalloc doesn't really support GFP_NOFAIL semantic right now for the same reason it doesn't support GFP_NOFS. So I am not sure this is a good idea.

Quoted 5 lines

The one
current caller looks like it would pass it, but I suppose we'd still need a mechanism to break out should a new caller not pass that flag. Would a fatal_signal_pending() check in the loop as well allow us to break out in the scenario that is reproduced here?

Yes that check would work as well I just thought the break out when the minsize request fails to be more logical. There might be other reasons to fail the request and looping here seems just wrong. But whatever you or other xfs people prefer.

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 51 lines · Michal Hocko

Quoted 49 lines · Brian Foster

Quoted 37 lines · Michal Hocko

On Thu 02-03-17 19:04:48, Tetsuo Handa wrote:
[...]

Quoted 4 lines

So, commit 5d17a73a2ebeb8d1("vmalloc: back off when the current task is killed") implemented __GFP_KILLABLE flag and automatically applied that flag. As a result, those who are not ready to fail upon SIGKILL are confused. ;-)

You are right! The function is documented it might fail but the code doesn't really allow that. This seems like a bug to me. What do you think about the following?
---
From d02cb0285d8ce3344fd64dc7e2912e9a04bef80d Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 2 Mar 2017 11:31:11 +0100
Subject: [PATCH] xfs: allow kmem_zalloc_greedy to fail

Even though kmem_zalloc_greedy is documented it might fail the current code doesn't really implement this properly and loops on the smallest allowed size for ever. This is a problem because vzalloc might fail permanently. Since 5d17a73a2ebe ("vmalloc: back off when the current task is killed") such a failure is much more probable than it used to be. Fix this by bailing out if the minimum size request failed.

This has been noticed by a hung generic/269 xfstest by Xiong Zhou.

Reported-by Xiong Zhou <[email protected]>

Analyzed-by Tetsuo Handa <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 fs/xfs/kmem.c | 2 ++
 1 file changed, 2 insertions(+)

fs/xfs/kmem.c +0 -0

index 339c696bbc01..ee95f5c6db45 100644

fs/xfs/kmem.c kmem_zalloc_greedy(size_t *size, size_t minsize, size_t maxsize) · line 41

Quoted 8 lines

fs/xfs/kmem.c +2 -0

@@ -34,6 +34,8 @@ kmem_zalloc_greedy(size_t *size, size_t minsize, size_t maxsize)
 	size_t		kmsize = maxsize;
 
 	while (!(ptr = vzalloc(kmsize))) {
+		if (kmsize == minsize)
+			break;
 		if ((kmsize >>= 1) <= minsize)
 			kmsize = minsize;
 	}

More consistent with the rest of the kmem code might be to accept a flags argument and do something like this based on KM_MAYFAIL.

Quoted 4 lines

Well, vmalloc doesn't really support GFP_NOFAIL semantic right now for the same reason it doesn't support GFP_NOFS. So I am not sure this is a good idea.

Not sure I follow..? I'm just suggesting to control the loop behavior based on the KM_ flag, not to do or change anything wrt to GFP_ flags.

Quoted 11 lines

Quoted 6 lines

The one
current caller looks like it would pass it, but I suppose we'd still need a mechanism to break out should a new caller not pass that flag. Would a fatal_signal_pending() check in the loop as well allow us to break out in the scenario that is reproduced here?

Yes that check would work as well I just thought the break out when the minsize request fails to be more logical. There might be other reasons to fail the request and looping here seems just wrong. But whatever you or other xfs people prefer.

There may be higher level reasons for why this code should or should not loop, that just seems like a separate issue to me. My thinking is more that this appears to be how every kmem_*() function operates today and it seems a bit out of place to change behavior of one to fix a bug.

Maybe I'm missing something though.. are we subject to the same general problem in any of the other kmem_*() functions that can currently loop indefinitely?

Brian

Quoted 3 lines

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 51 lines · Michal Hocko

Quoted 49 lines · Brian Foster

Quoted 37 lines · Michal Hocko

On Thu 02-03-17 19:04:48, Tetsuo Handa wrote:
[...]

Quoted 4 lines

So, commit 5d17a73a2ebeb8d1("vmalloc: back off when the current task is killed") implemented __GFP_KILLABLE flag and automatically applied that flag. As a result, those who are not ready to fail upon SIGKILL are confused. ;-)

You are right! The function is documented it might fail but the code doesn't really allow that. This seems like a bug to me. What do you think about the following?
---
From d02cb0285d8ce3344fd64dc7e2912e9a04bef80d Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 2 Mar 2017 11:31:11 +0100
Subject: [PATCH] xfs: allow kmem_zalloc_greedy to fail

Even though kmem_zalloc_greedy is documented it might fail the current code doesn't really implement this properly and loops on the smallest allowed size for ever. This is a problem because vzalloc might fail permanently. Since 5d17a73a2ebe ("vmalloc: back off when the current task is killed") such a failure is much more probable than it used to be. Fix this by bailing out if the minimum size request failed.

This has been noticed by a hung generic/269 xfstest by Xiong Zhou.

Reported-by Xiong Zhou <[email protected]>

Analyzed-by Tetsuo Handa <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 fs/xfs/kmem.c | 2 ++
 1 file changed, 2 insertions(+)

fs/xfs/kmem.c +0 -0

index 339c696bbc01..ee95f5c6db45 100644

fs/xfs/kmem.c kmem_zalloc_greedy(size_t *size, size_t minsize, size_t maxsize) · line 41

Quoted 8 lines

fs/xfs/kmem.c +2 -0

@@ -34,6 +34,8 @@ kmem_zalloc_greedy(size_t *size, size_t minsize, size_t maxsize)
 	size_t		kmsize = maxsize;
 
 	while (!(ptr = vzalloc(kmsize))) {
+		if (kmsize == minsize)
+			break;
 		if ((kmsize >>= 1) <= minsize)
 			kmsize = minsize;
 	}

More consistent with the rest of the kmem code might be to accept a flags argument and do something like this based on KM_MAYFAIL.

Quoted 4 lines

Well, vmalloc doesn't really support GFP_NOFAIL semantic right now for the same reason it doesn't support GFP_NOFS. So I am not sure this is a good idea.

Not sure I follow..? I'm just suggesting to control the loop behavior based on the KM_ flag, not to do or change anything wrt to GFP_ flags.

Quoted 52 lines · Brian Foster

Quoted 51 lines · Michal Hocko

Quoted 49 lines · Brian Foster

Quoted 37 lines · Michal Hocko

On Thu 02-03-17 19:04:48, Tetsuo Handa wrote:
[...]

Quoted 4 lines

So, commit 5d17a73a2ebeb8d1("vmalloc: back off when the current task is killed") implemented __GFP_KILLABLE flag and automatically applied that flag. As a result, those who are not ready to fail upon SIGKILL are confused. ;-)

You are right! The function is documented it might fail but the code doesn't really allow that. This seems like a bug to me. What do you think about the following?
---
From d02cb0285d8ce3344fd64dc7e2912e9a04bef80d Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 2 Mar 2017 11:31:11 +0100
Subject: [PATCH] xfs: allow kmem_zalloc_greedy to fail

Even though kmem_zalloc_greedy is documented it might fail the current code doesn't really implement this properly and loops on the smallest allowed size for ever. This is a problem because vzalloc might fail permanently. Since 5d17a73a2ebe ("vmalloc: back off when the current task is killed") such a failure is much more probable than it used to be. Fix this by bailing out if the minimum size request failed.

This has been noticed by a hung generic/269 xfstest by Xiong Zhou.

Reported-by Xiong Zhou <[email protected]>

Analyzed-by Tetsuo Handa <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 fs/xfs/kmem.c | 2 ++
 1 file changed, 2 insertions(+)

fs/xfs/kmem.c +0 -0

index 339c696bbc01..ee95f5c6db45 100644

fs/xfs/kmem.c kmem_zalloc_greedy(size_t *size, size_t minsize, size_t maxsize) · line 41

Quoted 8 lines

fs/xfs/kmem.c +2 -0

@@ -34,6 +34,8 @@ kmem_zalloc_greedy(size_t *size, size_t minsize, size_t maxsize)
 	size_t		kmsize = maxsize;
 
 	while (!(ptr = vzalloc(kmsize))) {
+		if (kmsize == minsize)
+			break;
 		if ((kmsize >>= 1) <= minsize)
 			kmsize = minsize;
 	}

More consistent with the rest of the kmem code might be to accept a flags argument and do something like this based on KM_MAYFAIL.

Quoted 7 lines

Quoted 4 lines

Well, vmalloc doesn't really support GFP_NOFAIL semantic right now for the same reason it doesn't support GFP_NOFS. So I am not sure this is a good idea.

Not sure I follow..? I'm just suggesting to control the loop behavior based on the KM_ flag, not to do or change anything wrt to GFP_ flags.

As Tetsuo already pointed out, vmalloc cannot really support never-fail semantic with the current implementation so the semantic would have to be implemented in kmem_zalloc_greedy and the only way to do that would be to loop there and this is rather nasty as you can see from the reported issue because the vmalloc failure might be permanent so there won't be any way to make a forward progress. Breaking out of the loop on fatal_signal_pending pending would break the non-failing sementic.

Besides that, there doesn't really seem to be any demand for this semantic in the first place so why to make this more complicated than necessary?

I see your argument about being in sync with other kmem helpers but those are bit different because regular page/slab allocators allow never fail semantic (even though this is mostly ignored by those helpers which implement their own retries but that is a different topic).

Signature · 8 lines

Michal Hocko
SUSE Labs

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

On Thu, Mar 02, 2017 at 02:27:55PM +0100, Michal Hocko wrote:

Quoted 5 lines

I see your argument about being in sync with other kmem helpers but those are bit different because regular page/slab allocators allow never fail semantic (even though this is mostly ignored by those helpers which implement their own retries but that is a different topic).

... but what I'm trying to understand here is whether this failure scenario is specific to vmalloc() or whether the other kmem_*() functions are susceptible to the same problem. For example, suppose we replaced this kmem_zalloc_greedy() call with a kmem_zalloc(PAGE_SIZE, KM_SLEEP) call. Could we hit the same problem if the process is killed?

Quoted 1 line · Brian Foster

On Thu, Mar 02, 2017 at 02:27:55PM +0100, Michal Hocko wrote:

[...]

Quoted 11 lines

Quoted 5 lines

I see your argument about being in sync with other kmem helpers but those are bit different because regular page/slab allocators allow never fail semantic (even though this is mostly ignored by those helpers which implement their own retries but that is a different topic).

... but what I'm trying to understand here is whether this failure scenario is specific to vmalloc() or whether the other kmem_*() functions are susceptible to the same problem. For example, suppose we replaced this kmem_zalloc_greedy() call with a kmem_zalloc(PAGE_SIZE, KM_SLEEP) call. Could we hit the same problem if the process is killed?

Well, kmem_zalloc uses kmalloc which can also fail when we are out of memory but in that case we can expect the OOM killer releasing some memory which would allow us to make a forward progress on the next retry. So essentially retrying around kmalloc is much more safe in this regard. Failing vmalloc might be permanent because there is no vmalloc space to allocate from or much more likely due to already mentioned patch. So vmalloc is different, really.

Signature · 8 lines

Michal Hocko
SUSE Labs

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

Quoted 15 lines · Michal Hocko

Quoted 1 line · Brian Foster

On Thu, Mar 02, 2017 at 02:27:55PM +0100, Michal Hocko wrote:

[...]

Quoted 11 lines

Quoted 5 lines

I see your argument about being in sync with other kmem helpers but those are bit different because regular page/slab allocators allow never fail semantic (even though this is mostly ignored by those helpers which implement their own retries but that is a different topic).

... but what I'm trying to understand here is whether this failure scenario is specific to vmalloc() or whether the other kmem_*() functions are susceptible to the same problem. For example, suppose we replaced this kmem_zalloc_greedy() call with a kmem_zalloc(PAGE_SIZE, KM_SLEEP) call. Could we hit the same problem if the process is killed?

Quoted 7 lines

Well, kmem_zalloc uses kmalloc which can also fail when we are out of memory but in that case we can expect the OOM killer releasing some memory which would allow us to make a forward progress on the next retry. So essentially retrying around kmalloc is much more safe in this regard. Failing vmalloc might be permanent because there is no vmalloc space to allocate from or much more likely due to already mentioned patch. So vmalloc is different, really.

Right.. that's why I'm asking. So it's technically possible but highly unlikely due to the different failure characteristics. That seems reasonable to me, then.

To be clear, do we understand what causes the vzalloc() failure to be effectively permanent in this specific reproducer? I know you mention above that we could be out of vmalloc space, but that doesn't clarify whether there are other potential failure paths or then what this has to do with the fact that the process was killed. Does the pending signal cause the subsequent failures or are you saying that there is some other root cause of the failure, this process would effectively be spinning here anyways, and we're just noticing it because it's trying to exit?

Quoted 28 lines · Brian Foster

Quoted 15 lines · Michal Hocko

Quoted 1 line · Brian Foster

On Thu, Mar 02, 2017 at 02:27:55PM +0100, Michal Hocko wrote:

[...]

Quoted 11 lines

Quoted 5 lines

I see your argument about being in sync with other kmem helpers but those are bit different because regular page/slab allocators allow never fail semantic (even though this is mostly ignored by those helpers which implement their own retries but that is a different topic).

... but what I'm trying to understand here is whether this failure scenario is specific to vmalloc() or whether the other kmem_*() functions are susceptible to the same problem. For example, suppose we replaced this kmem_zalloc_greedy() call with a kmem_zalloc(PAGE_SIZE, KM_SLEEP) call. Could we hit the same problem if the process is killed?

Quoted 7 lines

Well, kmem_zalloc uses kmalloc which can also fail when we are out of memory but in that case we can expect the OOM killer releasing some memory which would allow us to make a forward progress on the next retry. So essentially retrying around kmalloc is much more safe in this regard. Failing vmalloc might be permanent because there is no vmalloc space to allocate from or much more likely due to already mentioned patch. So vmalloc is different, really.

Right.. that's why I'm asking. So it's technically possible but highly unlikely due to the different failure characteristics. That seems reasonable to me, then.

Quoted 8 lines

To be clear, do we understand what causes the vzalloc() failure to be effectively permanent in this specific reproducer? I know you mention above that we could be out of vmalloc space, but that doesn't clarify whether there are other potential failure paths or then what this has to do with the fact that the process was killed. Does the pending signal cause the subsequent failures or are you saying that there is some other root cause of the failure, this process would effectively be spinning here anyways, and we're just noticing it because it's trying to exit?

In this particular case it is fatal_signal_pending that causes the permanent failure. This check has been added to prevent from complete memory reserves depletion on OOM when a killed task has a free ticket to reserves and vmalloc requests can be really large. In this case there was no OOM killer going on but fsstress has SIGKILL pending for other reason. Most probably as a result of the group_exit when all threads are killed (see zap_process). I could have turn fatal_signal_pending into tsk_is_oom_victim which would be less likely to hit but in principle fatal_signal_pending should be better because we do want to bail out when the process is existing as soon as possible.

What I really wanted to say is that there are other possible permanent failure paths in vmalloc AFAICS. They are much less probable but they still exist.

Does that make more sense now?

Signature · 8 lines

Michal Hocko
SUSE Labs

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

Quoted 49 lines · Michal Hocko

Quoted 28 lines · Brian Foster

Quoted 15 lines · Michal Hocko

Quoted 1 line · Brian Foster

On Thu, Mar 02, 2017 at 02:27:55PM +0100, Michal Hocko wrote:

[...]

Quoted 11 lines

Quoted 5 lines

I see your argument about being in sync with other kmem helpers but those are bit different because regular page/slab allocators allow never fail semantic (even though this is mostly ignored by those helpers which implement their own retries but that is a different topic).

... but what I'm trying to understand here is whether this failure scenario is specific to vmalloc() or whether the other kmem_*() functions are susceptible to the same problem. For example, suppose we replaced this kmem_zalloc_greedy() call with a kmem_zalloc(PAGE_SIZE, KM_SLEEP) call. Could we hit the same problem if the process is killed?

Quoted 7 lines

Well, kmem_zalloc uses kmalloc which can also fail when we are out of memory but in that case we can expect the OOM killer releasing some memory which would allow us to make a forward progress on the next retry. So essentially retrying around kmalloc is much more safe in this regard. Failing vmalloc might be permanent because there is no vmalloc space to allocate from or much more likely due to already mentioned patch. So vmalloc is different, really.

Right.. that's why I'm asking. So it's technically possible but highly unlikely due to the different failure characteristics. That seems reasonable to me, then.

Quoted 8 lines

To be clear, do we understand what causes the vzalloc() failure to be effectively permanent in this specific reproducer? I know you mention above that we could be out of vmalloc space, but that doesn't clarify whether there are other potential failure paths or then what this has to do with the fact that the process was killed. Does the pending signal cause the subsequent failures or are you saying that there is some other root cause of the failure, this process would effectively be spinning here anyways, and we're just noticing it because it's trying to exit?

In this particular case it is fatal_signal_pending that causes the permanent failure. This check has been added to prevent from complete memory reserves depletion on OOM when a killed task has a free ticket to reserves and vmalloc requests can be really large. In this case there was no OOM killer going on but fsstress has SIGKILL pending for other reason. Most probably as a result of the group_exit when all threads are killed (see zap_process). I could have turn fatal_signal_pending into tsk_is_oom_victim which would be less likely to hit but in principle fatal_signal_pending should be better because we do want to bail out when the process is existing as soon as possible.

Quoted 5 lines

What I really wanted to say is that there are other possible permanent failure paths in vmalloc AFAICS. They are much less probable but they still exist.

Does that make more sense now?

Yes, thanks. That explains why this crops up now where it hasn't in the past. Please include that background in the commit log description.

Also, that kind of makes me think that a fatal_signal_pending() check is still appropriate in the loop, even if we want to drop the infinite retry loop in kmem_zalloc_greedy() as well. There's no sense in doing however many retries are left before we return and that's also more explicit for the next person who goes to change this code in the future.

Otherwise, I'm fine with breaking the infinite retry loop at the same time. It looks like Christoph added this function originally so this should probably require his ack as well..

Quoted 54 lines · Brian Foster

Quoted 53 lines · Michal Hocko

Quoted 28 lines · Brian Foster

Quoted 15 lines · Michal Hocko

Quoted 1 line · Brian Foster

On Thu, Mar 02, 2017 at 02:27:55PM +0100, Michal Hocko wrote:

[...]

Quoted 11 lines

Quoted 5 lines

I see your argument about being in sync with other kmem helpers but those are bit different because regular page/slab allocators allow never fail semantic (even though this is mostly ignored by those helpers which implement their own retries but that is a different topic).

... but what I'm trying to understand here is whether this failure scenario is specific to vmalloc() or whether the other kmem_*() functions are susceptible to the same problem. For example, suppose we replaced this kmem_zalloc_greedy() call with a kmem_zalloc(PAGE_SIZE, KM_SLEEP) call. Could we hit the same problem if the process is killed?

Quoted 7 lines

Well, kmem_zalloc uses kmalloc which can also fail when we are out of memory but in that case we can expect the OOM killer releasing some memory which would allow us to make a forward progress on the next retry. So essentially retrying around kmalloc is much more safe in this regard. Failing vmalloc might be permanent because there is no vmalloc space to allocate from or much more likely due to already mentioned patch. So vmalloc is different, really.

Right.. that's why I'm asking. So it's technically possible but highly unlikely due to the different failure characteristics. That seems reasonable to me, then.

Quoted 8 lines

To be clear, do we understand what causes the vzalloc() failure to be effectively permanent in this specific reproducer? I know you mention above that we could be out of vmalloc space, but that doesn't clarify whether there are other potential failure paths or then what this has to do with the fact that the process was killed. Does the pending signal cause the subsequent failures or are you saying that there is some other root cause of the failure, this process would effectively be spinning here anyways, and we're just noticing it because it's trying to exit?

In this particular case it is fatal_signal_pending that causes the permanent failure. This check has been added to prevent from complete memory reserves depletion on OOM when a killed task has a free ticket to reserves and vmalloc requests can be really large. In this case there was no OOM killer going on but fsstress has SIGKILL pending for other reason. Most probably as a result of the group_exit when all threads are killed (see zap_process). I could have turn fatal_signal_pending into tsk_is_oom_victim which would be less likely to hit but in principle fatal_signal_pending should be better because we do want to bail out when the process is existing as soon as possible.

What I really wanted to say is that there are other possible permanent failure paths in vmalloc AFAICS. They are much less probable but they still exist.

Quoted 4 lines

Quoted 1 line

Does that make more sense now?

Yes, thanks. That explains why this crops up now where it hasn't in the past. Please include that background in the commit log description.

OK, does this sound better. I am open to any suggestions to improve this of course

: xfs: allow kmem_zalloc_greedy to fail
:
: Even though kmem_zalloc_greedy is documented it might fail the current : code doesn't really implement this properly and loops on the smallest : allowed size for ever. This is a problem because vzalloc might fail : permanently - we might run out of vmalloc space or since 5d17a73a2ebe : ("vmalloc: back off when the current task is killed") when the current : task is killed. The later one makes the failure scenario much more : probable than it used to be. Fix this by bailing out if the minimum size

: request failed.
:
: This has been noticed by a hung generic/269 xfstest by Xiong Zhou.
:
: fsstress: vmalloc: allocation failure, allocated 12288 of 20480 bytes, mode:0x14080c2(GFP_KERNEL|__GFP_HIGHMEM|__GFP_ZERO), nodemask=(null) : fsstress cpuset=/ mems_allowed=0-1
: CPU: 1 PID: 23460 Comm: fsstress Not tainted 4.10.0-master-45554b2+ #21
: Hardware name: HP ProLiant DL380 Gen9/ProLiant DL380 Gen9, BIOS P89 10/05/2016
: Call Trace:

:  dump_stack+0x63/0x87
:  warn_alloc+0x114/0x1c0
:  ? alloc_pages_current+0x88/0x120
:  __vmalloc_node_range+0x250/0x2a0
:  ? kmem_zalloc_greedy+0x2b/0x40 [xfs]
:  ? free_hot_cold_page+0x21f/0x280
:  vzalloc+0x54/0x60
:  ? kmem_zalloc_greedy+0x2b/0x40 [xfs]
:  kmem_zalloc_greedy+0x2b/0x40 [xfs]
:  xfs_bulkstat+0x11b/0x730 [xfs]
:  ? xfs_bulkstat_one_int+0x340/0x340 [xfs]
:  ? selinux_capable+0x20/0x30
:  ? security_capable+0x48/0x60
:  xfs_ioc_bulkstat+0xe4/0x190 [xfs]
:  xfs_file_ioctl+0x9dd/0xad0 [xfs]
:  ? do_filp_open+0xa5/0x100
:  do_vfs_ioctl+0xa7/0x5e0
:  SyS_ioctl+0x79/0x90
:  do_syscall_64+0x67/0x180
:  entry_SYSCALL64_slow_path+0x25/0x25

:
: fsstress keeps looping inside kmem_zalloc_greedy without any way out : because vmalloc keeps failing due to fatal_signal_pending. :
: Reported-by: Xiong Zhou <[email protected]>
: Analyzed-by: Tetsuo Handa <[email protected]> : Signed-off-by: Michal Hocko <[email protected]>

Quoted 5 lines

Also, that kind of makes me think that a fatal_signal_pending() check is still appropriate in the loop, even if we want to drop the infinite retry loop in kmem_zalloc_greedy() as well. There's no sense in doing however many retries are left before we return and that's also more explicit for the next person who goes to change this code in the future.

I am not objecting to adding fatal_signal_pending as well I just thought that from the logic POV breaking after reaching the minimum size is just the right thing to do. We can optimize further by checking fatal_signal_pending and reducing retries when we know it doesn't make much sense but that should be done on top as an optimization IMHO.

Quoted 3 lines

Otherwise, I'm fine with breaking the infinite retry loop at the same time. It looks like Christoph added this function originally so this should probably require his ack as well..

What do you think Christoph?

Signature · 2 lines

Michal Hocko
SUSE Labs

From: Michal Hocko <[email protected]>

It doesn't really make much sense to retry vmalloc request if the current task is killed. We should rather bail out as soon as possible and let it RIP as soon as possible. The current implementation of vmalloc will fail anyway.

Suggested-by Brian Foster <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---

 fs/xfs/kmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/xfs/kmem.c +1 -1

index ee95f5c6db45..01c52567a4ff 100644
@@ -34,7 +34,7 @@ kmem_zalloc_greedy(size_t *size, size_t minsize, size_t maxsize)
 	size_t		kmsize = maxsize;
 
 	while (!(ptr = vzalloc(kmsize))) {
-		if (kmsize == minsize)
+		if (kmsize == minsize || fatal_signal_pending(current))
 			break;
 		if ((kmsize >>= 1) <= minsize)
 			kmsize = minsize;

Signature · 6 lines

2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Michal Hocko <[email protected]>

Even though kmem_zalloc_greedy is documented it might fail the current code doesn't really implement this properly and loops on the smallest allowed size for ever. This is a problem because vzalloc might fail permanently - we might run out of vmalloc space or since 5d17a73a2ebe ("vmalloc: back off when the current task is killed") when the current task is killed. The later one makes the failure scenario much more probable than it used to be because it makes vmalloc() failures permanent for tasks with fatal signals pending.. Fix this by bailing out if the minimum size request failed.

This has been noticed by a hung generic/269 xfstest by Xiong Zhou.

fsstress: vmalloc: allocation failure, allocated 12288 of 20480 bytes, mode:0x14080c2(GFP_KERNEL|__GFP_HIGHMEM|__GFP_ZERO), nodemask=(null) fsstress cpuset=/ mems_allowed=0-1

CPU: 1 PID: 23460 Comm: fsstress Not tainted 4.10.0-master-45554b2+ #21
Hardware name: HP ProLiant DL380 Gen9/ProLiant DL380 Gen9, BIOS P89 10/05/2016
Call Trace:
 dump_stack+0x63/0x87
 warn_alloc+0x114/0x1c0
 ? alloc_pages_current+0x88/0x120
 __vmalloc_node_range+0x250/0x2a0
 ? kmem_zalloc_greedy+0x2b/0x40 [xfs]
 ? free_hot_cold_page+0x21f/0x280
 vzalloc+0x54/0x60
 ? kmem_zalloc_greedy+0x2b/0x40 [xfs]
 kmem_zalloc_greedy+0x2b/0x40 [xfs]
 xfs_bulkstat+0x11b/0x730 [xfs]
 ? xfs_bulkstat_one_int+0x340/0x340 [xfs]
 ? selinux_capable+0x20/0x30
 ? security_capable+0x48/0x60
 xfs_ioc_bulkstat+0xe4/0x190 [xfs]
 xfs_file_ioctl+0x9dd/0xad0 [xfs]
 ? do_filp_open+0xa5/0x100
 do_vfs_ioctl+0xa7/0x5e0
 SyS_ioctl+0x79/0x90
 do_syscall_64+0x67/0x180
 entry_SYSCALL64_slow_path+0x25/0x25

fsstress keeps looping inside kmem_zalloc_greedy without any way out because vmalloc keeps failing due to fatal_signal_pending.

Reported-by Xiong Zhou <[email protected]>

Analyzed-by Tetsuo Handa <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---

 fs/xfs/kmem.c | 2 ++
 1 file changed, 2 insertions(+)

fs/xfs/kmem.c +2 -0

index 339c696bbc01..ee95f5c6db45 100644
@@ -34,6 +34,8 @@ kmem_zalloc_greedy(size_t *size, size_t minsize, size_t maxsize)
 	size_t		kmsize = maxsize;
 
 	while (!(ptr = vzalloc(kmsize))) {
+		if (kmsize == minsize)
+			break;
 		if ((kmsize >>= 1) <= minsize)
 			kmsize = minsize;
 	}

Signature · 6 lines

2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Michal Hocko <[email protected]>

Even though kmem_zalloc_greedy is documented it might fail the current code doesn't really implement this properly and loops on the smallest allowed size for ever. This is a problem because vzalloc might fail permanently - we might run out of vmalloc space or since 5d17a73a2ebe ("vmalloc: back off when the current task is killed") when the current task is killed. The later one makes the failure scenario much more probable than it used to be because it makes vmalloc() failures permanent for tasks with fatal signals pending.. Fix this by bailing out if the minimum size request failed.

This has been noticed by a hung generic/269 xfstest by Xiong Zhou.

fsstress: vmalloc: allocation failure, allocated 12288 of 20480 bytes, mode:0x14080c2(GFP_KERNEL|__GFP_HIGHMEM|__GFP_ZERO), nodemask=(null) fsstress cpuset=/ mems_allowed=0-1

CPU: 1 PID: 23460 Comm: fsstress Not tainted 4.10.0-master-45554b2+ #21
Hardware name: HP ProLiant DL380 Gen9/ProLiant DL380 Gen9, BIOS P89 10/05/2016
Call Trace:
 dump_stack+0x63/0x87
 warn_alloc+0x114/0x1c0
 ? alloc_pages_current+0x88/0x120
 __vmalloc_node_range+0x250/0x2a0
 ? kmem_zalloc_greedy+0x2b/0x40 [xfs]
 ? free_hot_cold_page+0x21f/0x280
 vzalloc+0x54/0x60
 ? kmem_zalloc_greedy+0x2b/0x40 [xfs]
 kmem_zalloc_greedy+0x2b/0x40 [xfs]
 xfs_bulkstat+0x11b/0x730 [xfs]
 ? xfs_bulkstat_one_int+0x340/0x340 [xfs]
 ? selinux_capable+0x20/0x30
 ? security_capable+0x48/0x60
 xfs_ioc_bulkstat+0xe4/0x190 [xfs]
 xfs_file_ioctl+0x9dd/0xad0 [xfs]
 ? do_filp_open+0xa5/0x100
 do_vfs_ioctl+0xa7/0x5e0
 SyS_ioctl+0x79/0x90
 do_syscall_64+0x67/0x180
 entry_SYSCALL64_slow_path+0x25/0x25

fsstress keeps looping inside kmem_zalloc_greedy without any way out because vmalloc keeps failing due to fatal_signal_pending.

Reported-by Xiong Zhou <[email protected]>

Analyzed-by Tetsuo Handa <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 fs/xfs/kmem.c | 2 ++
 1 file changed, 2 insertions(+)

fs/xfs/kmem.c +2 -0

index 339c696bbc01..ee95f5c6db45 100644
@@ -34,6 +34,8 @@ kmem_zalloc_greedy(size_t *size, size_t minsize, size_t maxsize)
 	size_t		kmsize = maxsize;
 
 	while (!(ptr = vzalloc(kmsize))) {
+		if (kmsize == minsize)
+			break;
 		if ((kmsize >>= 1) <= minsize)
 			kmsize = minsize;
 	}

Signature · 1 line

2.11.0

I've just realized that Darrick was not on the CC list. Let's add him. I believe this patch should go in in the current cycle because 5d17a73a2ebe was merged in this merge window and it can be abused...

The other patch [1] is not that urgent.

[1] http://lkml.kernel.org/r/[email protected]

Quoted 57 lines

From: Michal Hocko <[email protected]>

Even though kmem_zalloc_greedy is documented it might fail the current code doesn't really implement this properly and loops on the smallest allowed size for ever. This is a problem because vzalloc might fail permanently - we might run out of vmalloc space or since 5d17a73a2ebe ("vmalloc: back off when the current task is killed") when the current task is killed. The later one makes the failure scenario much more probable than it used to be because it makes vmalloc() failures permanent for tasks with fatal signals pending.. Fix this by bailing out if the minimum size request failed.

This has been noticed by a hung generic/269 xfstest by Xiong Zhou.

fsstress: vmalloc: allocation failure, allocated 12288 of 20480 bytes, mode:0x14080c2(GFP_KERNEL|__GFP_HIGHMEM|__GFP_ZERO), nodemask=(null) fsstress cpuset=/ mems_allowed=0-1

CPU: 1 PID: 23460 Comm: fsstress Not tainted 4.10.0-master-45554b2+ #21
Hardware name: HP ProLiant DL380 Gen9/ProLiant DL380 Gen9, BIOS P89 10/05/2016
Call Trace:
 dump_stack+0x63/0x87
 warn_alloc+0x114/0x1c0
 ? alloc_pages_current+0x88/0x120
 __vmalloc_node_range+0x250/0x2a0
 ? kmem_zalloc_greedy+0x2b/0x40 [xfs]
 ? free_hot_cold_page+0x21f/0x280
 vzalloc+0x54/0x60
 ? kmem_zalloc_greedy+0x2b/0x40 [xfs]
 kmem_zalloc_greedy+0x2b/0x40 [xfs]
 xfs_bulkstat+0x11b/0x730 [xfs]
 ? xfs_bulkstat_one_int+0x340/0x340 [xfs]
 ? selinux_capable+0x20/0x30
 ? security_capable+0x48/0x60
 xfs_ioc_bulkstat+0xe4/0x190 [xfs]
 xfs_file_ioctl+0x9dd/0xad0 [xfs]
 ? do_filp_open+0xa5/0x100
 do_vfs_ioctl+0xa7/0x5e0
 SyS_ioctl+0x79/0x90
 do_syscall_64+0x67/0x180
 entry_SYSCALL64_slow_path+0x25/0x25

fsstress keeps looping inside kmem_zalloc_greedy without any way out because vmalloc keeps failing due to fatal_signal_pending.

Reported-by Xiong Zhou <[email protected]>

Analyzed-by Tetsuo Handa <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 fs/xfs/kmem.c | 2 ++
 1 file changed, 2 insertions(+)

fs/xfs/kmem.c +0 -0

index 339c696bbc01..ee95f5c6db45 100644
@@ -34,6 +34,8 @@ kmem_zalloc_greedy(size_t *size, size_t minsize, size_t maxsize)
 	size_t		kmsize = maxsize;
 

Quoted 9 lines

fs/xfs/kmem.c +2 -0

 	while (!(ptr = vzalloc(kmsize))) {
+		if (kmsize == minsize)
+			break;
 		if ((kmsize >>= 1) <= minsize)
 			kmsize = minsize;
 	}

Signature · 1 line

2.11.0

Signature · 6 lines

Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Michal Hocko <[email protected]>

Even though kmem_zalloc_greedy is documented it might fail the current code doesn't really implement this properly and loops on the smallest allowed size for ever. This is a problem because vzalloc might fail permanently - we might run out of vmalloc space or since 5d17a73a2ebe ("vmalloc: back off when the current task is killed") when the current task is killed. The later one makes the failure scenario much more probable than it used to be because it makes vmalloc() failures permanent for tasks with fatal signals pending.. Fix this by bailing out if the minimum size request failed.

This has been noticed by a hung generic/269 xfstest by Xiong Zhou.

fsstress: vmalloc: allocation failure, allocated 12288 of 20480 bytes, mode:0x14080c2(GFP_KERNEL|__GFP_HIGHMEM|__GFP_ZERO), nodemask=(null) fsstress cpuset=/ mems_allowed=0-1

CPU: 1 PID: 23460 Comm: fsstress Not tainted 4.10.0-master-45554b2+ #21
Hardware name: HP ProLiant DL380 Gen9/ProLiant DL380 Gen9, BIOS P89 10/05/2016
Call Trace:
 dump_stack+0x63/0x87
 warn_alloc+0x114/0x1c0
 ? alloc_pages_current+0x88/0x120
 __vmalloc_node_range+0x250/0x2a0
 ? kmem_zalloc_greedy+0x2b/0x40 [xfs]
 ? free_hot_cold_page+0x21f/0x280
 vzalloc+0x54/0x60
 ? kmem_zalloc_greedy+0x2b/0x40 [xfs]
 kmem_zalloc_greedy+0x2b/0x40 [xfs]
 xfs_bulkstat+0x11b/0x730 [xfs]
 ? xfs_bulkstat_one_int+0x340/0x340 [xfs]
 ? selinux_capable+0x20/0x30
 ? security_capable+0x48/0x60
 xfs_ioc_bulkstat+0xe4/0x190 [xfs]
 xfs_file_ioctl+0x9dd/0xad0 [xfs]
 ? do_filp_open+0xa5/0x100
 do_vfs_ioctl+0xa7/0x5e0
 SyS_ioctl+0x79/0x90
 do_syscall_64+0x67/0x180
 entry_SYSCALL64_slow_path+0x25/0x25

fsstress keeps looping inside kmem_zalloc_greedy without any way out because vmalloc keeps failing due to fatal_signal_pending.

Reported-by Xiong Zhou <[email protected]>

Analyzed-by Tetsuo Handa <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 fs/xfs/kmem.c | 2 ++
 1 file changed, 2 insertions(+)

fs/xfs/kmem.c +0 -0

index 339c696bbc01..ee95f5c6db45 100644

Quoted 57 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Even though kmem_zalloc_greedy is documented it might fail the current code doesn't really implement this properly and loops on the smallest allowed size for ever. This is a problem because vzalloc might fail permanently - we might run out of vmalloc space or since 5d17a73a2ebe ("vmalloc: back off when the current task is killed") when the current task is killed. The later one makes the failure scenario much more probable than it used to be because it makes vmalloc() failures permanent for tasks with fatal signals pending.. Fix this by bailing out if the minimum size request failed.

This has been noticed by a hung generic/269 xfstest by Xiong Zhou.

fsstress: vmalloc: allocation failure, allocated 12288 of 20480 bytes, mode:0x14080c2(GFP_KERNEL|__GFP_HIGHMEM|__GFP_ZERO), nodemask=(null) fsstress cpuset=/ mems_allowed=0-1

CPU: 1 PID: 23460 Comm: fsstress Not tainted 4.10.0-master-45554b2+ #21
Hardware name: HP ProLiant DL380 Gen9/ProLiant DL380 Gen9, BIOS P89 10/05/2016
Call Trace:
 dump_stack+0x63/0x87
 warn_alloc+0x114/0x1c0
 ? alloc_pages_current+0x88/0x120
 __vmalloc_node_range+0x250/0x2a0
 ? kmem_zalloc_greedy+0x2b/0x40 [xfs]
 ? free_hot_cold_page+0x21f/0x280
 vzalloc+0x54/0x60
 ? kmem_zalloc_greedy+0x2b/0x40 [xfs]
 kmem_zalloc_greedy+0x2b/0x40 [xfs]
 xfs_bulkstat+0x11b/0x730 [xfs]
 ? xfs_bulkstat_one_int+0x340/0x340 [xfs]
 ? selinux_capable+0x20/0x30
 ? security_capable+0x48/0x60
 xfs_ioc_bulkstat+0xe4/0x190 [xfs]
 xfs_file_ioctl+0x9dd/0xad0 [xfs]
 ? do_filp_open+0xa5/0x100
 do_vfs_ioctl+0xa7/0x5e0
 SyS_ioctl+0x79/0x90
 do_syscall_64+0x67/0x180
 entry_SYSCALL64_slow_path+0x25/0x25

fsstress keeps looping inside kmem_zalloc_greedy without any way out because vmalloc keeps failing due to fatal_signal_pending.

Reported-by Xiong Zhou <[email protected]>

Analyzed-by Tetsuo Handa <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 fs/xfs/kmem.c | 2 ++
 1 file changed, 2 insertions(+)

fs/xfs/kmem.c +0 -0

index 339c696bbc01..ee95f5c6db45 100644
@@ -34,6 +34,8 @@ kmem_zalloc_greedy(size_t *size, size_t minsize, size_t maxsize)
 	size_t		kmsize = maxsize;
 

fs/xfs/kmem.c kmem_zalloc_greedy(size_t *size, size_t minsize, size_t maxsize) · line 41

Quoted 6 lines

fs/xfs/kmem.c +2 -0

 	while (!(ptr = vzalloc(kmsize))) {
+		if (kmsize == minsize)
+			break;
 		if ((kmsize >>= 1) <= minsize)
 			kmsize = minsize;
 	}

Seems wrong to me - this function used to have lots of callers and over time we've slowly removed them or replaced them with something else. I'd suggest removing it completely, replacing the call sites with kmem_zalloc_large().

Cheers,

Dave.

Signature · 6 lines

Dave Chinner
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 58 lines · Dave Chinner

Quoted 57 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Even though kmem_zalloc_greedy is documented it might fail the current code doesn't really implement this properly and loops on the smallest allowed size for ever. This is a problem because vzalloc might fail permanently - we might run out of vmalloc space or since 5d17a73a2ebe ("vmalloc: back off when the current task is killed") when the current task is killed. The later one makes the failure scenario much more probable than it used to be because it makes vmalloc() failures permanent for tasks with fatal signals pending.. Fix this by bailing out if the minimum size request failed.

This has been noticed by a hung generic/269 xfstest by Xiong Zhou.

fsstress: vmalloc: allocation failure, allocated 12288 of 20480 bytes, mode:0x14080c2(GFP_KERNEL|__GFP_HIGHMEM|__GFP_ZERO), nodemask=(null) fsstress cpuset=/ mems_allowed=0-1

CPU: 1 PID: 23460 Comm: fsstress Not tainted 4.10.0-master-45554b2+ #21
Hardware name: HP ProLiant DL380 Gen9/ProLiant DL380 Gen9, BIOS P89 10/05/2016
Call Trace:
 dump_stack+0x63/0x87
 warn_alloc+0x114/0x1c0
 ? alloc_pages_current+0x88/0x120
 __vmalloc_node_range+0x250/0x2a0
 ? kmem_zalloc_greedy+0x2b/0x40 [xfs]
 ? free_hot_cold_page+0x21f/0x280
 vzalloc+0x54/0x60
 ? kmem_zalloc_greedy+0x2b/0x40 [xfs]
 kmem_zalloc_greedy+0x2b/0x40 [xfs]
 xfs_bulkstat+0x11b/0x730 [xfs]
 ? xfs_bulkstat_one_int+0x340/0x340 [xfs]
 ? selinux_capable+0x20/0x30
 ? security_capable+0x48/0x60
 xfs_ioc_bulkstat+0xe4/0x190 [xfs]
 xfs_file_ioctl+0x9dd/0xad0 [xfs]
 ? do_filp_open+0xa5/0x100
 do_vfs_ioctl+0xa7/0x5e0
 SyS_ioctl+0x79/0x90
 do_syscall_64+0x67/0x180
 entry_SYSCALL64_slow_path+0x25/0x25

fsstress keeps looping inside kmem_zalloc_greedy without any way out because vmalloc keeps failing due to fatal_signal_pending.

Reported-by Xiong Zhou <[email protected]>

Analyzed-by Tetsuo Handa <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 fs/xfs/kmem.c | 2 ++
 1 file changed, 2 insertions(+)

fs/xfs/kmem.c +0 -0

index 339c696bbc01..ee95f5c6db45 100644
@@ -34,6 +34,8 @@ kmem_zalloc_greedy(size_t *size, size_t minsize, size_t maxsize)
 	size_t		kmsize = maxsize;
 

Quoted 11 lines

fs/xfs/kmem.c kmem_zalloc_greedy(size_t *size, size_t minsize, size_t maxsize) · line 41

Quoted 6 lines

fs/xfs/kmem.c +2 -0

 	while (!(ptr = vzalloc(kmsize))) {
+		if (kmsize == minsize)
+			break;
 		if ((kmsize >>= 1) <= minsize)
 			kmsize = minsize;
 	}

Seems wrong to me - this function used to have lots of callers and over time we've slowly removed them or replaced them with something else. I'd suggest removing it completely, replacing the call sites with kmem_zalloc_large().

Heh. I thought the reason why _greedy still exists (for its sole user bulkstat) is that bulkstat had the flexibility to deal with receiving 0, 1, or 4 pages. So yeah, we could just kill it.

But thinking even more stingily about memory, are there applications that care about being able to bulkstat 16384 inodes at once?

How badly
does bulkstat need to be able to bulk-process more than a page's worth of inobt records, anyway?

Quoted 57 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Even though kmem_zalloc_greedy is documented it might fail the current code doesn't really implement this properly and loops on the smallest allowed size for ever. This is a problem because vzalloc might fail permanently - we might run out of vmalloc space or since 5d17a73a2ebe ("vmalloc: back off when the current task is killed") when the current task is killed. The later one makes the failure scenario much more probable than it used to be because it makes vmalloc() failures permanent for tasks with fatal signals pending.. Fix this by bailing out if the minimum size request failed.

This has been noticed by a hung generic/269 xfstest by Xiong Zhou.

fsstress: vmalloc: allocation failure, allocated 12288 of 20480 bytes, mode:0x14080c2(GFP_KERNEL|__GFP_HIGHMEM|__GFP_ZERO), nodemask=(null) fsstress cpuset=/ mems_allowed=0-1

CPU: 1 PID: 23460 Comm: fsstress Not tainted 4.10.0-master-45554b2+ #21
Hardware name: HP ProLiant DL380 Gen9/ProLiant DL380 Gen9, BIOS P89 10/05/2016
Call Trace:
 dump_stack+0x63/0x87
 warn_alloc+0x114/0x1c0
 ? alloc_pages_current+0x88/0x120
 __vmalloc_node_range+0x250/0x2a0
 ? kmem_zalloc_greedy+0x2b/0x40 [xfs]
 ? free_hot_cold_page+0x21f/0x280
 vzalloc+0x54/0x60
 ? kmem_zalloc_greedy+0x2b/0x40 [xfs]
 kmem_zalloc_greedy+0x2b/0x40 [xfs]
 xfs_bulkstat+0x11b/0x730 [xfs]
 ? xfs_bulkstat_one_int+0x340/0x340 [xfs]
 ? selinux_capable+0x20/0x30
 ? security_capable+0x48/0x60
 xfs_ioc_bulkstat+0xe4/0x190 [xfs]
 xfs_file_ioctl+0x9dd/0xad0 [xfs]
 ? do_filp_open+0xa5/0x100
 do_vfs_ioctl+0xa7/0x5e0
 SyS_ioctl+0x79/0x90
 do_syscall_64+0x67/0x180
 entry_SYSCALL64_slow_path+0x25/0x25

fsstress keeps looping inside kmem_zalloc_greedy without any way out because vmalloc keeps failing due to fatal_signal_pending.

Reported-by Xiong Zhou <[email protected]>

Analyzed-by Tetsuo Handa <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 fs/xfs/kmem.c | 2 ++
 1 file changed, 2 insertions(+)

fs/xfs/kmem.c +0 -0

index 339c696bbc01..ee95f5c6db45 100644
@@ -34,6 +34,8 @@ kmem_zalloc_greedy(size_t *size, size_t minsize, size_t maxsize)
 	size_t		kmsize = maxsize;
 

fs/xfs/kmem.c kmem_zalloc_greedy(size_t *size, size_t minsize, size_t maxsize) · line 41

Quoted 6 lines

fs/xfs/kmem.c +2 -0

 	while (!(ptr = vzalloc(kmsize))) {
+		if (kmsize == minsize)
+			break;
 		if ((kmsize >>= 1) <= minsize)
 			kmsize = minsize;
 	}

Seems wrong to me - this function used to have lots of callers and over time we've slowly removed them or replaced them with something else. I'd suggest removing it completely, replacing the call sites with kmem_zalloc_large().

On Thu, Mar 02, 2017 at 04:14:11PM +0100, Michal Hocko wrote:

Quoted 6 lines

I am not objecting to adding fatal_signal_pending as well I just thought that from the logic POV breaking after reaching the minimum size is just the right thing to do. We can optimize further by checking fatal_signal_pending and reducing retries when we know it doesn't make much sense but that should be done on top as an optimization IMHO.

I don't think of it as an optimization to not invoke calls (a non-deterministic number of times) that we know are going to fail, but

ultimately I don't care too much how it's framed or if it's done in separate patches or whatnot. As long as they are posted at the same time. ;)

Otherwise, I'm fine with breaking the infinite retry loop at the same time. It looks like Christoph added this function originally so this should probably require his ack as well..