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

David Sterba
2020-08-14T10:30:28Z

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.