Re: [PATCH v2 2/5] ata: libata-scsi: refactor ata_scsi_report_supported_opcodes()

[Niklas Cassel <cassel@kernel.org>]

I guess we could theoretically report WRITE SAME (16) support in
REPORTED SUPPORTED OPERATION CODES.

A WRITE SAME (16) command can either have the unmap bit set or not.

ata_scsi_write_same_xlat() already rejects a WRITE SAME (16) command that
does not have the unmap bit set:

        if (!unmap || (dev->quirks & ATA_QUIRK_NOTRIM) ||
            !ata_id_has_trim(dev->id)) {
                fp = 1;
                bp = 3;
                goto invalid_fld;
        }


I don't see anything in the SBC / SPC that forbids a device from acting
in this way. Some searching claims that some controllers do reject any
WRITE SAME (16) that does not have the unmap bit set.



To mark that we suport WRITE SAME (16) for deallocation, in the
Logical Block Provisioning VPD Page (0xB2), we currently set:

"
LBPWS (Logical Block Provisioning Write Same 16) Bit: This bit must be set
to 1 to indicate that the device explicitly supports setting the UNMAP bit
in a WRITE SAME (16) command to deallocate blocks.
"

This is done in ata_scsiop_inq_b2():
rbuf[5] = 1 << 6;



However, considering that we always reject a passthrough WRITE SAME (16),
I am not sure if we really want to report WRITE SAME (16) support in
REPORTED SUPPORTED OPERATION CODES.

If feels weird to report support for something, but if the user actually
tries to submit such a command via SG_IO, it would be rejected (even if
the unmap bit is set).

Perhaps it is best to just continue to using it internally as an
intermediate command/representation for REQ_OP_DISCARD ?



I did find this old series from Christoph, that adds a ATA_TRIM SCSI vendor
specific command to translate to DSM TRIM:
https://lore.kernel.org/all/20170320204319.12628-3-hch@lst.de/T/#u

I do like the idea, as we would no longer be limited by the sector size
(the SCSI Data-Out buffer), when issuing trims.

The downside is that it adds ATA specific code to sd.c (see
sd_setup_ata_trim_cmnd()), and I guess that is why the proposal was
NAKed by SCSI maintainers.

With the current solution, which is dependent on the sector size
(i.e. the WRITE SAME (16) Data-out buffer size), for a device with a
sector size of 512, we can issue TRIMs of size:

512 * 0xffff * 64 = 1 Gi + 1023 Mi + 992 Ki

A trim of 2 GB, so perhaps it is not so important to increase/migrate
away from the current solution (which is dependent on the sector size).

One of the biggest complaints Christoph had was the use of the ata_scsi_rbuf
and ata_scsi_rbuf_lock, and that concern seem to be addressed by my series:
https://lore.kernel.org/linux-ide/20260702105956.2058733-4-cassel@kernel.org/T/#u


Kind regards,
Niklas