Re: [PATCH v2] Documentation: add initial iomap kdoc

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, May 18, 2023 at 10:04:43PM -0700, Christoph Hellwig wrote:
> On Thu, May 18, 2023 at 08:01:05AM -0700, Luis Chamberlain wrote:
> > +        Mapping of heading styles within this document:
> > +        Heading 1 uses "====" above and below
> > +        Heading 2 uses "===="
> > +        Heading 3 uses "----"
> > +        Heading 4 uses "````"
> > +        Heading 5 uses "^^^^"
> > +        Heading 6 uses "~~~~"
> > +        Heading 7 uses "...."
> > +
> > +        Sections are manually numbered because apparently that's what everyone
> 
> Why are you picking different defaults then the rest of the kernel
> documentation?

I bet Luis copied that from the online fsck document.

IIRC the doc generator is smart enough to handle per-file heading usage.
The rst parser sourcecode doesn't seem to have harcoded defaults; every
time it sees an unfamiliar heading style in a .rst file, it adds that as
the next level down in the hierarchy.

Also, where are the "proper" headings documented for Documentation/?

(Skip to the end; it's late and I don't have time right now to read the
content of this patch.)

> > +
> > +A modern block abstraction
> > +==========================
> > +
> > +**iomap** allows filesystems to query storage media for data using *byte
> > +ranges*. Since block mapping are provided for a *byte ranges* for cache data in
> > +memory, in the page cache, naturally this implies operations on block ranges
> > +will also deal with *multipage* operations in the page cache. **Folios** are
> > +used to help provide *multipage* operations in memory for the *byte ranges*
> > +being worked on.
> 
> As mentioned you list time this information was circulated this is not
> true.  iomap itself has nothing to with blocks, and even less so with
> the page cache per se.  It just iterates over ranges of file data and
> applies work to it.
> 
> > +iomap IO interfaces
> > +===================
> > +
> > +You call **iomap** depending on the type of filesystem operation you are working
> > +on. We detail some of these interactions below.
> 
> Who is you?
> 
> > +
> > +iomap for bufferred IO writes
> > +-----------------------------
> > +
> > +You call **iomap** for buffered IO with:
> > +
> > + * ``iomap_file_buffered_write()`` - for buffered writes
> > + * ``iomap_page_mkwrite()`` - when dealing callbacks for
> > +    ``struct vm_operations_struct``
> > +
> > +  * ``struct vm_operations_struct.page_mkwrite()``
> > +  * ``struct vm_operations_struct.fault()``
> > +  * ``struct vm_operations_struct.huge_fault()``
> > +  * ``struct vm_operations_struct`.pfn_mkwrite()``
> > +
> > +You *may* use buffered writes to also deal with ``fallocate()``:
> > +
> > + * ``iomap_zero_range()`` on fallocate for zeroing
> > + * ``iomap_truncate_page()`` on fallocate for truncation
> > +
> > +Typically you'd also happen to use these on paths when updating an inode's size.
> 
> I'm not really sure what this is trying to explain.  It basically looks
> like filler text generated by machine learning algorithms..
> 
> The same is true for a large part of this document.
> 
> > +A filesystem also needs to call **iomap** when assisting the VFS manipulating a
> > +file into the page cache.
> 
> A file systsem doesn't _need_ to do anything.  It may chose to do
> things, and the iomap based helpers might be useful for that.  But
> again, I'm still not getting what this document is even trying to
> explain, as "to implement the method foo, use the iomap_foo" isn't
> really helping anyone.
> 
> > +Converting filesystems from buffer-head to iomap guide
> > +======================================================
> 
> If you want such a guide, please keep it in a separate file from the
> iomap API documentation.  I'd also suggest that you actually try such
> a conversion first, as that might help shaping the documentation :)
> 
> > +Testing Direct IO
> > +=================
> > +
> > +Other than fstests you can use LTP's dio, however this tests is limited as it
> > +does not test stale data.
> > +
> > +{{{
> > +./runltp -f dio -d /mnt1/scratch/tmp/
> > +}}}
> 
> How does this belong into an iomap documentation?  If LTPs dio is really
> all that useful we should import it into xfstests, btw.  I'm not sure it
> is, though.
> 
> > +We try to document known issues that folks should be aware of with **iomap** here.
> 
> Who is "we"?
> 
> > + * DOC: Introduction
> > + *
> > + * iomap allows filesystems to sequentially iterate over byte addressable block
> > + * ranges on an inode and apply operations to it.
> > + *
> > + * iomap grew out of the need to provide a modern block mapping abstraction for
> > + * filesystems with the different IO access methods they support and assisting
> > + * the VFS with manipulating files into the page cache. iomap helpers are
> > + * provided for each of these mechanisms. However, block mapping is just one of
> > + * the features of iomap, given iomap supports DAX IO for filesystems and also
> > + * supports such the ``lseek``/``llseek`` ``SEEK_DATA``/``SEEK_HOLE``
> > + * interfaces.
> > + *
> > + * Block mapping provides a mapping between data cached in memory and the
> > + * location on persistent storage where that data lives. `LWN has an great
> > + * review of the old buffer-heads block-mapping and why they are inefficient
> > + * <https://lwn.net/Articles/930173/>`, since the inception of Linux.  Since
> > + * **buffer-heads** work on a 512-byte block based paradigm, it creates an
> > + * overhead for modern storage media which no longer necessarily works only on
> > + * 512-blocks. iomap is flexible providing block ranges in *bytes*. iomap, with
> > + * the support of folios, provides a modern replacement for **buffer-heads**.
> > + */
> 
> I really don't want random blurbs and links like this in the main
> header.  If you want to ramble in a little howto that's fine, but the
> main header is not the place for it.
> 
> Also please keep improvements to the header in a separate patch from
> adding Documentation/ documents.

Frankly I don't really like the iomap.h changes -- that's going to blow
up the git blame on that file, just to produce a stilted-language
manpage.

Someone who wants to port a filesystem to iomap (or write a new fs) will
need a coherent narrative (you know, with paragraphs and sentences)
about how to build this piece and that.  The rst file under Documentation/
is the place for that, not trying to mash it into a C header.

--D