Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Michal Hocko]

On Mon 28-05-18 10:21:00, Nikolay Borisov wrote:
> 
> 
> On 25.05.2018 10:52, Michal Hocko wrote:
> > On Thu 24-05-18 09:37:18, Randy Dunlap wrote:
> >> On 05/24/2018 04:43 AM, Michal Hocko wrote:
> > [...]
> >>> +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.
> 
> 
> 
> 
> 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
> 
> 

Fixed
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Michal Hocko]

On Mon 28-05-18 10:21:00, Nikolay Borisov wrote:
> 
> 
> On 25.05.2018 10:52, Michal Hocko wrote:
> > On Thu 24-05-18 09:37:18, Randy Dunlap wrote:
> >> On 05/24/2018 04:43 AM, Michal Hocko wrote:
> > [...]
> >>> +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.
> 
> 
> 
> 
> 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
> 
> 

Fixed
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Michal Hocko]

On Mon 28-05-18 09:10:43, Randy Dunlap wrote:
> On 05/28/2018 02:21 AM, Michal Hocko wrote:
[...]
> > +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.

Dave has pointed that out.
"The restore function should be called when the critical section ends."

> > +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.

Fixed. Thanks
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Michal Hocko]

On Mon 28-05-18 09:10:43, Randy Dunlap wrote:
> On 05/28/2018 02:21 AM, Michal Hocko wrote:
[...]
> > +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.

Dave has pointed that out.
"The restore function should be called when the critical section ends."

> > +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.

Fixed. Thanks
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Michal Hocko]

On Tue 29-05-18 08:32:05, Dave Chinner wrote:
> On Mon, May 28, 2018 at 11:19:23AM +0200, Michal Hocko wrote:
> > On Mon 28-05-18 09:48:54, Dave Chinner wrote:
> > > On Fri, May 25, 2018 at 10:16:24AM +0200, Michal Hocko wrote:
> > > > On Fri 25-05-18 08:17:15, Dave Chinner wrote:
> > > > > On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:
> > > > [...]
> > > > > > +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.
> > > > 
> > > > Any better?
> > > > 
> > > > -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.
> > > > +
> > > > +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.
> >  
> > > 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?
> > 
> > diff --git a/Documentation/core-api/gfp_mask-from-fs-io.rst 
> > b/Documentation/core-api/gfp_mask-from-fs-io.rst
> > index c0ec212d6773..adac362b2875 100644
> > --- a/Documentation/core-api/gfp_mask-from-fs-io.rst
> > +++ b/Documentation/core-api/gfp_mask-from-fs-io.rst
> > @@ -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.
> >  
> >  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 ...

fixed

> But otherwise I think this is much better.

Thanks!

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Michal Hocko]

On Tue 29-05-18 08:32:05, Dave Chinner wrote:
> On Mon, May 28, 2018 at 11:19:23AM +0200, Michal Hocko wrote:
> > On Mon 28-05-18 09:48:54, Dave Chinner wrote:
> > > On Fri, May 25, 2018 at 10:16:24AM +0200, Michal Hocko wrote:
> > > > On Fri 25-05-18 08:17:15, Dave Chinner wrote:
> > > > > On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:
> > > > [...]
> > > > > > +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.
> > > > 
> > > > Any better?
> > > > 
> > > > -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.
> > > > +
> > > > +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.
> >  
> > > 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?
> > 
> > diff --git a/Documentation/core-api/gfp_mask-from-fs-io.rst 
> > b/Documentation/core-api/gfp_mask-from-fs-io.rst
> > index c0ec212d6773..adac362b2875 100644
> > --- a/Documentation/core-api/gfp_mask-from-fs-io.rst
> > +++ b/Documentation/core-api/gfp_mask-from-fs-io.rst
> > @@ -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.
> >  
> >  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 ...

fixed

> But otherwise I think this is much better.

Thanks!

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Dave Chinner]

