Re: [PATCH v4 1/4] btrfs: extent_io: Do extra check for extent buffer read write functions

[David Sterba <dsterba@suse.cz>]

On Fri, Aug 14, 2020 at 08:47:17AM +0800, Qu Wenruo wrote:
> 
> 
> On 2020/8/13 下午10:05, David Sterba wrote:
> > On Wed, Aug 12, 2020 at 02:05:06PM +0800, Qu Wenruo wrote:
> >> +/*
> >> + * Check if the [start, start + len) range is valid before reading/writing
> >> + * the eb.
> >> + * NOTE: @start and @len are offset *INSIDE* the eb, *NOT* logical address.
> >> + *
> >> + * Caller should not touch the dst/src memory if this function returns error.
> >> + */
> >> +static int check_eb_range(const struct extent_buffer *eb, unsigned long start,
> >> +			  unsigned long len)
> >> +{
> >> +	/* start, start + len should not go beyond eb->len nor overflow */
> >> +	if (unlikely(start > eb->len || start + len > eb->len ||
> >> +		     len > eb->len)) {
> >> +		btrfs_warn(eb->fs_info,
> >> +"btrfs: bad eb rw request, eb bytenr=%llu len=%lu rw start=%lu len=%lu\n",
> >> +			   eb->start, eb->len, start, len);
> >> +		WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
> >> +		return -EINVAL;
> >> +	}
> >> +	return 0;
> >> +}
> > 
> > This helper is similar to the check_setget_bounds that have some
> > performance impact,
> > https://lore.kernel.org/linux-btrfs/20200730110943.GE3703@twin.jikos.cz/
> > .
> > 
> > The extent buffer helpers are not called that often as the setget
> > helpers but still it could be improved to avoid the function call
> > penalty on the hot path.
> > 
> > static inline in check_eb_range(...) {
> > 	if (unlikely(out of range))
> > 		return report_eb_range(...)
> > 	return 0;
> > }
> 
> I thought we should avoid manual inline, but let the compiler to
> determine if it's needed.
> 
> Or in this particular case, we're better than the compiler?

In general it shouldn't be necessary to inline or partition the
functions. In the check_setget case it had a noticeable impact on
performance, so crafting the hot path manually produces a better
assembly and does not depend on the compiler optimizations. The
important part here is that this has been analyzed and measured that it
really makes a difference.

For sanity checks I think we should try to make it as fast as possible,
it's better to have them then not but we also don't want to sacrifice
performance. I haven't analyzed the asm code impact in this patch but
the pattern and flow of check_eb_range is the same.