Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
IronPort-SDR: fJprAIa4DXztqPNQeZ/FKZeE3T8sWIVyeRtGQO0qMC8EbitSQ8HHQiuUkJnqn3BUe5X+oBEvOH
 0Jb1vFjMHgNbLJ27sbQF4BMewXiHF7+ZjpidtSogUyCw18+PxjEvNwGnvrfXGUgbrwwXysL1jl
 8rESV0xfI2QjnTYXFKnFSf7WKzO1/iN6AEpW9/j+mnxfLmoMWYJuyMsgef2ICvKtpJyh2IKlxm
 mTnAGZEEr0iHgQMAY/3qUslJEwDUJn6mlWV12TmdFoXTBF/KmWRHQ8FZhEDKXHohXJuV2NN1FT
 P8jgEf+ye9yzUlsIrWPJ8HuI
IronPort-SDR: FB5AAJYNbAwl1yUWc2yDFJhEMI2P6aJRowZKEoi1qzgP/E+4w2atN/U1LMBWl+DnFe+RAQ6gHn
 gWIRu88+VQCPDJ2MDG20VHwh5hs31dyPnBdOxzpApBtRp9AZyi3VvXj/sZpg7p/fhXc5GSGekk
 v8EcJggQVqr3yjVgQpyvdO91XCodnDrbEvCiOF1Ozzr0OTkfZ2WUSNzX1Y/RLZBqFi9icK9SYZ
 olzMRZmdccptJnSh/yb1ntjzHkNrIhRTT6mUa6lMhwYhrIGGmJaW9Z4P/nPeEieGVWLxQMWEKC
 Yfc=
WDCIronportException: Internal
Message-ID: <c702f06e-b7da-92be-3c4f-5dd405600235@opensource.wdc.com>
Date:   Thu, 16 Jun 2022 11:47:03 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [PATCH RFC v2 03/18] scsi: core: Implement reserved command
 handling
Content-Language: en-US
To:     John Garry <john.garry@huawei.com>,
        Bart Van Assche <bvanassche@acm.org>, axboe@kernel.dk,
        jejb@linux.ibm.com, martin.petersen@oracle.com, brking@us.ibm.com,
        hare@suse.de, hch@lst.de
Cc:     linux-block@vger.kernel.org, linux-ide@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org,
        chenxiang66@hisilicon.com
References: <1654770559-101375-1-git-send-email-john.garry@huawei.com>
 <1654770559-101375-4-git-send-email-john.garry@huawei.com>
 <b4a0ede5-95a3-4388-e808-7627b5484d01@opensource.wdc.com>
 <9e89360d-3325-92af-0436-b34df748f3e2@acm.org>
 <e36bba7e-d78d-27b4-a0e2-9d921bc82f5d@opensource.wdc.com>
 <3a27b6ff-e495-8f11-6925-1487c9d14fa9@huawei.com>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <3a27b6ff-e495-8f11-6925-1487c9d14fa9@huawei.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On 6/15/22 16:35, John Garry wrote:
> On 15/06/2022 00:43, Damien Le Moal wrote:
>> On 6/15/22 03:20, Bart Van Assche wrote:
>>> On 6/13/22 00:01, Damien Le Moal wrote:
>>>> On 6/9/22 19:29, John Garry wrote:
>>>>> +=C2=A0=C2=A0=C2=A0 /*
>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0 * This determines how many commands the H=
BA will set aside
>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0 * for internal commands. This number will=
 be added to
>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0 * @can_queue to calcumate the maximum num=
ber of simultaneous
>>>>
>>>> s/calcumate/calculate
>>>>
>>>> But this is weird. For SATA, can_queue is 32. Having reserved comman=
ds,
>>>> that number needs to stay the same. We cannot have more than 32 tags=
.
>>>> I think keeping can_queue as the max queue depth with at most
>>>> nr_reserved_cmds tags reserved is better.
>>>>
>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0 * commands sent to the host.
>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0 */
>>>>> +=C2=A0=C2=A0=C2=A0 int nr_reserved_cmds;
>>>
>>> +1 for Damien's request. I also prefer to keep can_queue as the maxim=
um
>>> queue depth, whether or not nr_reserved_cmds has been set.
>>
>> For non SATA drives, I still think that is a good idea. However, for=20
>> SATA,
>> we always have the internal tag command that is special. With John's
>> change, it would have to be reserved but that means we are down to 31 =
max
>> QD,
>=20
> My intention is to keep regular tag depth at 32 for SATA. We add an=20
> extra tag as a reserved tag. Indeed, this is called a 'tag', but it's=20
> just really the placeholder for what will be the ATA_TAG_INTERNAL reque=
st.
>=20
> About how we set scsi_host.can_queue, in this series we set .can_queue=20
> as max regular tags, and the handling is as follows:
>=20
> scsi_mq_setup_tags():
> tag_set->queue_depth =3D shost->can_queue + shost->nr_reserved_cmds
> tag_set->reserved_tags =3D shost->nr_reserved_cmds
>=20
> So we honour the rule that blk_mq_tag_set.queue_depth is the total tag=20
> depth, including reserved.
>=20
> Incidentally I think Christoph prefers to keep .can_queue at total max=20
> tags including reserved:
> https://lore.kernel.org/linux-scsi/337339b7-6f4a-a25c-f11c-7f701b42d6a8=
@suse.de/=20
>=20
>=20
>> so going backward several years... That internal tag for ATA does not
>> need to be reserved since this command is always used when the drive i=
s
>> idle and no other NCQ commands are on-going.
>=20
> So do you mean that ATA_TAG_INTERNAL qc is used for other commands apar=
t=20
> from internal commands?

No. It is used only for internal commands. What I meant to say is that=20
currently, internal commands are issued only on device scan, device=20
revalidate and error handling. All of these phases are done with the=20
device under EH with the issuing path stopped and all commands=20
completed, so no regular commands can be issued. Only internal ones, non=20
NCQ, using the ATA_TAG_INTERNAL. So strictly speaking, we should not=20
need to reserve that internal tag at all.

>=20
>>
>> So the solution to all this is a likely a little more complicated if w=
e
>> want to keep ATA max QD to 32.
>>
>=20
> thanks,
> John


--=20
Damien Le Moal
Western Digital Research