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

[Josef Bacik <josef@toxicpanda.com>]

On Tue, Oct 19, 2021 at 04:54:12PM +0200, David Sterba wrote:
> On Mon, Oct 18, 2021 at 05:28:38PM -0400, Josef Bacik wrote:
> > On Mon, Oct 18, 2021 at 07:38:03PM +0200, David Sterba wrote:
> > > 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?
> 
> Nothing needs to be passed around, it will be hidden inside the
> transaction start/end, the only thing the caller needs to do is to
> define the variable and pass it to to transaction start call. The NOFS
> section will then apply to any calls until the transaction end call.
> 
> --- a/fs/btrfs/delayed-inode.c
> +++ b/fs/btrfs/delayed-inode.c
> @@ -1232,6 +1232,7 @@ int btrfs_commit_inode_delayed_inode(struct btrfs_inode *inode)
>         struct btrfs_path *path;
>         struct btrfs_block_rsv *block_rsv;
>         int ret;
> +       DEFINE_TCTX(tctx);
>  
>         if (!delayed_node)
>                 return 0;
> @@ -1244,7 +1245,7 @@ int btrfs_commit_inode_delayed_inode(struct btrfs_inode *inode)
>         }
>         mutex_unlock(&delayed_node->mutex);
>  
> -       trans = btrfs_join_transaction(delayed_node->root, NULL);
> +       trans = btrfs_join_transaction(delayed_node->root, tctx);
>         if (IS_ERR(trans)) {
>                 ret = PTR_ERR(trans);
>                 goto out;
> @@ -1271,7 +1272,7 @@ int btrfs_commit_inode_delayed_inode(struct btrfs_inode *inode)
>         btrfs_free_path(path);
>         trans->block_rsv = block_rsv;
>  trans_out:
> -       btrfs_end_transaction(trans, NULL);
> +       btrfs_end_transaction(trans, tctx);
>         btrfs_btree_balance_dirty(fs_info);
>  out:
>         btrfs_release_delayed_node(delayed_node);
> ---
> 
> 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?

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

trans = btrfs_join_transaction_nofs()/btrfs_start_transaction_nofs()

and then handle it internally?

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

Thanks,

Josef