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@gmail.com wrote:
On Thu, Dec 5, 2024 at 9:32 AM Max Kellermann max.kellermann@ionos.com wrote:
On Fri, Nov 29, 2024 at 9:06 AM Max Kellermann max.kellermann@ionos.com wrote:
On Thu, Nov 28, 2024 at 1:18 PM Alex Markuze amarkuze@redhat.com 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