On Mon, May 28, 2018 at 11:19:23AM +0200, Michal Hocko wrote:
> On Mon 28-05-18 09:48:54, Dave Chinner wrote:
> > On Fri, May 25, 2018 at 10:16:24AM +0200, Michal Hocko wrote:
> > > On Fri 25-05-18 08:17:15, Dave Chinner wrote:
> > > > On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:
> > > [...]
> > > > > +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.
> > > 
> > > Any better?
> > > 
> > > -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.
> > > +
> > > +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.
>  
> > 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?
> 
> diff --git a/Documentation/core-api/gfp_mask-from-fs-io.rst 
> b/Documentation/core-api/gfp_mask-from-fs-io.rst
> index c0ec212d6773..adac362b2875 100644
> --- a/Documentation/core-api/gfp_mask-from-fs-io.rst
> +++ b/Documentation/core-api/gfp_mask-from-fs-io.rst
> @@ -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.
>  
>  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.

Cheers,

Dave.
-- 
Dave Chinner
da...@fromorbit.com

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Dave Chinner]

On Mon, May 28, 2018 at 11:19:23AM +0200, Michal Hocko wrote:
> On Mon 28-05-18 09:48:54, Dave Chinner wrote:
> > On Fri, May 25, 2018 at 10:16:24AM +0200, Michal Hocko wrote:
> > > On Fri 25-05-18 08:17:15, Dave Chinner wrote:
> > > > On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:
> > > [...]
> > > > > +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.
> > > 
> > > Any better?
> > > 
> > > -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.
> > > +
> > > +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.
>  
> > 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?
> 
> diff --git a/Documentation/core-api/gfp_mask-from-fs-io.rst 
> b/Documentation/core-api/gfp_mask-from-fs-io.rst
> index c0ec212d6773..adac362b2875 100644
> --- a/Documentation/core-api/gfp_mask-from-fs-io.rst
> +++ b/Documentation/core-api/gfp_mask-from-fs-io.rst
> @@ -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.
>  
>  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.

Cheers,

Dave.
-- 
Dave Chinner
da...@fromorbit.com

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Randy Dunlap]

On 05/28/2018 02:21 AM, Michal Hocko wrote:
> On Sun 27-05-18 15:47:22, Mike Rapoport wrote:
>> On Fri, May 25, 2018 at 10:16:24AM +0200, Michal Hocko wrote:
>>> On Fri 25-05-18 08:17:15, Dave Chinner wrote:
 On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:
>>> [...]
> +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.
>>>
>>> Any better?
>>>
>>> -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
>  
>>> +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
> 
> diff --git a/Documentation/core-api/gfp_mask-from-fs-io.rst 
> b/Documentation/core-api/gfp_mask-from-fs-io.rst
> index c0ec212d6773..0cff411693ab 100644
> --- a/Documentation/core-api/gfp_mask-from-fs-io.rst
> +++ b/Documentation/core-api/gfp_mask-from-fs-io.rst
> @@ -34,12 +34,15 @@ 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
> -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.
> -
> -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.
> +critical section wrt. the reclaim is started - e.g. lock shared with the

Please spell out "with respect to".

> +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.

> +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.


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


-- 
~Randy

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Randy Dunlap]

On 05/28/2018 02:21 AM, Michal Hocko wrote:
> On Sun 27-05-18 15:47:22, Mike Rapoport wrote:
>> On Fri, May 25, 2018 at 10:16:24AM +0200, Michal Hocko wrote:
>>> On Fri 25-05-18 08:17:15, Dave Chinner wrote:
 On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:
>>> [...]
> +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.
>>>
>>> Any better?
>>>
>>> -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
>  
>>> +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
> 
> diff --git a/Documentation/core-api/gfp_mask-from-fs-io.rst 
> b/Documentation/core-api/gfp_mask-from-fs-io.rst
> index c0ec212d6773..0cff411693ab 100644
> --- a/Documentation/core-api/gfp_mask-from-fs-io.rst
> +++ b/Documentation/core-api/gfp_mask-from-fs-io.rst
> @@ -34,12 +34,15 @@ 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
> -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.
> -
> -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.
> +critical section wrt. the reclaim is started - e.g. lock shared with the

Please spell out "with respect to".

> +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.

> +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.


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


-- 
~Randy

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Vlastimil Babka]

On 05/25/2018 09:52 AM, Michal Hocko wrote:
> On Thu 24-05-18 09:37:18, Randy Dunlap wrote:
>> On 05/24/2018 04:43 AM, Michal Hocko wrote:
> [...]
>>> +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

In that case "latter" is the proper word AFAIK. You could also use
"former" instead of "first". Or maybe just repeat the flag names to
avoid confusion...

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Vlastimil Babka]

