From nobody Thu Dec 30 23:06:13 2021 X-Original-To: scsi@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id A583A192FABC for ; Thu, 30 Dec 2021 23:06:15 +0000 (UTC) (envelope-from mavbsd@gmail.com) Received: from mail-qv1-xf29.google.com (mail-qv1-xf29.google.com [IPv6:2607:f8b0:4864:20::f29]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4JQ3mR40RMz4gwK; Thu, 30 Dec 2021 23:06:15 +0000 (UTC) (envelope-from mavbsd@gmail.com) Received: by mail-qv1-xf29.google.com with SMTP id q4so23311881qvh.9; Thu, 30 Dec 2021 15:06:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=sender:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=Lra7g97bsiqtdauEnV8uzHdNlijxWybX2UMKpvMR/tU=; b=iPk1UIYRs68oRUYzDNH6p1kJ2Ul9Ei5CynlVuyYRxnN22O07xUHNNnE5np7FjduXjs SBon1Mq+IbXXe9/pirdvAcv+Hf5gdhOR7ZWCWBzOprS7TwwSAB7UitmKEchfim9Ctsjp hxM8A/dPShnqJ1u/ayX5HyKZePqi++AgvQ5omirvOm+9kXa8bNDjaCyHowbEyyX9x14m QUrFtrMuJkGyKYgcv4/yESHeJMDcaETlcXEKliJlxQh7+jg8pNP+QjMXsjmxRGL7Lbwy 6woXPnGeg6BwSHhY9fyk54c+MAXx/DlHuwYTRUzJ6tJN2VHv2mri11q9hy4CXk0Syogx O0nA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:sender:message-id:date:mime-version:user-agent :subject:content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=Lra7g97bsiqtdauEnV8uzHdNlijxWybX2UMKpvMR/tU=; b=piXJiwUU9w1xVXNCs3O1fYSwd8uszgJmsRGM1nABktf0Y8qEfmKzPvsN8earUCUkqQ rxWlukNbnTl062VUDuaLIJyVS4WqE2x45GWp3FKipPualwEgF7YPqygTKogymWDKIvTM 7CtXgc0dYb1XgEJQmEAx/PAoYKVYIH4DrwWqiaY91rmPVpvQkdhq4aUoC3GcgtQTZvGF vr3VjVKX3EvZM/bL2EMh0ZB9fvWMsKoehUAIKUwYWmu8j7NSaz4tYBfWJ8VZUhux9gTQ DOjov03aMzMGJPNOcLBhBB5wtCCyaqpOoaL9ZeRTjTCvN5QYFMKAscw2T2T/gnZxTKLn j+FQ== X-Gm-Message-State: AOAM530r/l0vQPrq2c5D3wZ+ssGVz2Hi0N3SxfTXUVlXfDhzanrLuLCY MLxv9XyXmgZ398p2mIbizIr0prJ1zbU= X-Google-Smtp-Source: ABdhPJyokY8apqfl8U0eJLN0cqhYdJXe1+TIslRlHeq3c+gaXC2F2486r3eWKHDHO6rP0c5f77lXYw== X-Received: by 2002:a05:6214:2263:: with SMTP id gs3mr13867651qvb.88.1640905574343; Thu, 30 Dec 2021 15:06:14 -0800 (PST) Received: from [10.231.1.66] ([38.32.73.2]) by smtp.gmail.com with ESMTPSA id y17sm21640451qtj.75.2021.12.30.15.06.13 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 30 Dec 2021 15:06:13 -0800 (PST) Message-ID: <2ce80c64-6954-21d0-74eb-eeb88e289350@FreeBSD.org> Date: Thu, 30 Dec 2021 18:06:13 -0500 List-Id: SCSI subsystem List-Archive: https://lists.freebsd.org/archives/freebsd-scsi List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-scsi@freebsd.org X-BeenThere: freebsd-scsi@freebsd.org MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:91.0) Gecko/20100101 Thunderbird/91.4.1 Subject: Re: iSCSI target: Handling in-flight requests during ctld shutdown Content-Language: en-US To: John Baldwin Cc: =?UTF-8?Q?Edward_Tomasz_Napiera=c5=82a?= , scsi@FreeBSD.org, Ken Merry References: <42e175d9-1693-29e2-0b5b-3fa513aa2a2d@FreeBSD.org> <7f1a1a65-199d-858a-792c-42871d1df13e@FreeBSD.org> <68ad0ffd-79fb-973d-e5ba-92deee352d44@FreeBSD.org> From: Alexander Motin In-Reply-To: <68ad0ffd-79fb-973d-e5ba-92deee352d44@FreeBSD.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 4JQ3mR40RMz4gwK X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-ThisMailContainsUnwantedMimeParts: N On 30.12.2021 17:41, John Baldwin wrote: > On 12/30/21 12:10 PM, Alexander Motin wrote: >> The checks were in all backends, until in 2c7dc6bae9fd I've moved them >> to ctl_datamove_done_process().  Backends don't have any special >> knowledge now to process specific data move error codes.  It was >> complete code duplication between backends and I've tried to organize >> that a bit.  Unfortunately we do not have formalized data move error >> statuses in CTL to do more.  It would be much more interesting (and >> complicated ;)) if iSCSI (for example) could report data transfer error >> without killing connection, but that area in general is not very >> specified in SCSI, since usually each transport has own means of >> recovery. > > Hmmm, but this commit seems to have actually broken this case.  That is, > now the possibly uninitialized data is written to the backing store: > >         /* > -        * We set status at this point for read commands, and write > -        * commands with errors. > +        * We set status at this point for read and compare commands. >          */ > -       if (io->io_hdr.flags & CTL_FLAG_ABORT) { > -               ; > -       } else if ((io->io_hdr.port_status != 0) && > -           ((io->io_hdr.status & CTL_STATUS_MASK) == CTL_STATUS_NONE || > -            (io->io_hdr.status & CTL_STATUS_MASK) == CTL_SUCCESS)) { > -               ctl_set_internal_failure(&io->scsiio, /*sks_valid*/ 1, > -                   /*retry_count*/ io->io_hdr.port_status); > -       } else if (io->scsiio.kern_data_resid != 0 && > -           (io->io_hdr.flags & CTL_FLAG_DATA_MASK) == CTL_FLAG_DATA_OUT && > -           ((io->io_hdr.status & CTL_STATUS_MASK) == CTL_STATUS_NONE || > -            (io->io_hdr.status & CTL_STATUS_MASK) == CTL_SUCCESS)) { > -               ctl_set_invalid_field_ciu(&io->scsiio); > -       } else if ((io->io_hdr.port_status == 0) && > -           ((io->io_hdr.status & CTL_STATUS_MASK) == CTL_STATUS_NONE)) { > +       if ((io->io_hdr.flags & CTL_FLAG_ABORT) == 0 && > +           (io->io_hdr.status & CTL_STATUS_MASK) == CTL_STATUS_NONE) { >                 lbalen = ARGS(beio->io); >                 if (lbalen->flags & CTL_LLF_READ) { >                         ctl_set_success(&io->scsiio); > > As ctl_datamove_done_process() will call ctl_set_internal_failure(), but > ctl_datamove_done() will still call be_move_done which runs the code above. > The code above doesn't check port_status anywhere, so it will still happily > write the stale data to the backend unless CTL_FLAG_ABORT is set.  Do the > backends need to be checking port_status == 0 again in addition to > CTL_FLAG_ABORT then? No. Backends check for ((io->io_hdr.status & CTL_STATUS_MASK) == CTL_STATUS_NONE) . Notice that you've received that HARDWARE ERROR status, not garbage. >>>    I do see checks for CTL_FLAG_ABORT, and the handler >>> for >>> the CTL_TASK_I_T_NEXUS_RESET does set CTL_FLAG_ABORT on pending >>> requests. >>> >>> For the tasks in sciscsi_session_terminate_tasks(), those should already >>> have >>> CTL_FLAG_ABORT set anyway, but it wouldn't hurt if it were set again by >>> cfiscsi_data_wait_abort().  For the the cfiscsi_task_management_done >>> case I'm >>> less certain, but I suspect there too that returning an internal error >>> status >>> back to the initiator is not expected and that it would be better to >>> just set >>> CTL_FLAG_ABORT and drop any response? >> >> cfiscsi_data_wait_abort() does only one specific thing -- it aborts >> waiting for data and completes the data move operation, allowing backend >> to continue (or at least free resources, etc).  It is only >> cfiscsi_session_terminate_tasks() knows that it should abort data moves >> while it aborts tasks, since it knows that the connection is dead. >> cfiscsi_task_management_done() is another special case, since iSCSI >> explicitly mentions that there will be no further exchanges related to >> that task, so the data move should explicitly return.  But in general >> case I think it would be nice to not combine those two facts of aborting >> task and aborting data transfer, since the last may not necessary imply >> the first, as the first does not imply the last. >> >> It is tempting to just set the flag, but I suspect it may cause more >> problems (I am worrying about HA peer update).  I'd prefer the race >> inside the transport to be handled inside the transport.  May be we >> could just add another flag to be set under the session lock inside >> cfiscsi_session_terminate_tasks() after it aborted all the tasks, but >> before it start to abort transfers itself and use it inside >> cfiscsi_datamove_out() instead of cs_terminating. > > I think though that even that flag wouldn't quite help as you would still > have the problem that the internal_error response would still be sent on > the wire if we don't abort the entire task vs aborting just the move. No. cfiscsi_datamove_out() called before the new flag is set would still try to send R2T over the dying connection to be aborted by the cfiscsi_session_terminate_tasks() few milliseconds later. cfiscsi_data_wait_abort() would only be needed if cfiscsi_session_terminate_tasks() has already passed through the data waiters list and waiting for the last tasks completion. > It may be that all I really need to do is fix the one case in > cfiscsi_datamove_out() to set CTL_FLAG_ABORT rather than changing > cfiscsi_data_move_abort()? I still think it won't be needed with the new flag I proposed, that would be cleaner IMO. -- Alexander Motin