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