On 05/25/2018 09:52 AM, Michal Hocko wrote:
> On Thu 24-05-18 09:37:18, Randy Dunlap wrote:
>> On 05/24/2018 04:43 AM, Michal Hocko wrote:
> [...]
>>> +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

In that case "latter" is the proper word AFAIK. You could also use
"former" instead of "first". Or maybe just repeat the flag names to
avoid confusion...

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Michal Hocko]

On Mon 28-05-18 09:48:54, Dave Chinner wrote:
> On Fri, May 25, 2018 at 10:16:24AM +0200, Michal Hocko wrote:
> > On Fri 25-05-18 08:17:15, Dave Chinner wrote:
> > > On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:
> > [...]
> > > > +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.
> > 
> > Any better?
> > 
> > -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.
> > +
> > +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.
 
> 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?

diff --git a/Documentation/core-api/gfp_mask-from-fs-io.rst 
b/Documentation/core-api/gfp_mask-from-fs-io.rst
index c0ec212d6773..adac362b2875 100644
--- a/Documentation/core-api/gfp_mask-from-fs-io.rst
+++ b/Documentation/core-api/gfp_mask-from-fs-io.rst
@@ -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.
 
 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
+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 memalloc_noio_save is safe to be called from an existing NOIO or NOFS scope.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Michal Hocko]

On Mon 28-05-18 09:48:54, Dave Chinner wrote:
> On Fri, May 25, 2018 at 10:16:24AM +0200, Michal Hocko wrote:
> > On Fri 25-05-18 08:17:15, Dave Chinner wrote:
> > > On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:
> > [...]
> > > > +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.
> > 
> > Any better?
> > 
> > -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.
> > +
> > +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.
 
> 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?

diff --git a/Documentation/core-api/gfp_mask-from-fs-io.rst 
b/Documentation/core-api/gfp_mask-from-fs-io.rst
index c0ec212d6773..adac362b2875 100644
--- a/Documentation/core-api/gfp_mask-from-fs-io.rst
+++ b/Documentation/core-api/gfp_mask-from-fs-io.rst
@@ -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.
 
 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
+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 memalloc_noio_save is safe to be called from an existing NOIO or NOFS scope.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Michal Hocko]

On Sun 27-05-18 15:47:22, Mike Rapoport wrote:
> On Fri, May 25, 2018 at 10:16:24AM +0200, Michal Hocko wrote:
> > On Fri 25-05-18 08:17:15, Dave Chinner wrote:
> > > On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:
> > [...]
> > > > +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.
> > 
> > Any better?
> > 
> > -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
 
> > +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

diff --git a/Documentation/core-api/gfp_mask-from-fs-io.rst 
b/Documentation/core-api/gfp_mask-from-fs-io.rst
index c0ec212d6773..0cff411693ab 100644
--- a/Documentation/core-api/gfp_mask-from-fs-io.rst
+++ b/Documentation/core-api/gfp_mask-from-fs-io.rst
@@ -34,12 +34,15 @@ 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
-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.
-
-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.
+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
+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.
 
 What about __vmalloc(GFP_NOFS)
 ==

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Nikolay Borisov]

On 25.05.2018 10:52, Michal Hocko wrote:
> On Thu 24-05-18 09:37:18, Randy Dunlap wrote:
>> On 05/24/2018 04:43 AM, Michal Hocko wrote:
> [...]
>>> +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.




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




> 

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Michal Hocko]

On Sun 27-05-18 15:47:22, Mike Rapoport wrote:
> On Fri, May 25, 2018 at 10:16:24AM +0200, Michal Hocko wrote:
> > On Fri 25-05-18 08:17:15, Dave Chinner wrote:
> > > On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:
> > [...]
> > > > +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.
> > 
> > Any better?
> > 
> > -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
 
> > +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

diff --git a/Documentation/core-api/gfp_mask-from-fs-io.rst 
b/Documentation/core-api/gfp_mask-from-fs-io.rst
index c0ec212d6773..0cff411693ab 100644
--- a/Documentation/core-api/gfp_mask-from-fs-io.rst
+++ b/Documentation/core-api/gfp_mask-from-fs-io.rst
@@ -34,12 +34,15 @@ 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
-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.
-
-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.
+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
+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.
 
 What about __vmalloc(GFP_NOFS)
 ==

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Nikolay Borisov]

