Re: [PATCH 1/2] Documentation: iomap: Add missing flags description

[John Garry <john.g.garry@oracle.com>]

On 04/04/2025 11:23, Ritesh Harjani (IBM) wrote:
> John Garry<john.g.garry@oracle.com> writes:
> 
>> On 03/04/2025 19:22, Ritesh Harjani (IBM) wrote:
>>
>> IMHO, This document seems to be updated a lot, to the point where I
>> think that it has too much detail.
>>
> Perhaps this [1] can change your mind? Just the second paragraph of this
> article might be good reason to keep the design doc updated with latest
> changes in the iomap code.
> 
> [1]:https://urldefense.com/v3/__https://lwn.net/Articles/935934/__;!! 
> ACWV5N9M2RV99hQ! 
> M5YtnH5eBpf0C629QX_zsHZjxSMfWBW8svEup_qNhkg2ie5uqB81lAEO_3DR2pKKSYqUZgLGXiUyQUqi_mjMeZc$ 

I am happy to see documentation, but I think that there is too much 
fine-grained detail in this case.

For my large atomic writes support for XFS series, I am looking at this 
document and thinking that I need to update it again as I am introducing 
a new error code for iomap_dio_rw(). I don't want to have to update the 
document every time I touch iomap or related code.

>>>       * **IOMAP_F_PRIVATE**: Starting with this value, the upper bits can
>>>         be set by the filesystem for its own purposes.
>> Is this comment now out of date according to your change in 923936efeb74?
>>
> Yup. Thanks for catching that. I am thinking we can update this to:
> 
>     * **IOMAP_F_PRIVATE**: This flag is reserved for filesystem private use.
>       Currently only gfs2 uses this for implementing buffer head metadata
>       boundary. 

do we really want to update the doc (or even the iomap.h) if some other 
FS uses this flag? I don't think so.

> This is done by gfs2 to avoid fetching the next mapping as
>       otherwise it could likely incur an additional I/O to fetch the
>       indirect metadata block.
> 
> If this looks good to others too I will update this in the v2.
> 
> Though, I now wonder whether gfs2 can also just use the IOMAP_F_BOUNDARY
> flag instead of using IOMAP_F_PRIVATE?

I'm not sure

> 
>>>    
>>> @@ -250,6 +255,11 @@ The fields are as follows:
>>>         block assigned to it yet and the file system will do that in the bio
>>>         submission handler, splitting the I/O as needed.
>>>    
>>> +   * **IOMAP_F_ATOMIC_BIO**: Indicates that write I/O must be submitted
>>> +     with the ``REQ_ATOMIC`` flag set in the bio.
>> This is effectively the same comment as iomap.h
>>
>>> Filesystems need to set
>>> +     this flag to inform iomap that the write I/O operation requires
>>> +     torn-write protection based on HW-offload mechanism.
>> Personally I think that this is obvious. If not, the reader should check
>> the xfs and ext4 example in the code.
>>
> It's just my opinion, but sometimes including examples of how such flags
> are used in the code - within the design document, can help the reader
> better understand their context and purpose.

Sure, but you need to consider the burden of maintaining this document 
and whether it is even 100% accurate always.