Re: [PATCH 1/1] iscsi: fix regression caused by session lock patch

[Or Gerlitz <gerlitz.or@gmail.com>]

On Thu, Nov 12, 2015 at 10:58 PM, Mike Christie <michaelc@cs.wisc.edu> wrote:
> On 11/12/2015 06:03 AM, Sagi Grimberg wrote:
>>> The bug is caused by this patch:
>>>
>>> 659743b02c411075b26601725947b21df0bb29c8
>>>
>>> which allowed the task lists to be manipulated under different locks
>>> in the xmit and completion path.

>>> To fix the oops this patch just reverts that patch. It also reverts
>>> these 2 patches for regressions that were also a result of that patch:

>> Whoa now Mike, this is a major change. Can we take a step back and think
>> about this for a second?

> The issue has been on the open-iscsi list for a week! You are on the
> list still right? Or is even ccd on the thread.

The email you sent me a week ago also cc-ed open-iscsi hence was
routed by a rule in my mailer that made it to land in my open-iscsi
subscription folder which is don't visit much nowadays. Only when you
posted to linux-scsi we saw that and responded within few hours, to
begin with.

>> Can you provide a more detailed analysis of why is this list corruption
>> triggered? What scenario was not honoring the locking policy?

> Basic locking around a linked list bug. iscsi_queuecommand adds it under
> the frwd lock and iscsi_complete_task was taking it off the back_lock.

>> This code has been running reliably in our labs for a long time now
>> (both iser and tcp).

> The patch has caused multiple regressions, did not even compile when
> sent to me, and was poorly reviewed and I have not heard from you guys
> in a week. Given the issues the patch has had and the current time, I do
> not feel comfortable with it anymore. I want to re-review it and fix it
> up when there is more time.

Mike (Hi),

It's a complex patch that touches all the iscsi transports, and yes,
when it was send to you the 1st time, there was build error on one of
the offload transports (bad! but happens) and yes, as you pointed, one
static checker fix + one bug fix for it went upstream after this has
been merged, happens too.

What makes you say it was poorly reviewed?

And now after few years in upstream a possibly real bug was found
(happens), why rush and revert? lets see if we can fix.

Or.