Re: [PATCH] btrfs: add stub argument to transaction API

[Josef Bacik <josef@toxicpanda.com>]

On Mon, Oct 18, 2021 at 07:38:03PM +0200, David Sterba wrote:
> This is minimum preparatory patch (still big) for the scoped NOFS
> changes. I'd like to get it merged for 5.16 so I can start the
> conversion. To keep patch conflicts low I'm going to apply as one of the
> last patches before code freeze, as we're in rc6 time it's a good
> opportunity to still catch the next dev cycle.
> 
> ---
> 
> This is preparatory work for scope NOFS [1]. The section that must not
> recurse back to the filesystem would be enclosed in
> memalloc_nofs_save/memalloc_nofs_restore, wrapped by our transaction
> API. The new argument will store the context data for the NOFS status.
> 
> This patch doesn't change anything yet, only extends the functions to
> make it easier to integrate with new patches and develop the real scope
> NOFS changes in incrementally.
> 
> Until the rest of the changes lands, use NULL for the argument.
> 
> References:
> 
> * https://lwn.net/Articles/635354/ (2015)
> * https://lwn.net/Articles/723317/ (2017)
> * https://lwn.net/Articles/710545/ (2017)
> * every now and then somebody asks about GFP_NOFAIL and GFP_NOFS
> 
> Note: full conversion to scoped NOFS is a long term plan with following
> phases:
> 
> 1. switch GFP_NOFS to GFP_KERNEL where it's safe without any additional
>    changes (context is safe, eg. in tests)
> 2. switch GFP_NOFS to memalloc_nofs_* + GFP_KERNEL in cases where we
>    know it's unsafe to do plain GFP_KERNEL, eg. with an open transaction
> 3. introduce framework to do transaction start/end scope NOFS
>    protection, no actual changes
> 4. incrementally switch function by function to use the local
>    transaction context to track the NOFS context
> 5. once finished, remove the memalloc_nofs_* calls
> 
> 1 and 2 are basically done, 3 is this patch.
> 
> Why the stub/context argument is needed: the NOFS protection is per call
> site, so it must be set and reset in the caller thread, so any
> allocations between btrfs_start_transaction and btrfs_end_transaction
> are safe. We can't store it in the transaction handle, because it's not
> passed everywhere, eg. to various helpers in btrfs and potentially in
> other subsystems.
> 

So the plan is to instead pass the tctx around everywhere to carry the flags?  I
thought the whole point of memalloc_nofs_save() is that we don't have to pass
gfp_t's around everywhere, it just knows what we're supposed to be doing?  So
the trans should be able to hold the flags since we only care about starting it
and restoring it, correct?  Or am I wrong and we do actually need to pass this
thing around?  In which case can't we still just save it in the trans handle,
and pass the u32 around where appropriate?  Thanks,

Josef