On 25.05.2018 10:52, Michal Hocko wrote:
> On Thu 24-05-18 09:37:18, Randy Dunlap wrote:
>> On 05/24/2018 04:43 AM, Michal Hocko wrote:
> [...]
>>> +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.




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




> 

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Dave Chinner]

On Fri, May 25, 2018 at 10:16:24AM +0200, Michal Hocko wrote:
> On Fri 25-05-18 08:17:15, Dave Chinner wrote:
> > On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:
> [...]
> > > +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.
> 
> Any better?
> 
> -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.
> +
> +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.

Cheers,

Dave.
-- 
Dave Chinner
da...@fromorbit.com

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Dave Chinner]

On Fri, May 25, 2018 at 10:16:24AM +0200, Michal Hocko wrote:
> On Fri 25-05-18 08:17:15, Dave Chinner wrote:
> > On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:
> [...]
> > > +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.
> 
> Any better?
> 
> -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.
> +
> +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.

Cheers,

Dave.
-- 
Dave Chinner
da...@fromorbit.com

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Mike Rapoport]

On Fri, May 25, 2018 at 10:16:24AM +0200, Michal Hocko wrote:
> On Fri 25-05-18 08:17:15, Dave Chinner wrote:
> > On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:
> [...]
> > > +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.
> 
> Any better?
> 
> -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"

> +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

>  What about __vmalloc(GFP_NOFS)
>  ==
> -- 
> Michal Hocko
> SUSE Labs
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Mike Rapoport]

On Fri, May 25, 2018 at 10:16:24AM +0200, Michal Hocko wrote:
> On Fri 25-05-18 08:17:15, Dave Chinner wrote:
> > On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:
> [...]
> > > +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.
> 
> Any better?
> 
> -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"

> +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

>  What about __vmalloc(GFP_NOFS)
>  ==
> -- 
> Michal Hocko
> SUSE Labs
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Michal Hocko]

On Fri 25-05-18 08:17:15, Dave Chinner wrote:
> On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:
[...]
> > +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.

Any better?

-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.
+
+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.
 
 What about __vmalloc(GFP_NOFS)
 ==
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Michal Hocko]

On Fri 25-05-18 08:17:15, Dave Chinner wrote:
> On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:
[...]
> > +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.

Any better?

-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.
+
+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.
 
 What about __vmalloc(GFP_NOFS)
 ==
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Michal Hocko]

On Thu 24-05-18 14:52:02, Jonathan Corbet wrote:
> On Thu, 24 May 2018 13:43:41 +0200
> Michal Hocko  wrote:
> 
> > From: Michal Hocko 
> > 
> > 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" 
> > Cc: David Sterba 
> > Requested-by: "Theodore Y. Ts'o" 
> > Signed-off-by: Michal Hocko 
> > ---
> > 
> > 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/20180424183536.gf30...@thunk.org
> >  .../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.

I am not really familiar with how the whole rst thing works.

diff --git a/Documentation/core-api/index.rst b/Documentation/core-api/index.rst
index c670a8031786..8a5f48ef16f2 100644
--- a/Documentation/core-api/index.rst
+++ b/Documentation/core-api/index.rst
@@ -25,6 +25,7 @@ Core utilities
genalloc
errseq
printk-formats
+   gfp_mask-from-fs-io
 
 Interfaces for kernel debugging
 ===

This?

> 
> > diff --git a/Documentation/core-api/gfp_mask-from-fs-io.rst 
> > b/Documentation/core-api/gfp_mask-from-fs-io.rst
> > new file mode 100644
> > index ..e8b2678e959b
> > --- /dev/null
> > +++ b/Documentation/core-api/gfp_mask-from-fs-io.rst
> > @@ -0,0 +1,55 @@
> > +=
> > +GFP masks used from FS/IO context
> > +=
> > +
> > +:Date: Mapy, 2018
> 
> Ah...the wonderful month of Mapy:)

fixed

> > +:Author: Michal Hocko 
> > +
> > +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
> 
> "resp." is indeed a bit terse.  Even spelled out as "respectively", though,

OK s@resp\.@respectively@g

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

Basically yes. There are two cases here. NOFS and NOIO. The later being
a subset of the first. I didn't really want to repeat the whole thing
for NOIO.

