Message-ID: <1459963704.2372.43.camel@linux.vnet.ibm.com>
Date:	Wed, 06 Apr 2016 13:28:24 -0400
From:	James Bottomley <jejb@...ux.vnet.ibm.com>
To:	Bastien Philbert <bastienphilbert@...il.com>,
	Julian Calaby <julian.calaby@...il.com>
Cc:	"Martin K. Petersen" <martin.petersen@...cle.com>,
	linux-scsi <linux-scsi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] csiostor: Fix backwards locking in the function
 __csio_unreg_rnode

On Wed, 2016-04-06 at 13:23 -0400, Bastien Philbert wrote:
> 
> On 2016-04-06 01:14 PM, James Bottomley wrote:
> > On Wed, 2016-04-06 at 10:36 -0400, Bastien Philbert wrote:
> > > 
> > > On 2016-04-06 10:24 AM, James Bottomley wrote:
> > > > On Wed, 2016-04-06 at 10:11 -0400, Bastien Philbert wrote:
> > > > > 
> > > > > On 2016-04-06 09:38 AM, James Bottomley wrote:
> > > > > > On Wed, 2016-04-06 at 09:21 -0400, Bastien Philbert wrote:
> > > > > > > 
> > > > > > > On 2016-04-06 03:48 AM, Julian Calaby wrote:
> > > > > > > > Hi Bastien,
> > > > > > > > 
> > > > > > > > On Wed, Apr 6, 2016 at 7:19 AM, Bastien Philbert
> > > > > > > > <bastienphilbert@...il.com> wrote:
> > > > > > > > > This fixes backwards locking in the function
> > > > > > > > > __csio_unreg_rnode
> > > > > > > > > to
> > > > > > > > > properly lock before the call to the function
> > > > > > > > > csio_unreg_rnode
> > > > > > > > > and
> > > > > > > > > not unlock with spin_unlock_irq as this would not
> > > > > > > > > allow
> > > > > > > > > the
> > > > > > > > > proper
> > > > > > > > > protection for concurrent access on the shared
> > > > > > > > > csio_hw
> > > > > > > > > structure
> > > > > > > > > pointer hw. In addition switch the locking after the
> > > > > > > > > critical
> > > > > > > > > region
> > > > > > > > > function call to properly unlock instead with
> > > > > > > > > spin_unlock_irq
> > > > > > > > > on
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Bastien Philbert <
> > > > > > > > > bastienphilbert@...il.com>
> > > > > > > > > ---
> > > > > > > > >  drivers/scsi/csiostor/csio_rnode.c | 4 ++--
> > > > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/scsi/csiostor/csio_rnode.c
> > > > > > > > > b/drivers/scsi/csiostor/csio_rnode.c
> > > > > > > > > index e9c3b04..029a09e 100644
> > > > > > > > > --- a/drivers/scsi/csiostor/csio_rnode.c
> > > > > > > > > +++ b/drivers/scsi/csiostor/csio_rnode.c
> > > > > > > > > @@ -580,9 +580,9 @@ __csio_unreg_rnode(struct
> > > > > > > > > csio_rnode
> > > > > > > > > *rn)
> > > > > > > > >                 ln->last_scan_ntgts--;
> > > > > > > > >         }
> > > > > > > > > 
> > > > > > > > > -       spin_unlock_irq(&hw->lock);
> > > > > > > > > -       csio_unreg_rnode(rn);
> > > > > > > > >         spin_lock_irq(&hw->lock);
> > > > > > > > > +       csio_unreg_rnode(rn);
> > > > > > > > > +       spin_unlock_irq(&hw->lock);
> > > > > > > > 
> > > > > > > > Are you _certain_ this is correct? This construct
> > > > > > > > usually
> > > > > > > > appears
> > > > > > > > when
> > > > > > > > a function has a particular lock held, then needs to
> > > > > > > > unlock
> > > > > > > > it
> > > > > > > > to
> > > > > > > > call
> > > > > > > > some other function. Are you _certain_ that this isn't
> > > > > > > > the
> > > > > > > > case?
> > > > > > > > 
> > > > > > > > Thanks,
> > > > > > > > 
> > > > > > > Yes I am pretty certain this is correct. I checked the
> > > > > > > paths
> > > > > > > that
> > > > > > > called this function
> > > > > > > and it was weired that none of them gradded the spinlock
> > > > > > > before
> > > > > > > hand.
> > > > > > 
> > > > > > That's not good enough.  If your theory is correct, lockdep
> > > > > > should
> > > > > > be
> > > > > > dropping an already unlocked assertion in this codepath ...
> > > > > > do
> > > > > > you
> > > > > > see
> > > > > > this?
> > > > > > 
> > > > > > James
> > > > > > 
> > > > > > 
> > > > > Yes I do.
> > > > 
> > > > You mean you don't see the lockdep assert, since you're asking
> > > > to 
> > > > drop the patch?
> > > > 
> > > > >  For now just drop the patch but I am still concerned that we
> > > > > are
> > > > > double unlocking here.
> > > > 
> > > > Really, no.  The pattern in the code indicates the lock is
> > > > expected 
> > > > to be held.  This can be wrong (sometimes code moves or people
> > > > forget), but if it is wrong we'll get an assert about unlock of
> > > > an 
> > > > already unlocked lock.  If there's no assert, the lock is held
> > > > on 
> > > > entry and the code is correct.
> > > > 
> > > > You're proposing patches based on misunderstandings of the code
> > > > which aren't backed up by actual issues and wasting everyone's
> > > > time 
> > > > to look at them.  Please begin with the hard evidence of a
> > > > problem 
> > > > first, so post the lockdep assert in the changelog so we know 
> > > > there's a real problem.
> > > > 
> > > > James
> > > > 
> > > Certainly James. I think I just got carried away with the last
> > > few 
> > > patches :(.
> > 
> > Is this Nick Krause?  An email reply that Martin forwarded but the 
> > list didn't pick up (because it had a html part) suggests this. 
> >  What you're doing is what got you banned from LKML the last time: 
> > sending patches without evidence there's a problem or understanding 
> > the code you're patching.  Repeating the behaviour under a new 
> > identity isn't going to help improve your standing.
> > 
> > James
> > 
> No I am not Nick Krause. I am just aware of how he got banned a few 
> years ago. That email was a mistake by typo and was hoping nobody 
> picked it up as they would then believe I was Nick Krause.

Hm, OK, but currently you are repeating his behaviour ... please don't
send any more patches until they're about real problems backed by
actual data.

Thanks,

James

Powered by blists - more mailing lists