On Fri, Apr 19, 2024 at 10:28:41AM -0700, Marius Fleischer wrote:
> Hi Dave,
>
> Thank you so much for your quick response to this!
>
>
> > kernel version: 5.15.156
> >
> > Really old kernel.
> >
> >
> I'm sorry, I was under the impression that 5.15.156 is an actively
> maintained kernel as
> it is one of the current LTS version. Is this not the case? I'd love your
> insight on this.
It might be maintained, but it's still a really old kernel. Lots of
meaningful stuff has changed since 5.15.0 was release by Linus...
> > > We took a very brief look at the code. Is it possible that there is a
> > check
> > > missing for the return value of kvmalloc at fs/xfs/xfs_log_cil.c:224?
> > >
> > > lv = kvmalloc(buf_size, GFP_KERNEL);
> > > memset(lv, 0, xlog_cil_iovec_space(niovecs));
> >
> > I've never seen that memory allocation fail there, and that code has
> > been using an unchecked, open coded kvmalloc() for well over a
> > decade. We replaced it with a direct call to kvmalloc() in 5.15,
> > but the failure semantics here never changed.
> >
> > But I guess it could fail if error injection is enabled,
> > and because we used to call __vmalloc() directly it may never have
> > had errors injected?
> >
> > But, regardless, that's completely irrelevant.
> >
> > We replaced the kvmalloc() call there with a guaranteed "no fail"
> > open coded loop in 5.17 for performance reasons, so this open coded
> > kvmalloc() call only existed in 5.15 and 5.16. See commit
> > 8dc9384b7d75 ("xfs: reduce kvmalloc overhead for CIL shadow
> > buffers").
> >
> >
> I am not sure I am completely following you. If I understand you correctly
> (and after
> checking out the commit you linked - thanks for that!), is it correct that
> it would be
> better to patch this by replacing it with the same open coded kvmalloc as
> in 5.17?
What I'm saying is that the code was changed soon afterwards for
different reasons, but hte newer code would also address the issue
you saw.
> Do you think it would make sense to backport that patch to 5.15.156 (the
> LTS kernel)?
That's up to the 5.15 LTS maintainers to decide. They also need to
weigh up the fact that xlog_cil_kvmalloc() doesn't exist anymore in
the upstream code base. i.e. we found other places that had the same
kvmalloc() performance problems, and so lifted that code up further
and used it in other places in XFS....
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
