[14/14] target: Fix handling of removed LUNs

On 03/01/2018 04:26 PM, Bart Van Assche wrote:
> Send a valid ASC / ASCQ combination back to the initiator if a SCSI
> command is received after a LUN has been removed. This patch fixes
> the following call trace:
> 
> WARNING: CPU: 0 PID: 4 at drivers/target/target_core_transport.c:3131 translate_sense_reason+0x164/0x190 [target_core_mod]
> Workqueue: ib-comp-wq ib_cq_poll_work [ib_core]
> RIP: 0010:translate_sense_reason+0x164/0x190 [target_core_mod]
> Call Trace:
> transport_send_check_condition_and_sense+0x95/0x1c0 [target_core_mod]
> transport_generic_request_failure+0x102/0x270 [target_core_mod]
> transport_generic_new_cmd+0x138/0x340 [target_core_mod]
> transport_handle_cdb_direct+0x2f/0x80 [target_core_mod]
> target_submit_cmd_map_sgls+0x212/0x2a0 [target_core_mod]
> srpt_handle_new_iu+0x244/0x680 [ib_srpt]
> __ib_process_cq+0x6d/0xc0 [ib_core]
> ib_cq_poll_work+0x18/0x50 [ib_core]
> process_one_work+0x20b/0x6a0
> worker_thread+0x35/0x380
> kthread+0x117/0x130
> ret_from_fork+0x24/0x30
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Mike Christie <mchristi@redhat.com>
> ---
>  drivers/target/target_core_ua.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/target/target_core_ua.c b/drivers/target/target_core_ua.c
> index be25eb807a5f..08c1e550babd 100644
> --- a/drivers/target/target_core_ua.c
> +++ b/drivers/target/target_core_ua.c
> @@ -218,17 +218,15 @@ void core_scsi3_ua_for_check_condition(
>  		return;
>  
>  	nacl = sess->se_node_acl;
> -	if (!nacl)
> +	if (WARN_ON_ONCE(!nacl))
>  		return;
>  
>  	rcu_read_lock();
>  	deve = target_nacl_find_deve(nacl, cmd->orig_fe_lun);
> -	if (!deve) {
> -		rcu_read_unlock();
> -		return;
> -	}
> -	if (!atomic_read(&deve->ua_count)) {
> +	if (!deve || !atomic_read(&deve->ua_count)) {
>  		rcu_read_unlock();
> +		*asc = 0x25;
> +		*ascq = 0;
> 

Hey Bart, I figured out how to hit this. If you just queue up a UA doing
something like changing the ALUA state, then unmap the LUN while IO is
running you will hit it. You do need a little luck for the timing,
because you have to unmap the LUN right after target_scsi3_ua_check does
its target_nacl_find_deve call so it is still in the list there but not
when you call core_scsi3_ua_for_check_condition.

I was in the middle of fixing up the sense key value issue in the patch
like we discussed before (use illegal request instead of unit
attention), but it looks like there was another bug. If we have 2
commands going to the same device and they run target_scsi3_ua_check and
see ua_count=1 TCM_CHECK_CONDITION_UNIT_ATTENTION is returned for both.
They then call core_scsi3_ua_for_check_condition and one of them
deallocates a UA dropping ua_count=0. The second command then runs the
!atomic_read(&deve->ua_count) check and sees the other command has
decremented it. Or the second one might have passed the ua_count check
in core_scsi3_ua_for_check_condition and it runs the loop but nothing is
found since the first command has already removed it. We then return
bogus asc/ascq values.

In the attached patch I just return busy status for this race case. It
seemed easier than trying to add more locking and error handling.

Some notes/questions on some of the code the patch touched though.

If translate_sense_reason failed due to a short buffer it returned
-EINVAL. transport_send_check_condition_and_sense would then end up
calling transport_handle_queue_full/transport_complete_qf and that would
just end up failing the command with
TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE. Do you know if that long journey
intended or just an accident?

In this patch I fixed that so it just translated the error to
TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE immediately. If the patch is ok
and we meant to return that error code for the short buffer then I can
break that out into another patch.

[14/14] target: Fix handling of removed LUNs

Commit Message

Patch