> 
> > +the gfp mask when calling an allocator. GFP_NOFS resp. GFP_NOIO can be
> 
> Here too.
> 
> > +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" ?

OK

> > +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! :)

Most probably yes ;) I thought I've done that but that was probably in a
different universe. This probably?

diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
index e1f8411e6b80..f49ece8ee37a 100644
--- a/include/linux/sched/mm.h
+++ b/include/linux/sched/mm.h
@@ -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 

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Michal Hocko]

On Thu 24-05-18 14:52:02, Jonathan Corbet wrote:
> On Thu, 24 May 2018 13:43:41 +0200
> Michal Hocko  wrote:
> 
> > From: Michal Hocko 
> > 
> > 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" 
> > Cc: David Sterba 
> > Requested-by: "Theodore Y. Ts'o" 
> > Signed-off-by: Michal Hocko 
> > ---
> > 
> > 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/20180424183536.gf30...@thunk.org
> >  .../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.

I am not really familiar with how the whole rst thing works.

diff --git a/Documentation/core-api/index.rst b/Documentation/core-api/index.rst
index c670a8031786..8a5f48ef16f2 100644
--- a/Documentation/core-api/index.rst
+++ b/Documentation/core-api/index.rst
@@ -25,6 +25,7 @@ Core utilities
genalloc
errseq
printk-formats
+   gfp_mask-from-fs-io
 
 Interfaces for kernel debugging
 ===

This?

> 
> > diff --git a/Documentation/core-api/gfp_mask-from-fs-io.rst 
> > b/Documentation/core-api/gfp_mask-from-fs-io.rst
> > new file mode 100644
> > index ..e8b2678e959b
> > --- /dev/null
> > +++ b/Documentation/core-api/gfp_mask-from-fs-io.rst
> > @@ -0,0 +1,55 @@
> > +=
> > +GFP masks used from FS/IO context
> > +=
> > +
> > +:Date: Mapy, 2018
> 
> Ah...the wonderful month of Mapy:)

fixed

> > +:Author: Michal Hocko 
> > +
> > +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
> 
> "resp." is indeed a bit terse.  Even spelled out as "respectively", though,

OK s@resp\.@respectively@g

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

Basically yes. There are two cases here. NOFS and NOIO. The later being
a subset of the first. I didn't really want to repeat the whole thing
for NOIO.

> 
> > +the gfp mask when calling an allocator. GFP_NOFS resp. GFP_NOIO can be
> 
> Here too.
> 
> > +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" ?

OK

> > +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! :)

Most probably yes ;) I thought I've done that but that was probably in a
different universe. This probably?

diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
index e1f8411e6b80..f49ece8ee37a 100644
--- a/include/linux/sched/mm.h
+++ b/include/linux/sched/mm.h
@@ -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.
+ */
 

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Michal Hocko]

On Thu 24-05-18 09:37:18, Randy Dunlap wrote:
> On 05/24/2018 04:43 AM, Michal Hocko wrote:
[...]
> > +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
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Michal Hocko]

On Thu 24-05-18 09:37:18, Randy Dunlap wrote:
> On 05/24/2018 04:43 AM, Michal Hocko wrote:
[...]
> > +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
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Theodore Y. Ts'o]

On Fri, May 25, 2018 at 08:17:15AM +1000, Dave Chinner wrote:
> On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:
> > From: Michal Hocko 
> > 
> > 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" 
> > Cc: David Sterba 
> > Requested-by: "Theodore Y. Ts'o" 
> > Signed-off-by: Michal Hocko 
> 
> Yay, Documentation! :)

Indeed, many thanks!!!

- Ted

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Theodore Y. Ts'o]

On Fri, May 25, 2018 at 08:17:15AM +1000, Dave Chinner wrote:
> On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:
> > From: Michal Hocko 
> > 
> > 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" 
> > Cc: David Sterba 
> > Requested-by: "Theodore Y. Ts'o" 
> > Signed-off-by: Michal Hocko 
> 
> Yay, Documentation! :)

Indeed, many thanks!!!

- Ted

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Dave Chinner]

On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:
> From: Michal Hocko 
> 
> 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" 
> Cc: David Sterba 
> Requested-by: "Theodore Y. Ts'o" 
> Signed-off-by: Michal Hocko 

Yay, Documentation! :)

