All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Sterba <dsterba@suse.cz>
To: Sun YangKai <sunk67188@gmail.com>
Cc: frank.li@vivo.com, clm@fb.com, dsterba@suse.com,
	josef@toxicpanda.com, linux-btrfs@vger.kernel.org,
	linux-kernel@vger.kernel.org, neelx@suse.com
Subject: Re: [PATCH 1/3] btrfs: get rid of path allocation in btrfs_del_inode_extref()
Date: Tue, 15 Apr 2025 17:56:37 +0200	[thread overview]
Message-ID: <20250415155637.GG16750@suse.cz> (raw)
In-Reply-To: <3353953.aeNJFYEL58@saltykitkat>

On Tue, Apr 15, 2025 at 10:45:06PM +0800, Sun YangKai wrote:
> It seems a nice try to reduce path allocation and improve performance.
> 
> But it also seems make the code less maintainable. I would prefer to  have a 
> comment saying something like the @path argument is just for reuse the 
> btrfs_path allocation and only a released or empty btrfs_path should be used 
> here.

Yes, this should be there, though we use the pattern of passing existing
path to functions so this within what'd consider OK.

> Also, although the path passed is released, it seems the bit flags are still 
> passed, which makes the behavior of the functions a little different. But it 
> seems fine since those bit flags are never set in this code path.

Also a good point, the path should be in a pristine state, as if it were
just allocated. Releasing paths in other functions may want to keep the
bits but in this case we're crossing a function boundary and the same
assumptions may not be the same.

Release resets the ->nodes, so what's left is from ->slots until the the
end of the structure. And a helper for that would be desirable rather
than opencoding that.

  reply	other threads:[~2025-04-15 15:56 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-04-15  3:38 [PATCH 1/3] btrfs: get rid of path allocation in btrfs_del_inode_extref() Yangtao Li
2025-04-15  3:38 ` [PATCH 2/3] btrfs: get rid of path allocation in btrfs_insert_inode_extref() Yangtao Li
2025-04-15  3:38 ` [PATCH 3/3] btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_truncate_inode_items() Yangtao Li
2025-04-15 14:45 ` [PATCH 1/3] btrfs: get rid of path allocation in btrfs_del_inode_extref() Sun YangKai
2025-04-15 15:56   ` David Sterba [this message]
2025-04-16 13:24     ` 回复: " 李扬韬
2025-04-16 13:37       ` Filipe Manana
2025-04-16 19:11         ` David Sterba
2025-04-16 19:40           ` Filipe Manana
2025-04-17 14:15             ` 回复: " 李扬韬
2025-04-17 14:36               ` Filipe Manana
2025-04-16 19:14       ` 回复: " David Sterba

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20250415155637.GG16750@suse.cz \
    --to=dsterba@suse.cz \
    --cc=clm@fb.com \
    --cc=dsterba@suse.com \
    --cc=frank.li@vivo.com \
    --cc=josef@toxicpanda.com \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=neelx@suse.com \
    --cc=sunk67188@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.