[NFS-Ganesha-Devel] Re: Improving mdcache_lru_cleanup_try_push

Okay, I think this should fix it: https://review.gerrithub.io/c/ffilz/nfs-ganesha/+/487898

Daniel On 3/23/20 9:52 AM, Daniel Gryniewicz wrote: > On 3/20/20 6:56 AM, Ashish Sangwan wrote: >> On Tue, Mar 17, 2020 at 6:57 PM Daniel Gryniewicz <dang(a)redhat.com&gt; >> wrote: >>> >>> Replying inline, so I can handle both emails at once. >>> >>> On 3/15/20 1:17 PM, Ashish Sangwan wrote: >>>> The other issue I saw with objects lying around even after the >>>> corresponding export is deleted is when the export is created again >>>> with same attributes. >>>> We start getting cache hits for the old objects. The "struct >>>> fsal_filesystem *fs" for these objects is now pointing to the fs which >>>> we already freed in vfs_unexport_filesystems by calling >>>> release_posix_file_system() >>> >>> I'm not sure this is a problem. The old cache entry is being found >>> because VFS generates the same key the second time around as it did on >>> the first time around. This means that the cache entry would be the >>> same, and so would be recreated with the same data. Re-using the old >>> cache entry should be fine. And, if the export is not re-created >>> immediately, those entries will be recycled by the LRU. >> >> I am able to hit "Use after free" crash with ASAN. >> As mentioned above, it's because the cache entry's fs pointer is >> pointing to freed memory. > > Okay, that's definitely an issue. > > Daniel >> >>> >>>> >>>> On Sun, Mar 15, 2020 at 10:09 PM Ashish Sangwan >>>> <ashishsangwan2(a)gmail.com&gt; wrote: >>>>> >>>>> Hi Daniel, >>>>> >>>>> It looks like mdcache_lru_cleanup_try_push() can be improved for the >>>>> cases when there are ops executing parallely with an unexport request. >>>>> In this case mdcache_lru_cleanup_try_push() is not able to drop the >>>>> sentinel ref and mark the entry for cleanup. This is because one extra >>>>> ref is taken by the op which is currently executing. The total number >>>>> of refs are > 2. >>>>> >>>>> Once the current op execution finishes (after the execution of >>>>> mdcache_unexport()) the sentinel ref remains and the entry is also not >>>>> marked for cleanup even though no export is referring to it >>>>> (entry->first_export_id == -1). >>>>> Is it possible to modify mdcache_lru_cleanup_try_push() such that even >>>>> if the refcnt is > 2 but if the entry->first_export_id == -1 (the >>>>> current export which is going away was the only one referring to this >>>>> entry), we should drop the sentinel ref and mark the entry for >>>>> cleanup? This will ensure we cleanup the entries when the current op >>>>> does put_ref and these are reused faster than other mdcache entries >>>>> which are actually pointing to valid exports. >>>>> >>> >>> This would be possible, but I don't think it would help significantly. >>> It narrows the race window by ~9 lines of code (admittedly including 3 >>> lock ops), but the window between when the ref it taken and >>> first_export_id is set to -1 still exists. >>> >>> These entries are not leaked, the LRU will recycle them eventually, >>> since only their sentinel ref is left. Or, if the export is re-created >>> exactly, they can be reused in the new export. >>> >>> Daniel >>> >>