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

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

[Ritesh Harjani (IBM)]

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.
+
    * **IOMAP_F_PRIVATE**: Starting with this value, the upper bits can
      be set by the filesystem for its own purposes.
 
@@ -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. Filesystems need to set
+     this flag to inform iomap that the write I/O operation requires
+     torn-write protection based on HW-offload mechanism.
+
    These flags can be set by iomap itself during file operations.
    The filesystem should supply an ``->iomap_end`` function if it needs
    to observe these flags:
-- 
2.48.1

[PATCH 2/2] iomap: trace: Add missing flags to [IOMAP_|IOMAP_F_]FLAGS_STRINGS

[Ritesh Harjani (IBM)]

This adds missing iomap flags to IOMAP_FLAGS_STRINGS &
IOMAP_F_FLAGS_STRINGS for tracing. While we are at it, let's also print
values of iomap->type & iomap->flags.

e.g. trace for ATOMIC_BIO flag set
xfs_io-1203    [000] .....   183.001559: iomap_iter_dstmap: dev 8:32 ino 0xc bdev 8:32 addr 0x84200000 offset 0x0 length 0x10000 type MAPPED (0x2) flags DIRTY|ATOMIC_BIO (0x102)

e.g. trace with DONTCACHE flag set
xfs_io-1110    [007] .....   238.780532: iomap_iter: dev 8:16 ino 0x83 pos 0x1000 length 0x1000 status 0 flags WRITE|DONTCACHE (0x401) ops xfs_buffered_write_iomap_ops caller iomap_file_buffered_write+0xab/0x0

Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/iomap/trace.h | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/fs/iomap/trace.h b/fs/iomap/trace.h
index 9eab2c8ac3c5..455cc6f90be0 100644
--- a/fs/iomap/trace.h
+++ b/fs/iomap/trace.h
@@ -99,7 +99,11 @@ DEFINE_RANGE_EVENT(iomap_dio_rw_queued);
 	{ IOMAP_FAULT,		"FAULT" }, \
 	{ IOMAP_DIRECT,		"DIRECT" }, \
 	{ IOMAP_NOWAIT,		"NOWAIT" }, \
-	{ IOMAP_ATOMIC,		"ATOMIC" }
+	{ IOMAP_OVERWRITE_ONLY,	"OVERWRITE_ONLY" }, \
+	{ IOMAP_UNSHARE,	"UNSHARE" }, \
+	{ IOMAP_DAX,		"DAX" }, \
+	{ IOMAP_ATOMIC,		"ATOMIC" }, \
+	{ IOMAP_DONTCACHE,	"DONTCACHE" }

 #define IOMAP_F_FLAGS_STRINGS \
 	{ IOMAP_F_NEW,		"NEW" }, \
@@ -107,7 +111,14 @@ DEFINE_RANGE_EVENT(iomap_dio_rw_queued);
 	{ IOMAP_F_SHARED,	"SHARED" }, \
 	{ IOMAP_F_MERGED,	"MERGED" }, \
 	{ IOMAP_F_BUFFER_HEAD,	"BH" }, \
-	{ IOMAP_F_SIZE_CHANGED,	"SIZE_CHANGED" }
+	{ IOMAP_F_XATTR,	"XATTR" }, \
+	{ IOMAP_F_BOUNDARY,	"BOUNDARY" }, \
+	{ IOMAP_F_ANON_WRITE,	"ANON_WRITE" }, \
+	{ IOMAP_F_ATOMIC_BIO,	"ATOMIC_BIO" }, \
+	{ IOMAP_F_PRIVATE,	"PRIVATE" }, \
+	{ IOMAP_F_SIZE_CHANGED,	"SIZE_CHANGED" }, \
+	{ IOMAP_F_STALE,	"STALE" }
+

 #define IOMAP_DIO_STRINGS \
 	{IOMAP_DIO_FORCE_WAIT,	"DIO_FORCE_WAIT" }, \
