| Message ID | 3170ab367b5b350c60564886a72719ccf573d01c.1743691371.git.ritesh.list@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] Documentation: iomap: Add missing flags description | expand |
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:
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
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:
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(+)