On 05/27/15 20:08, vasu.dev@xxxxxxxxxxxxxxx wrote:
On Wed, 2015-05-27 at 15:14 +0200, Bart Van Assche wrote:
On 05/27/15 03:13, vasu.dev@xxxxxxxxxxxxxxx wrote:
...but question is why even we added schedule()
which is now causing this side effect in this commit:-
commit 7030fd626129ec4d616784516a462d317c251d39
Author: Bart Van Assche<bvanassche@xxxxxxx>
Date: Sat Aug 17 20:34:43 2013 +0000
May be we should remove schedule() there which again does ep->ex_lock
drop and re-grab logic for schedule. Do we really need schedule there
added by above patch ?
The layers above libfc need a way to wait until ongoing invocations of
their own response handler have finished. This is why the schedule() call
has been added in fc_seq_set_resp().
Bart is that from LIO or SCST or SCSI ?
Hello Vasu,
Both FCoE target drivers need this functionality. When an FCoE initiator
submits e.g. a LUN RESET while a SCSI WRITE is in progress, an FCoE
target driver needs a way to let the WRITE command finish in such a way
that data frames associated with the WRITE that arrive after the WRITE
command has been aborted do not cause a kernel crash. Both FCoE target
drivers use fc_seq_set_resp() to let 'ep->arg' point at the SCSI command
associated with an FC exchange. SCSI abort and LUN reset processing
happens concurrently with regular command processing in a FCoE SCSI
target driver. This means that an fc_seq_set_resp(sp, NULL, NULL) call
is needed to avoid that ep->arg becomes a dangling pointer and also that
the context from where that call occurs must wait until any ongoing
ep->resp calls have finished.
It is too restricting to their underlying any HBA by not letting them
call into them while spin lock held or bh disabled, especially here
schedule is hardly doing anything useful other than yielding and then
resuming back to grab the lock again, so basically its caller call flow
was just paused using schedule instead let that flow do any thing useful
during schedule() preemption.
static void fc_seq_set_resp(struct fc_seq *sp,
void (*resp)(struct fc_seq *, struct
fc_frame *,
void *),
void *arg)
{
struct fc_exch *ep = fc_seq_exch(sp);
DEFINE_WAIT(wait);
spin_lock_bh(&ep->ex_lock);
while (ep->resp_active && ep->resp_task != current) {
prepare_to_wait(&ep->resp_wq, &wait,
TASK_UNINTERRUPTIBLE);
spin_unlock_bh(&ep->ex_lock);
schedule();
spin_lock_bh(&ep->ex_lock);
}
finish_wait(&ep->resp_wq, &wait);
ep->resp = resp;
ep->arg = arg;
spin_unlock_bh(&ep->ex_lock);
}
May be remove the schedule() is the proper fix here, instead I'd have
gone with revised patch below but that seems to have issues as well, see
details below.
Would removing schedule() from that function cause it to busy-wait or
did I perhaps misunderstand something ?
[ ... ]
diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
index 30f9ef0..4a1fe55 100644
--- a/drivers/scsi/libfc/fc_exch.c
+++ b/drivers/scsi/libfc/fc_exch.c
@@ -709,8 +709,8 @@ static int fc_seq_exch_abort(const struct fc_seq *req_sp,
* Since fc_exch_done() invokes fc_seq_set_resp() it is guaranteed that that
* ep->resp() won't be invoked after fc_exch_done() has returned.
*
- * The response handler itself may invoke fc_exch_done(), which will clear the
- * ep->resp pointer.
+ * The response handler itself may invoke fc_exch_done(), which will set
+ * FC_EX_DONE.
*
* Return value:
* Returns true if and only if ep->resp has been invoked.
@@ -730,7 +730,8 @@ static bool fc_invoke_resp(struct fc_exch *ep, struct fc_seq *sp,
arg = ep->arg;
spin_unlock_bh(&ep->ex_lock);
- if (resp) {
+ if (resp && (!(ep->state & (FC_EX_RST_CLEANUP | FC_EX_DONE)) ||
+ IS_ERR(fp))) {
resp(sp, fp, arg);
This will prevent any clean up during exches reset which are meant to
reset upper layers of exch block in libfc stack.
Can you clarify this further ? Isn't exchange cleanup always done by
passing an error pointer to the 'fp' argument ? The above modification
still causes the function 'resp' points at to be called if IS_ERR(fp) is
true.
res = true;
}
@@ -939,7 +940,6 @@ static void fc_exch_done(struct fc_seq *sp)
rc = fc_exch_done_locked(ep);
spin_unlock_bh(&ep->ex_lock);
- fc_seq_set_resp(sp, NULL, ep->arg);
We had resp set to NULL here to prevent any subsequent call back and
that won't happen anymore.
Are you sure that it is essential to reset 'resp' to prevent subsequent
callbacks ? Since fc_exch_done() sets FC_EX_DONE, since all ep->resp
calls happen through fc_invoke_resp() and since this patch modifies
fc_invoke_resp() such that it tests the FC_EX_DONE flag before invoking
ep->resp, isn't that sufficient to prevent that new invocations of
ep->resp will occur after fc_exch_done() has returned ?
Thanks,
Bart.
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
