* [PATCH 1/4] Staging: lustre: sparse static warning fix [not found] <1431974091-26363-1-git-send-email-adrianremonda@gmail.com> @ 2015-05-18 18:34 ` Adrian Remonda 2015-05-18 18:34 ` [PATCH 2/4] " Adrian Remonda 2015-05-18 21:23 ` [PATCH 1/4] Staging: lustre: sparse static " Dan Carpenter 0 siblings, 2 replies; 15+ messages in thread From: Adrian Remonda @ 2015-05-18 18:34 UTC (permalink / raw) Cc: Adrian Remonda, Oleg Drokin, Andreas Dilger, Greg Kroah-Hartman, Julia Lawall, Aya Mahfouz, Melike Yurtoglu, Greg Donald, Joe Perches, Omar Sandoval, moderated list:STAGING - LUSTRE..., open list:STAGING SUBSYSTEM, open list This patch clears the warning given by sparse that the function should be static. Done by moving the declaration of the structure 'nrs_conf_fifo' into the header 'ptlrpc_internal.h' modified: drivers/staging/lustre/lustre/ptlrpc/nrs.c modified: drivers/staging/lustre/lustre/ptlrpc/ptlrpc_internal.h Signed-off-by: Adrian Remonda <adrianremonda@gmail.com> --- drivers/staging/lustre/lustre/ptlrpc/nrs.c | 4 ---- drivers/staging/lustre/lustre/ptlrpc/ptlrpc_internal.h | 4 ++++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/staging/lustre/lustre/ptlrpc/nrs.c b/drivers/staging/lustre/lustre/ptlrpc/nrs.c index 63a05f4a902d..159db41c53a0 100644 --- a/drivers/staging/lustre/lustre/ptlrpc/nrs.c +++ b/drivers/staging/lustre/lustre/ptlrpc/nrs.c @@ -1698,10 +1698,6 @@ out: return rc; } - -/* ptlrpc/nrs_fifo.c */ -extern struct ptlrpc_nrs_pol_conf nrs_conf_fifo; - /** * Adds all policies that ship with the ptlrpc module, to NRS core's list of * policies \e nrs_core.nrs_policies. diff --git a/drivers/staging/lustre/lustre/ptlrpc/ptlrpc_internal.h b/drivers/staging/lustre/lustre/ptlrpc/ptlrpc_internal.h index a66dc3c6da41..e07e2aacc62c 100644 --- a/drivers/staging/lustre/lustre/ptlrpc/ptlrpc_internal.h +++ b/drivers/staging/lustre/lustre/ptlrpc/ptlrpc_internal.h @@ -309,4 +309,8 @@ static inline void ptlrpc_reqset_put(struct ptlrpc_request_set *set) if (atomic_dec_and_test(&set->set_refcount)) OBD_FREE_PTR(set); } + +/* ptlrpc/nrs_fifo.c */ +extern struct ptlrpc_nrs_pol_conf nrs_conf_fifo; + #endif /* PTLRPC_INTERNAL_H */ -- 2.1.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/4] Staging: lustre: sparse static warning fix 2015-05-18 18:34 ` [PATCH 1/4] Staging: lustre: sparse static warning fix Adrian Remonda @ 2015-05-18 18:34 ` Adrian Remonda 2015-05-18 18:34 ` [PATCH 3/4] Staging: lustre: Fixed typo Adrian Remonda 2015-05-18 21:23 ` [PATCH 1/4] Staging: lustre: sparse static " Dan Carpenter 1 sibling, 1 reply; 15+ messages in thread From: Adrian Remonda @ 2015-05-18 18:34 UTC (permalink / raw) Cc: Adrian Remonda, Oleg Drokin, Andreas Dilger, Greg Kroah-Hartman, Julia Lawall, Joe Perches, Greg Donald, Melike Yurtoglu, Tristan Lelong, Omar Sandoval, moderated list:STAGING - LUSTRE..., open list:STAGING SUBSYSTEM, open list This patch clears the warning given by sparse that the function should be static. Done by moving the declaration of the structure 'nrs_core' into the header 'ptlrpc_internal.h' modified: drivers/staging/lustre/lustre/ptlrpc/lproc_ptlrpc.c modified: drivers/staging/lustre/lustre/ptlrpc/ptlrpc_internal.h Signed-off-by: Adrian Remonda <adrianremonda@gmail.com> --- drivers/staging/lustre/lustre/ptlrpc/lproc_ptlrpc.c | 6 ------ drivers/staging/lustre/lustre/ptlrpc/ptlrpc_internal.h | 7 +++++++ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/staging/lustre/lustre/ptlrpc/lproc_ptlrpc.c b/drivers/staging/lustre/lustre/ptlrpc/lproc_ptlrpc.c index aeceef5152ac..300310e064f5 100644 --- a/drivers/staging/lustre/lustre/ptlrpc/lproc_ptlrpc.c +++ b/drivers/staging/lustre/lustre/ptlrpc/lproc_ptlrpc.c @@ -417,12 +417,6 @@ ptlrpc_lprocfs_threads_max_seq_write(struct file *file, LPROC_SEQ_FOPS(ptlrpc_lprocfs_threads_max); /** - * \addtogoup nrs - * @{ - */ -extern struct nrs_core nrs_core; - -/** * Translates \e ptlrpc_nrs_pol_state values to human-readable strings. * * \param[in] state The policy state diff --git a/drivers/staging/lustre/lustre/ptlrpc/ptlrpc_internal.h b/drivers/staging/lustre/lustre/ptlrpc/ptlrpc_internal.h index e07e2aacc62c..f9bfcff2ac9b 100644 --- a/drivers/staging/lustre/lustre/ptlrpc/ptlrpc_internal.h +++ b/drivers/staging/lustre/lustre/ptlrpc/ptlrpc_internal.h @@ -313,4 +313,11 @@ static inline void ptlrpc_reqset_put(struct ptlrpc_request_set *set) /* ptlrpc/nrs_fifo.c */ extern struct ptlrpc_nrs_pol_conf nrs_conf_fifo; +/** + * \addtogoup nrs + * @{ + */ +extern struct nrs_core nrs_core; + + #endif /* PTLRPC_INTERNAL_H */ -- 2.1.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/4] Staging: lustre: Fixed typo 2015-05-18 18:34 ` [PATCH 2/4] " Adrian Remonda @ 2015-05-18 18:34 ` Adrian Remonda 2015-05-18 18:34 ` [PATCH 4/4] Staging: lustre: sparse lock warning fix Adrian Remonda 0 siblings, 1 reply; 15+ messages in thread From: Adrian Remonda @ 2015-05-18 18:34 UTC (permalink / raw) Cc: Adrian Remonda, Oleg Drokin, Andreas Dilger, Greg Kroah-Hartman, Julia Lawall, Joe Perches, Greg Donald, Aya Mahfouz, Melike Yurtoglu, moderated list:STAGING - LUSTRE..., open list:STAGING SUBSYSTEM, open list In the explanation of the function the name of the function was incorrect Signed-off-by: Adrian Remonda <adrianremonda@gmail.com> --- drivers/staging/lustre/lustre/ptlrpc/nrs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/lustre/lustre/ptlrpc/nrs.c b/drivers/staging/lustre/lustre/ptlrpc/nrs.c index 159db41c53a0..43da95f0bce2 100644 --- a/drivers/staging/lustre/lustre/ptlrpc/nrs.c +++ b/drivers/staging/lustre/lustre/ptlrpc/nrs.c @@ -478,7 +478,7 @@ static void nrs_resource_get_safe(struct ptlrpc_nrs *nrs, * * \param resp the resource hierarchy that is being released * - * \see ptlrpcnrs_req_hp_move() + * \see ptlrpc_nrs_req_hp_move() * \see ptlrpc_nrs_req_finalize() */ static void nrs_resource_put_safe(struct ptlrpc_nrs_resource **resp) -- 2.1.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 4/4] Staging: lustre: sparse lock warning fix 2015-05-18 18:34 ` [PATCH 3/4] Staging: lustre: Fixed typo Adrian Remonda @ 2015-05-18 18:34 ` Adrian Remonda 2015-05-18 21:21 ` Dan Carpenter 0 siblings, 1 reply; 15+ messages in thread From: Adrian Remonda @ 2015-05-18 18:34 UTC (permalink / raw) Cc: Adrian Remonda, Oleg Drokin, Andreas Dilger, Greg Kroah-Hartman, Julia Lawall, Joe Perches, Aya Mahfouz, Greg Donald, moderated list:STAGING - LUSTRE..., open list:STAGING SUBSYSTEM, open list Fixed sparse warning: context imbalance in 'nrs_resource_put_safe' - 'different lock contexts for basic block' by releasing the lock on each iteration of the for loop. Signed-off-by: Adrian Remonda <adrianremonda@gmail.com> --- drivers/staging/lustre/lustre/ptlrpc/nrs.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/staging/lustre/lustre/ptlrpc/nrs.c b/drivers/staging/lustre/lustre/ptlrpc/nrs.c index 43da95f0bce2..3a1722437ca1 100644 --- a/drivers/staging/lustre/lustre/ptlrpc/nrs.c +++ b/drivers/staging/lustre/lustre/ptlrpc/nrs.c @@ -503,13 +503,11 @@ static void nrs_resource_put_safe(struct ptlrpc_nrs_resource **resp) if (nrs == NULL) { nrs = pols[i]->pol_nrs; - spin_lock(&nrs->nrs_lock); } + spin_lock(&nrs->nrs_lock); nrs_policy_put_locked(pols[i]); - } - - if (nrs != NULL) spin_unlock(&nrs->nrs_lock); + } } /** -- 2.1.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] Staging: lustre: sparse lock warning fix 2015-05-18 18:34 ` [PATCH 4/4] Staging: lustre: sparse lock warning fix Adrian Remonda @ 2015-05-18 21:21 ` Dan Carpenter 2015-05-20 16:51 ` Dilger, Andreas 2015-05-21 8:15 ` AdrianRemonda 0 siblings, 2 replies; 15+ messages in thread From: Dan Carpenter @ 2015-05-18 21:21 UTC (permalink / raw) To: Adrian Remonda Cc: open list:STAGING SUBSYSTEM, moderated list:STAGING - LUSTRE..., Andreas Dilger, Greg Donald, open list, Oleg Drokin, Julia Lawall, Greg Kroah-Hartman, Joe Perches On Mon, May 18, 2015 at 08:34:51PM +0200, Adrian Remonda wrote: > Fixed sparse warning: context imbalance in 'nrs_resource_put_safe' - > 'different lock contexts for basic block' by releasing the lock on each > iteration of the for loop. > That changelog doesn't sound correct at all. That's not a correct motivation or explanation. I reviewed the patch and it's likely going to cause dead locks. The code is trying to take the spinlock for the first pointer in the array and release it at the end. Now it takes the first pointer's spinlock a bunch of times (dead lock) and releases it once (will not happen because we are already dead). regards, dan carpenter ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] Staging: lustre: sparse lock warning fix 2015-05-18 21:21 ` Dan Carpenter @ 2015-05-20 16:51 ` Dilger, Andreas 2015-05-20 19:29 ` Dan Carpenter 2015-05-21 8:15 ` AdrianRemonda 1 sibling, 1 reply; 15+ messages in thread From: Dilger, Andreas @ 2015-05-20 16:51 UTC (permalink / raw) To: Dan Carpenter, Adrian Remonda Cc: open list:STAGING SUBSYSTEM, moderated list:STAGING - LUSTRE..., Greg Donald, open list, Drokin, Oleg, Julia Lawall, Greg Kroah-Hartman, Joe Perches On 2015/05/18, 3:21 PM, "Dan Carpenter" <dan.carpenter@oracle.com> wrote: >On Mon, May 18, 2015 at 08:34:51PM +0200, Adrian Remonda wrote: >> Fixed sparse warning: context imbalance in 'nrs_resource_put_safe' - >> 'different lock contexts for basic block' by releasing the lock on each >> iteration of the for loop. >> > >That changelog doesn't sound correct at all. That's not a correct >motivation or explanation. > >I reviewed the patch and it's likely going to cause dead locks. The code >is trying to take the spinlock for the first pointer in the array and >release it at the end. Now it takes the first pointer's spinlock a >bunch of times (dead lock) and releases it once (will not happen because >we are already dead). It isn't clear to me what the checkpatch complaint actually means? Is it that the spin_lock() and spin_unlock() calls have different amounts of indentation? Cheers, Andreas -- Andreas Dilger Lustre Software Architect Intel High Performance Data Division ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] Staging: lustre: sparse lock warning fix 2015-05-20 16:51 ` Dilger, Andreas @ 2015-05-20 19:29 ` Dan Carpenter 2015-05-20 19:42 ` Dan Carpenter 0 siblings, 1 reply; 15+ messages in thread From: Dan Carpenter @ 2015-05-20 19:29 UTC (permalink / raw) To: Dilger, Andreas Cc: Adrian Remonda, open list:STAGING SUBSYSTEM, moderated list:STAGING - LUSTRE..., Greg Donald, open list, Drokin, Oleg, Julia Lawall, Greg Kroah-Hartman, Joe Perches On Wed, May 20, 2015 at 04:51:59PM +0000, Dilger, Andreas wrote: > On 2015/05/18, 3:21 PM, "Dan Carpenter" <dan.carpenter@oracle.com> wrote: > > >On Mon, May 18, 2015 at 08:34:51PM +0200, Adrian Remonda wrote: > >> Fixed sparse warning: context imbalance in 'nrs_resource_put_safe' - > >> 'different lock contexts for basic block' by releasing the lock on each > >> iteration of the for loop. > >> > > > >That changelog doesn't sound correct at all. That's not a correct > >motivation or explanation. > > > >I reviewed the patch and it's likely going to cause dead locks. The code > >is trying to take the spinlock for the first pointer in the array and > >release it at the end. Now it takes the first pointer's spinlock a > >bunch of times (dead lock) and releases it once (will not happen because > >we are already dead). > > It isn't clear to me what the checkpatch complaint actually means? Is it > that the spin_lock() and spin_unlock() calls have different amounts of > indentation? > It's not a checkpatch.pl warning, it's a Sparse warning. Sparse is crappy at reporting locking bugs. It's mostly false positives. I think it's saying that some paths lock and unlock some don't. Smatch is also fairly crappy at finding locking bugs, unfortunately. I need to re-write it using modern features and cross function analysis. regards, dan carpenter ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] Staging: lustre: sparse lock warning fix 2015-05-20 19:29 ` Dan Carpenter @ 2015-05-20 19:42 ` Dan Carpenter 2015-05-20 22:51 ` Dilger, Andreas 0 siblings, 1 reply; 15+ messages in thread From: Dan Carpenter @ 2015-05-20 19:42 UTC (permalink / raw) To: Dilger, Andreas Cc: open list:STAGING SUBSYSTEM, Julia Lawall, Adrian Remonda, open list, Drokin, Oleg, Greg Donald, Greg Kroah-Hartman, moderated list:STAGING - LUSTRE..., Joe Perches In Smatch, it the equivalent warning is turned off by default because there are too many false positives, but you can enable it with the --spammy flag. kchecker --spammy drivers/staging/lustre/lustre/ptlrpc/nrs.c drivers/staging/lustre/lustre/ptlrpc/nrs.c:512 nrs_resource_put_safe() warn: 'spin_lock:&nrs->nrs_lock' is sometimes locked here and sometimes unlocked. regards, dan carpenter ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] Staging: lustre: sparse lock warning fix 2015-05-20 19:42 ` Dan Carpenter @ 2015-05-20 22:51 ` Dilger, Andreas 2015-05-22 13:38 ` Dan Carpenter 0 siblings, 1 reply; 15+ messages in thread From: Dilger, Andreas @ 2015-05-20 22:51 UTC (permalink / raw) To: Dan Carpenter Cc: open list:STAGING SUBSYSTEM, Julia Lawall, Adrian Remonda, open list, Drokin, Oleg, Greg Donald, Greg Kroah-Hartman, moderated list:STAGING - LUSTRE..., Joe Perches On 2015/05/20, 1:42 PM, "Dan Carpenter" <dan.carpenter@oracle.com> wrote: >In Smatch, it the equivalent warning is turned off by default because >there are too many false positives, but you can enable it with the >--spammy flag. > >kchecker --spammy drivers/staging/lustre/lustre/ptlrpc/nrs.c > >drivers/staging/lustre/lustre/ptlrpc/nrs.c:512 nrs_resource_put_safe() >warn: 'spin_lock:&nrs->nrs_lock' is sometimes locked here and sometimes >unlocked. Would this be happier with something like: for (i = 0; i < NRS_RES_MAX; i++) { if (pols[i] == NULL) continue; if (nrs == NULL) { nrs = pols[i]->pol_nrs; if (likely(nrs != NULL)) /* make sparse happy */ spin_lock(&nrs->nrs_lock); } nrs_policy_put_locked(pols[i]); } if (nrs != NULL) spin_unlock(&nrs->nrs_lock); so that the "if" conditions are the same? The code definitely doesn't have a bug, because the lock is only locked once when nrs is first set, and only unlocked if it is set. Or is there a comment to put there that will quiet the static checker? Cheers, Andreas -- Andreas Dilger Lustre Software Architect Intel High Performance Data Division ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] Staging: lustre: sparse lock warning fix 2015-05-20 22:51 ` Dilger, Andreas @ 2015-05-22 13:38 ` Dan Carpenter 0 siblings, 0 replies; 15+ messages in thread From: Dan Carpenter @ 2015-05-22 13:38 UTC (permalink / raw) To: Dilger, Andreas Cc: open list:STAGING SUBSYSTEM, Greg Donald, Adrian Remonda, open list, Drokin, Oleg, Julia Lawall, Greg Kroah-Hartman, moderated list:STAGING - LUSTRE..., Joe Perches On Wed, May 20, 2015 at 10:51:34PM +0000, Dilger, Andreas wrote: > On 2015/05/20, 1:42 PM, "Dan Carpenter" <dan.carpenter@oracle.com> wrote: > > >In Smatch, it the equivalent warning is turned off by default because > >there are too many false positives, but you can enable it with the > >--spammy flag. > > > >kchecker --spammy drivers/staging/lustre/lustre/ptlrpc/nrs.c > > > >drivers/staging/lustre/lustre/ptlrpc/nrs.c:512 nrs_resource_put_safe() > >warn: 'spin_lock:&nrs->nrs_lock' is sometimes locked here and sometimes > >unlocked. > > Would this be happier with something like: > > for (i = 0; i < NRS_RES_MAX; i++) { > if (pols[i] == NULL) > continue; > > > if (nrs == NULL) { > nrs = pols[i]->pol_nrs; > if (likely(nrs != NULL)) /* make sparse happy */ > spin_lock(&nrs->nrs_lock); > } > nrs_policy_put_locked(pols[i]); > } > > if (nrs != NULL) > spin_unlock(&nrs->nrs_lock); > > so that the "if" conditions are the same? The code definitely doesn't > have a bug, because the lock is only locked once when nrs is first set, > and only unlocked if it is set. Or is there a comment to put there that > will quiet the static checker? Forget about Sparse, it's good at some things and it's fast but it has crappy flow analysis compared to Smatch. Adding that check does actually silence the warning in Smatch but it's a bad idea. Smatch is supposed to know that "nrs" is non-NULL because of the dereference on the next line. I think this is a recent regression. I'll look into it. Smatch doesn't have a way to silence false positives. It's still developing, so many of these false positives can be solved by improving the flow analysis. regards, dan carpenter ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] Staging: lustre: sparse lock warning fix 2015-05-18 21:21 ` Dan Carpenter 2015-05-20 16:51 ` Dilger, Andreas @ 2015-05-21 8:15 ` AdrianRemonda 2015-05-21 15:12 ` Dan Carpenter 1 sibling, 1 reply; 15+ messages in thread From: AdrianRemonda @ 2015-05-21 8:15 UTC (permalink / raw) To: Dan Carpenter Cc: open list:STAGING SUBSYSTEM, moderated list:STAGING - LUSTRE..., Andreas Dilger, Greg Donald, open list, Oleg Drokin, Julia Lawall, Greg Kroah-Hartman, Joe Perches On Tue, May 19, 2015 at 12:21:15AM +0300, Dan Carpenter wrote: > On Mon, May 18, 2015 at 08:34:51PM +0200, Adrian Remonda wrote: > > Fixed sparse warning: context imbalance in 'nrs_resource_put_safe' - > > 'different lock contexts for basic block' by releasing the lock on each > > iteration of the for loop. > > > > That changelog doesn't sound correct at all. That's not a correct > motivation or explanation. > > I reviewed the patch and it's likely going to cause dead locks. The code > is trying to take the spinlock for the first pointer in the array and > release it at the end. Now it takes the first pointer's spinlock a > bunch of times (dead lock) and releases it once (will not happen because > we are already dead). > > Hello Dan, thanks for the comments. The code would end up looking as next, I don't undertand where the deadlock would be. I know the older code would work, I just changed it to keep the lock locked the less time as possible. for (i = 0; i < NRS_RES_MAX; i++) { if (pols[i] == NULL) continue; if (nrs == NULL) { nrs = pols[i]->pol_nrs; } spin_lock(&nrs->nrs_lock); nrs_policy_put_locked(pols[i]); spin_unlock(&nrs->nrs_lock); } best regards, Adrian ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] Staging: lustre: sparse lock warning fix 2015-05-21 8:15 ` AdrianRemonda @ 2015-05-21 15:12 ` Dan Carpenter 2015-05-21 15:33 ` Drokin, Oleg 0 siblings, 1 reply; 15+ messages in thread From: Dan Carpenter @ 2015-05-21 15:12 UTC (permalink / raw) To: AdrianRemonda Cc: open list:STAGING SUBSYSTEM, Andreas Dilger, Julia Lawall, Greg Kroah-Hartman, open list, Oleg Drokin, Greg Donald, moderated list:STAGING - LUSTRE..., Joe Perches Oh, sorry, I didn't read your patch very carefully. It won't cause a deadlock. But I'm going to assume it's still not right until lustre expert Acks it. How well do you understand lustre locking? regards, dan carpenter ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] Staging: lustre: sparse lock warning fix 2015-05-21 15:12 ` Dan Carpenter @ 2015-05-21 15:33 ` Drokin, Oleg 2015-05-22 1:11 ` Nikitas Angelinas 0 siblings, 1 reply; 15+ messages in thread From: Drokin, Oleg @ 2015-05-21 15:33 UTC (permalink / raw) To: Dan Carpenter, nikitas_angelinas, Zhen, Liang Cc: AdrianRemonda, open list:STAGING SUBSYSTEM, Dilger, Andreas, Julia Lawall, Greg Kroah-Hartman, open list, Greg Donald, moderated list:STAGING - LUSTRE..., Joe Perches On May 21, 2015, at 11:12 AM, Dan Carpenter wrote: > Oh, sorry, I didn't read your patch very carefully. It won't cause a > deadlock. But I'm going to assume it's still not right until lustre > expert Acks it. I just took a closer look and it appears original code is buggy and the patch just propagates the bugginess. If we look at the nrs_policy_put_locked, it eventually ends up in nrs_policy_stop0, it would hold a lock on whatever happened to be the first policy in the array not NULL. But nrs_policy_stop0 would unlock the lock on the policy it was called on (already likely a deadlock material) and then relock it. The problems would arise only if there are more than one nrs policy registered which is theoretically possible, but certainly makes no sense a client (besides, none of the advanced NRS policies made it in anyway and I almost feel like they just add unnecessary complication in client-only code). The code looks elaborate enough as if the first policy lock is to be always used as the guardian lock, but then stop0 behavior might be a bug then? Or it's possible we never end up in stop0 due to nrs state machine? Let's see what Nikitas and Liang remember about any of this (one of them is the original author of this code, but I am not sure who.) Nikitas, Liang: The code in question is in nrs_resource_put_safe: for (i = 0; i < NRS_RES_MAX; i++) { if (pols[i] == NULL) continue; if (nrs == NULL) { nrs = pols[i]->pol_nrs; spin_lock(&nrs->nrs_lock); } nrs_policy_put_locked(pols[i]); } if (nrs != NULL) spin_unlock(&nrs->nrs_lock); ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] Staging: lustre: sparse lock warning fix 2015-05-21 15:33 ` Drokin, Oleg @ 2015-05-22 1:11 ` Nikitas Angelinas 0 siblings, 0 replies; 15+ messages in thread From: Nikitas Angelinas @ 2015-05-22 1:11 UTC (permalink / raw) To: Drokin, Oleg Cc: Dan Carpenter, nikitas_angelinas, Zhen, Liang, AdrianRemonda, open list:STAGING SUBSYSTEM, Dilger, Andreas, Julia Lawall, Greg Kroah-Hartman, open list, Greg Donald, moderated list:STAGING - LUSTRE..., Joe Perches (Apologies for the double-posting to individual recipients; resending in plain text to get through vger list filters.) The current code does not have a bug in this path (until we find one, of course). nrs_resource_put_safe() releases references to the related NRS resource hierarchies and policies for an RPC; as Oleg pointed out, the server-side code which is included in the Intel/OpenSFS tree (http://git.whamcloud.com/fs/lustre-release.git) has more than just the FIFO policy, and so up to NRS_RES_MAX (currently 2) policies can be related with an NRS head (struct ptlrpc_nrs). nrs_resource_put_safe() takes the NRS head spinlock (nrs_lock), releases references to the NRS head's policies, and releases the same spinlock. nrs_policy_stop0() can be entered from nrs_policy_put_locked(), but whichever policy is passed to it, it will release the same spinlock, as all policies for an RPC share the same NRS head, and thus the same nrs_lock. Liang wrote this part; I believe the aim is to avoid a lock dance of nrs_lock when releasing policy references for the NRS head; the proposed patch would add the lock dance, so I am not sure it would help much; not that it would hurt, but I don't see a good reason to add it, other than to suppress the sparse warning. Btw, I am guessing that the reason for having a part of both NRS and the server-side of the PTLRPC code in the kernel is only to satisfy build dependencies, and this code is unreachable; i.e. as the TODO file suggests, the client and server sides of PTLRPC will be decoupled, and so this code will eventually be removed from the kernel, likely before transitioning from staging. Cheers, Nikitas On Thu, May 21, 2015 at 8:33 AM, Drokin, Oleg <oleg.drokin@intel.com> wrote: > On May 21, 2015, at 11:12 AM, Dan Carpenter wrote: > >> Oh, sorry, I didn't read your patch very carefully. It won't cause a >> deadlock. But I'm going to assume it's still not right until lustre >> expert Acks it. > > I just took a closer look and it appears original code is buggy and the patch just propagates the bugginess. > > If we look at the nrs_policy_put_locked, it eventually ends up in nrs_policy_stop0, > it would hold a lock on whatever happened to be the first policy in the array not NULL. > But nrs_policy_stop0 would unlock the lock on the policy it was called on (already likely a deadlock material) and then relock it. > > The problems would arise only if there are more than one nrs policy registered which is theoretically possible, but certainly makes no sense a client (besides, none of the advanced NRS policies > made it in anyway and I almost feel like they just add unnecessary complication in client-only code). > > The code looks elaborate enough as if the first policy lock is to be always used as the guardian lock, but then stop0 behavior might be a bug then? > Or it's possible we never end up in stop0 due to nrs state machine? > Let's see what Nikitas and Liang remember about any of this (one of them is the original author of this code, but I am not sure who.) > > Nikitas, Liang: The code in question is in nrs_resource_put_safe: > for (i = 0; i < NRS_RES_MAX; i++) { > if (pols[i] == NULL) > continue; > > if (nrs == NULL) { > nrs = pols[i]->pol_nrs; > spin_lock(&nrs->nrs_lock); > } > nrs_policy_put_locked(pols[i]); > } > > if (nrs != NULL) > spin_unlock(&nrs->nrs_lock); > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] Staging: lustre: sparse static warning fix 2015-05-18 18:34 ` [PATCH 1/4] Staging: lustre: sparse static warning fix Adrian Remonda 2015-05-18 18:34 ` [PATCH 2/4] " Adrian Remonda @ 2015-05-18 21:23 ` Dan Carpenter 1 sibling, 0 replies; 15+ messages in thread From: Dan Carpenter @ 2015-05-18 21:23 UTC (permalink / raw) To: Adrian Remonda Cc: open list:STAGING SUBSYSTEM, moderated list:STAGING - LUSTRE..., Andreas Dilger, Greg Donald, Melike Yurtoglu, open list, Oleg Drokin, Julia Lawall, Greg Kroah-Hartman, Joe Perches, Omar Sandoval On Mon, May 18, 2015 at 08:34:48PM +0200, Adrian Remonda wrote: > This patch clears the warning given by sparse that the function should be static. > Done by moving the declaration of the structure 'nrs_conf_fifo' into the header > 'ptlrpc_internal.h' > > modified: drivers/staging/lustre/lustre/ptlrpc/nrs.c > modified: drivers/staging/lustre/lustre/ptlrpc/ptlrpc_internal.h Don't include these lines in the changelog. regards, dan carpenter ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2015-05-22 13:38 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <1431974091-26363-1-git-send-email-adrianremonda@gmail.com> 2015-05-18 18:34 ` [PATCH 1/4] Staging: lustre: sparse static warning fix Adrian Remonda 2015-05-18 18:34 ` [PATCH 2/4] " Adrian Remonda 2015-05-18 18:34 ` [PATCH 3/4] Staging: lustre: Fixed typo Adrian Remonda 2015-05-18 18:34 ` [PATCH 4/4] Staging: lustre: sparse lock warning fix Adrian Remonda 2015-05-18 21:21 ` Dan Carpenter 2015-05-20 16:51 ` Dilger, Andreas 2015-05-20 19:29 ` Dan Carpenter 2015-05-20 19:42 ` Dan Carpenter 2015-05-20 22:51 ` Dilger, Andreas 2015-05-22 13:38 ` Dan Carpenter 2015-05-21 8:15 ` AdrianRemonda 2015-05-21 15:12 ` Dan Carpenter 2015-05-21 15:33 ` Drokin, Oleg 2015-05-22 1:11 ` Nikitas Angelinas 2015-05-18 21:23 ` [PATCH 1/4] Staging: lustre: sparse static " Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).