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 ?
>
> Hello Vasu,
>
> 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 ?
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.
> Regarding avoiding to drop and re-grab
> the lock, how about replacing patch 4/4 by the patch below ?
>
> Thanks,
>
> Bart.
>
>
> [PATCH] libfc: Fix the device reset implementation
>
> Avoid that fc_fcp_cleanup_each_cmd() triggers the following bug:
>
> BUG: scheduling while atomic: sg_reset/1512/0x00000202
> 1 lock held by sg_reset/1512:
> #0: (&(&fsp->scsi_pkt_lock)->rlock){+.-...}, at: [<ffffffffc0225cd5>] fc_fcp_cleanup_each_cmd.isra.21+0xa5/0x150 [libfc]
> Preemption disabled at:[<ffffffffc0225cd5>] fc_fcp_cleanup_each_cmd.isra.21+0xa5/0x150 [libfc]
> Call Trace:
> [<ffffffff816c612c>] dump_stack+0x4f/0x7b
> [<ffffffff810828bc>] __schedule_bug+0x6c/0xd0
> [<ffffffff816c87aa>] __schedule+0x71a/0xa10
> [<ffffffff816c8ad2>] schedule+0x32/0x80
> [<ffffffffc0217eac>] fc_seq_set_resp+0xac/0x100 [libfc]
> [<ffffffffc0218b11>] fc_exch_done+0x41/0x60 [libfc]
> [<ffffffffc0225cff>] fc_fcp_cleanup_each_cmd.isra.21+0xcf/0x150 [libfc]
> [<ffffffffc0225f43>] fc_eh_device_reset+0x1c3/0x270 [libfc]
> [<ffffffff814a2cc9>] scsi_try_bus_device_reset+0x29/0x60
> [<ffffffff814a3908>] scsi_ioctl_reset+0x258/0x2d0
> [<ffffffff814a2650>] scsi_ioctl+0x150/0x440
> [<ffffffff814b3a9d>] sd_ioctl+0xad/0x120
> [<ffffffff8132f266>] blkdev_ioctl+0x1b6/0x810
> [<ffffffff811da608>] block_ioctl+0x38/0x40
> [<ffffffff811b4e08>] do_vfs_ioctl+0x2f8/0x530
> [<ffffffff811b50c1>] SyS_ioctl+0x81/0xa0
> [<ffffffff816cf8b2>] system_call_fastpath+0x16/0x7a
>
> Signed-off-by: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
> Cc: Vasu Dev <vasu.dev@xxxxxxxxx>
> Cc: stable <stable@xxxxxxxxxxxxxxx>
> ---
> drivers/scsi/libfc/fc_exch.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> 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.
> 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.
Thanks,
Vasu
> if (!rc)
> fc_exch_delete(ep);
> }
--
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
