I agree this function needs work, there is a major performance issue in there as well. One step at a time. Meanwhile I need this patch to be acked so It can move to the main branch, as it fixes multiple bugs seen in production. On Tue, Dec 10, 2024 at 9:28 PM Viacheslav Dubeyko <Slava.Dubeyko@xxxxxxx> wrote: > > On Tue, 2024-12-10 at 21:12 +0200, Alex Markuze wrote: > > The main goal of this patch is to solve erroneous read sizes and > > overflows. > > The convoluted 'if else' chain is a recipe for disaster. Currently, > > exec stops immediately on first ret that indicates an error. > > If you have additional refactoring thoughts feel free to add more > > patches, This is mainly a bug fix, that solves both the immediate > > overflow bug and attempts to make this code more manageable to > > mitigate future bugs. > > I see your point. I simply see several cases of ret > 0 check: > > https://elixir.bootlin.com/linux/v6.12.3/source/fs/ceph/file.c#L1150 > https://elixir.bootlin.com/linux/v6.12.3/source/fs/ceph/file.c#L1158 > https://elixir.bootlin.com/linux/v6.12.3/source/fs/ceph/file.c#L1163 <- > your fix here > https://elixir.bootlin.com/linux/v6.12.3/source/fs/ceph/file.c#L1192 <- > your fix here too > https://elixir.bootlin.com/linux/v6.12.3/source/fs/ceph/file.c#L1236 > > And there are places to check ret for negative values: > > https://elixir.bootlin.com/linux/v6.12.3/source/fs/ceph/file.c#L1160 > https://elixir.bootlin.com/linux/v6.12.3/source/fs/ceph/file.c#L1226 > > These checks distributes in the function's code, it could be confusing > and, potentially, be the source of new bugs during modifications. I > simply have feelings that this logic somehow requires refactoring to > improve the execution flow. But if you would like not to do it, then I > am OK with it. > > Thanks, > Slava. > >
