Re: [PATCH v3] btrfs: try to search for data csums in commit root

[David Sterba <dsterba@suse.cz>]

On Tue, Oct 15, 2024 at 11:01:44AM -0700, Boris Burkov wrote:
> > > +#define BTRFS_BIO_FLAG_CSUM_SEARCH_COMMIT_ROOT	1U << (BIO_FLAG_LAST + 1)
> > 
> > All expressions in macros should be in ( ), namelly when there's a
> > potential for operator precedence change like with "<<" after the macro
> > is expanded in some code.
> > 
> > > +#define BTRFS_BIO_PRIVATE_FLAG_MASK	(BTRFS_BIO_FLAG_CSUM_SEARCH_COMMIT_ROOT)
> > 
> > Do you plan to add more such flags to private? This looks line one level
> > more of abstraction than we need for this optimization. This could be
> > simply used as BTRFS_BIO_FLAG_CSUM_SEARCH_COMMIT_ROOT. And for that
> > reason the helpers do not need to sound generic that it manipulates
> > 'private', the names can be btrfs_bio_set_csum_search_commit_root(),
> > which is IMHO expressing the same semantics.
> 
> I don't plan to add any more btrfs private bio flags. I made the
> judgment that doing it generically with checking (particularly for the
> important and explicit clear before submit) was better than the one off,
> because it made what was being done as clear as possible. It's still not
> perfectly clear, because of the whole "private flags" name not being
> perfect.
> 
> Since there is only one user, I totally see the argument for just doing
> it as a one-off. Would you like me to rewrite it to
> btrfs_bio_set_csum_search_commit_root?

Yes please, make it specific for the csum search root. Making it generic
works when we have at least two, in use or soon to be used, but as you
say no plans for more so it's best to keep it suitable what we need now.
If it is for any reason needaed in the future we know how to extend the
interface, so no big deal.