This is a bad patch, I don't appreciate partial fixes that introduce
unnecessary complications to the code, and it conflicts with the
complete fix in the other thread.
Please coordinate with me in the future.
On Thu, Dec 5, 2024 at 1:25 PM Ilya Dryomov <idryomov@xxxxxxxxx> wrote:
>
> On Thu, Dec 5, 2024 at 9:32 AM Max Kellermann <max.kellermann@xxxxxxxxx> wrote:
> >
> > On Fri, Nov 29, 2024 at 9:06 AM Max Kellermann <max.kellermann@xxxxxxxxx> wrote:
> > >
> > > On Thu, Nov 28, 2024 at 1:18 PM Alex Markuze <amarkuze@xxxxxxxxxx> wrote:
> > > > Pages are freed in `ceph_osdc_put_request`, trying to release them
> > > > this way will end badly.
> > >
> > > Is there anybody else who can explain this to me?
> > > I believe Alex is wrong and my patch is correct, but maybe I'm missing
> > > something.
> >
> > It's been a week. Is there really nobody who understands this piece of
> > code? I believe I do understand it, but my understanding conflicts
> > with Alex's, and he's the expert (and I'm not).
>
> Hi Max,
>
> Your understanding is correct. Pages would be freed automatically
> together with the request only if the ownership is transferred by
> passing true for own_pages to osd_req_op_extent_osd_data_pages(), which
> __ceph_sync_read() doesn't do.
>
> These error path leaks were introduced in commits 03bc06c7b0bd ("ceph:
> add new mount option to enable sparse reads") and f0fe1e54cfcf ("ceph:
> plumb in decryption during reads") with support for fscrypt. Looking
> at the former commit, it looks like a similar leak was introduced in
> ceph_direct_read_write() too -- on bvecs instead of pages.
>
> I have applied this patch and will take care of the leak on bvecs
> myself because I think I see other issues there.
>
> Thanks,
>
> Ilya
>
