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

Ritesh Harjani
2024-02-25T12:21:50Z

Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:

> John Garry <john.g.garry@oracle.com> writes:
>
>> +
>> + 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;
>
> The last condition looks wrong. Shouldn't it be end - 1?
>
>> +
>> + return false;
>> +}
>
> Can we do something like this?
>
> static bool rq_straddles_atomic_write_boundary(struct request *rq,
> unsigned int start_adjust,
> unsigned int end_adjust)
> {
> unsigned int boundary = queue_atomic_write_boundary_bytes(rq->q);
> unsigned long boundary_mask;
> unsigned long start_rq_pos, end_rq_pos;
>
> if (!boundary)
> return false;
>
> start_rq_pos = blk_rq_pos(rq) << SECTOR_SHIFT;
> end_rq_pos = start_rq_pos + blk_rq_bytes(rq);

my bad. I meant this...

end_rq_pos = start_rq_pos + blk_rq_bytes(rq) - 1;
>
> start_rq_pos -= start_adjust;
> end_rq_pos += end_adjust;
>
> boundary_mask = boundary - 1;
>
> if ((start_rq_pos | boundary_mask) != (end_rq_pos | boundary_mask))
> return true;
>
> return false;
> }
>
> I was thinking this check should cover all cases? Thoughts?
>
>

-ritesh