@@ -138,7 +149,7 @@ DECLARE_EVENT_CLASS(iomap_class,
 		__entry->bdev = iomap->bdev ? iomap->bdev->bd_dev : 0;
 	),
 	TP_printk("dev %d:%d ino 0x%llx bdev %d:%d addr 0x%llx offset 0x%llx "
-		  "length 0x%llx type %s flags %s",
+		  "length 0x%llx type %s (0x%x) flags %s (0x%x)",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->ino,
 		  MAJOR(__entry->bdev), MINOR(__entry->bdev),
@@ -146,7 +157,9 @@ DECLARE_EVENT_CLASS(iomap_class,
 		  __entry->offset,
 		  __entry->length,
 		  __print_symbolic(__entry->type, IOMAP_TYPE_STRINGS),
-		  __print_flags(__entry->flags, "|", IOMAP_F_FLAGS_STRINGS))
+		  __entry->type,
+		  __print_flags(__entry->flags, "|", IOMAP_F_FLAGS_STRINGS),
+		  __entry->flags)
 )

 #define DEFINE_IOMAP_EVENT(name)		\
@@ -185,7 +198,7 @@ TRACE_EVENT(iomap_writepage_map,
 		__entry->bdev = iomap->bdev ? iomap->bdev->bd_dev : 0;
 	),
 	TP_printk("dev %d:%d ino 0x%llx bdev %d:%d pos 0x%llx dirty len 0x%llx "
-		  "addr 0x%llx offset 0x%llx length 0x%llx type %s flags %s",
+		  "addr 0x%llx offset 0x%llx length 0x%llx type %s (0x%x) flags %s (0x%x)",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->ino,
 		  MAJOR(__entry->bdev), MINOR(__entry->bdev),
@@ -195,7 +208,9 @@ TRACE_EVENT(iomap_writepage_map,
 		  __entry->offset,
 		  __entry->length,
 		  __print_symbolic(__entry->type, IOMAP_TYPE_STRINGS),
-		  __print_flags(__entry->flags, "|", IOMAP_F_FLAGS_STRINGS))
+		  __entry->type,
+		  __print_flags(__entry->flags, "|", IOMAP_F_FLAGS_STRINGS),
+		  __entry->flags)
 );

 TRACE_EVENT(iomap_iter,
--
2.48.1

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

[John Garry]

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.

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

This is just effectively the same comment as in the code - what's the 
use in this?

> +     Currently XFS uses this to prevent merging of ioends across RTG
> +     (realtime group) boundaries.
> +
>      * **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?

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

> +
>      These flags can be set by iomap itself during file operations.
>      The filesystem should supply an ``->iomap_end`` function if it needs
>      to observe these flags:

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

[Ritesh Harjani]

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

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

[John Garry]

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.

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

[Christoph Hellwig]

On Fri, Apr 04, 2025 at 10:36:32AM +0100, John Garry wrote:
> > @@ -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.
> 
> This is just effectively the same comment as in the code - what's the use in
> this?

Darrick asked for this file to have full comments.  I'm more on your
side here as a lot of this seem redundant.

> 
> > +     Currently XFS uses this to prevent merging of ioends across RTG
> > +     (realtime group) boundaries.
> > +
> >      * **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?

Also we probably should not detail file system behavior here, but a
high level description of what it is useful.

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

[Darrick J. Wong]

On Fri, Apr 04, 2025 at 10:36:32AM +0100, John Garry wrote:
> 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.

The goal of this document is to capture the designers' mental models of
how the iomap code solves specific problems.  In other words, it's a
human language description of what the author thinks the code should be
doing to solve the problem.  The documentation should be written at one
conceptual level higher than the code itself to help newcomers grok the
iomap APIs without having to reverse-guess our intent from the existing
clients (ext4/xfs/etc).

I don't know if that helps, but "too much" is subjective.

> > 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.
> 
> This is just effectively the same comment as in the code - what's the use in
> this?

Specific terms like "IOMAP_F_BOUNDARY" need to be defined before they
can be discussed.  Yes, it's redundant with the more terse description
in the C header.  No, we should not make people cross reference two
separate documents for basic definitions.

If you know how to do the magic sphinx stuff to auto-merge the C
comments into the rst then I'm all ears.

> > +     Currently XFS uses this to prevent merging of ioends across RTG
> > +     (realtime group) boundaries.

I think this sentence should go farther in capturing why the flag is
used --

"Zone XFS uses this flag to prevent an ioend from being combined with a
previously generated ioend because writes cannot span a zone boundary."

and later for the ext2 iomap port:

"ext2 uses this flag as an IO submission boundary when an indirect
mapping block comes immediately after a file data extent to try to
submit IOs in linear order."

Though now that I've written that, I'm not so sure IOMAP_F_BOUNDARY is
the right flag for this -- for zns xfs it's a completion boundary, for
ext2 it's a submission boundary.

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

I wish you'd quote subject lines so I could search them in my mailbox.
You might have a mental index of commit ids, but I do not.

923936efeb74b3 ("iomap: Fix conflicting values of iomap flags")

So, yes.

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

The purpose of documentation is to help someone who knows their
filesystem well and iomap less well to construct a mental model of how
iomap works so they can make their filesystem use the IO path without
implementing their own.  Yes, it's obvious and redundant to all of us
who have spent time wrangling iomap, but so are the signs that tell you
what street you're on.  They're not for the people who live there,
they're for people who are lost and want to get somewhere.

> If not, the reader should check the
> xfs and ext4 example in the code.

No.  Someone who's unfamiliar with iomap should not have to learn
*another filesystem's codebase* to grok the infrastructure.  That's how
buffer heads are and struct pages were, which is to say a drag on
everyone getting anything done.

--D

> 
> > +
> >      These flags can be set by iomap itself during file operations.
> >      The filesystem should supply an ``->iomap_end`` function if it needs
> >      to observe these flags:
> 
> 

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

[Darrick J. Wong]

On Thu, Apr 03, 2025 at 11:52:27PM +0530, Ritesh Harjani (IBM) wrote:
> 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.

Hrm, ok.  Based on hch's comment about not mentioning specific fs
behavior, I think I'll suggest something more like:

IOMAP_F_BOUNDARY: This I/O and its completion must not be merged with
any other I/O or completion.  Filesystems must use this when submitting
I/O to devices that cannot handle I/O crossing certain LBAs (e.g. ZNS
devices).  This flag applies only to buffered I/O writeback; all other
functions ignore it.

>     * **IOMAP_F_PRIVATE**: Starting with this value, the upper bits can
>       be set by the filesystem for its own purposes.
>  
> @@ -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. Filesystems need to set
> +     this flag to inform iomap that the write I/O operation requires
> +     torn-write protection based on HW-offload mechanism.

They must also ensure that mapping updates upon the completion of the
I/O must be performed in a single metadata update.

--D

> +
>     These flags can be set by iomap itself during file operations.
>     The filesystem should supply an ``->iomap_end`` function if it needs
>     to observe these flags:
> -- 
> 2.48.1
> 
> 

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

[Darrick J. Wong]

On Mon, Apr 07, 2025 at 09:45:04AM +0100, John Garry wrote:
> 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.

You're changing shared code APIs.  The documentation must be updated, so
everyone else can better understand the new code.  This is about
reducing friction and confusion in the fs development community.

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

No.  I've changd my mind, you all are right that these design docs
shouldn't be mentioning specific client filesystems.  Just leave it at

"This flag is reserved for filesystem private use."

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

You'll have to ask Andreas Gruenbacher.

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

If you don't care for the burden of making shared code easier for others
to figure out once you've changed it, then please don't work on it.

--D

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

[Darrick J. Wong]

On Mon, Apr 07, 2025 at 01:50:00AM -0700, Christoph Hellwig wrote:
> On Fri, Apr 04, 2025 at 10:36:32AM +0100, John Garry wrote:
> > > @@ -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.
> > 
> > This is just effectively the same comment as in the code - what's the use in
> > this?
> 
> Darrick asked for this file to have full comments.  I'm more on your
> side here as a lot of this seem redundant.

Yes, some of it duplicates iomap.h's more terse comments.  I'm willing
to be flexible about some of this, so long as the documentation helps
the /next/ person to understand iomap and how to use it.

> > > +     Currently XFS uses this to prevent merging of ioends across RTG
> > > +     (realtime group) boundaries.
> > > +
> > >      * **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?
> 
> Also we probably should not detail file system behavior here, but a
> high level description of what it is useful.

Agreed (now).

--D

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

[Ritesh Harjani]

"Darrick J. Wong" <djwong@kernel.org> writes:

> On Thu, Apr 03, 2025 at 11:52:27PM +0530, Ritesh Harjani (IBM) wrote:
>> 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.
>
> Hrm, ok.  Based on hch's comment about not mentioning specific fs
> behavior, I think I'll suggest something more like:
>
> IOMAP_F_BOUNDARY: This I/O and its completion must not be merged with
> any other I/O or completion.  Filesystems must use this when submitting
> I/O to devices that cannot handle I/O crossing certain LBAs (e.g. ZNS
> devices).  This flag applies only to buffered I/O writeback; all other
> functions ignore it.
>

Sure.

>>     * **IOMAP_F_PRIVATE**: Starting with this value, the upper bits can
>>       be set by the filesystem for its own purposes.
>>  
>> @@ -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. Filesystems need to set
>> +     this flag to inform iomap that the write I/O operation requires
>> +     torn-write protection based on HW-offload mechanism.
>
> They must also ensure that mapping updates upon the completion of the
> I/O must be performed in a single metadata update.
>

Thanks Darrick, for the clarification and the review comments on this
patch. Once the remaining tracing patch is reviewed, I will incorporate
these comments in v2.

-ritesh

Re: [PATCH 2/2] iomap: trace: Add missing flags to [IOMAP_|IOMAP_F_]FLAGS_STRINGS

[Darrick J. Wong]

On Thu, Apr 03, 2025 at 11:52:28PM +0530, Ritesh Harjani (IBM) wrote:
> This adds missing iomap flags to IOMAP_FLAGS_STRINGS &
> IOMAP_F_FLAGS_STRINGS for tracing. While we are at it, let's also print
> values of iomap->type & iomap->flags.
> 
> e.g. trace for ATOMIC_BIO flag set
> xfs_io-1203    [000] .....   183.001559: iomap_iter_dstmap: dev 8:32 ino 0xc bdev 8:32 addr 0x84200000 offset 0x0 length 0x10000 type MAPPED (0x2) flags DIRTY|ATOMIC_BIO (0x102)
> 
> e.g. trace with DONTCACHE flag set
> xfs_io-1110    [007] .....   238.780532: iomap_iter: dev 8:16 ino 0x83 pos 0x1000 length 0x1000 status 0 flags WRITE|DONTCACHE (0x401) ops xfs_buffered_write_iomap_ops caller iomap_file_buffered_write+0xab/0x0
> 
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

Seems reasonable to me
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/iomap/trace.h | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/iomap/trace.h b/fs/iomap/trace.h
> index 9eab2c8ac3c5..455cc6f90be0 100644
> --- a/fs/iomap/trace.h
> +++ b/fs/iomap/trace.h
> @@ -99,7 +99,11 @@ DEFINE_RANGE_EVENT(iomap_dio_rw_queued);
>  	{ IOMAP_FAULT,		"FAULT" }, \
>  	{ IOMAP_DIRECT,		"DIRECT" }, \
>  	{ IOMAP_NOWAIT,		"NOWAIT" }, \
> -	{ IOMAP_ATOMIC,		"ATOMIC" }
> +	{ IOMAP_OVERWRITE_ONLY,	"OVERWRITE_ONLY" }, \
> +	{ IOMAP_UNSHARE,	"UNSHARE" }, \
> +	{ IOMAP_DAX,		"DAX" }, \
> +	{ IOMAP_ATOMIC,		"ATOMIC" }, \
> +	{ IOMAP_DONTCACHE,	"DONTCACHE" }
> 
>  #define IOMAP_F_FLAGS_STRINGS \
>  	{ IOMAP_F_NEW,		"NEW" }, \
> @@ -107,7 +111,14 @@ DEFINE_RANGE_EVENT(iomap_dio_rw_queued);
>  	{ IOMAP_F_SHARED,	"SHARED" }, \
>  	{ IOMAP_F_MERGED,	"MERGED" }, \
>  	{ IOMAP_F_BUFFER_HEAD,	"BH" }, \
> -	{ IOMAP_F_SIZE_CHANGED,	"SIZE_CHANGED" }
> +	{ IOMAP_F_XATTR,	"XATTR" }, \
> +	{ IOMAP_F_BOUNDARY,	"BOUNDARY" }, \
> +	{ IOMAP_F_ANON_WRITE,	"ANON_WRITE" }, \
> +	{ IOMAP_F_ATOMIC_BIO,	"ATOMIC_BIO" }, \
> +	{ IOMAP_F_PRIVATE,	"PRIVATE" }, \
> +	{ IOMAP_F_SIZE_CHANGED,	"SIZE_CHANGED" }, \
> +	{ IOMAP_F_STALE,	"STALE" }
> +
> 
>  #define IOMAP_DIO_STRINGS \
>  	{IOMAP_DIO_FORCE_WAIT,	"DIO_FORCE_WAIT" }, \
> @@ -138,7 +149,7 @@ DECLARE_EVENT_CLASS(iomap_class,
>  		__entry->bdev = iomap->bdev ? iomap->bdev->bd_dev : 0;
>  	),
>  	TP_printk("dev %d:%d ino 0x%llx bdev %d:%d addr 0x%llx offset 0x%llx "
> -		  "length 0x%llx type %s flags %s",
> +		  "length 0x%llx type %s (0x%x) flags %s (0x%x)",
>  		  MAJOR(__entry->dev), MINOR(__entry->dev),
>  		  __entry->ino,
>  		  MAJOR(__entry->bdev), MINOR(__entry->bdev),
> @@ -146,7 +157,9 @@ DECLARE_EVENT_CLASS(iomap_class,
>  		  __entry->offset,
>  		  __entry->length,
>  		  __print_symbolic(__entry->type, IOMAP_TYPE_STRINGS),
> -		  __print_flags(__entry->flags, "|", IOMAP_F_FLAGS_STRINGS))
> +		  __entry->type,
> +		  __print_flags(__entry->flags, "|", IOMAP_F_FLAGS_STRINGS),
> +		  __entry->flags)
>  )
> 
>  #define DEFINE_IOMAP_EVENT(name)		\
> @@ -185,7 +198,7 @@ TRACE_EVENT(iomap_writepage_map,
>  		__entry->bdev = iomap->bdev ? iomap->bdev->bd_dev : 0;
>  	),
>  	TP_printk("dev %d:%d ino 0x%llx bdev %d:%d pos 0x%llx dirty len 0x%llx "
> -		  "addr 0x%llx offset 0x%llx length 0x%llx type %s flags %s",
> +		  "addr 0x%llx offset 0x%llx length 0x%llx type %s (0x%x) flags %s (0x%x)",
>  		  MAJOR(__entry->dev), MINOR(__entry->dev),
>  		  __entry->ino,
>  		  MAJOR(__entry->bdev), MINOR(__entry->bdev),
> @@ -195,7 +208,9 @@ TRACE_EVENT(iomap_writepage_map,
>  		  __entry->offset,
>  		  __entry->length,
>  		  __print_symbolic(__entry->type, IOMAP_TYPE_STRINGS),
> -		  __print_flags(__entry->flags, "|", IOMAP_F_FLAGS_STRINGS))
> +		  __entry->type,
> +		  __print_flags(__entry->flags, "|", IOMAP_F_FLAGS_STRINGS),
> +		  __entry->flags)
>  );
> 
>  TRACE_EVENT(iomap_iter,
> --
> 2.48.1
> 
>