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

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

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://lwn.net/Articles/935934/

>> Let's document the use of these flags in iomap design doc where other
>> flags are defined too -
>> 
>> - IOMAP_F_BOUNDARY was added by XFS to prevent merging of ioends
>>    across RTG boundaries.
>> - IOMAP_F_ATOMIC_BIO was added for supporting atomic I/O operations
>>    for filesystems to inform the iomap that it needs HW-offload based
>>    mechanism for torn-write protection
>> 
>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> ---
>>   Documentation/filesystems/iomap/design.rst | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>> 
>> diff --git a/Documentation/filesystems/iomap/design.rst b/Documentation/filesystems/iomap/design.rst
>> index e29651a42eec..b916e85bc930 100644
>> --- a/Documentation/filesystems/iomap/design.rst
>> +++ b/Documentation/filesystems/iomap/design.rst
>> @@ -243,6 +243,11 @@ The fields are as follows:
>>        regular file data.
>>        This is only useful for FIEMAP.
>>   
>> +   * **IOMAP_F_BOUNDARY**: This indicates that I/O and I/O completions
>> +     for this iomap must never be merged with the mapping before it.
>> +     Currently XFS uses this to prevent merging of ioends across RTG
>> +     (realtime group) boundaries.

>
> This is just effectively the same comment as in the code -

I am happy to add/update if you would like to add more details.

> what's the use in this?
>

To keep the iomap design doc updated with the latest changes.

>> +
>>      * **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. 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? 

>>   
>> @@ -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. 

Thanks for the review!

-ritesh