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

[Jonathan Corbet <corbet@lwn.net>]

Luis Chamberlain <mcgrof@kernel.org> writes:

> To help with iomap adoption / porting I set out the goal to try to
> help improve the iomap documentation and get general guidance for
> filesystem conversions over from buffer-head in time for this year's
> LSFMM. The end results thanks to the review of Darrick, Christoph and
> others is on the kernelnewbies wiki [0].
>
> This brings this forward a relevant subset of that documentation to
> the kernel in kdoc format and also kdoc'ifies the existing documentation
> on iomap.h.

OK, I've had a read through it.  Thanks again for doing it, we
definitely need this.  There are typos and such that Randy has already
pointed out, so I won't bother with those.  My main comment is mainly a
high level one, along with a handful of nits.

My high-level question is: who is the audience for this document?  I'm
guessing it's filesystem developers?  Whoever it is, the document could
benefit from an introductory section, aimed at that audience, saying how
to get *started* with iomap.  What include files do I need?  How do I
provide a set of iomap callbacks and make them visible to the VFS?
Without that sort of stuff, it makes for a rough jumping-in experience.

The nits:

> diff --git a/Documentation/filesystems/iomap.rst b/Documentation/filesystems/iomap.rst
> new file mode 100644
> index 000000000000..be487030fcff
> --- /dev/null
> +++ b/Documentation/filesystems/iomap.rst
> @@ -0,0 +1,253 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +.. _iomap:

I don't think you use this label anywhere, so it doesn't need to be here.

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

We have a set of conventions for section headings, nicely documented in
Documentation/doc-guide/sphinx.rst.  This hierarchy doesn't quite match
it, but you don't get far enough into it to hit the differences.  I'd
just take this out.

> +
> +        Sections are manually numbered because apparently that's what everyone
> +        does in the kernel.

The sections are *not* manually numbered, which I think is entirely
fine.  But that makes this comment a bit weird.

> +.. contents:: Table of Contents
> +   :local:
> +
> +=====
> +iomap
> +=====
> +
> +.. kernel-doc:: include/linux/iomap.h
> +
> +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.

This text (and the document as a whole) is a bit heavy on the markup.
I'd consider taking some of it out to improve the plain-text readability.

[...]

> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index e2b836c2e119..ee4b026995ac 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -10,6 +10,30 @@
>  #include <linux/mm_types.h>
>  #include <linux/blkdev.h>
>  
> +/**
> + * 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**.

As much as I love to see LWN references embedded in as many kernel files
as possible, I'm honestly not sure that the iomap documentation needs to
talk about buffer heads at all - except maybe for help in replacing
them.  In particular, I don't think that the documentation needs to
justify iomap's existence; we can look at the commit logs if we need to
remind ourselves of that.

Thanks,

jon