Re: [PATCH v5 5/7] ata: libata: Fix FUA handling in ata_build_rw_tf()

From: Damien Le Moal <damien.lemoal@opensource.wdc.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-ide@vger.kernel.org, linux-block@vger.kernel.org,
	Jens Axboe <axboe@kernel.dk>,
	"Maciej S . Szmigiero" <mail@maciej.szmigiero.name>,
	Hannes Reinecke <hare@suse.de>
Subject: Re: [PATCH v5 5/7] ata: libata: Fix FUA handling in ata_build_rw_tf()
Date: Mon, 7 Nov 2022 21:32:56 +0900	[thread overview]
Message-ID: <e4f4f8a6-34e1-428a-7e1f-dc66ded93887@opensource.wdc.com> (raw)
In-Reply-To: <20221107121627.GA17441@lst.de>

On 11/7/22 21:16, Christoph Hellwig wrote:
> On Mon, Nov 07, 2022 at 09:12:57PM +0900, Damien Le Moal wrote:
>> On 11/7/22 14:50, Christoph Hellwig wrote:
>>> On Mon, Nov 07, 2022 at 09:50:19AM +0900, Damien Le Moal wrote:
>>>> Finally, since the block layer should never issue a FUA read
>>>> request, warn in ata_build_rw_tf() if we see such request.
>>>
>>> Couldn't this be triggered using SG_IO passthrough with a SCSI 
>>> WRITE* command that has the FUA bit set?
>>
>> Yes indeed. Should I drop the warn ?
> 
> I think the warn needs to go.  But don't we also need to handle the
> non-NCQ fua case if we don't want to break pure passthrough appliations?
> Or do we simply not care?  In the latter case we'll at least need a
> comment documenting that tradeoff.

I am tempted to say "not care" since it has been like this since forever,
silently letting FUA read through that do not do FUA at all...

I am also tempted to say "let's add a check and fail those FUA reads that
are not supported", that is, essentially stop hiding errors to the user.
Bad passthrough applications that were working would stop working. Is that
considered breaking the app if it was bad in the first place ?

-- 
Damien Le Moal
Western Digital Research