vmalloc with GFP_NOFS

Hi,
it seems that we still have few vmalloc users who perform GFP_NOFS allocation:
drivers/mtd/ubi/io.c
fs/ext4/xattr.c
fs/gfs2/dir.c
fs/gfs2/quota.c
fs/nfs/blocklayout/extent_tree.c
fs/ubifs/debug.c
fs/ubifs/lprops.c
fs/ubifs/lpt_commit.c
fs/ubifs/orphan.c

Unfortunatelly vmalloc doesn't suppoer GFP_NOFS semantinc properly because we do have hardocded GFP_KERNEL allocations deep inside the vmalloc layers. That means that if GFP_NOFS really protects from recursion into the fs deadlocks then the vmalloc call is broken.

What to do about this? Well, there are two things. Firstly, it would be really great to double check whether the GFP_NOFS is really needed. I cannot judge that because I am not familiar with the code. It would be great if the respective maintainers (hopefully get_maintainer.sh pointed me to all relevant ones). If there is not reclaim recursion issue then simply use the standard vmalloc (aka GFP_KERNEL request).

If the use is really valid then we have a way to do the vmalloc allocation properly. We have memalloc_nofs_{save,restore} scope api. How does that work? You simply call memalloc_nofs_save when the reclaim recursion critical section starts (e.g. when you take a lock which is then used in the reclaim path - e.g. shrinker) and memalloc_nofs_restore when the critical section ends. _All_ allocations within that scope will get GFP_NOFS semantic automagically. If you are not sure about the scope itself then the easiest workaround is to wrap the vmalloc itself with a big fat comment that this should be revisited.

Does that sound like something that can be done in a reasonable time? I have tried to bring this up in the past but our speed is glacial and there are attempts to do hacks like checking for abusers inside the vmalloc which is just too ugly to live.

Please do not hesitate to get back to me if something is not clear.

Thanks!

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 42 lines · Michal Hocko

Hi,
it seems that we still have few vmalloc users who perform GFP_NOFS allocation:
drivers/mtd/ubi/io.c
fs/ext4/xattr.c
fs/gfs2/dir.c
fs/gfs2/quota.c
fs/nfs/blocklayout/extent_tree.c
fs/ubifs/debug.c
fs/ubifs/lprops.c
fs/ubifs/lpt_commit.c
fs/ubifs/orphan.c

Unfortunatelly vmalloc doesn't suppoer GFP_NOFS semantinc properly because we do have hardocded GFP_KERNEL allocations deep inside the vmalloc layers. That means that if GFP_NOFS really protects from recursion into the fs deadlocks then the vmalloc call is broken.

What to do about this? Well, there are two things. Firstly, it would be really great to double check whether the GFP_NOFS is really needed. I cannot judge that because I am not familiar with the code. It would be great if the respective maintainers (hopefully get_maintainer.sh pointed me to all relevant ones). If there is not reclaim recursion issue then simply use the standard vmalloc (aka GFP_KERNEL request).

If the use is really valid then we have a way to do the vmalloc allocation properly. We have memalloc_nofs_{save,restore} scope api. How does that work? You simply call memalloc_nofs_save when the reclaim recursion critical section starts (e.g. when you take a lock which is then used in the reclaim path - e.g. shrinker) and memalloc_nofs_restore when the critical section ends. _All_ allocations within that scope will get GFP_NOFS semantic automagically. If you are not sure about the scope itself then the easiest workaround is to wrap the vmalloc itself with a big fat comment that this should be revisited.

Does that sound like something that can be done in a reasonable time? I have tried to bring this up in the past but our speed is glacial and there are attempts to do hacks like checking for abusers inside the vmalloc which is just too ugly to live.

Please do not hesitate to get back to me if something is not clear.

Quoted 4 lines

Thanks!

Signature · 2 lines

Michal Hocko
SUSE Labs

I made a patch that adds memalloc_noio/fs_save around these calls a year
ago: http://lkml.iu.edu/hypermail/linux/kernel/1707.0/01376.html

Quoted 51 lines · Mikulas Patocka

Quoted 46 lines · Michal Hocko

Hi,
it seems that we still have few vmalloc users who perform GFP_NOFS allocation:
drivers/mtd/ubi/io.c
fs/ext4/xattr.c
fs/gfs2/dir.c
fs/gfs2/quota.c
fs/nfs/blocklayout/extent_tree.c
fs/ubifs/debug.c
fs/ubifs/lprops.c
fs/ubifs/lpt_commit.c
fs/ubifs/orphan.c

Unfortunatelly vmalloc doesn't suppoer GFP_NOFS semantinc properly because we do have hardocded GFP_KERNEL allocations deep inside the vmalloc layers. That means that if GFP_NOFS really protects from recursion into the fs deadlocks then the vmalloc call is broken.

What to do about this? Well, there are two things. Firstly, it would be really great to double check whether the GFP_NOFS is really needed. I cannot judge that because I am not familiar with the code. It would be great if the respective maintainers (hopefully get_maintainer.sh pointed me to all relevant ones). If there is not reclaim recursion issue then simply use the standard vmalloc (aka GFP_KERNEL request).

If the use is really valid then we have a way to do the vmalloc allocation properly. We have memalloc_nofs_{save,restore} scope api. How does that work? You simply call memalloc_nofs_save when the reclaim recursion critical section starts (e.g. when you take a lock which is then used in the reclaim path - e.g. shrinker) and memalloc_nofs_restore when the critical section ends. _All_ allocations within that scope will get GFP_NOFS semantic automagically. If you are not sure about the scope itself then the easiest workaround is to wrap the vmalloc itself with a big fat comment that this should be revisited.

Does that sound like something that can be done in a reasonable time? I have tried to bring this up in the past but our speed is glacial and there are attempts to do hacks like checking for abusers inside the vmalloc which is just too ugly to live.

Please do not hesitate to get back to me if something is not clear.

Thanks!

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 2 lines

I made a patch that adds memalloc_noio/fs_save around these calls a year
ago: http://lkml.iu.edu/hypermail/linux/kernel/1707.0/01376.html

Yeah, and that is the wrong approach.

Let's try to fix this properly
this time. As the above outlines, the worst case we can end up mid-term would be to wrap vmalloc calls with the scope api with a TODO. But I am pretty sure the respective maintainers can come up with a better solution. I am definitely willing to help here.

Signature · 2 lines

Michal Hocko
SUSE Labs

fs/ext4/xattr.c

What to do about this? Well, there are two things. Firstly, it would be really great to double check whether the GFP_NOFS is really needed. I cannot judge that because I am not familiar with the code.

If the use is really valid then we have a way to do the vmalloc allocation properly. We have memalloc_nofs_{save,restore} scope api. How does that work? You simply call memalloc_nofs_save when the reclaim recursion critical section starts (e.g. when you take a lock which is then used in the reclaim path - e.g. shrinker) and memalloc_nofs_restore when the critical section ends. _All_ allocations within that scope will get GFP_NOFS semantic automagically. If you are not sure about the scope itself then the easiest workaround is to wrap the vmalloc itself with a big fat comment that this should be revisited.

Quoted 5 lines · Michal Hocko

fs/ext4/xattr.c

What to do about this? Well, there are two things. Firstly, it would be really great to double check whether the GFP_NOFS is really needed. I cannot judge that because I am not familiar with the code.

*Most* of the time it's not needed, but there are times when it is. We could be more smart about sending down GFP_NOFS only when it is needed.

