On Tue, Oct 19, 2021 at 03:57:38PM -0400, Josef Bacik wrote:

On Tue, Oct 19, 2021 at 04:54:12PM +0200, David Sterba wrote:

quoted

On Mon, Oct 18, 2021 at 05:28:38PM -0400, Josef Bacik wrote:

quoted

On Mon, Oct 18, 2021 at 07:38:03PM +0200, David Sterba wrote:

This is what needs to be done per caller.

Ooooh so we don't want to enforce NOFS for ALL trans handles, just some of them?

Practically all callers. If a transaction is running, a GFP_KERNEL
allocation could recurse back to btrfs when allocator decides to flush
some memory. The NOFS protection at the transaction start call wold not
be needed in case it's been already set by the caller for some reason.

If that's the case can't we just do

trans = btrfs_join_transaction_nofs()/btrfs_start_transaction_nofs()

and then handle it internally?

This could work in case btrfs_join_transaction_nofs could store the
nofs_flags in a way that btrfs_start_transaction_nofs would see. But ...

quoted

quoted

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,

I had to dig in my memory why we can't store it in the transaction
handle, because this is naturally less intrusive. But it does not work.

There are two things:

1) In a function that starts/joins a transaction, the NOFS scope is from
   that call until the transaction end. This is caller-specific.
   Like in the example above, any allocation with GFP_KERNEL happening
   will be safe and not recurse back to btrfs.

2) Transaction handle is not caller-specific and is allocated when the
   transaction starts (ie. a new kmem_cache_alloc call is done). Any
   caller of transaction start will only increase the reference count.

Right but we only really need to do the release when we free the trans handle,
so in fact we can just leave it for the end of btrfs_end_transaction() when we
free the trans handle and still be good.

Filipe's response made me think again if the lifetime of the NOFS scope
does match the exact lifetime of transaction handle, meaning it could be
stored inside it and the whole point of this patch would be moot.

I don't have a clear answer right now and would like to experiment with
that before pushing even this stub, because reverting it again in the
future would not be that funny.