> ---
> 
> 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/20180424183536.gf30...@thunk.org
>  .../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
> 
> diff --git a/Documentation/core-api/gfp_mask-from-fs-io.rst 
> b/Documentation/core-api/gfp_mask-from-fs-io.rst
> new file mode 100644
> index ..e8b2678e959b
> --- /dev/null
> +++ b/Documentation/core-api/gfp_mask-from-fs-io.rst
> @@ -0,0 +1,55 @@
> +=
> +GFP masks used from FS/IO context
> +=
> +
> +:Date: Mapy, 2018
> +:Author: Michal Hocko 
> +
> +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``,
> +``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.

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.

Cheers,

Dave.
-- 
Dave Chinner
da...@fromorbit.com

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Dave Chinner]

On Thu, May 24, 2018 at 01:43:41PM +0200, Michal Hocko wrote:
> From: Michal Hocko 
> 
> 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" 
> Cc: David Sterba 
> Requested-by: "Theodore Y. Ts'o" 
> Signed-off-by: Michal Hocko 

Yay, Documentation! :)

> ---
> 
> 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/20180424183536.gf30...@thunk.org
>  .../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
> 
> diff --git a/Documentation/core-api/gfp_mask-from-fs-io.rst 
> b/Documentation/core-api/gfp_mask-from-fs-io.rst
> new file mode 100644
> index ..e8b2678e959b
> --- /dev/null
> +++ b/Documentation/core-api/gfp_mask-from-fs-io.rst
> @@ -0,0 +1,55 @@
> +=
> +GFP masks used from FS/IO context
> +=
> +
> +:Date: Mapy, 2018
> +:Author: Michal Hocko 
> +
> +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``,
> +``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.

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.

Cheers,

Dave.
-- 
Dave Chinner
da...@fromorbit.com

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Jonathan Corbet]

On Thu, 24 May 2018 13:43:41 +0200
Michal Hocko  wrote:

> From: Michal Hocko 
> 
> 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" 
> Cc: David Sterba 
> Requested-by: "Theodore Y. Ts'o" 
> Signed-off-by: Michal Hocko 
> ---
> 
> 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/20180424183536.gf30...@thunk.org
>  .../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.

> diff --git a/Documentation/core-api/gfp_mask-from-fs-io.rst 
> b/Documentation/core-api/gfp_mask-from-fs-io.rst
> new file mode 100644
> index ..e8b2678e959b
> --- /dev/null
> +++ b/Documentation/core-api/gfp_mask-from-fs-io.rst
> @@ -0,0 +1,55 @@
> +=
> +GFP masks used from FS/IO context
> +=
> +
> +:Date: Mapy, 2018

Ah...the wonderful month of Mapy:)

> +:Author: Michal Hocko 
> +
> +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

"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"?

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

Here too.

> +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" ?

> +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! :)

> +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 ?

> +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.

Agree with others on "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.

Thanks,

jon

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Jonathan Corbet]

On Thu, 24 May 2018 13:43:41 +0200
Michal Hocko  wrote:

> From: Michal Hocko 
> 
> 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" 
> Cc: David Sterba 
> Requested-by: "Theodore Y. Ts'o" 
> Signed-off-by: Michal Hocko 
> ---
> 
> 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/20180424183536.gf30...@thunk.org
>  .../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.

> diff --git a/Documentation/core-api/gfp_mask-from-fs-io.rst 
> b/Documentation/core-api/gfp_mask-from-fs-io.rst
> new file mode 100644
> index ..e8b2678e959b
> --- /dev/null
> +++ b/Documentation/core-api/gfp_mask-from-fs-io.rst
> @@ -0,0 +1,55 @@
> +=
> +GFP masks used from FS/IO context
> +=
> +
> +:Date: Mapy, 2018

Ah...the wonderful month of Mapy:)

> +:Author: Michal Hocko 
> +
> +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

"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"?

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

Here too.

> +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" ?

> +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! :)

> +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 ?

> +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.

Agree with others on "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.

Thanks,

jon

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Randy Dunlap]