If we are sending too many GFP_NOFS's allocations such that it's causing heartburn, we could fix this. (xattr commands are rare enough that I dind't think it was worth it to modulate the GFP flags for this particular case, but we could make it be smarter if it would help.)

Quoted 9 lines

If the use is really valid then we have a way to do the vmalloc allocation properly. We have memalloc_nofs_{save,restore} scope api. How does that work? You simply call memalloc_nofs_save when the reclaim recursion critical section starts (e.g. when you take a lock which is then used in the reclaim path - e.g. shrinker) and memalloc_nofs_restore when the critical section ends. _All_ allocations within that scope will get GFP_NOFS semantic automagically. If you are not sure about the scope itself then the easiest workaround is to wrap the vmalloc itself with a big fat comment that this should be revisited.

This is something we could do in ext4. It hadn't been high priority, because we've been rather overloaded.

As a suggestion, could you take
documentation about how to convert to the memalloc_nofs_{save,restore} scope api (which I think you've written about e-mails at length before), and put that into a file in Documentation/core-api?

The question I was trying to figure out which triggered the above request is how/whether to gradually convert to that scope API. Is it safe to add the memalloc_nofs_{save,restore} to code and keep the GFP_NOFS flags until we're sure we got it all right, for all of the code paths, and then drop the GFP_NOFS?

Quoted 4 lines

As a suggestion, could you take
documentation about how to convert to the memalloc_nofs_{save,restore} scope api (which I think you've written about e-mails at length before), and put that into a file in Documentation/core-api?

I can.

On Tue 24-04-18 13:25:42, Michal Hocko wrote:
[...]

Quoted 6 lines

Quoted 4 lines

As a suggestion, could you take
documentation about how to convert to the memalloc_nofs_{save,restore} scope api (which I think you've written about e-mails at length before), and put that into a file in Documentation/core-api?

I can.

Does something like the below sound reasonable/helpful?

On Tue 24-04-18 13:25:42, Michal Hocko wrote:
[...]

Quoted 6 lines

Quoted 4 lines

As a suggestion, could you take
documentation about how to convert to the memalloc_nofs_{save,restore} scope api (which I think you've written about e-mails at length before), and put that into a file in Documentation/core-api?

I can.

Does something like the below sound reasonable/helpful?
---
=================================
GFP masks used from FS/IO context
=================================

:Date: Mapy, 2018
:Author: Michal Hocko <[email protected]>

Introduction
============

FS resp. IO submitting code paths have to be careful when allocating

memory to prevent from potential recursion deadlocks caused by direct memory reclaim calling back into the FS/IO path and block on already held resources (e.g. locks). Traditional way to avoid this problem

is to clear __GFP_FS resp. __GFP_IO (note the later implies clearing the first as well) in the gfp mask when calling an allocator. GFP_NOFS resp. GFP_NOIO can be used as shortcut.

This has been the traditional way to avoid deadlocks since ages. It

turned out though that above approach has led to abuses when the restricted gfp mask is used "just in case" without a deeper consideration which leads to problems because an excessive use of GFP_NOFS/GFP_NOIO can lead to memory over-reclaim or other memory reclaim issues.

New API
=======

Since 4.12 we do have a generic scope API for both NOFS and NOIO context ``memalloc_nofs_save``, ``memalloc_nofs_restore`` resp. ``memalloc_noio_save``, ``memalloc_noio_restore`` which allow to mark a scope to be a critical section from the memory reclaim recursion into FS/IO POV. Any allocation from that scope will inherently drop __GFP_FS resp. __GFP_IO from the given mask so no memory allocation can recurse back in the FS/IO.

FS/IO code then simply calls the appropriate save function right at the layer where a lock taken from the reclaim context (e.g. shrinker) is taken and the corresponding restore function when the lock is released. All that ideally along with an explanation what is the reclaim context for easier maintenance.

What about __vmalloc(GFP_NOFS)
==============================

vmalloc doesn't support GFP_NOFS semantic because there are hardcoded GFP_KERNEL allocations deep inside the allocator which are quit non-trivial

to fix up. That means that calling ``vmalloc`` with GFP_NOFS/GFP_NOIO is almost always a bug. The good news is that the NOFS/NOIO semantic can be achieved by the scope api.

In the ideal world, upper layers should already mark dangerous contexts and so no special care is required and vmalloc should be called without any problems. Sometimes if the context is not really clear or there are layering violations then the recommended way around that is to wrap ``vmalloc`` by the scope API with a comment explaining the problem.

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 15 lines · Michal Hocko

On Tue 24-04-18 13:25:42, Michal Hocko wrote:
[...]

Quoted 6 lines

Quoted 4 lines

As a suggestion, could you take
documentation about how to convert to the memalloc_nofs_{save,restore} scope api (which I think you've written about e-mails at length before), and put that into a file in Documentation/core-api?

I can.

Does something like the below sound reasonable/helpful?
---
=================================
GFP masks used from FS/IO context
=================================

Quoted 7 lines

:Date: Mapy, 2018
:Author: Michal Hocko <[email protected]>

Introduction
============

FS resp. IO submitting code paths have to be careful when allocating

Not sure what 'FS resp. IO' means here -- 'FS and IO' ?

(Or is this one of those things where this looks like plain English text but in reality it's some sort of markup that I'm not so familiar with?)

Confused because I've seen 'resp.' used as shorthand for 'responsible'...

Quoted 3 lines

memory to prevent from potential recursion deadlocks caused by direct memory reclaim calling back into the FS/IO path and block on already held resources (e.g. locks). Traditional way to avoid this problem

'The traditional way to avoid this deadlock problem...'

Quoted 5 lines

is to clear __GFP_FS resp. __GFP_IO (note the later implies clearing the first as well) in the gfp mask when calling an allocator. GFP_NOFS resp. GFP_NOIO can be used as shortcut.

This has been the traditional way to avoid deadlocks since ages. It

I think this sentence is a little redundant with the previous sentence, you could chop it out and join this paragraph to the one before it.

Quoted 18 lines

turned out though that above approach has led to abuses when the restricted gfp mask is used "just in case" without a deeper consideration which leads to problems because an excessive use of GFP_NOFS/GFP_NOIO can lead to memory over-reclaim or other memory reclaim issues.

New API
=======

Since 4.12 we do have a generic scope API for both NOFS and NOIO context ``memalloc_nofs_save``, ``memalloc_nofs_restore`` resp. ``memalloc_noio_save``, ``memalloc_noio_restore`` which allow to mark a scope to be a critical section from the memory reclaim recursion into FS/IO POV. Any allocation from that scope will inherently drop __GFP_FS resp. __GFP_IO from the given mask so no memory allocation can recurse back in the FS/IO.

FS/IO code then simply calls the appropriate save function right at the layer where a lock taken from the reclaim context (e.g. shrinker) is taken and the corresponding restore function when the lock is

Quoted 8 lines

released. All that ideally along with an explanation what is the reclaim context for easier maintenance.

What about __vmalloc(GFP_NOFS)
==============================

vmalloc doesn't support GFP_NOFS semantic because there are hardcoded GFP_KERNEL allocations deep inside the allocator which are quit non-trivial

...which are quite non-trivial...

Quoted 9 lines

to fix up. That means that calling ``vmalloc`` with GFP_NOFS/GFP_NOIO is almost always a bug. The good news is that the NOFS/NOIO semantic can be achieved by the scope api.

In the ideal world, upper layers should already mark dangerous contexts and so no special care is required and vmalloc should be called without any problems. Sometimes if the context is not really clear or there are layering violations then the recommended way around that is to wrap ``vmalloc`` by the scope API with a comment explaining the problem.

Otherwise looks ok to me based on my understanding of how all this is supposed to work...

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 3 lines

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 1 line · Darrick J. Wong

On Wed, May 09, 2018 at 03:42:22PM +0200, Michal Hocko wrote:

Quoted 3 lines

Quoted 3 lines

FS/IO code then simply calls the appropriate save function right at the layer where a lock taken from the reclaim context (e.g. shrinker) is taken and the corresponding restore function when the lock is

Seems like the second "is taken" got there by mistake

Quoted 15 lines · Michal Hocko

On Tue 24-04-18 13:25:42, Michal Hocko wrote:
[...]

Quoted 6 lines

Quoted 4 lines

As a suggestion, could you take
documentation about how to convert to the memalloc_nofs_{save,restore} scope api (which I think you've written about e-mails at length before), and put that into a file in Documentation/core-api?

I can.

Does something like the below sound reasonable/helpful?
---
=================================
GFP masks used from FS/IO context
=================================

Quoted 7 lines

:Date: Mapy, 2018
:Author: Michal Hocko <[email protected]>

Introduction
============

FS resp. IO submitting code paths have to be careful when allocating

Not sure what 'FS resp. IO' means here -- 'FS and IO' ?

(Or is this one of those things where this looks like plain English text but in reality it's some sort of markup that I'm not so familiar with?)

Confused because I've seen 'resp.' used as shorthand for 'responsible'...

Quoted 4 lines

memory to prevent from potential recursion deadlocks caused by direct memory reclaim calling back into the FS/IO path and block on already held resources (e.g. locks). Traditional way to avoid this problem

'The traditional way to avoid this deadlock problem...'

Quoted 5 lines

is to clear __GFP_FS resp. __GFP_IO (note the later implies clearing the first as well) in the gfp mask when calling an allocator. GFP_NOFS resp. GFP_NOIO can be used as shortcut.

This has been the traditional way to avoid deadlocks since ages. It

I think this sentence is a little redundant with the previous sentence, you could chop it out and join this paragraph to the one before it.

Quoted 22 lines

turned out though that above approach has led to abuses when the restricted gfp mask is used "just in case" without a deeper consideration which leads to problems because an excessive use of GFP_NOFS/GFP_NOIO can lead to memory over-reclaim or other memory reclaim issues.

New API
=======

Since 4.12 we do have a generic scope API for both NOFS and NOIO context ``memalloc_nofs_save``, ``memalloc_nofs_restore`` resp. ``memalloc_noio_save``, ``memalloc_noio_restore`` which allow to mark a scope to be a critical section from the memory reclaim recursion into FS/IO POV. Any allocation from that scope will inherently drop __GFP_FS resp. __GFP_IO from the given mask so no memory allocation can recurse back in the FS/IO.

FS/IO code then simply calls the appropriate save function right at the layer where a lock taken from the reclaim context (e.g. shrinker) is taken and the corresponding restore function when the lock is released. All that ideally along with an explanation what is the reclaim context for easier maintenance.

Quoted 5 lines

What about __vmalloc(GFP_NOFS)
==============================

vmalloc doesn't support GFP_NOFS semantic because there are hardcoded GFP_KERNEL allocations deep inside the allocator which are quit non-trivial

...which are quite non-trivial...

Quoted 9 lines

to fix up. That means that calling ``vmalloc`` with GFP_NOFS/GFP_NOIO is almost always a bug. The good news is that the NOFS/NOIO semantic can be achieved by the scope api.

In the ideal world, upper layers should already mark dangerous contexts and so no special care is required and vmalloc should be called without any problems. Sometimes if the context is not really clear or there are layering violations then the recommended way around that is to wrap ``vmalloc`` by the scope API with a comment explaining the problem.

Otherwise looks ok to me based on my understanding of how all this is supposed to work...

Reviewed-by Darrick J. Wong <[email protected]>

Quoted 29 lines · Darrick J. Wong

Quoted 15 lines · Michal Hocko

On Tue 24-04-18 13:25:42, Michal Hocko wrote:
[...]

Quoted 6 lines

Quoted 4 lines

As a suggestion, could you take
documentation about how to convert to the memalloc_nofs_{save,restore} scope api (which I think you've written about e-mails at length before), and put that into a file in Documentation/core-api?

I can.

Does something like the below sound reasonable/helpful?
---
=================================
GFP masks used from FS/IO context
=================================

Quoted 7 lines

:Date: Mapy, 2018
:Author: Michal Hocko <[email protected]>

Introduction
============

FS resp. IO submitting code paths have to be careful when allocating

Not sure what 'FS resp. IO' means here -- 'FS and IO' ?

(Or is this one of those things where this looks like plain English text but in reality it's some sort of markup that I'm not so familiar with?)

Quoted 2 lines

Confused because I've seen 'resp.' used as shorthand for 'responsible'...

Well, I've tried to cover both. Filesystem and IO code paths which allocate while in sensitive context. IO submission is kinda clear but I am not sure what a general term for filsystem code paths would be. I would be greatful for any hints here.

Quoted 7 lines

Quoted 5 lines

memory to prevent from potential recursion deadlocks caused by direct memory reclaim calling back into the FS/IO path and block on already held resources (e.g. locks). Traditional way to avoid this problem

'The traditional way to avoid this deadlock problem...'

Done

Quoted 8 lines

Quoted 5 lines

is to clear __GFP_FS resp. __GFP_IO (note the later implies clearing the first as well) in the gfp mask when calling an allocator. GFP_NOFS resp. GFP_NOIO can be used as shortcut.

This has been the traditional way to avoid deadlocks since ages. It

I think this sentence is a little redundant with the previous sentence, you could chop it out and join this paragraph to the one before it.

OK

Quoted 22 lines

Quoted 22 lines

turned out though that above approach has led to abuses when the restricted gfp mask is used "just in case" without a deeper consideration which leads to problems because an excessive use of GFP_NOFS/GFP_NOIO can lead to memory over-reclaim or other memory reclaim issues.

New API
=======

Since 4.12 we do have a generic scope API for both NOFS and NOIO context ``memalloc_nofs_save``, ``memalloc_nofs_restore`` resp. ``memalloc_noio_save``, ``memalloc_noio_restore`` which allow to mark a scope to be a critical section from the memory reclaim recursion into FS/IO POV. Any allocation from that scope will inherently drop __GFP_FS resp. __GFP_IO from the given mask so no memory allocation can recurse back in the FS/IO.

FS/IO code then simply calls the appropriate save function right at the layer where a lock taken from the reclaim context (e.g. shrinker) is taken and the corresponding restore function when the lock is released. All that ideally along with an explanation what is the reclaim context for easier maintenance.

Quoted 7 lines

Quoted 5 lines

What about __vmalloc(GFP_NOFS)
==============================

vmalloc doesn't support GFP_NOFS semantic because there are hardcoded GFP_KERNEL allocations deep inside the allocator which are quit non-trivial

...which are quite non-trivial...

fixed

Quoted 10 lines

Quoted 9 lines

to fix up. That means that calling ``vmalloc`` with GFP_NOFS/GFP_NOIO is almost always a bug. The good news is that the NOFS/NOIO semantic can be achieved by the scope api.

In the ideal world, upper layers should already mark dangerous contexts and so no special care is required and vmalloc should be called without any problems. Sometimes if the context is not really clear or there are layering violations then the recommended way around that is to wrap ``vmalloc`` by the scope API with a comment explaining the problem.

Quoted 4 lines

Otherwise looks ok to me based on my understanding of how all this is supposed to work...

Reviewed-by Darrick J. Wong <[email protected]>

Thanks for your review!

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 1 line · Michal Hocko

On Wed 09-05-18 08:13:51, Darrick J. Wong wrote:

Quoted 7 lines

Quoted 7 lines

Quoted 1 line

FS resp. IO submitting code paths have to be careful when allocating

Not sure what 'FS resp. IO' means here -- 'FS and IO' ?

(Or is this one of those things where this looks like plain English text but in reality it's some sort of markup that I'm not so familiar with?)

Quoted 7 lines

Quoted 2 lines

Confused because I've seen 'resp.' used as shorthand for 'responsible'...

Well, I've tried to cover both. Filesystem and IO code paths which allocate while in sensitive context. IO submission is kinda clear but I am not sure what a general term for filsystem code paths would be. I would be greatful for any hints here.

"Code paths in the filesystem and IO stacks must be careful when allocating memory to prevent recursion deadlocks caused by direct memory reclaim calling back into the FS or IO paths and blocking on already held resources (e.g. locks)." ?

Quoted 2 lines · Darrick J. Wong

Quoted 1 line · Michal Hocko

On Wed 09-05-18 08:13:51, Darrick J. Wong wrote:

[...]

Quoted 15 lines

Quoted 14 lines

Quoted 9 lines

Quoted 1 line

FS resp. IO submitting code paths have to be careful when allocating

Not sure what 'FS resp. IO' means here -- 'FS and IO' ?

(Or is this one of those things where this looks like plain English text but in reality it's some sort of markup that I'm not so familiar with?)

Confused because I've seen 'resp.' used as shorthand for 'responsible'...

Well, I've tried to cover both. Filesystem and IO code paths which allocate while in sensitive context. IO submission is kinda clear but I am not sure what a general term for filsystem code paths would be. I would be greatful for any hints here.

Quoted 4 lines

"Code paths in the filesystem and IO stacks must be careful when allocating memory to prevent recursion deadlocks caused by direct memory reclaim calling back into the FS or IO paths and blocking on already held resources (e.g. locks)." ?

Great, thanks!

From: Michal Hocko <[email protected]>

Although the api is documented in the source code Ted has pointed out that there is no mention in the core-api Documentation and there are people looking there to find answers how to use a specific API.

Cc "Darrick J. Wong" <[email protected]>

Cc David Sterba <[email protected]>

Requested-by "Theodore Y. Ts'o" <[email protected]>

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

---

Hi Johnatan,
Ted has proposed this at LSFMM and then we discussed that briefly on the mailing list [1]. I received some useful feedback from Darrick and Dave which has been (hopefully) integrated. Then the thing fall off my radar rediscovering it now when doing some cleanup. Could you take the patch please?

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

 .../core-api/gfp_mask-from-fs-io.rst          | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/core-api/gfp_mask-from-fs-io.rst

Documentation/core-api/gfp_mask-from-fs-io.rst new file +8 -0

new file mode 100644
index 000000000000..e8b2678e959b
@@ -0,0 +1,55 @@
+=================================
+GFP masks used from FS/IO context
+=================================
+
+:Date: Mapy, 2018
+:Author: Michal Hocko <[email protected]>
+
+Introduction

Documentation/core-api/gfp_mask-from-fs-io.rst new file +37 -1

+the gfp mask when calling an allocator. GFP_NOFS resp. GFP_NOIO can be
+used as shortcut. It turned out though that above approach has led to
+abuses when the restricted gfp mask is used "just in case" without a
+deeper consideration which leads to problems because an excessive use
+of GFP_NOFS/GFP_NOIO can lead to memory over-reclaim or other memory
+reclaim issues.
+
+New API
+========
+
+Since 4.12 we do have a generic scope API for both NOFS and NOIO context
+``memalloc_nofs_save``, ``memalloc_nofs_restore`` resp. ``memalloc_noio_save``,
+``memalloc_noio_restore`` which allow to mark a scope to be a critical
+section from the memory reclaim recursion into FS/IO POV. Any allocation
+from that scope will inherently drop __GFP_FS resp. __GFP_IO from the given
+mask so no memory allocation can recurse back in the FS/IO.
+
+FS/IO code then simply calls the appropriate save function right at the
+layer where a lock taken from the reclaim context (e.g. shrinker) and
+the corresponding restore function when the lock is released. All that
+ideally along with an explanation what is the reclaim context for easier
+maintenance.
+
+What about __vmalloc(GFP_NOFS)
+==============================
+
+vmalloc doesn't support GFP_NOFS semantic because there are hardcoded
+GFP_KERNEL allocations deep inside the allocator which are quite non-trivial
+to fix up. That means that calling ``vmalloc`` with GFP_NOFS/GFP_NOIO is
+almost always a bug. The good news is that the NOFS/NOIO semantic can be
+achieved by the scope api.
+
+In the ideal world, upper layers should already mark dangerous contexts
+and so no special care is required and vmalloc should be called without
+any problems. Sometimes if the context is not really clear or there are
+layering violations then the recommended way around that is to wrap ``vmalloc``
+by the scope API with a comment explaining the problem.
--

2.17.0

On Thu, May 24, 2018 at 4:43 AM, Michal Hocko <[email protected]> wrote:

Documentation/core-api/gfp_mask-from-fs-io.rst line 18

Quoted 2 lines

Documentation/core-api/gfp_mask-from-fs-io.rst new file +2 -0

+The traditional way to avoid this deadlock problem is to clear __GFP_FS
+resp. __GFP_IO (note the later implies clearing the first as well) in

Is resp. == respectively? Why not use the full word (here and below)?

From: Michal Hocko <[email protected]>

Although the api is documented in the source code Ted has pointed out that there is no mention in the core-api Documentation and there are people looking there to find answers how to use a specific API.

Cc "Darrick J. Wong" <[email protected]>

Cc David Sterba <[email protected]>

Requested-by "Theodore Y. Ts'o" <[email protected]>

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

---

Hi Johnatan,
Ted has proposed this at LSFMM and then we discussed that briefly on the mailing list [1]. I received some useful feedback from Darrick and Dave which has been (hopefully) integrated. Then the thing fall off my radar rediscovering it now when doing some cleanup. Could you take the patch please?

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

 .../core-api/gfp_mask-from-fs-io.rst          | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/core-api/gfp_mask-from-fs-io.rst

Documentation/core-api/gfp_mask-from-fs-io.rst new file +8 -0

new file mode 100644
index 000000000000..e8b2678e959b
@@ -0,0 +1,55 @@
+=================================
+GFP masks used from FS/IO context
+=================================
+
+:Date: Mapy, 2018
+:Author: Michal Hocko <[email protected]>
+
+Introduction

Documentation/core-api/gfp_mask-from-fs-io.rst new file +4 -0

+the gfp mask when calling an allocator. GFP_NOFS resp. GFP_NOIO can be
+used as shortcut. It turned out though that above approach has led to
+abuses when the restricted gfp mask is used "just in case" without a
+deeper consideration which leads to problems because an excessive use

Documentation/core-api/gfp_mask-from-fs-io.rst new file +7 -0

+from that scope will inherently drop __GFP_FS resp. __GFP_IO from the given
+mask so no memory allocation can recurse back in the FS/IO.
+
+FS/IO code then simply calls the appropriate save function right at the
+layer where a lock taken from the reclaim context (e.g. shrinker) and
+the corresponding restore function when the lock is released. All that
+ideally along with an explanation what is the reclaim context for easier

Documentation/core-api/gfp_mask-from-fs-io.rst new file +6 -0

+
+In the ideal world, upper layers should already mark dangerous contexts
+and so no special care is required and vmalloc should be called without
+any problems. Sometimes if the context is not really clear or there are
+layering violations then the recommended way around that is to wrap ``vmalloc``
+by the scope API with a comment explaining the problem.

On 05/24/2018 04:43 AM, Michal Hocko wrote:

Documentation/core-api/gfp_mask-from-fs-io.rst line 18

Quoted 2 lines

Documentation/core-api/gfp_mask-from-fs-io.rst new file +2 -0

+The traditional way to avoid this deadlock problem is to clear __GFP_FS
+resp. __GFP_IO (note the later implies clearing the first as well) in

latter

Quoted 1 line · Randy Dunlap

On 05/24/2018 04:43 AM, Michal Hocko wrote:

[...]

Quoted 4 lines

Documentation/core-api/gfp_mask-from-fs-io.rst line 18

Quoted 2 lines

Documentation/core-api/gfp_mask-from-fs-io.rst new file +2 -0

+The traditional way to avoid this deadlock problem is to clear __GFP_FS
+resp. __GFP_IO (note the later implies clearing the first as well) in

latter

?
No I really meant that clearing __GFP_IO implies __GFP_FS clearing

Quoted 10 lines · Michal Hocko

Quoted 1 line · Randy Dunlap

On 05/24/2018 04:43 AM, Michal Hocko wrote:

[...]

Quoted 4 lines

Documentation/core-api/gfp_mask-from-fs-io.rst line 18

Quoted 2 lines

Documentation/core-api/gfp_mask-from-fs-io.rst new file +2 -0

+The traditional way to avoid this deadlock problem is to clear __GFP_FS
+resp. __GFP_IO (note the later implies clearing the first as well) in

latter

?
No I really meant that clearing __GFP_IO implies __GFP_FS clearing

Sorry to barge in like that, but Randy is right.

<NIT WARNING>

https://www.merriam-webster.com/dictionary/latter

" of, relating to, or being the second of two groups or things or the last of several groups or things referred to

</NIT WARNING>

Quoted 1 line · Randy Dunlap

On 05/24/2018 04:43 AM, Michal Hocko wrote:

[...]

Quoted 4 lines

Documentation/core-api/gfp_mask-from-fs-io.rst line 18

Quoted 2 lines

Documentation/core-api/gfp_mask-from-fs-io.rst new file +2 -0

+The traditional way to avoid this deadlock problem is to clear __GFP_FS
+resp. __GFP_IO (note the later implies clearing the first as well) in

latter

?
No I really meant that clearing __GFP_IO implies __GFP_FS clearing

From: Michal Hocko <[email protected]>

Although the api is documented in the source code Ted has pointed out that there is no mention in the core-api Documentation and there are people looking there to find answers how to use a specific API.

Cc "Darrick J. Wong" <[email protected]>

Cc David Sterba <[email protected]>

Requested-by "Theodore Y. Ts'o" <[email protected]>

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

---

Hi Johnatan,
Ted has proposed this at LSFMM and then we discussed that briefly on the mailing list [1]. I received some useful feedback from Darrick and Dave which has been (hopefully) integrated. Then the thing fall off my radar rediscovering it now when doing some cleanup. Could you take the patch please?

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

 .../core-api/gfp_mask-from-fs-io.rst          | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/core-api/gfp_mask-from-fs-io.rst

Quoted 19 lines · On Thu, 24 May 2018 13:43:41 +0200 Michal Hocko <[email protected]> wrote:

From: Michal Hocko <[email protected]>

Although the api is documented in the source code Ted has pointed out that there is no mention in the core-api Documentation and there are people looking there to find answers how to use a specific API.

Cc "Darrick J. Wong" <[email protected]>

Cc David Sterba <[email protected]>

Requested-by "Theodore Y. Ts'o" <[email protected]>

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

---

Hi Johnatan,
Ted has proposed this at LSFMM and then we discussed that briefly on the mailing list [1]. I received some useful feedback from Darrick and Dave which has been (hopefully) integrated. Then the thing fall off my radar rediscovering it now when doing some cleanup. Could you take the patch please?

Quoted 4 lines

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

 .../core-api/gfp_mask-from-fs-io.rst          | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/core-api/gfp_mask-from-fs-io.rst

So you create the rst file, but don't add it in index.rst; that means it won't be a part of the docs build and Sphinx will complain.

Documentation/core-api/gfp_mask-from-fs-io.rst line 5

Quoted 12 lines

Documentation/core-api/gfp_mask-from-fs-io.rst new file +5 -0

new file mode 100644
index 000000000000..e8b2678e959b
@@ -0,0 +1,55 @@
+=================================
+GFP masks used from FS/IO context
+=================================
+
+:Date: Mapy, 2018

Ah...the wonderful month of Mapy....:)

Quoted 7 lines

Documentation/core-api/gfp_mask-from-fs-io.rst new file +7 -0

+:Author: Michal Hocko <[email protected]>
+
+Introduction
+============
+
+Code paths in the filesystem and IO stacks must be careful when
+allocating memory to prevent recursion deadlocks caused by direct

Documentation/core-api/gfp_mask-from-fs-io.rst line 18

Quoted 6 lines

Documentation/core-api/gfp_mask-from-fs-io.rst new file +6 -0

+memory reclaim calling back into the FS or IO paths and blocking on
+already held resources (e.g. locks - most commonly those used for the
+transaction context).
+
+The traditional way to avoid this deadlock problem is to clear __GFP_FS
+resp. __GFP_IO (note the later implies clearing the first as well) in

"resp." is indeed a bit terse. Even spelled out as "respectively", though,

I'm not sure what the word is intended to mean here. Did you mean "or"?

Documentation/core-api/gfp_mask-from-fs-io.rst line 19

Quoted 2 lines

Documentation/core-api/gfp_mask-from-fs-io.rst new file +1 -0

+the gfp mask when calling an allocator. GFP_NOFS resp. GFP_NOIO can be

Here too.

Documentation/core-api/gfp_mask-from-fs-io.rst line 32

Quoted 13 lines

Documentation/core-api/gfp_mask-from-fs-io.rst new file +13 -0

+used as shortcut. It turned out though that above approach has led to
+abuses when the restricted gfp mask is used "just in case" without a
+deeper consideration which leads to problems because an excessive use
+of GFP_NOFS/GFP_NOIO can lead to memory over-reclaim or other memory
+reclaim issues.
+
+New API
+========
+
+Since 4.12 we do have a generic scope API for both NOFS and NOIO context
+``memalloc_nofs_save``, ``memalloc_nofs_restore`` resp. ``memalloc_noio_save``,
+``memalloc_noio_restore`` which allow to mark a scope to be a critical
+section from the memory reclaim recursion into FS/IO POV. Any allocation

"from a filesystem or I/O point of view" ?

Documentation/core-api/gfp_mask-from-fs-io.rst line 34

Quoted 2 lines

Documentation/core-api/gfp_mask-from-fs-io.rst new file +2 -0

+from that scope will inherently drop __GFP_FS resp. __GFP_IO from the given
+mask so no memory allocation can recurse back in the FS/IO.

Wouldn't it be nice if those functions had kerneldoc comments that could be pulled in here! :)

Documentation/core-api/gfp_mask-from-fs-io.rst line 36

Quoted 2 lines

Documentation/core-api/gfp_mask-from-fs-io.rst new file +2 -0

+FS/IO code then simply calls the appropriate save function right at the
+layer where a lock taken from the reclaim context (e.g. shrinker) and

where a lock *is* taken ?

From: Michal Hocko <[email protected]>

Although the api is documented in the source code Ted has pointed out that there is no mention in the core-api Documentation and there are people looking there to find answers how to use a specific API.

Cc "Darrick J. Wong" <[email protected]>

Cc David Sterba <[email protected]>

Requested-by "Theodore Y. Ts'o" <[email protected]>

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

---

Hi Johnatan,
Ted has proposed this at LSFMM and then we discussed that briefly on the mailing list [1]. I received some useful feedback from Darrick and Dave which has been (hopefully) integrated. Then the thing fall off my radar rediscovering it now when doing some cleanup. Could you take the patch please?

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

 .../core-api/gfp_mask-from-fs-io.rst          | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/core-api/gfp_mask-from-fs-io.rst

Documentation/core-api/gfp_mask-from-fs-io.rst new file +30 -0

new file mode 100644
index 000000000000..e8b2678e959b
@@ -0,0 +1,55 @@
+=================================
+GFP masks used from FS/IO context
+=================================
+
+:Date: Mapy, 2018
+:Author: Michal Hocko <[email protected]>
+
+Introduction
+============
+
+Code paths in the filesystem and IO stacks must be careful when
+allocating memory to prevent recursion deadlocks caused by direct
+memory reclaim calling back into the FS or IO paths and blocking on
+already held resources (e.g. locks - most commonly those used for the
+transaction context).
+
+The traditional way to avoid this deadlock problem is to clear __GFP_FS
+resp. __GFP_IO (note the later implies clearing the first as well) in
+the gfp mask when calling an allocator. GFP_NOFS resp. GFP_NOIO can be
+used as shortcut. It turned out though that above approach has led to
+abuses when the restricted gfp mask is used "just in case" without a
+deeper consideration which leads to problems because an excessive use
+of GFP_NOFS/GFP_NOIO can lead to memory over-reclaim or other memory
+reclaim issues.
+
+New API
+========
+
+Since 4.12 we do have a generic scope API for both NOFS and NOIO context
+``memalloc_nofs_save``, ``memalloc_nofs_restore`` resp. ``memalloc_noio_save``,

Quoted 6 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Although the api is documented in the source code Ted has pointed out that there is no mention in the core-api Documentation and there are people looking there to find answers how to use a specific API.

Quoted 4 lines

Cc "Darrick J. Wong" <[email protected]>

Cc David Sterba <[email protected]>

Requested-by "Theodore Y. Ts'o" <[email protected]>

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

Yay, Documentation! :)

On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:

Documentation/core-api/gfp_mask-from-fs-io.rst line 40

Quoted 5 lines

Documentation/core-api/gfp_mask-from-fs-io.rst new file +5 -0

+FS/IO code then simply calls the appropriate save function right at the
+layer where a lock taken from the reclaim context (e.g. shrinker) and
+the corresponding restore function when the lock is released. All that
+ideally along with an explanation what is the reclaim context for easier
+maintenance.

This paragraph doesn't make much sense to me. I think you're trying to say that we should call the appropriate save function "before locks are taken that a reclaim context (e.g a shrinker) might require access to."

I think it's also worth making a note about recursive/nested save/restore stacking, because it's not clear from this description that this is allowed and will work as long as inner save/restore calls are fully nested inside outer save/restore contexts.

Quoted 1 line · Dave Chinner

On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:

[...]

Quoted 11 lines

Documentation/core-api/gfp_mask-from-fs-io.rst line 40

Quoted 5 lines

Documentation/core-api/gfp_mask-from-fs-io.rst new file +5 -0

+FS/IO code then simply calls the appropriate save function right at the
+layer where a lock taken from the reclaim context (e.g. shrinker) and
+the corresponding restore function when the lock is released. All that
+ideally along with an explanation what is the reclaim context for easier
+maintenance.

This paragraph doesn't make much sense to me. I think you're trying to say that we should call the appropriate save function "before locks are taken that a reclaim context (e.g a shrinker) might require access to."

Quoted 4 lines

I think it's also worth making a note about recursive/nested save/restore stacking, because it's not clear from this description that this is allowed and will work as long as inner save/restore calls are fully nested inside outer save/restore contexts.

Any better?

What about __vmalloc(GFP_NOFS)
==============================

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 21 lines · Michal Hocko

Quoted 1 line · Dave Chinner

On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:

[...]

Quoted 11 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+FS/IO code then simply calls the appropriate save function right at the
+layer where a lock taken from the reclaim context (e.g. shrinker) and
+the corresponding restore function when the lock is released. All that
+ideally along with an explanation what is the reclaim context for easier
+maintenance.

This paragraph doesn't make much sense to me. I think you're trying to say that we should call the appropriate save function "before locks are taken that a reclaim context (e.g a shrinker) might require access to."

Quoted 4 lines

I think it's also worth making a note about recursive/nested save/restore stacking, because it's not clear from this description that this is allowed and will work as long as inner save/restore calls are fully nested inside outer save/restore contexts.

Any better?

Review

Quoted 8 lines

(quoted diff) +3 -5

-FS/IO code then simply calls the appropriate save function right at the
-layer where a lock taken from the reclaim context (e.g. shrinker) and
-the corresponding restore function when the lock is released. All that
-ideally along with an explanation what is the reclaim context for easier
-maintenance.
+FS/IO code then simply calls the appropriate save function before any
+lock shared with the reclaim context is taken.  The corresponding
+restore function when the lock is released. All that ideally along with

Maybe: "The corresponding restore function is called when the lock is released"

Review

Quoted 4 lines

(quoted diff) +4 -0

+an explanation what is the reclaim context for easier maintenance.
+
+Please note that the proper pairing of save/restore function allows nesting
+so memalloc_noio_save is safe to be called from an existing NOIO or NOFS scope.

so it is safe to call memalloc_noio_save from an existing NOIO or NOFS scope

Quoted 22 lines · Mike Rapoport

Quoted 21 lines · Michal Hocko

Quoted 1 line · Dave Chinner

On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:

[...]

Quoted 11 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+FS/IO code then simply calls the appropriate save function right at the
+layer where a lock taken from the reclaim context (e.g. shrinker) and
+the corresponding restore function when the lock is released. All that
+ideally along with an explanation what is the reclaim context for easier
+maintenance.

This paragraph doesn't make much sense to me. I think you're trying to say that we should call the appropriate save function "before locks are taken that a reclaim context (e.g a shrinker) might require access to."

Quoted 4 lines

I think it's also worth making a note about recursive/nested save/restore stacking, because it's not clear from this description that this is allowed and will work as long as inner save/restore calls are fully nested inside outer save/restore contexts.

Any better?

Quoted 11 lines

Review

Quoted 8 lines

(quoted diff) +3 -5

-FS/IO code then simply calls the appropriate save function right at the
-layer where a lock taken from the reclaim context (e.g. shrinker) and
-the corresponding restore function when the lock is released. All that
-ideally along with an explanation what is the reclaim context for easier
-maintenance.
+FS/IO code then simply calls the appropriate save function before any
+lock shared with the reclaim context is taken.  The corresponding
+restore function when the lock is released. All that ideally along with

Maybe: "The corresponding restore function is called when the lock is released"

This will get rewritten some more based on comments from Dave

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+an explanation what is the reclaim context for easier maintenance.
+
+Please note that the proper pairing of save/restore function allows nesting
+so memalloc_noio_save is safe to be called from an existing NOIO or NOFS scope.

so it is safe to call memalloc_noio_save from an existing NOIO or NOFS scope

Here is what I have right now on top

Documentation/core-api/gfp_mask-from-fs-io.rst +0 -0

index c0ec212d6773..0cff411693ab 100644

Documentation/core-api/gfp_mask-from-fs-io.rst +0 -0

 What about __vmalloc(GFP_NOFS)
 ==============================
 

On 05/28/2018 02:21 AM, Michal Hocko wrote:

Documentation/core-api/gfp_mask-from-fs-io.rst scope will inherently drop __GFP_FS respectively __GFP_IO from the given · line 39

Quoted 2 lines

Documentation/core-api/gfp_mask-from-fs-io.rst +2 -0

+reclaim context or when a transaction context nesting would be possible
+via reclaim. The corresponding restore function when the critical

"The corresponding restore ... ends." << That is not a complete sentence. It's missing something.

Documentation/core-api/gfp_mask-from-fs-io.rst scope will inherently drop __GFP_FS respectively __GFP_IO from the given · line 45

Quoted 6 lines

Documentation/core-api/gfp_mask-from-fs-io.rst +6 -0

+section ends. All that ideally along with an explanation what is
+the reclaim context for easier maintenance.
+
+Please note that the proper pairing of save/restore function allows
+nesting so it is safe to call ``memalloc_noio_save`` respectively
+``memalloc_noio_restore`` from an existing NOIO or NOFS scope.

Please note that the proper pairing of save/restore functions allows nesting so it is safe to call ``memalloc_noio_save`` or ``memalloc_noio_restore`` respectively from an existing NOIO or NOFS scope.

Quoted 1 line · Dave Chinner

On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:

[...]

Quoted 11 lines

Documentation/core-api/gfp_mask-from-fs-io.rst line 40

Quoted 5 lines

Documentation/core-api/gfp_mask-from-fs-io.rst new file +5 -0

+FS/IO code then simply calls the appropriate save function right at the
+layer where a lock taken from the reclaim context (e.g. shrinker) and
+the corresponding restore function when the lock is released. All that
+ideally along with an explanation what is the reclaim context for easier
+maintenance.

This paragraph doesn't make much sense to me. I think you're trying to say that we should call the appropriate save function "before locks are taken that a reclaim context (e.g a shrinker) might require access to."

Quoted 4 lines

I think it's also worth making a note about recursive/nested save/restore stacking, because it's not clear from this description that this is allowed and will work as long as inner save/restore calls are fully nested inside outer save/restore contexts.

Any better?

(quoted diff) +0 -2

-FS/IO code then simply calls the appropriate save function right at the
-layer where a lock taken from the reclaim context (e.g. shrinker) and

Quoted 31 lines · Michal Hocko

Quoted 1 line · Dave Chinner

On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:

[...]

Quoted 11 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+FS/IO code then simply calls the appropriate save function right at the
+layer where a lock taken from the reclaim context (e.g. shrinker) and
+the corresponding restore function when the lock is released. All that
+ideally along with an explanation what is the reclaim context for easier
+maintenance.

This paragraph doesn't make much sense to me. I think you're trying to say that we should call the appropriate save function "before locks are taken that a reclaim context (e.g a shrinker) might require access to."

Quoted 4 lines

I think it's also worth making a note about recursive/nested save/restore stacking, because it's not clear from this description that this is allowed and will work as long as inner save/restore calls are fully nested inside outer save/restore contexts.

Any better?

(quoted diff) +5 -5

-FS/IO code then simply calls the appropriate save function right at the
-layer where a lock taken from the reclaim context (e.g. shrinker) and
-the corresponding restore function when the lock is released. All that
-ideally along with an explanation what is the reclaim context for easier
-maintenance.
+FS/IO code then simply calls the appropriate save function before any
+lock shared with the reclaim context is taken.  The corresponding
+restore function when the lock is released. All that ideally along with
+an explanation what is the reclaim context for easier maintenance.
+

Review

Quoted 2 lines

(quoted diff) +2 -0

+Please note that the proper pairing of save/restore function allows nesting
+so memalloc_noio_save is safe to be called from an existing NOIO or NOFS scope.

It's better, but the talk of this being necessary for locking makes me cringe. XFS doesn't do it for locking reasons - it does it largely for preventing transaction context nesting, which has all sorts of problems that cause hangs (e.g. log space reservations can't be filled) that aren't directly locking related.

i.e we should be talking about using these functions around contexts where recursion back into the filesystem through reclaim is problematic, not that "holding locks" is problematic. Locks can be used as an example of a problematic context, but locks are not the only recursion issue that require GFP_NOFS allocation contexts to avoid.

Quoted 32 lines · Dave Chinner

Quoted 31 lines · Michal Hocko

Quoted 1 line · Dave Chinner

On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:

[...]

Quoted 11 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+FS/IO code then simply calls the appropriate save function right at the
+layer where a lock taken from the reclaim context (e.g. shrinker) and
+the corresponding restore function when the lock is released. All that
+ideally along with an explanation what is the reclaim context for easier
+maintenance.

This paragraph doesn't make much sense to me. I think you're trying to say that we should call the appropriate save function "before locks are taken that a reclaim context (e.g a shrinker) might require access to."

Quoted 4 lines

I think it's also worth making a note about recursive/nested save/restore stacking, because it's not clear from this description that this is allowed and will work as long as inner save/restore calls are fully nested inside outer save/restore contexts.

Any better?

(quoted diff) +5 -5

-FS/IO code then simply calls the appropriate save function right at the
-layer where a lock taken from the reclaim context (e.g. shrinker) and
-the corresponding restore function when the lock is released. All that
-ideally along with an explanation what is the reclaim context for easier
-maintenance.
+FS/IO code then simply calls the appropriate save function before any
+lock shared with the reclaim context is taken.  The corresponding
+restore function when the lock is released. All that ideally along with
+an explanation what is the reclaim context for easier maintenance.
+

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+Please note that the proper pairing of save/restore function allows nesting
+so memalloc_noio_save is safe to be called from an existing NOIO or NOFS scope.

It's better, but the talk of this being necessary for locking makes me cringe. XFS doesn't do it for locking reasons - it does it largely for preventing transaction context nesting, which has all sorts of problems that cause hangs (e.g. log space reservations can't be filled) that aren't directly locking related.

Yeah, I wanted to not mention locks as much as possible.

Quoted 6 lines

i.e we should be talking about using these functions around contexts where recursion back into the filesystem through reclaim is problematic, not that "holding locks" is problematic. Locks can be used as an example of a problematic context, but locks are not the only recursion issue that require GFP_NOFS allocation contexts to avoid.

agreed. Do you have any suggestion how to add a more abstract wording that would not make head spinning?

I've tried the following. Any better?

Documentation/core-api/gfp_mask-from-fs-io.rst +0 -0

index c0ec212d6773..adac362b2875 100644

Quoted 63 lines · Michal Hocko

Quoted 32 lines · Dave Chinner

Quoted 31 lines · Michal Hocko

Quoted 1 line · Dave Chinner

On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:

[...]

Quoted 11 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+FS/IO code then simply calls the appropriate save function right at the
+layer where a lock taken from the reclaim context (e.g. shrinker) and
+the corresponding restore function when the lock is released. All that
+ideally along with an explanation what is the reclaim context for easier
+maintenance.

This paragraph doesn't make much sense to me. I think you're trying to say that we should call the appropriate save function "before locks are taken that a reclaim context (e.g a shrinker) might require access to."

Quoted 4 lines

I think it's also worth making a note about recursive/nested save/restore stacking, because it's not clear from this description that this is allowed and will work as long as inner save/restore calls are fully nested inside outer save/restore contexts.

Any better?

(quoted diff) +5 -5

-FS/IO code then simply calls the appropriate save function right at the
-layer where a lock taken from the reclaim context (e.g. shrinker) and
-the corresponding restore function when the lock is released. All that
-ideally along with an explanation what is the reclaim context for easier
-maintenance.
+FS/IO code then simply calls the appropriate save function before any
+lock shared with the reclaim context is taken.  The corresponding
+restore function when the lock is released. All that ideally along with
+an explanation what is the reclaim context for easier maintenance.
+

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+Please note that the proper pairing of save/restore function allows nesting
+so memalloc_noio_save is safe to be called from an existing NOIO or NOFS scope.

It's better, but the talk of this being necessary for locking makes me cringe. XFS doesn't do it for locking reasons - it does it largely for preventing transaction context nesting, which has all sorts of problems that cause hangs (e.g. log space reservations can't be filled) that aren't directly locking related.

Yeah, I wanted to not mention locks as much as possible.

Quoted 6 lines

i.e we should be talking about using these functions around contexts where recursion back into the filesystem through reclaim is problematic, not that "holding locks" is problematic. Locks can be used as an example of a problematic context, but locks are not the only recursion issue that require GFP_NOFS allocation contexts to avoid.

agreed. Do you have any suggestion how to add a more abstract wording that would not make head spinning?

I've tried the following. Any better?

Documentation/core-api/gfp_mask-from-fs-io.rst +0 -0

index c0ec212d6773..adac362b2875 100644
@@ -34,9 +34,11 @@ scope will inherently drop __GFP_FS respectively __GFP_IO from the given
 mask so no memory allocation can recurse back in the FS/IO.
 

Documentation/core-api/gfp_mask-from-fs-io.rst scope will inherently drop __GFP_FS respectively __GFP_IO from the given · line 39

Quoted 7 lines

Documentation/core-api/gfp_mask-from-fs-io.rst +3 -3

 FS/IO code then simply calls the appropriate save function before any
-lock shared with the reclaim context is taken.  The corresponding
-restore function when the lock is released. All that ideally along with
-an explanation what is the reclaim context for easier maintenance.
+critical section wrt. the reclaim is started - e.g. lock shared with the
+reclaim context or when a transaction context nesting would be possible
+via reclaim. The corresponding restore function when the critical

.... restore function should be called when ...

But otherwise I think this is much better.

From: Michal Hocko <[email protected]>

Although the api is documented in the source code Ted has pointed out that there is no mention in the core-api Documentation and there are people looking there to find answers how to use a specific API.

Changes since v1
- add kerneldoc for the api - suggested by Johnatan
- review feedback from Dave and Johnatan
- feedback from Dave about more general critical context rather than locking
- feedback from Mike
- typo fixed - Randy, Dave

Requested-by "Theodore Y. Ts'o" <[email protected]>

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

From: Michal Hocko <[email protected]>

Although the api is documented in the source code Ted has pointed out that there is no mention in the core-api Documentation and there are people looking there to find answers how to use a specific API.

Changes since v1
- add kerneldoc for the api - suggested by Johnatan
- review feedback from Dave and Johnatan
- feedback from Dave about more general critical context rather than locking
- feedback from Mike
- typo fixed - Randy, Dave

Requested-by "Theodore Y. Ts'o" <[email protected]>

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

---
 .../core-api/gfp_mask-from-fs-io.rst          | 61 +++++++++++++++++++
 Documentation/core-api/index.rst              |  1 +
 include/linux/sched/mm.h                      | 38 ++++++++++++
 3 files changed, 100 insertions(+)
 create mode 100644 Documentation/core-api/gfp_mask-from-fs-io.rst

Documentation/core-api/gfp_mask-from-fs-io.rst new file +61 -0

new file mode 100644
index 000000000000..2dc442b04a77
@@ -0,0 +1,61 @@
+=================================
+GFP masks used from FS/IO context
+=================================
+
+:Date: May, 2018
+:Author: Michal Hocko <[email protected]>
+
+Introduction
+============
+
+Code paths in the filesystem and IO stacks must be careful when
+allocating memory to prevent recursion deadlocks caused by direct
+memory reclaim calling back into the FS or IO paths and blocking on
+already held resources (e.g. locks - most commonly those used for the
+transaction context).
+
+The traditional way to avoid this deadlock problem is to clear __GFP_FS
+respectively __GFP_IO (note the latter implies clearing the first as well) in
+the gfp mask when calling an allocator. GFP_NOFS respectively GFP_NOIO can be
+used as shortcut. It turned out though that above approach has led to
+abuses when the restricted gfp mask is used "just in case" without a
+deeper consideration which leads to problems because an excessive use
+of GFP_NOFS/GFP_NOIO can lead to memory over-reclaim or other memory
+reclaim issues.
+
+New API
+========
+
+Since 4.12 we do have a generic scope API for both NOFS and NOIO context
+``memalloc_nofs_save``, ``memalloc_nofs_restore`` respectively ``memalloc_noio_save``,
+``memalloc_noio_restore`` which allow to mark a scope to be a critical
+section from a filesystem or I/O point of view. Any allocation from that
+scope will inherently drop __GFP_FS respectively __GFP_IO from the given
+mask so no memory allocation can recurse back in the FS/IO.
+
+FS/IO code then simply calls the appropriate save function before
+any critical section with respect to the reclaim is started - e.g.
+lock shared with the reclaim context or when a transaction context
+nesting would be possible via reclaim. The restore function should be
+called when the critical section ends. All that ideally along with an
+explanation what is the reclaim context for easier maintenance.
+
+Please note that the proper pairing of save/restore functions
+allows nesting so it is safe to call ``memalloc_noio_save`` or
+``memalloc_noio_restore`` respectively from an existing NOIO or NOFS
+scope.
+
+What about __vmalloc(GFP_NOFS)
+==============================
+
+vmalloc doesn't support GFP_NOFS semantic because there are hardcoded
+GFP_KERNEL allocations deep inside the allocator which are quite non-trivial
+to fix up. That means that calling ``vmalloc`` with GFP_NOFS/GFP_NOIO is
+almost always a bug. The good news is that the NOFS/NOIO semantic can be
+achieved by the scope API.
+
+In the ideal world, upper layers should already mark dangerous contexts
+and so no special care is required and vmalloc should be called without
+any problems. Sometimes if the context is not really clear or there are
+layering violations then the recommended way around that is to wrap ``vmalloc``
+by the scope API with a comment explaining the problem.

Documentation/core-api/index.rst +1 -0

index c670a8031786..8a5f48ef16f2 100644
@@ -25,6 +25,7 @@ Core utilities
    genalloc
    errseq
    printk-formats
+   gfp_mask-from-fs-io
 
 Interfaces for kernel debugging
 ===============================

include/linux/sched/mm.h +38 -0

index e1f8411e6b80..af5ba077bbc4 100644
@@ -166,6 +166,17 @@ static inline void fs_reclaim_acquire(gfp_t gfp_mask) { }
 static inline void fs_reclaim_release(gfp_t gfp_mask) { }
 #endif
 
+/**
+ * memalloc_noio_save - Marks implicit GFP_NOIO allocation scope.
+ *
+ * This functions marks the beginning of the GFP_NOIO allocation scope.
+ * All further allocations will implicitly drop __GFP_IO flag and so
+ * they are safe for the IO critical section from the allocation recursion
+ * point of view. Use memalloc_noio_restore to end the scope with flags
+ * returned by this function.
+ *
+ * This function is safe to be used from any context.
+ */
 static inline unsigned int memalloc_noio_save(void)
 {
 	unsigned int flags = current->flags & PF_MEMALLOC_NOIO;

@@ -173,11 +184,30 @@ static inline unsigned int memalloc_noio_save(void)
 	return flags;
 }
 
+/**
+ * memalloc_noio_restore - Ends the implicit GFP_NOIO scope.
+ * @flags: Flags to restore.
+ *
+ * Ends the implicit GFP_NOIO scope started by memalloc_noio_save function.
+ * Always make sure that that the given flags is the return value from the
+ * pairing memalloc_noio_save call.
+ */
 static inline void memalloc_noio_restore(unsigned int flags)
 {
 	current->flags = (current->flags & ~PF_MEMALLOC_NOIO) | flags;
 }
 
+/**
+ * memalloc_nofs_save - Marks implicit GFP_NOFS allocation scope.
+ *
+ * This functions marks the beginning of the GFP_NOFS allocation scope.
+ * All further allocations will implicitly drop __GFP_FS flag and so
+ * they are safe for the FS critical section from the allocation recursion
+ * point of view. Use memalloc_nofs_restore to end the scope with flags
+ * returned by this function.
+ *
+ * This function is safe to be used from any context.
+ */
 static inline unsigned int memalloc_nofs_save(void)
 {
 	unsigned int flags = current->flags & PF_MEMALLOC_NOFS;

@@ -185,6 +215,14 @@ static inline unsigned int memalloc_nofs_save(void)
 	return flags;
 }
 
+/**
+ * memalloc_nofs_restore - Ends the implicit GFP_NOFS scope.
+ * @flags: Flags to restore.
+ *
+ * Ends the implicit GFP_NOFS scope started by memalloc_nofs_save function.
+ * Always make sure that that the given flags is the return value from the
+ * pairing memalloc_nofs_save call.
+ */
 static inline void memalloc_nofs_restore(unsigned int flags)
 {
 	current->flags = (current->flags & ~PF_MEMALLOC_NOFS) | flags;

Signature · 1 line

2.17.0

Although the api is documented in the source code Ted has pointed out that there is no mention in the core-api Documentation and there are people looking there to find answers how to use a specific API.

Quoted 3 lines · On Tue, 29 May 2018 10:26:44 +0200 Michal Hocko <[email protected]> wrote:

Although the api is documented in the source code Ted has pointed out that there is no mention in the core-api Documentation and there are people looking there to find answers how to use a specific API.

So, I still think that this:

Review

Quoted 2 lines

(quoted diff) +2 -0

+The traditional way to avoid this deadlock problem is to clear __GFP_FS
+respectively __GFP_IO (note the latter implies clearing the first as well) in

doesn't read the way you intend it to. But we've sent you in more than enough circles on this already, so I went ahead and applied it; wording can always be tweaked later.

You added the kerneldoc comments, but didn't bring them into your new document. I'm going to tack this on afterward, hopefully nobody will object.

Quoted 5 lines · Michal Hocko

fs/ext4/xattr.c

What to do about this? Well, there are two things. Firstly, it would be really great to double check whether the GFP_NOFS is really needed. I cannot judge that because I am not familiar with the code.

*Most* of the time it's not needed, but there are times when it is. We could be more smart about sending down GFP_NOFS only when it is needed. If we are sending too many GFP_NOFS's allocations such that it's causing heartburn, we could fix this. (xattr commands are rare enough that I dind't think it was worth it to modulate the GFP flags for this particular case, but we could make it be smarter if it would help.)

fs/ubifs/debug.c

fs/ubifs/lprops.c

fs/ubifs/lpt_commit.c

fs/ubifs/orphan.c

Am Dienstag, 24. April 2018, 18:27:12 CEST schrieb Michal Hocko:

Quoted 1 line

fs/ubifs/debug.c

This one is just for debugging.
So, preallocating + locking would not hurt much.

Quoted 1 line

fs/ubifs/lprops.c

Ditto.

Quoted 1 line

fs/ubifs/lpt_commit.c

Here we use it also only in debugging mode and in one case for fatal error reporting.
No hot paths.

Quoted 1 line

fs/ubifs/orphan.c

Also only for debugging.
Getting rid of vmalloc with GFP_NOFS in UBIFS is no big problem. I can prepare a patch.

Quoted 3 lines

Also only for debugging.
Getting rid of vmalloc with GFP_NOFS in UBIFS is no big problem. I can prepare a patch.

Cool!

Anyway, if UBIFS has some reclaim recursion critical sections in general it would be really great to have them documented and that is where the scope api is really handy. Just add the scope and document what is the recursion issue. This will help people reading the code as well. Ideally there shouldn't be any explicit GFP_NOFS in the code.

Am Dienstag, 24. April 2018, 21:28:03 CEST schrieb Michal Hocko:

Quoted 6 lines

Quoted 3 lines

Also only for debugging.
Getting rid of vmalloc with GFP_NOFS in UBIFS is no big problem. I can prepare a patch.

Cool!

Quoted 5 lines

Anyway, if UBIFS has some reclaim recursion critical sections in general it would be really great to have them documented and that is where the scope api is really handy. Just add the scope and document what is the recursion issue. This will help people reading the code as well. Ideally there shouldn't be any explicit GFP_NOFS in the code.

So in a perfect world a filesystem calls memalloc_nofs_save/restore and always uses GFP_KERNEL for kmalloc/vmalloc?

Quoted 13 lines · Richard Weinberger

Am Dienstag, 24. April 2018, 21:28:03 CEST schrieb Michal Hocko:

Quoted 11 lines

Quoted 3 lines

Also only for debugging.
Getting rid of vmalloc with GFP_NOFS in UBIFS is no big problem. I can prepare a patch.

Cool!

Anyway, if UBIFS has some reclaim recursion critical sections in general it would be really great to have them documented and that is where the scope api is really handy. Just add the scope and document what is the recursion issue. This will help people reading the code as well. Ideally there shouldn't be any explicit GFP_NOFS in the code.

Quoted 2 lines

So in a perfect world a filesystem calls memalloc_nofs_save/restore and always uses GFP_KERNEL for kmalloc/vmalloc?

Exactly! And in a dream world those memalloc_nofs_save act as a documentation of the reclaim recursion documentation ;)

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 17 lines · Michal Hocko

Quoted 15 lines · Richard Weinberger

Am Dienstag, 24. April 2018, 21:28:03 CEST schrieb Michal Hocko:

Quoted 11 lines

Quoted 3 lines

Also only for debugging.
Getting rid of vmalloc with GFP_NOFS in UBIFS is no big problem. I can prepare a patch.

Cool!

Anyway, if UBIFS has some reclaim recursion critical sections in general it would be really great to have them documented and that is where the scope api is really handy. Just add the scope and document what is the recursion issue. This will help people reading the code as well. Ideally there shouldn't be any explicit GFP_NOFS in the code.

So in a perfect world a filesystem calls memalloc_nofs_save/restore and always uses GFP_KERNEL for kmalloc/vmalloc?

Quoted 5 lines

Exactly! And in a dream world those memalloc_nofs_save act as a documentation of the reclaim recursion documentation ;)

Signature · 2 lines

Michal Hocko
SUSE Labs

BTW. should memalloc_nofs_save and memalloc_noio_save be merged into just one that prevents both I/O and FS recursion?

memalloc_nofs_save allows submitting bios to I/O stack and the bios created under memalloc_nofs_save could be sent to the loop device and the loop device calls the filesystem...

Quoted 27 lines · Mikulas Patocka

Quoted 22 lines · Michal Hocko

Quoted 7 lines · Richard Weinberger

Am Dienstag, 24. April 2018, 21:28:03 CEST schrieb Michal Hocko:

Quoted 6 lines

Quoted 3 lines

Also only for debugging.
Getting rid of vmalloc with GFP_NOFS in UBIFS is no big problem. I can prepare a patch.

Cool!

Quoted 8 lines

Quoted 5 lines

Anyway, if UBIFS has some reclaim recursion critical sections in general it would be really great to have them documented and that is where the scope api is really handy. Just add the scope and document what is the recursion issue. This will help people reading the code as well. Ideally there shouldn't be any explicit GFP_NOFS in the code.

So in a perfect world a filesystem calls memalloc_nofs_save/restore and always uses GFP_KERNEL for kmalloc/vmalloc?

Exactly! And in a dream world those memalloc_nofs_save act as a documentation of the reclaim recursion documentation ;)

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 2 lines

BTW. should memalloc_nofs_save and memalloc_noio_save be merged into just one that prevents both I/O and FS recursion?

Why should FS usage stop IO altogether?

Quoted 3 lines

memalloc_nofs_save allows submitting bios to I/O stack and the bios created under memalloc_nofs_save could be sent to the loop device and the loop device calls the filesystem...

Don't those use NOIO context?

Quoted 28 lines · Michal Hocko

Quoted 27 lines · Mikulas Patocka

Quoted 22 lines · Michal Hocko

Quoted 7 lines · Richard Weinberger

Am Dienstag, 24. April 2018, 21:28:03 CEST schrieb Michal Hocko:

Quoted 6 lines

Quoted 3 lines

Also only for debugging.
Getting rid of vmalloc with GFP_NOFS in UBIFS is no big problem. I can prepare a patch.

Cool!

Quoted 8 lines

Quoted 5 lines

Anyway, if UBIFS has some reclaim recursion critical sections in general it would be really great to have them documented and that is where the scope api is really handy. Just add the scope and document what is the recursion issue. This will help people reading the code as well. Ideally there shouldn't be any explicit GFP_NOFS in the code.

So in a perfect world a filesystem calls memalloc_nofs_save/restore and always uses GFP_KERNEL for kmalloc/vmalloc?

Exactly! And in a dream world those memalloc_nofs_save act as a documentation of the reclaim recursion documentation ;)

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 4 lines

Quoted 2 lines

BTW. should memalloc_nofs_save and memalloc_noio_save be merged into just one that prevents both I/O and FS recursion?

Why should FS usage stop IO altogether?

Because the IO may reach loop and loop may redirect it to the same filesystem that is running under memalloc_nofs_save and deadlock.

Quoted 5 lines

Quoted 3 lines

memalloc_nofs_save allows submitting bios to I/O stack and the bios created under memalloc_nofs_save could be sent to the loop device and the loop device calls the filesystem...

Don't those use NOIO context?

What do you mean?

Quoted 21 lines · Mikulas Patocka

Quoted 17 lines · Michal Hocko

Quoted 15 lines · Mikulas Patocka

Quoted 8 lines · Michal Hocko

Quoted 2 lines

So in a perfect world a filesystem calls memalloc_nofs_save/restore and always uses GFP_KERNEL for kmalloc/vmalloc?

Exactly! And in a dream world those memalloc_nofs_save act as a documentation of the reclaim recursion documentation ;)

Signature · 2 lines

Michal Hocko
SUSE Labs

BTW. should memalloc_nofs_save and memalloc_noio_save be merged into just one that prevents both I/O and FS recursion?

Quoted 4 lines

Quoted 1 line

Why should FS usage stop IO altogether?

Because the IO may reach loop and loop may redirect it to the same filesystem that is running under memalloc_nofs_save and deadlock.

So what is the difference with the current GFP_NOFS?

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

memalloc_nofs_save allows submitting bios to I/O stack and the bios created under memalloc_nofs_save could be sent to the loop device and the loop device calls the filesystem...

Don't those use NOIO context?

What do you mean?

That the loop driver should make sure it will not recurse. The scope API doesn't add anything new here.

Quoted 27 lines · Michal Hocko

Quoted 25 lines · Mikulas Patocka

Quoted 14 lines · Michal Hocko

Quoted 13 lines · Mikulas Patocka

Quoted 8 lines · Michal Hocko

Quoted 2 lines

So in a perfect world a filesystem calls memalloc_nofs_save/restore and always uses GFP_KERNEL for kmalloc/vmalloc?

Exactly! And in a dream world those memalloc_nofs_save act as a documentation of the reclaim recursion documentation ;)

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 4 lines

Quoted 2 lines

BTW. should memalloc_nofs_save and memalloc_noio_save be merged into just one that prevents both I/O and FS recursion?

Why should FS usage stop IO altogether?

Because the IO may reach loop and loop may redirect it to the same filesystem that is running under memalloc_nofs_save and deadlock.

Quoted 1 line

So what is the difference with the current GFP_NOFS?

My point is that filesystems should use GFP_NOIO too. If alloc_pages(GFP_NOFS) issues some random I/O to some block device, the I/O may be end up being redirected (via block loop device) to the filesystem that is calling alloc_pages(GFP_NOFS).

Am Dienstag, 24. April 2018, 18:27:12 CEST schrieb Michal Hocko:

Quoted 1 line

fs/ubifs/debug.c

This one is just for debugging.
So, preallocating + locking would not hurt much.

Quoted 1 line

fs/ubifs/lprops.c

Ditto.

Quoted 1 line

fs/ubifs/lpt_commit.c

Here we use it also only in debugging mode and in one case for fatal error reporting.
No hot paths.

Quoted 1 line

fs/ubifs/orphan.c

Also only for debugging.
Getting rid of vmalloc with GFP_NOFS in UBIFS is no big problem. I can prepare a patch.

Hi,
it seems that we still have few vmalloc users who perform GFP_NOFS allocation:
drivers/mtd/ubi/io.c

Hi,
it seems that we still have few vmalloc users who perform GFP_NOFS allocation:
drivers/mtd/ubi/io.c

fs/ext4/xattr.c
fs/gfs2/dir.c
fs/gfs2/quota.c
fs/nfs/blocklayout/extent_tree.c
fs/ubifs/debug.c
fs/ubifs/lprops.c
fs/ubifs/lpt_commit.c
fs/ubifs/orphan.c

Hi,
it seems that we still have few vmalloc users who perform GFP_NOFS allocation:
drivers/mtd/ubi/io.c
fs/ext4/xattr.c
fs/gfs2/dir.c
fs/gfs2/quota.c
fs/nfs/blocklayout/extent_tree.c
fs/ubifs/debug.c
fs/ubifs/lprops.c
fs/ubifs/lpt_commit.c
fs/ubifs/orphan.c

Unfortunatelly vmalloc doesn't suppoer GFP_NOFS semantinc properly because we do have hardocded GFP_KERNEL allocations deep inside the vmalloc layers. That means that if GFP_NOFS really protects from recursion into the fs deadlocks then the vmalloc call is broken.

What to do about this? Well, there are two things. Firstly, it would be really great to double check whether the GFP_NOFS is really needed. I cannot judge that because I am not familiar with the code. It would be great if the respective maintainers (hopefully get_maintainer.sh pointed me to all relevant ones). If there is not reclaim recursion issue then simply use the standard vmalloc (aka GFP_KERNEL request).

If the use is really valid then we have a way to do the vmalloc allocation properly. We have memalloc_nofs_{save,restore} scope api. How does that work? You simply call memalloc_nofs_save when the reclaim recursion critical section starts (e.g. when you take a lock which is then used in the reclaim path - e.g. shrinker) and memalloc_nofs_restore when the critical section ends. _All_ allocations within that scope will get GFP_NOFS semantic automagically. If you are not sure about the scope itself then the easiest workaround is to wrap the vmalloc itself with a big fat comment that this should be revisited.

Does that sound like something that can be done in a reasonable time? I have tried to bring this up in the past but our speed is glacial and there are attempts to do hacks like checking for abusers inside the vmalloc which is just too ugly to live.

Please do not hesitate to get back to me if something is not clear.

Thanks!

It would be good to fix this, and it has been known as an issue for a long time. We might well be able to make use of the new API though. It might be as simple as adding the calls when we get & release glocks, but I'd have to check the code to be sure,

On Tue 24-04-18 20:26:23, Steven Whitehouse wrote:
[...]

Quoted 4 lines

It would be good to fix this, and it has been known as an issue for a long time. We might well be able to make use of the new API though. It might be as simple as adding the calls when we get & release glocks, but I'd have to check the code to be sure,

Yeah, starting with annotating those locking contexts and how document how their are used in the reclaim is the great first step. This has to be done per-fs obviously.