Re: [PATCH] Documentation: add initial iomap kdoc

[Bagas Sanjaya <bagasdotme@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8083 bytes --]

On Thu, May 18, 2023 at 07:40:37AM -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 "...."

Everyone can distinguished those heading levels without telling them.

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

I don't see any section numbering here (forget to add one?).

> +.. contents:: Table of Contents
> +   :local:

I see the toctree before actual doc title. Maybe move down toctree below
it?

> +You can call into **iomap** for reading, ie, dealing with the filesystems's
> +``struct file_operations``:
> +
> + * ``struct file_operations.read_iter()``: note that depending on the type of read your filesystem
> +   might use ``iomap_dio_rw()`` for direct IO, generic_file_read_iter() for buffered IO and

Try to be consistent on inlining code keywords (identifiers, function names, etc).

> +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/
> +}}}

Use literal code block for above shell snippet:

---- >8 ----
diff --git a/Documentation/filesystems/iomap.rst b/Documentation/filesystems/iomap.rst
index 51be574b5fe32a..6918a4cc5e3b9b 100644
--- a/Documentation/filesystems/iomap.rst
+++ b/Documentation/filesystems/iomap.rst
@@ -213,9 +213,9 @@ 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/
-}}}
+::
+
+    ./runltp -f dio -d /mnt1/scratch/tmp/
 
 Known issues and future improvements
 ====================================

> +References
> +==========
> +
> +  *  `Presentation on iomap evolution`<https://docs.google.com/presentation/d/e/2PACX-1vSN4TmhiTu1c6HNv6_gJZFqbFZpbF7GkABllSwJw5iLnSYKkkO-etQJ3AySYEbgJA/pub?start=true&loop=false&delayms=3000&slide=id.g189cfd05063_0_185>`
> +  * `LWN review on deprecating buffer-heads <https://lwn.net/Articles/930173/>`

I don't see clickable hyperlinks on above external references, so I have to
fix them up:

---- >8 ----
diff --git a/Documentation/filesystems/iomap.rst b/Documentation/filesystems/iomap.rst
index 75716d4e2f4537..51be574b5fe32a 100644
--- a/Documentation/filesystems/iomap.rst
+++ b/Documentation/filesystems/iomap.rst
@@ -230,5 +230,5 @@ We try to document known issues that folks should be aware of with **iomap** her
 References
 ==========
 
-  *  `Presentation on iomap evolution`<https://docs.google.com/presentation/d/e/2PACX-1vSN4TmhiTu1c6HNv6_gJZFqbFZpbF7GkABllSwJw5iLnSYKkkO-etQJ3AySYEbgJA/pub?start=true&loop=false&delayms=3000&slide=id.g189cfd05063_0_185>`
-  * `LWN review on deprecating buffer-heads <https://lwn.net/Articles/930173/>`
+  *  `Presentation on iomap evolution <https://docs.google.com/presentation/d/e/2PACX-1vSN4TmhiTu1c6HNv6_gJZFqbFZpbF7GkABllSwJw5iLnSYKkkO-etQJ3AySYEbgJA/pub?start=true&loop=false&delayms=3000&slide=id.g189cfd05063_0_185>`_
+  * `LWN review on deprecating buffer-heads <https://lwn.net/Articles/930173/>`_
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index cfb724a4c65280..af5e6c0e84923f 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -25,7 +25,7 @@
  *
  * 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.
+ * 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

> -/*
> - * Flags reported by the file system from iomap_begin:
> +/**
> + * DOC:  Flags reported by the file system from iomap_begin
>   *
> - * IOMAP_F_NEW indicates that the blocks have been newly allocated and need
> - * zeroing for areas that no data is copied to.
> + * * IOMAP_F_NEW: indicates that the blocks have been newly allocated and need
> + *	zeroing for areas that no data is copied to.
>   *
> - * IOMAP_F_DIRTY indicates the inode has uncommitted metadata needed to access
> - * written data and requires fdatasync to commit them to persistent storage.
> - * This needs to take into account metadata changes that *may* be made at IO
> - * completion, such as file size updates from direct IO.
> + * * IOMAP_F_DIRTY: indicates the inode has uncommitted metadata needed to access
> + *	written data and requires fdatasync to commit them to persistent storage.
> + *	This needs to take into account metadata changes that *may* be made at IO
> + *	completion, such as file size updates from direct IO.
>   *
> - * IOMAP_F_SHARED indicates that the blocks are shared, and will need to be
> - * unshared as part a write.
> + * * IOMAP_F_SHARED: indicates that the blocks are shared, and will need to be
> + *	unshared as part a write.
>   *
> - * IOMAP_F_MERGED indicates that the iomap contains the merge of multiple block
> - * mappings.
> + * * IOMAP_F_MERGED: indicates that the iomap contains the merge of multiple block
> + *	mappings.
>   *
> - * IOMAP_F_BUFFER_HEAD indicates that the file system requires the use of
> - * buffer heads for this mapping.
> + * * IOMAP_F_BUFFER_HEAD: indicates that the file system requires the use of
> + *	buffer heads for this mapping.
>   *
> - * IOMAP_F_XATTR indicates that the iomap is for an extended attribute extent
> - * rather than a file data extent.
> + * * IOMAP_F_XATTR: indicates that the iomap is for an extended attribute extent
> + *	rather than a file data extent.
>   */

Why don't use kernel-doc comments to describe flags?

> +/**
> + * struct iomap_folio_ops - buffered writes folio folio reference count helpers
> + *
> + * A filesystem can optionally set folio_ops in a &struct iomap mapping it returns to
> + * override the default get_folio and put_folio for each folio written to.  This only applies
> + * to buffered writes as unbuffered writes will not typically have folios associated with them.
> + *
> + * @get_folio: iomap defaults to iomap_get_folio() (which calls __filemap_get_folio()) if the
> + * 	filesystem did not provide a get folio op.
> + *
> + * @put_folio: when get_folio succeeds, put_folio will always be called to do any cleanup work
> + * 	necessary. put_folio is responsible for unlocking and putting @folio.
> + *
> + * @iomap_valid: check that the cached iomap still maps correctly to the filesystem's internal
> + * 	extent map. FS internal extent maps can change while iomap is iterating a cached iomap, so
> + * 	this hook allows iomap to detect that the iomap needs to be refreshed during a long running
> + * 	write operation.
> + *
> + *	The filesystem can store internal state (e.g. a sequence number) in iomap->validity_cookie
> + *	when the iomap is first mapped to be able to detect changes between mapping time and whenever
> + *	.iomap_valid() is called.
> + *
> + *	This is called with the folio over the specified file position held locked by the iomap code.
> + *	This is useful for filesystems that have dynamic mappings (e.g. anything other than zonefs).
> + *	An example reason as to why this is necessary is writeback doesn't take the vfs locks.

Nice, this one has kernel-doc.

Thanks for review.

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]