Hitting a crash in mdcache_lru_cleanup_push.
Hello Daniel/Frank, While debugging a crash from 2.7.1 Ganesha, I see a potential race between the two paths below: Thread 1 (waiting for the qlock to insert to LRU) nfs4_mds_putfh -> mdcache_create_handle -> mdcache_locate_host -> mdcache_new_entry -> mdcache_lru_insert -> lru_insert_entry Thread 2 (unlink the same object) - since the object is already in mdcache at this point, I believe other threads will get it. fsal_remove -> mdcache_unlink -> _mdc_unreachable -> _mdcache_kill_entry -> mdcache_lru_cleanup_push The second thread will find the lru something like this: $5 = {q = {next = 0x0, prev = 0x0}, qid = LRU_ENTRY_NONE, refcnt = 2, flags = 0, lane = 12, cf = 0} So, the below code will end up crashing: if (!(lru->qid == LRU_ENTRY_CLEANUP)) { struct lru_q *q; /* out with the old queue */ q = lru_queue_of(entry); <<-- q will be NULL because qid == LRU_ENTRY_NONE Should Thread 2 just ignore if q is NULL and let Thread1's operation to free the entry later? Also, please let me know if there is any recent fixes in this area. Thanks, Pradeep
Someone (from amazon?) reported this issue but didn't follow up with a patch. So we ended up with a patch in our code base. See commit ce4c049b0871eaae78325bb30a1ef1df3a14e77c in our repo ( https://github.com/ganltc/nfs-ganesha.git branch ibm2.7). Regards, Malahal. On Fri, Apr 17, 2020 at 6:29 AM Pradeep Thomas <pradeepthomas@gmail.com> wrote:
Hello Daniel/Frank,
While debugging a crash from 2.7.1 Ganesha, I see a potential race between the two paths below:
Thread 1 (waiting for the qlock to insert to LRU) nfs4_mds_putfh -> mdcache_create_handle -> mdcache_locate_host -> mdcache_new_entry -> mdcache_lru_insert -> lru_insert_entry
Thread 2 (unlink the same object) - since the object is already in mdcache at this point, I believe other threads will get it. fsal_remove -> mdcache_unlink -> _mdc_unreachable -> _mdcache_kill_entry -> mdcache_lru_cleanup_push
The second thread will find the lru something like this: $5 = {q = {next = 0x0, prev = 0x0}, qid = LRU_ENTRY_NONE, refcnt = 2, flags = 0, lane = 12, cf = 0}
So, the below code will end up crashing: if (!(lru->qid == LRU_ENTRY_CLEANUP)) { struct lru_q *q;
/* out with the old queue */ q = lru_queue_of(entry); <<-- q will be NULL because qid == LRU_ENTRY_NONE
Should Thread 2 just ignore if q is NULL and let Thread1's operation to free the entry later?
Also, please let me know if there is any recent fixes in this area.
Thanks, Pradeep _______________________________________________ Devel mailing list -- devel@lists.nfs-ganesha.org To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org
Hmm, it looks like this IS still an issue upstream. Do you know why an upstream patch didn’t get submitted? I think it could also be fixed by moving the mdcache_lru_insert above cih_set_latched (since that no longer can fail). Frank From: Malahal Naineni [mailto:malahal@gmail.com] Sent: Thursday, April 16, 2020 9:08 PM To: Pradeep Thomas <pradeepthomas@gmail.com> Cc: nfs-ganesha <devel@lists.nfs-ganesha.org> Subject: [NFS-Ganesha-Devel] Re: Hitting a crash in mdcache_lru_cleanup_push. Someone (from amazon?) reported this issue but didn't follow up with a patch. So we ended up with a patch in our code base. See commit ce4c049b0871eaae78325bb30a1ef1df3a14e77c in our repo (https://github.com/ganltc/nfs-ganesha.git branch ibm2.7). Regards, Malahal. On Fri, Apr 17, 2020 at 6:29 AM Pradeep Thomas <pradeepthomas@gmail.com <mailto:pradeepthomas@gmail.com> > wrote: Hello Daniel/Frank, While debugging a crash from 2.7.1 Ganesha, I see a potential race between the two paths below: Thread 1 (waiting for the qlock to insert to LRU) nfs4_mds_putfh -> mdcache_create_handle -> mdcache_locate_host -> mdcache_new_entry -> mdcache_lru_insert -> lru_insert_entry Thread 2 (unlink the same object) - since the object is already in mdcache at this point, I believe other threads will get it. fsal_remove -> mdcache_unlink -> _mdc_unreachable -> _mdcache_kill_entry -> mdcache_lru_cleanup_push The second thread will find the lru something like this: $5 = {q = {next = 0x0, prev = 0x0}, qid = LRU_ENTRY_NONE, refcnt = 2, flags = 0, lane = 12, cf = 0} So, the below code will end up crashing: if (!(lru->qid == LRU_ENTRY_CLEANUP)) { struct lru_q *q; /* out with the old queue */ q = lru_queue_of(entry); <<-- q will be NULL because qid == LRU_ENTRY_NONE Should Thread 2 just ignore if q is NULL and let Thread1's operation to free the entry later? Also, please let me know if there is any recent fixes in this area. Thanks, Pradeep _______________________________________________ Devel mailing list -- devel@lists.nfs-ganesha.org <mailto:devel@lists.nfs-ganesha.org> To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org <mailto:devel-leave@lists.nfs-ganesha.org>
2.7.6 is now almost a year old, and includes lot of MDCACHE fixes, including LRU related fixes, since 2.7.1. In addition, 2.8 and 3.x both have additional fixes. I'd estimate at least a dozen total. Daniel On 4/16/20 8:59 PM, Pradeep Thomas wrote:
Hello Daniel/Frank,
While debugging a crash from 2.7.1 Ganesha, I see a potential race between the two paths below:
Thread 1 (waiting for the qlock to insert to LRU) nfs4_mds_putfh -> mdcache_create_handle -> mdcache_locate_host -> mdcache_new_entry -> mdcache_lru_insert -> lru_insert_entry
Thread 2 (unlink the same object) - since the object is already in mdcache at this point, I believe other threads will get it. fsal_remove -> mdcache_unlink -> _mdc_unreachable -> _mdcache_kill_entry -> mdcache_lru_cleanup_push
The second thread will find the lru something like this: $5 = {q = {next = 0x0, prev = 0x0}, qid = LRU_ENTRY_NONE, refcnt = 2, flags = 0, lane = 12, cf = 0}
So, the below code will end up crashing: if (!(lru->qid == LRU_ENTRY_CLEANUP)) { struct lru_q *q;
/* out with the old queue */ q = lru_queue_of(entry); <<-- q will be NULL because qid == LRU_ENTRY_NONE
Should Thread 2 just ignore if q is NULL and let Thread1's operation to free the entry later?
Also, please let me know if there is any recent fixes in this area.
Thanks, Pradeep _______________________________________________ Devel mailing list -- devel@lists.nfs-ganesha.org To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org
Thanks Daniel, Malahal. Looking at latest code base, looks like we can still hit this race. In mdcache_new_entry(), latch is released before mdcache_lru_insert(). So, it is likely that another thread could get the new entry and try removing it and end up crashing mdcache_lru_cleanup_push. Looks like we need something like what Malahal posted above. Thanks, Pradeep
2.7.6 is now almost a year old, and includes lot of MDCACHE fixes, including LRU related fixes, since 2.7.1. In addition, 2.8 and 3.x both have additional fixes. I'd estimate at least a dozen total.
Daniel
On 4/16/20 8:59 PM, Pradeep Thomas wrote:
Ok, I'm not sure how this didn't go upstream, but here's an upstream patch: https://review.gerrithub.io/c/ffilz/nfs-ganesha/+/490335 Frank
-----Original Message----- From: Pradeep Thomas [mailto:pradeepthomas@gmail.com] Sent: Friday, April 17, 2020 7:06 AM To: devel@lists.nfs-ganesha.org Subject: [NFS-Ganesha-Devel] Re: Hitting a crash in mdcache_lru_cleanup_push.
Thanks Daniel, Malahal.
Looking at latest code base, looks like we can still hit this race. In mdcache_new_entry(), latch is released before mdcache_lru_insert(). So, it is likely that another thread could get the new entry and try removing it and end up crashing mdcache_lru_cleanup_push.
Looks like we need something like what Malahal posted above.
Thanks, Pradeep
2.7.6 is now almost a year old, and includes lot of MDCACHE fixes, including LRU related fixes, since 2.7.1. In addition, 2.8 and 3.x both have additional fixes. I'd estimate at least a dozen total.
Daniel
On 4/16/20 8:59 PM, Pradeep Thomas wrote:
Devel mailing list -- devel@lists.nfs-ganesha.org To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org
participants (4)
-
Daniel Gryniewicz -
Frank Filz -
Malahal Naineni -
Pradeep Thomas