On 05/24/2018 04:43 AM, Michal Hocko wrote:
> From: Michal Hocko 
> 
> 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" 
> Cc: David Sterba 
> Requested-by: "Theodore Y. Ts'o" 
> Signed-off-by: Michal Hocko 
> ---
> 
> 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/20180424183536.gf30...@thunk.org
>  .../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
> 
> diff --git a/Documentation/core-api/gfp_mask-from-fs-io.rst 
> b/Documentation/core-api/gfp_mask-from-fs-io.rst
> new file mode 100644
> index ..e8b2678e959b
> --- /dev/null
> +++ b/Documentation/core-api/gfp_mask-from-fs-io.rst
> @@ -0,0 +1,55 @@
> +=
> +GFP masks used from FS/IO context
> +=
> +
> +:Date: Mapy, 2018
> +:Author: Michal Hocko 
> +
> +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

latter

> +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

s/POV/point of view/ or whatever it is.

> +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.

I would prefer s/api/API/ throughout.

> +
> +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.
> 


-- 
~Randy

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Randy Dunlap]

On 05/24/2018 04:43 AM, Michal Hocko wrote:
> From: Michal Hocko 
> 
> 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" 
> Cc: David Sterba 
> Requested-by: "Theodore Y. Ts'o" 
> Signed-off-by: Michal Hocko 
> ---
> 
> 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/20180424183536.gf30...@thunk.org
>  .../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
> 
> diff --git a/Documentation/core-api/gfp_mask-from-fs-io.rst 
> b/Documentation/core-api/gfp_mask-from-fs-io.rst
> new file mode 100644
> index ..e8b2678e959b
> --- /dev/null
> +++ b/Documentation/core-api/gfp_mask-from-fs-io.rst
> @@ -0,0 +1,55 @@
> +=
> +GFP masks used from FS/IO context
> +=
> +
> +:Date: Mapy, 2018
> +:Author: Michal Hocko 
> +
> +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

latter

> +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

s/POV/point of view/ or whatever it is.

> +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.

I would prefer s/api/API/ throughout.

> +
> +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.
> 


-- 
~Randy

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Michal Hocko]

On Thu 24-05-18 07:33:39, Shakeel Butt wrote:
> On Thu, May 24, 2018 at 4:43 AM, Michal Hocko  wrote:
[...]
> > +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)?

yes. Because I was lazy ;)

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Michal Hocko]

On Thu 24-05-18 07:33:39, Shakeel Butt wrote:
> On Thu, May 24, 2018 at 4:43 AM, Michal Hocko  wrote:
[...]
> > +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)?

yes. Because I was lazy ;)

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Shakeel Butt]

On Thu, May 24, 2018 at 4:43 AM, Michal Hocko  wrote:
> From: Michal Hocko 
>
> 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" 
> Cc: David Sterba 
> Requested-by: "Theodore Y. Ts'o" 
> Signed-off-by: Michal Hocko 
> ---
>
> 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/20180424183536.gf30...@thunk.org
>  .../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
>
> diff --git a/Documentation/core-api/gfp_mask-from-fs-io.rst 
> b/Documentation/core-api/gfp_mask-from-fs-io.rst
> new file mode 100644
> index ..e8b2678e959b
> --- /dev/null
> +++ b/Documentation/core-api/gfp_mask-from-fs-io.rst
> @@ -0,0 +1,55 @@
> +=
> +GFP masks used from FS/IO context
> +=
> +
> +:Date: Mapy, 2018
> +:Author: Michal Hocko 
> +
> +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

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

> +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
>

Re: [PATCH] doc: document scope NOFS, NOIO APIs

[Shakeel Butt]

On Thu, May 24, 2018 at 4:43 AM, Michal Hocko  wrote:
> From: Michal Hocko 
>
> 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" 
> Cc: David Sterba 
> Requested-by: "Theodore Y. Ts'o" 
> Signed-off-by: Michal Hocko 
> ---
>
> 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/20180424183536.gf30...@thunk.org
>  .../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
>
> diff --git a/Documentation/core-api/gfp_mask-from-fs-io.rst 
> b/Documentation/core-api/gfp_mask-from-fs-io.rst
> new file mode 100644
> index ..e8b2678e959b
> --- /dev/null
> +++ b/Documentation/core-api/gfp_mask-from-fs-io.rst
> @@ -0,0 +1,55 @@
> +=
> +GFP masks used from FS/IO context
> +=
> +
> +:Date: Mapy, 2018
> +:Author: Michal Hocko 
> +
> +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

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

> +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
>