Re: [PATCH 01/25] ata: scsi: rename flag ATA_QCFLAG_FAILED to ATA_QCFLAG_EH

From: John Garry <john.g.garry@oracle.com>
To: Niklas Cassel <niklas.cassel@wdc.com>,
	Damien Le Moal <damien.lemoal@opensource.wdc.com>,
	Mikael Pettersson <mikpelinux@gmail.com>,
	Brian King <brking@us.ibm.com>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Hannes Reinecke <hare@suse.de>,
	linux-scsi@vger.kernel.org, linux-ide@vger.kernel.org
Subject: Re: [PATCH 01/25] ata: scsi: rename flag ATA_QCFLAG_FAILED to ATA_QCFLAG_EH
Date: Wed, 21 Dec 2022 11:47:17 +0000	[thread overview]
Message-ID: <a86d8ae7-907b-a004-916d-e3354c3f88e8@oracle.com> (raw)
In-Reply-To: <20221208105947.2399894-2-niklas.cassel@wdc.com>

On 08/12/2022 10:59, Niklas Cassel wrote:
> The name ATA_QCFLAG_FAILED is misleading since it does not mean that a
> QC was actually an error.

"QC was actually an error" sounds odd. Maybe "...it does not mean that a
QC completed in error or not at all". I'm not sure on a good suggestion.

> It means that libata decided to schedule EH
> for the QC, so the QC is now owned by the libata error handler (EH).
> 
> The normal execution path is responsible for not accessing a QC owned
> by EH. libata core enforces the rule by returning NULL from
> ata_qc_from_tag() for QCs owned by EH.
> 
> It is quite easy to mistake that a QC marked with ATA_QCFLAG_FAILED was
> an error. However, a QC that was actually an error is instead indicated
> by having qc->err_mask set. E.g. when we have a NCQ error, we abort all
> QCs, which currently will mark all QCs as ATA_QCFLAG_FAILED. However, it
> will only be a single QC that is an error (i.e. has qc->err_mask set).
> 
> Rename ATA_QCFLAG_FAILED to ATA_QCFLAG_EH to more clearly highlight that
> this flag simply means that a QC is now owned by EH. This new name will
> not mislead to think that the QC was an error (which is instead
> indicated by having qc->err_mask set).
> 
> This also makes it more obvious that the EH code skips all QCs that do
> not have ATA_QCFLAG_EH set (rather than ATA_QCFLAG_FAILED), since the EH
> code should simply only care about QCs that are owned by EH itself.
> 
> Signed-off-by: Niklas Cassel<niklas.cassel@wdc.com>

FWIW,

Reviewed-by: John Garry <john.g.garry@oracle.com>