Re: [PATCH 3/3] scsi: sync cache on write

[Caleb Connolly <caleb.connolly@linaro.org>]

On 3/25/25 16:18, neil.armstrong@linaro.org wrote:
> On 25/03/2025 14:49, Caleb Connolly wrote:
>>
>>
>> On 3/25/25 14:33, Neil Armstrong wrote:
>>> On 25/03/2025 14:02, Caleb Connolly wrote:
>>>> We don't have a mechanism to safely shutdown block devices prior to a
>>>> baord reset or driver removal. Prevent data loss by synchronizing the
>>>> SCSI cache after every write.
>>>>
>>>> In particular this solves the issue of capsule updates looping on some
>>>> devices because the board resets immediately after deleting the capsule
>>>> file and this write wouldn't be flushed in time.
>>>>
>>>> This may impact NAND wear, but should be negligible given the usecases
>>>> for disk write in U-Boot.
>>>>
>>>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>>>> ---
>>>>   drivers/scsi/scsi.c | 28 ++++++++++++++++++++++++++++
>>>>   1 file changed, 28 insertions(+)
>>>>
>>>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>>>> index 
>>>> 1aa7fbdbb5278e387de72a3c3e73d19ea0342fff..1b71e580b89b100395ea132a4976366a713cba8a 100644
>>>> --- a/drivers/scsi/scsi.c
>>>> +++ b/drivers/scsi/scsi.c
>>>> @@ -77,8 +77,25 @@ static void scsi_setup_inquiry(struct scsi_cmd 
>>>> *pccb)
>>>>       pccb->cmdlen = 6;
>>>>       pccb->msgout[0] = SCSI_IDENTIFY; /* NOT USED */
>>>>   }
>>>> +static void scsi_setup_sync_cache(struct scsi_cmd *pccb, lbaint_t 
>>>> start,
>>>> +                  unsigned short blocks)
>>>> +{
>>>> +    pccb->cmd[0] = SCSI_SYNC_CACHE;
>>>> +    pccb->cmd[1] = 0;
>>>> +    pccb->cmd[2] = (unsigned char)(start >> 24) & 0xff;
>>>> +    pccb->cmd[3] = (unsigned char)(start >> 16) & 0xff;
>>>> +    pccb->cmd[4] = (unsigned char)(start >> 8) & 0xff;
>>>> +    pccb->cmd[5] = (unsigned char)start & 0xff;
>>>> +    pccb->cmd[6] = 0;
>>>> +    pccb->cmd[7] = (unsigned char)(blocks >> 8) & 0xff;
>>>> +    pccb->cmd[8] = (unsigned char)blocks & 0xff;
>>>> +    pccb->cmd[9] = 0;
>>>> +    pccb->cmdlen = 10;
>>>> +    pccb->msgout[0] = SCSI_IDENTIFY; /* NOT USED */
>>>> +}
>>>> +
>>>>   static void scsi_setup_read_ext(struct scsi_cmd *pccb, lbaint_t 
>>>> start,
>>>>                   unsigned short blocks)
>>>>   {
>>>>       pccb->cmd[0] = SCSI_READ10;
>>>> @@ -235,8 +252,19 @@ static ulong scsi_write(struct udevice *dev, 
>>>> lbaint_t blknr, lbaint_t blkcnt,
>>>>               blkcnt -= blks;
>>>>               break;
>>>>           }
>>>> +        /*
>>>> +         * Ensure the writes are flushed to disk so we don't lose data
>>>> +         * on board reset.
>>>> +         * FIXME: this ought to be in a SCSI shutdown path instead
>>>> +         */
>>>> +        scsi_setup_sync_cache(pccb, start, smallblks);
>>>
>>> Why can't this be done at the end of scsi_write() ?
>>
>> We have the same limitation that we can only flush SCSI_MAX_BLK blocks 
>> at once. It could be done in a second loop at the end but I figured 
>> it's more sensible to just interleave them.
> 
> Indeed you're right, another solution is to leave count to 0, and it 
> will sync all data after start:
> ````
> If the LBA and COUNT arguments are both zero (their defaults) then all 
> blocks in the cache are synchronized
> If LBA is greater than zero while COUNT is zero then blocks in the cache 
> whose addresses are from and including LBA to the highest lba on the 
> device are synchronized.
> ```
> 
> And it seems Linux only passes 0 parameters, and just flushes all data 
> in cache:
> https://elixir.bootlin.com/linux/v6.13.7/source/drivers/scsi/sd.c#L1145
> 
> So either you leave is as is so all data is written as soon as possible,
> or we wait until the end of the write and flush everything.

Ahh, damn I should have checked for that!
> 
> I think the last one is just simpler and safer.

You're totally right XD

Thanks
> 
> Neil
> 
>>>
>>>> +        if (scsi_exec(bdev, pccb)) {
>>>> +            scsi_print_error(pccb);
>>>> +            break;
>>>> +        }
>>>> +
>>>>           if (blks > max_blks) {
>>>>               start += max_blks;
>>>>               blks -= max_blks;
>>>>           } else {
>>>>
>>>
>>> And perhaps a sync subcommand of the scsi command would be nice to 
>>> have !
>>
>> Good idea, that would tie in nicely if/when we get a .sync() op for 
>> block devices and call it prior to board reset.
>>>
>>> Neil
>>>
>>
> 

-- 
Caleb (they/them)