> On Nov 15, 2015, at 4:10 AM, Or Gerlitz <gerlitz.or@xxxxxxxxx> wrote: > > On Fri, Nov 13, 2015 at 6:51 PM, Mike Christie <michaelc@xxxxxxxxxxx> wrote: >> On 11/13/2015 09:06 AM, Or Gerlitz wrote: >>>> 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. >> >> A patch should not cause this many issues. >> >>> What makes you say it was poorly reviewed? >> >> I just did not do a good job at looking at the patch. I should have >> caught all of these issues. >> >> - The bnx2i cleanup_task bug should have been obvious, especially for me >> because I had commented about the back lock and the abort path. >> >> - This oops, was so basic. Incorrect locking around a linked list being >> accessed from 2 threads is really one of those 1st year kernel >> programmer things. > > Mike, Chris > > After the locking change, adding a task to any of the connection > mgmtqueue, cmdqueue, or requeue lists is under the session forward lock. > > Removing tasks from any of these lists in iscsi_data_xmit is under > the session forward lock and **before** calling down to the transport > to handle the task. > > The iscsi_complete_task helper was added by Mike's commit > 3bbaaad95fd38ded "[SCSI] libiscsi: handle cleanup task races" > and is indeed typically called under the backward lock && has this section > > + if (!list_empty(&task->running)) > + list_del_init(&task->running); > > which per my reading of the code never comes into play, can you comment? I had sent this to Sagi and your mellanox email the other day: > The bug occurs when a target completes a command while we are still > processing it. If we are doing a WRITE and the iscsi_task > is on the cmdqueue because we are handling a R2T. The target shouldn't > send a Check Condition at this time, but some do. If that happens, then > iscsi_queuecommand could be adding a new task to the cmdqueue, while the > recv path is handling the CC for the task with the outsanding R2T. The > recv path iscsi_complete_task call sees that task it on the cmdqueue and > deletes it from the list at the same time iscsi_queuecommand is adding a new > task. > > This should not happen per the iscsi spec. There is some wording about > waiting to finish the sequence in progress, but targets goof this up. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
