[PATCH 1/1] staging: erofs: Add function comment for erofs/super.c

[PATCH 1/1] staging: erofs: Add function comment for erofs/super.c

[arshad hussain]

Re-sending. Previous patch I put in very old kernel-newbies address.

Subject: [PATCH 1/1] staging: erofs: Add function comment for
 erofs/super.c

This patch adds functions comment for file erofs/super.c in
sphinx format.

Signed-off-by: Arshad Hussain <arshad.super@gmail.com>
---
 drivers/staging/erofs/super.c | 133 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 130 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index 15c784fba879..6d2b93d94839 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -30,6 +30,11 @@ static void init_once(void *ptr)
        inode_init_once(&vi->vfs_inode);
 }

+/*
+ * erofs_init_inode_cache(): Create & initialize inode cache
+ *
+ * Returns: 0 on Success. Errno Otherwise.
+ */
 static int __init erofs_init_inode_cache(void)
 {
        erofs_inode_cachep = kmem_cache_create("erofs_inode",
@@ -39,11 +44,21 @@ static int __init erofs_init_inode_cache(void)
        return erofs_inode_cachep != NULL ? 0 : -ENOMEM;
 }

+/*
+ * erofs_exit_inode_cache(): Destroy inode cache
+ */
 static void erofs_exit_inode_cache(void)
 {
        kmem_cache_destroy(erofs_inode_cachep);
 }

+/*
+ * alloc_inode(): Allocate and initialize inode
+ *
+ * @sb: VFS superblock structure
+ *
+ * Returns: Pointer to inode structure
+ */
 static struct inode *alloc_inode(struct super_block *sb)
 {
        struct erofs_vnode *vi =
@@ -57,6 +72,11 @@ static struct inode *alloc_inode(struct super_block *sb)
        return &vi->vfs_inode;
 }

+/*
+ * i_callback(): Release slab Memory
+ *
+ * @head: RCU callback structure
+ */
 static void i_callback(struct rcu_head *head)
 {
        struct inode *inode = container_of(head, struct inode, i_rcu);
@@ -71,11 +91,23 @@ static void i_callback(struct rcu_head *head)
        kmem_cache_free(erofs_inode_cachep, vi);
 }

+/*
+ * destroy_inode(): Release Inode Resources allocated by alloc_inode()
+ *
+ * @inode: VFS Inode structure
+ */
 static void destroy_inode(struct inode *inode)
 {
        call_rcu(&inode->i_rcu, i_callback);
 }

+/*
+ * superblock_read(): Read superblock into buffer
+ *
+ * @sb: VFS Superblock structure
+ *
+ * Returns: 0 on Success. Errno otherwise
+ */
 static int superblock_read(struct super_block *sb)
 {
        struct erofs_sb_info *sbi;
@@ -230,6 +262,14 @@ static match_table_t erofs_tokens = {
        {Opt_err, NULL}
 };

+/*
+ * parse_options(): Parse and set additional mount option for erofs
+ *
+ * @sb: VFS superblock structure
+ * @options: Mount options
+ *
+ * Returns: 0 on Success. Errno otherwise
+ */
 static int parse_options(struct super_block *sb, char *options)
 {
        substring_t args[MAX_OPT_ARGS];
@@ -351,6 +391,22 @@ static struct inode
*erofs_init_managed_cache(struct super_block *sb)

 #endif

+/*
+ * erofs_read_super(): Helper function to erofs_fill_super()
+ *
+ * @sb: VFS superblock structure
+ * @dev_name: Name of device where erofs is mounted
+ * @data: Additional mount options
+ * @silent: if TRUE print message on error.
+ *         Silent/quite otherwise
+ *
+ * Helper function to actual callback function erofs_fill_super
+ * This function sets the blocksize. Reads the superblock into
+ * buffer. Parses and sets mount options. Populates root inode
+ * and creates root directory.
+ *
+ * Returns: 0 on Success. Errno otherwise
+ */
 static int erofs_read_super(struct super_block *sb,
        const char *dev_name, void *data, int silent)
 {
@@ -473,7 +529,11 @@ static int erofs_read_super(struct super_block *sb,
 }

 /*
- * could be triggered after deactivate_locked_super()
+ * erofs_put_super(): Free superblock structure
+ *
+ * @sb: VFS superblock structure
+ *
+ * Could be triggered after deactivate_locked_super()
  * is called, thus including umount and failed to initialize.
  */
 static void erofs_put_super(struct super_block *sb)
@@ -513,7 +573,20 @@ struct erofs_mount_private {
        char *options;
 };

-/* support mount_bdev() with options */
+/*
+ * erofs_fill_super(): Fill erofs superblock
+ *
+ * @sb: VFS superblock structure
+ * @_priv: Additional Mount options
+ * @silent: if TRUE print message on error.
+ *         Silent/quite otherwise
+ *
+ * This function fills erofs superblock information into
+ * the VFS superblock stucture. Additionally it supports
+ * mount_bdev() with options
+ *
+ * Returns: 0 on Success
+ */
 static int erofs_fill_super(struct super_block *sb,
        void *_priv, int silent)
 {
@@ -523,6 +596,17 @@ static int erofs_fill_super(struct super_block *sb,
                priv->options, silent);
 }

+/*
+ * erofs_mount(): Function called when erofs is mounted
+ *
+ * @fs_type: Describes the erofs filesystem
+ * @flags: Mount flags/options
+ * @dev_name: Name of device where erofs is mounted
+ * @data: Mount options
+ *
+ * Returns: Pointer to VFS dentry structure.
+ *         The root dentry.
+ */
 static struct dentry *erofs_mount(
        struct file_system_type *fs_type, int flags,
        const char *dev_name, void *data)
@@ -532,10 +616,21 @@ static struct dentry *erofs_mount(
                .options = data
        };

+       /*
+        * Mount filesystem residing on a block device
+        * provide a callback (erofs_fill_super)  to
+        * populate superblock
+        */
        return mount_bdev(fs_type, flags, dev_name,
                &priv, erofs_fill_super);
 }

+/*
+ * erofs_kill_sb(): Called when erofs is getting shutdown/killed
+ *
+ * @sb: VFS superblock structure
+ *
+ */
 static void erofs_kill_sb(struct super_block *sb)
 {
        kill_block_super(sb);
@@ -550,6 +645,11 @@ static struct file_system_type erofs_fs_type = {
 };
 MODULE_ALIAS_FS("erofs");

+/*
+ * erofs_module_init(): Called when module is loaded
+ *
+ * returns: 0 on Success. Error Otherwise
+ */
 static int __init erofs_module_init(void)
 {
        int err;
@@ -586,6 +686,9 @@ static int __init erofs_module_init(void)
        return err;
 }

+/*
+ * erofs_module_exit(): Called when module is un-loaded
+ */
 static void __exit erofs_module_exit(void)
 {
        unregister_filesystem(&erofs_fs_type);
@@ -595,7 +698,14 @@ static void __exit erofs_module_exit(void)
        infoln("successfully finalize erofs");
 }

-/* get filesystem statistics */
+/*
+ * erofs_statfs(): Get erofs statistics
+ *
+ * @dentry: Directory from where stats is collected
+ * @buf: Buffer where stats info is stored
+ *
+ * Returns: 0 on Success
+ */
 static int erofs_statfs(struct dentry *dentry, struct kstatfs *buf)
 {
        struct super_block *sb = dentry->d_sb;
@@ -617,6 +727,14 @@ static int erofs_statfs(struct dentry *dentry,
struct kstatfs *buf)
        return 0;
 }

+/*
+ * erofs_show_options():
+ *
+ * @dentry:
+ * @buf:
+ *
+ * Returns: 0 on Success
+ */
 static int erofs_show_options(struct seq_file *seq, struct dentry *root)
 {
        struct erofs_sb_info *sbi __maybe_unused = EROFS_SB(root->d_sb);
@@ -639,6 +757,15 @@ static int erofs_show_options(struct seq_file
*seq, struct dentry *root)
        return 0;
 }

+/*
+ * erofs_remount(): Called when erofs is remounted
+ *
+ * @sb: VFS superblock structure
+ * @flags: Mount flags/options
+ * @data: Mount options
+ *
+ * Returns: 0 on Success. Errno otherwise
+ */
 static int erofs_remount(struct super_block *sb, int *flags, char *data)
 {
        struct erofs_sb_info *sbi = EROFS_SB(sb);
--
2.14.3

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: [PATCH 1/1] staging: erofs: Add function comment for erofs/super.c

[valdis.kletnieks]

On Tue, 12 Mar 2019 23:33:47 +0530, arshad hussain said:
> Re-sending. Previous patch I put in very old kernel-newbies address.
>
> Subject: [PATCH 1/1] staging: erofs: Add function comment for
>  erofs/super.c
>
> This patch adds functions comment for file erofs/super.c in
> sphinx format.
>
> Signed-off-by: Arshad Hussain <arshad.super@gmail.com>
> ---
>  drivers/staging/erofs/super.c | 133 ++++++++++++++++++++++++++++++++++++++++
+-
>  1 file changed, 130 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
> index 15c784fba879..6d2b93d94839 100644
> --- a/drivers/staging/erofs/super.c
> +++ b/drivers/staging/erofs/super.c
> @@ -30,6 +30,11 @@ static void init_once(void *ptr)
>         inode_init_once(&vi->vfs_inode);
>  }
>
> +/*

Actually, if you're trying to do sphinx/kerneldoc format, the first line of the
block comment should be '/**' with two asterisks.

Also, all kernel patches should be cc'd to linux-kernel@vger.kernel.org as well as
the maintainers and subsystem lists.


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: [PATCH 1/1] staging: erofs: Add function comment for erofs/super.c

[Greg KH]

On Tue, Mar 12, 2019 at 11:33:47PM +0530, arshad hussain wrote:
> Re-sending. Previous patch I put in very old kernel-newbies address.
> 
> Subject: [PATCH 1/1] staging: erofs: Add function comment for
>  erofs/super.c

You can't have a "Subject" line in the middle of the changelog :(

Please fix up and resend properly with git send-email and also use
scripts/get_maintainer.pl to find what lists to cc:

thanks,

greg k-h

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: [PATCH 1/1] staging: erofs: Add function comment for erofs/super.c

[Gao Xiang]

Hi Arshad,

Thanks for your patch :)
Apart from the comment from Greg, please check my following ideas...

On 2019/3/13 2:03, arshad hussain wrote:
> Re-sending. Previous patch I put in very old kernel-newbies address.
> 
> Subject: [PATCH 1/1] staging: erofs: Add function comment for
>  erofs/super.c
> 
> This patch adds functions comment for file erofs/super.c in
> sphinx format.
> 
> Signed-off-by: Arshad Hussain <arshad.super@gmail.com>
> ---
>  drivers/staging/erofs/super.c | 133 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 130 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
> index 15c784fba879..6d2b93d94839 100644
> --- a/drivers/staging/erofs/super.c
> +++ b/drivers/staging/erofs/super.c
> @@ -30,6 +30,11 @@ static void init_once(void *ptr)
>         inode_init_once(&vi->vfs_inode);
>  }
> 
> +/*
> + * erofs_init_inode_cache(): Create & initialize inode cache
> + *
> + * Returns: 0 on Success. Errno Otherwise.
> + */
>  static int __init erofs_init_inode_cache(void)
>  {
>         erofs_inode_cachep = kmem_cache_create("erofs_inode",
> @@ -39,11 +44,21 @@ static int __init erofs_init_inode_cache(void)
>         return erofs_inode_cachep != NULL ? 0 : -ENOMEM;
>  }
> 
> +/*
> + * erofs_exit_inode_cache(): Destroy inode cache
> + */
>  static void erofs_exit_inode_cache(void)
>  {
>         kmem_cache_destroy(erofs_inode_cachep);
>  }
> 
> +/*
> + * alloc_inode(): Allocate and initialize inode
> + *
> + * @sb: VFS superblock structure
> + *
> + * Returns: Pointer to inode structure
> + */

Actually I think it is not necessary to comment all the functions -- since
it's actually no special compared with other filesystems, and the function
names also indicates the real propose (these functions are not APIs), eg.

erofs_init_inode_cache(): Create & initialize inode cache
erofs_exit_inode_cache() - Destroy inode cache
alloc_inode(): Allocate and initialize inode
destroy_inode(): Release Inode Resources allocated by alloc_inode()
erofs_module_init() - Called when module is loaded
erofs_module_exit() - Called when module is un-loaded

It is useful to comment specific main functions in detail, such as:
superblock_read, parse_options, erofs_read_super, erofs_put_super,
erofs_statfs, erofs_remount (since read-only fs will add SB_RDONLY at any time).


>  static struct inode *alloc_inode(struct super_block *sb)
>  {
>         struct erofs_vnode *vi =
> @@ -57,6 +72,11 @@ static struct inode *alloc_inode(struct super_block *sb)
>         return &vi->vfs_inode;
>  }
> 
> +/*
> + * i_callback(): Release slab Memory
> + *
> + * @head: RCU callback structure
> + */

i_callback is used to free inode structure and other memory related to this inode
after the grace period in order to fulfill all rcu read requests.

"Release slab Memory" is not a very useful description for this function.

Thanks,
Gao Xiang

>  static void i_callback(struct rcu_head *head)
>  {
>         struct inode *inode = container_of(head, struct inode, i_rcu);
> @@ -71,11 +91,23 @@ static void i_callback(struct rcu_head *head)
>         kmem_cache_free(erofs_inode_cachep, vi);
>  }
> 
> +/*
> + * destroy_inode(): Release Inode Resources allocated by alloc_inode()
> + *
> + * @inode: VFS Inode structure
> + */
>  static void destroy_inode(struct inode *inode)
>  {
>         call_rcu(&inode->i_rcu, i_callback);
>  }
> 
> +/*
> + * superblock_read(): Read superblock into buffer
> + *
> + * @sb: VFS Superblock structure
> + *
> + * Returns: 0 on Success. Errno otherwise
> + */
>  static int superblock_read(struct super_block *sb)
>  {
>         struct erofs_sb_info *sbi;
> @@ -230,6 +262,14 @@ static match_table_t erofs_tokens = {
>         {Opt_err, NULL}
>  };
> 
> +/*
> + * parse_options(): Parse and set additional mount option for erofs
> + *
> + * @sb: VFS superblock structure
> + * @options: Mount options
> + *
> + * Returns: 0 on Success. Errno otherwise
> + */
>  static int parse_options(struct super_block *sb, char *options)
>  {
>         substring_t args[MAX_OPT_ARGS];
> @@ -351,6 +391,22 @@ static struct inode
> *erofs_init_managed_cache(struct super_block *sb)
> 
>  #endif
> 
> +/*
> + * erofs_read_super(): Helper function to erofs_fill_super()
> + *
> + * @sb: VFS superblock structure
> + * @dev_name: Name of device where erofs is mounted
> + * @data: Additional mount options
> + * @silent: if TRUE print message on error.
> + *         Silent/quite otherwise
> + *
> + * Helper function to actual callback function erofs_fill_super
> + * This function sets the blocksize. Reads the superblock into
> + * buffer. Parses and sets mount options. Populates root inode
> + * and creates root directory.
> + *
> + * Returns: 0 on Success. Errno otherwise
> + */
>  static int erofs_read_super(struct super_block *sb,
>         const char *dev_name, void *data, int silent)
>  {
> @@ -473,7 +529,11 @@ static int erofs_read_super(struct super_block *sb,
>  }
> 
>  /*
> - * could be triggered after deactivate_locked_super()
> + * erofs_put_super(): Free superblock structure
> + *
> + * @sb: VFS superblock structure
> + *
> + * Could be triggered after deactivate_locked_super()
>   * is called, thus including umount and failed to initialize.
>   */
>  static void erofs_put_super(struct super_block *sb)
> @@ -513,7 +573,20 @@ struct erofs_mount_private {
>         char *options;
>  };
> 
> -/* support mount_bdev() with options */
> +/*
> + * erofs_fill_super(): Fill erofs superblock
> + *
> + * @sb: VFS superblock structure
> + * @_priv: Additional Mount options
> + * @silent: if TRUE print message on error.
> + *         Silent/quite otherwise
> + *
> + * This function fills erofs superblock information into
> + * the VFS superblock stucture. Additionally it supports
> + * mount_bdev() with options
> + *
> + * Returns: 0 on Success
> + */
>  static int erofs_fill_super(struct super_block *sb,
>         void *_priv, int silent)
>  {
> @@ -523,6 +596,17 @@ static int erofs_fill_super(struct super_block *sb,
>                 priv->options, silent);
>  }
> 
> +/*
> + * erofs_mount(): Function called when erofs is mounted
> + *
> + * @fs_type: Describes the erofs filesystem
> + * @flags: Mount flags/options
> + * @dev_name: Name of device where erofs is mounted
> + * @data: Mount options
> + *
> + * Returns: Pointer to VFS dentry structure.
> + *         The root dentry.
> + */
>  static struct dentry *erofs_mount(
>         struct file_system_type *fs_type, int flags,
>         const char *dev_name, void *data)
> @@ -532,10 +616,21 @@ static struct dentry *erofs_mount(
>                 .options = data
>         };
> 
> +       /*
> +        * Mount filesystem residing on a block device
> +        * provide a callback (erofs_fill_super)  to
> +        * populate superblock
> +        */
>         return mount_bdev(fs_type, flags, dev_name,
>                 &priv, erofs_fill_super);
>  }
> 
> +/*
> + * erofs_kill_sb(): Called when erofs is getting shutdown/killed
> + *
> + * @sb: VFS superblock structure
> + *
> + */
>  static void erofs_kill_sb(struct super_block *sb)
>  {
>         kill_block_super(sb);
> @@ -550,6 +645,11 @@ static struct file_system_type erofs_fs_type = {
>  };
>  MODULE_ALIAS_FS("erofs");
> 
> +/*
> + * erofs_module_init(): Called when module is loaded
> + *
> + * returns: 0 on Success. Error Otherwise
> + */
>  static int __init erofs_module_init(void)
>  {
>         int err;
> @@ -586,6 +686,9 @@ static int __init erofs_module_init(void)
>         return err;
>  }
> 
> +/*
> + * erofs_module_exit(): Called when module is un-loaded
> + */
>  static void __exit erofs_module_exit(void)
>  {
>         unregister_filesystem(&erofs_fs_type);
> @@ -595,7 +698,14 @@ static void __exit erofs_module_exit(void)
>         infoln("successfully finalize erofs");
>  }
> 
> -/* get filesystem statistics */
> +/*
> + * erofs_statfs(): Get erofs statistics
> + *
> + * @dentry: Directory from where stats is collected
> + * @buf: Buffer where stats info is stored
> + *
> + * Returns: 0 on Success
> + */
>  static int erofs_statfs(struct dentry *dentry, struct kstatfs *buf)
>  {
>         struct super_block *sb = dentry->d_sb;
> @@ -617,6 +727,14 @@ static int erofs_statfs(struct dentry *dentry,
> struct kstatfs *buf)
>         return 0;
>  }
> 
> +/*
> + * erofs_show_options():
> + *
> + * @dentry:
> + * @buf:
> + *
> + * Returns: 0 on Success
> + */
>  static int erofs_show_options(struct seq_file *seq, struct dentry *root)
>  {
>         struct erofs_sb_info *sbi __maybe_unused = EROFS_SB(root->d_sb);
> @@ -639,6 +757,15 @@ static int erofs_show_options(struct seq_file
> *seq, struct dentry *root)
>         return 0;
>  }
> 
> +/*
> + * erofs_remount(): Called when erofs is remounted
> + *
> + * @sb: VFS superblock structure
> + * @flags: Mount flags/options
> + * @data: Mount options
> + *
> + * Returns: 0 on Success. Errno otherwise
> + */
>  static int erofs_remount(struct super_block *sb, int *flags, char *data)
>  {
>         struct erofs_sb_info *sbi = EROFS_SB(sb);
> --
> 2.14.3
> 

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: [PATCH 1/1] staging: erofs: Add function comment for erofs/super.c

[valdis.kletnieks]

On Wed, 13 Mar 2019 08:48:26 +0800, Gao Xiang said:
(Reversing order of  comments)
> "Release slab Memory" is not a very useful description for this function.

Agreed.

> i_callback is used to free inode structure and other memory related to this inode

However, that's not a bad description at all. :)

(Though I'd consider renaming the function to 'reap_inode_mem()' or something
like that so no description is needed. :)




_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: [PATCH 1/1] staging: erofs: Add function comment for erofs/super.c

[Gao Xiang]

Hi Valdis,

On 2019/3/13 14:41, valdis.kletnieks@vt.edu wrote:
> On Wed, 13 Mar 2019 08:48:26 +0800, Gao Xiang said:
> (Reversing order of  comments)
>> "Release slab Memory" is not a very useful description for this function.
> 
> Agreed.
> 
>> i_callback is used to free inode structure and other memory related to this inode
> 
> However, that's not a bad description at all. :)
> 
> (Though I'd consider renaming the function to 'reap_inode_mem()' or something
> like that so no description is needed. :)

i_callback is an common function names for this usage, see:

reiserfs/super.c
653:static void reiserfs_i_callback(struct rcu_head *head)
661:    call_rcu(&inode->i_rcu, reiserfs_i_callback);

romfs/super.c
384:static void romfs_i_callback(struct rcu_head *head)
393:    call_rcu(&inode->i_rcu, romfs_i_callback);

hfsplus/super.c
631:static void hfsplus_i_callback(struct rcu_head *head)
640:    call_rcu(&inode->i_rcu, hfsplus_i_callback);

qnx4/inode.c
345:static void qnx4_i_callback(struct rcu_head *head)
353:    call_rcu(&inode->i_rcu, qnx4_i_callback);

gfs2/super.c
1740:static void gfs2_i_callback(struct rcu_head *head)
1748:   call_rcu(&inode->i_rcu, gfs2_i_callback);

minix/inode.c
71:static void minix_i_callback(struct rcu_head *head)
79:     call_rcu(&inode->i_rcu, minix_i_callback);

jfs/super.c
127:static void jfs_i_callback(struct rcu_head *head)
147:    call_rcu(&inode->i_rcu, jfs_i_callback);

udf/super.c
164:static void udf_i_callback(struct rcu_head *head)
172:    call_rcu(&inode->i_rcu, udf_i_callback);

ufs/super.c
1452:static void ufs_i_callback(struct rcu_head *head)
1460:   call_rcu(&inode->i_rcu, ufs_i_callback);

openpromfs/inode.c
339:static void openprom_i_callback(struct rcu_head *head)
347:    call_rcu(&inode->i_rcu, openprom_i_callback);

ubifs/super.c
275:static void ubifs_i_callback(struct rcu_head *head)
287:    call_rcu(&inode->i_rcu, ubifs_i_callback);

Therefore fs guy should know what it does by the name convension :)

Anyway, I think this patch is useful with some modification *thumb*

Thanks,
Gao Xiang

> 
> 
> 

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies