Re: [PATCH] Documentation: add initial iomap kdoc

[Randy Dunlap <rdunlap@infradead.org>]

Hi Dave,

On 5/19/23 16:25, Dave Chinner wrote:
> On Fri, May 19, 2023 at 08:13:50AM -0700, Randy Dunlap wrote:
>>
>>
>> On 5/19/23 02:28, Bagas Sanjaya wrote:
>>>> +/**
>>>> + * 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?
>>>
>>
>> Because kernel-doc handles functions, structs, unions, and enums.
>> Not defines.
> 
> So perhaps that should be fixed first?
> 
> I seriously dislike the implication here that we should accept
> poorly/inconsistently written comments and code just to work around
> deficiencies in documentation tooling.
> 
> Either modify the code to work cleanly and consistently with the
> tooling (e.g. change the code to use enums rather than #defines), or
> fix the tools that don't work with macro definitions in a way that
> matches the existing code documentation standards.
> 
> Forcing developers, reviewers and maintainers to understand, accept
> and then maintain inconsistent crap in the code just because some
> tool they never use is deficient is pretty much my definition of an
> unacceptible engineering process.

I started looking into this. It looks like Mauro added more support
to scripts/kernel-doc for typedefs and macros while I wasn't looking.
I don't know how well it works, but it's not clear to me how we
would want this to look.

For example, take the first set of defines from <linux/iomap.h> that
Luis modified:

/*
 * Types of block ranges for iomap mappings:
 */
#define IOMAP_HOLE	0	/* no blocks allocated, need allocation */
#define IOMAP_DELALLOC	1	/* delayed allocation blocks */
#define IOMAP_MAPPED	2	/* blocks allocated at @addr */
#define IOMAP_UNWRITTEN	3	/* blocks allocated at @addr in unwritten state */
#define IOMAP_INLINE	4	/* data inline in the inode */

and Luis changed that to:

/**
 * DOC: iomap block ranges types
 *
 * * IOMAP_HOLE		- no blocks allocated, need allocation
 * * IOMAP_DELALLOC	- delayed allocation blocks
 * * IOMAP_MAPPED	- blocks allocated at @addr
 * * IOMAP_UNWRITTEN	- blocks allocated at @addr in unwritten state
 * * IOMAP_INLINE	- data inline in the inode
 */


How would we want that to look? How would we express it in kernel-doc
format?  Currently it would have to be one macro at a time I think.

/*
 * Types of block ranges for iomap mappings:
 */
/**
 * IOMAP_HOLE - no blocks allocated, need allocation
 */
#define IOMAP_HOLE	0
/**
 * IOMAP_DELALLOC - delayed allocation blocks
 */
#define IOMAP_DELALLOC	1
/**
 * IOMAP_MAPPED - blocks allocated at @addr
 */
#define IOMAP_MAPPED	2
/**
 * IOMAP_UNWRITTEN - blocks allocated at @addr in unwritten state
 */
#define IOMAP_UNWRITTEN	3
/**
 * IOMAP_INLINE - data inline in the inode
 */
#define IOMAP_INLINE	4

That's ugly to my eyes. And it doesn't collect the set of macros
together in any manner.
The modification that Luis made looks pretty good to me ATM.

-- 
~Randy