Re: [PATCH v4 05/11] block: Add core atomic write support

John Garry
2024-02-20T10:02:15Z

On 19/02/2024 22:58, Dave Chinner wrote:
> On Mon, Feb 19, 2024 at 01:01:03PM +0000, John Garry wrote:
>> Add atomic write support as follows:
>> diff --git a/block/blk-merge.c b/block/blk-merge.c
>> index 74e9e775f13d..12a75a252ca2 100644
>> --- a/block/blk-merge.c
>> +++ b/block/blk-merge.c
>> @@ -18,6 +18,42 @@
>> #include "blk-rq-qos.h"
>> #include "blk-throttle.h"
>>
>> +static bool rq_straddles_atomic_write_boundary(struct request *rq,
>> + unsigned int front,
>> + unsigned int back)
>> +{
>> + unsigned int boundary = queue_atomic_write_boundary_bytes(rq->q);
>> + unsigned int mask, imask;
>> + loff_t start, end;
>> +
>> + if (!boundary)
>> + return false;
>> +
>> + start = rq->__sector << SECTOR_SHIFT;
>> + end = start + rq->__data_len;
>> +
>> + start -= front;
>> + end += back;
>> +
>> + /* We're longer than the boundary, so must be crossing it */
>> + if (end - start > boundary)
>> + return true;
>> +
>> + mask = boundary - 1;
>> +
>> + /* start/end are boundary-aligned, so cannot be crossing */
>> + if (!(start & mask) || !(end & mask))
>> + return false;
>> +
>> + imask = ~mask;
>> +
>> + /* Top bits are different, so crossed a boundary */
>> + if ((start & imask) != (end & imask))
>> + return true;
>> +
>> + return false;
>> +}
> I have no way of verifying this function is doing what it is
> supposed to because it's function is undocumented. I have no idea
> what the front/back variables are supposed to represent, and so no
> clue if they are being applied properly.

I'll add proper function header documentation.

>
> That said, it's also applying unsigned 32 bit mask variables to
> signed 64 bit quantities and trying to do things like "high bit changed"
> checks on the 64 bit variable. This just smells like a future
> source of "large offsets don't work like we expected!" bugs.

I'll change variables to be unsigned and also all the same size.

>
>> diff --git a/block/blk-settings.c b/block/blk-settings.c
>> index 06ea91e51b8b..176f26374abc 100644
>> --- a/block/blk-settings.c
>> +++ b/block/blk-settings.c
>> @@ -59,6 +59,13 @@ void blk_set_default_limits(struct queue_limits *lim)
>> lim->zoned = false;
>> lim->zone_write_granularity = 0;
>> lim->dma_alignment = 511;
>> + lim->atomic_write_hw_max_sectors = 0;
>> + lim->atomic_write_max_sectors = 0;
>> + lim->atomic_write_hw_boundary_sectors = 0;
>> + lim->atomic_write_hw_unit_min_sectors = 0;
>> + lim->atomic_write_unit_min_sectors = 0;
>> + lim->atomic_write_hw_unit_max_sectors = 0;
>> + lim->atomic_write_unit_max_sectors = 0;
>> }
> Seems to me this function would do better to just
>
> memset(lim, 0, sizeof(*lim));
>
> and then set all the non-zero fields.

Christoph responded about limits here and in
blk_queue_atomic_write_max_bytes(), so please let me know if still some
concerns.

Thanks,
John