Re: [PATCH 18/21] scsi: sd: Support reading atomic properties from block limits VPD

From: Bart Van Assche <bvanassche@acm.org>
To: John Garry <john.g.garry@oracle.com>,
	axboe@kernel.dk, kbusch@kernel.org, hch@lst.de, sagi@grimberg.me,
	jejb@linux.ibm.com, martin.petersen@oracle.com,
	djwong@kernel.org, viro@zeniv.linux.org.uk, brauner@kernel.org,
	chandan.babu@oracle.com, dchinner@redhat.com
Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-nvme@lists.infradead.org, linux-xfs@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, tytso@mit.edu, jbongio@google.com,
	linux-api@vger.kernel.org
Subject: Re: [PATCH 18/21] scsi: sd: Support reading atomic properties from block limits VPD
Date: Fri, 6 Oct 2023 10:52:40 -0700	[thread overview]
Message-ID: <a7a24914-4940-4a23-b439-bc8f0ad99212@acm.org> (raw)
In-Reply-To: <53bfe07e-e125-7a69-4f89-481c10e0959e@oracle.com>

On 10/2/23 04:27, John Garry wrote:
> On 29/09/2023 18:54, Bart Van Assche wrote:
>> On 9/29/23 03:27, John Garry wrote:
>>> +static void sd_config_atomic(struct scsi_disk *sdkp)
>>> +{
>>> +    unsigned int logical_block_size = sdkp->device->sector_size;
>>> +    struct request_queue *q = sdkp->disk->queue;
>>> +
>>> +    if (sdkp->max_atomic) {
>>
>> Please use the "return early" style here to keep the indentation
>> level in this function low.
> 
> ok, fine.
> 
>>
>>> +        unsigned int max_atomic = max_t(unsigned int,
>>> +            rounddown_pow_of_two(sdkp->max_atomic),
>>> +            rounddown_pow_of_two(sdkp->max_atomic_with_boundary));
>>> +        unsigned int unit_min = sdkp->atomic_granularity ?
>>> +            rounddown_pow_of_two(sdkp->atomic_granularity) :
>>> +            physical_block_size_sectors;
>>> +        unsigned int unit_max = max_atomic;
>>> +
>>> +        if (sdkp->max_atomic_boundary)
>>> +            unit_max = min_t(unsigned int, unit_max,
>>> +                rounddown_pow_of_two(sdkp->max_atomic_boundary));
>>
>> Why does "rounddown_pow_of_two()" occur in the above code?
> 
> I assume that you are talking about all the code above to calculate 
> atomic write values for the device.
> 
> The reason is that atomic write unit min and max are always a power-of-2 
> - see rules described earlier - as so that we why we rounddown to a 
> power-of-2.

 From SBC-5: "The ATOMIC ALIGNMENT field indicates the required alignment
of the starting LBA in an atomic write command. If the ATOMIC ALIGNMENT
field is set to 0000_0000h, then there is no alignment requirement for
atomic write commands.

The ATOMIC TRANSFER LENGTH GRANULARITY field indicates the minimum
transfer length for an atomic write command. Atomic write operations are
required to have a transfer length that is a multiple of the atomic
transfer length granularity. An ATOMIC TRANSFER LENGTH GRANULARITY field
set to 0000_0000h indicates that there is no atomic transfer length
granularity requirement."

I think the above means that it is wrong to round down the ATOMIC
TRANSFER LENGTH GRANULARITY or the ATOMIC BOUNDARY values.

Thanks,

Bart.