[Ocfs2-devel] [PATCH 0/5] ocfs2: sysfs and cleanup

[Ocfs2-devel] [PATCH 0/5] ocfs2: sysfs and cleanup

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

This adds /sys/fs/ocfs2/<s_id> to the sys filesystem. All other
files are added in this directory. This is done by:
1. Adding kobject in the ocfs2_super structure
2. Using ocfs2_sb_attr which encompass all the show() and store() functions

This also reduce code and does away with unnecessary data structure,
but adds some in the ocfs2_super for house keeping.

Hopefully, this would be used to further add configurable parameters
for the super block.

Suggestions/Criticism welcome.

-- 
Goldwyn

[Ocfs2-devel] [PATCH 1/5] ocfs2: Provisions for sysfs entries

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

This adds /sys/fs/ocfs2/<s_id> to the sys filesystem. This
is done by adding the kobj into the ocfs2_super. All other
files are added in this directory.

Introduce ocfs2_sb_attr which encompasses the store() and show() functions.
Move all the important data structures with respect to filechecks
to ocfs2_super.

More superblock information should move in here.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/ocfs2/Makefile |  3 +-
 fs/ocfs2/ocfs2.h  |  4 +++
 fs/ocfs2/super.c  |  7 +++--
 fs/ocfs2/sysfs.c  | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ocfs2/sysfs.h  |  9 ++++++
 5 files changed, 111 insertions(+), 4 deletions(-)
 create mode 100644 fs/ocfs2/sysfs.c
 create mode 100644 fs/ocfs2/sysfs.h

diff --git a/fs/ocfs2/Makefile b/fs/ocfs2/Makefile
index e27e652..716ed45 100644
--- a/fs/ocfs2/Makefile
+++ b/fs/ocfs2/Makefile
@@ -41,7 +41,8 @@ ocfs2-objs := \
 	quota_local.o		\
 	quota_global.o		\
 	xattr.o			\
-	acl.o	\
+	acl.o			\
+	sysfs.o			\
 	filecheck.o
 
 ocfs2_stackglue-objs := stackglue.o
diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
index e63af7d..8e66cdf 100644
--- a/fs/ocfs2/ocfs2.h
+++ b/fs/ocfs2/ocfs2.h
@@ -37,6 +37,7 @@
 #include <linux/mutex.h>
 #include <linux/lockdep.h>
 #include <linux/jbd2.h>
+#include <linux/kobject.h>
 
 /* For union ocfs2_dlm_lksb */
 #include "stackglue.h"
@@ -472,6 +473,9 @@ struct ocfs2_super
 	 * workqueue and schedule on our own.
 	 */
 	struct workqueue_struct *ocfs2_wq;
+
+	struct kobject kobj;
+	struct completion kobj_unregister;
 };
 
 #define OCFS2_SB(sb)	    ((struct ocfs2_super *)(sb)->s_fs_info)
diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index d7cae33..96b7a9f 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -75,6 +75,7 @@
 
 #include "buffer_head_io.h"
 #include "filecheck.h"
+#include "sysfs.h"
 
 static struct kmem_cache *ocfs2_inode_cachep;
 struct kmem_cache *ocfs2_dquot_cachep;
@@ -1200,8 +1201,8 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent)
 	/* Start this when the mount is almost sure of being successful */
 	ocfs2_orphan_scan_start(osb);
 
-	/* Create filecheck sysfile /sys/fs/ocfs2/<devname>/filecheck */
-	ocfs2_filecheck_create_sysfs(sb);
+	/* Create sysfs entries */
+	ocfs2_sysfs_sb_init(sb);
 
 	return status;
 
@@ -1651,9 +1652,9 @@ static void ocfs2_put_super(struct super_block *sb)
 {
 	trace_ocfs2_put_super(sb);
 
+	ocfs2_sysfs_sb_exit(sb);
 	ocfs2_sync_blockdev(sb);
 	ocfs2_dismount_volume(sb, 0);
-	ocfs2_filecheck_remove_sysfs(sb);
 }
 
 static int ocfs2_statfs(struct dentry *dentry, struct kstatfs *buf)
diff --git a/fs/ocfs2/sysfs.c b/fs/ocfs2/sysfs.c
new file mode 100644
index 0000000..f0b7435
--- /dev/null
+++ b/fs/ocfs2/sysfs.c
@@ -0,0 +1,92 @@
+#include "ocfs2.h"
+#include "sysfs.h"
+
+struct ocfs2_sb_attr {
+	struct attribute attr;
+	ssize_t (*show)(struct ocfs2_super *, struct ocfs2_sb_attr *,
+			char *buf);
+	ssize_t (*store)(struct ocfs2_super *, struct ocfs2_sb_attr *,
+			const char *buf, size_t count);
+};
+
+#define OCFS2_SB_ATTR(_name, _mode) \
+struct ocfs2_sb_attr sb_attr_##_name = __ATTR(name, _mode, _show, _store)
+
+#define OCFS2_SB_ATTR_RO(_name) \
+struct ocfs2_sb_attr sb_attr_##_name = __ATTR_RO(_name)
+
+static ssize_t ocfs2_sb_attr_show(struct kobject *kobj,
+		struct attribute *attr, char *buf)
+{
+	struct ocfs2_sb_attr *oa =
+		container_of(attr, struct ocfs2_sb_attr, attr);
+	struct ocfs2_super *osb = container_of(kobj, struct ocfs2_super, kobj);
+	if (!oa->show)
+		return -EIO;
+
+	return oa->show(osb, oa, buf);
+}
+
+static ssize_t ocfs2_sb_attr_store(struct kobject *kobj,
+		struct attribute *attr, const char *buf, size_t count)
+{
+	struct ocfs2_sb_attr *oa =
+		container_of(attr, struct ocfs2_sb_attr, attr);
+	struct ocfs2_super *osb = container_of(kobj, struct ocfs2_super, kobj);
+	if (!oa->show)
+		return -EIO;
+
+	return oa->store(osb, oa, buf, count);
+}
+
+static ssize_t slot_num_show(struct ocfs2_super *osb,
+			     struct ocfs2_sb_attr *attr,
+	  		     char *buf)
+{
+	return sprintf(buf, "%d\n", osb->slot_num);
+}
+
+static void sb_release(struct kobject *kobj)
+{
+	struct ocfs2_super *osb = container_of(kobj, struct ocfs2_super, kobj);
+	complete(&osb->kobj_unregister);
+}
+
+static const struct sysfs_ops ocfs2_sb_sysfs_ops = {
+	.show = ocfs2_sb_attr_show,
+	.store = ocfs2_sb_attr_store,
+};
+
+static OCFS2_SB_ATTR_RO(slot_num);
+static struct attribute *ocfs2_sb_attrs[] = {
+	&sb_attr_slot_num.attr,
+	NULL
+};
+
+static struct kobj_type ocfs2_sb_ktype = {
+	.sysfs_ops 	= &ocfs2_sb_sysfs_ops,
+	.default_attrs 	= ocfs2_sb_attrs,
+	.release	= sb_release,
+};
+
+
+int ocfs2_sysfs_sb_init(struct super_block *sb)
+{
+	int err;
+	struct ocfs2_super *osb = OCFS2_SB(sb);
+	init_completion(&osb->kobj_unregister);
+	osb->kobj.kset = ocfs2_kset;
+	err = kobject_init_and_add(&osb->kobj, &ocfs2_sb_ktype, NULL, "%s", sb->s_id);
+	return err;
+
+}
+
+void ocfs2_sysfs_sb_exit(struct super_block *sb)
+{
+	struct ocfs2_super *osb = OCFS2_SB(sb);
+	kobject_del(&osb->kobj);
+	kobject_put(&osb->kobj);
+	wait_for_completion(&osb->kobj_unregister);
+}
+
+
diff --git a/fs/ocfs2/sysfs.h b/fs/ocfs2/sysfs.h
new file mode 100644
index 0000000..d929ac1
--- /dev/null
+++ b/fs/ocfs2/sysfs.h
@@ -0,0 +1,9 @@
+
+
+#ifndef _SYS_H
+#define _SYS_H
+
+int ocfs2_sysfs_sb_init(struct super_block *sb);
+void ocfs2_sysfs_sb_exit(struct super_block *sb);
+
+#endif
-- 
2.6.6

[Ocfs2-devel] [PATCH 1/5] ocfs2: Provisions for sysfs entries

[Joseph Qi]

On 2016/5/11 1:52, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> This adds /sys/fs/ocfs2/<s_id> to the sys filesystem. This
> is done by adding the kobj into the ocfs2_super. All other
> files are added in this directory.
> 
> Introduce ocfs2_sb_attr which encompasses the store() and show() functions.
> Move all the important data structures with respect to filechecks
> to ocfs2_super.
> 
> More superblock information should move in here.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/ocfs2/Makefile |  3 +-
>  fs/ocfs2/ocfs2.h  |  4 +++
>  fs/ocfs2/super.c  |  7 +++--
>  fs/ocfs2/sysfs.c  | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/ocfs2/sysfs.h  |  9 ++++++
>  5 files changed, 111 insertions(+), 4 deletions(-)
>  create mode 100644 fs/ocfs2/sysfs.c
>  create mode 100644 fs/ocfs2/sysfs.h
> 
> diff --git a/fs/ocfs2/Makefile b/fs/ocfs2/Makefile
> index e27e652..716ed45 100644
> --- a/fs/ocfs2/Makefile
> +++ b/fs/ocfs2/Makefile
> @@ -41,7 +41,8 @@ ocfs2-objs := \
>  	quota_local.o		\
>  	quota_global.o		\
>  	xattr.o			\
> -	acl.o	\
> +	acl.o			\
> +	sysfs.o			\
>  	filecheck.o
>  
>  ocfs2_stackglue-objs := stackglue.o
> diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
> index e63af7d..8e66cdf 100644
> --- a/fs/ocfs2/ocfs2.h
> +++ b/fs/ocfs2/ocfs2.h
> @@ -37,6 +37,7 @@
>  #include <linux/mutex.h>
>  #include <linux/lockdep.h>
>  #include <linux/jbd2.h>
> +#include <linux/kobject.h>
>  
>  /* For union ocfs2_dlm_lksb */
>  #include "stackglue.h"
> @@ -472,6 +473,9 @@ struct ocfs2_super
>  	 * workqueue and schedule on our own.
>  	 */
>  	struct workqueue_struct *ocfs2_wq;
> +
> +	struct kobject kobj;
> +	struct completion kobj_unregister;
>  };
>  
>  #define OCFS2_SB(sb)	    ((struct ocfs2_super *)(sb)->s_fs_info)
> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
> index d7cae33..96b7a9f 100644
> --- a/fs/ocfs2/super.c
> +++ b/fs/ocfs2/super.c
> @@ -75,6 +75,7 @@
>  
>  #include "buffer_head_io.h"
>  #include "filecheck.h"
> +#include "sysfs.h"
>  
>  static struct kmem_cache *ocfs2_inode_cachep;
>  struct kmem_cache *ocfs2_dquot_cachep;
> @@ -1200,8 +1201,8 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent)
>  	/* Start this when the mount is almost sure of being successful */
>  	ocfs2_orphan_scan_start(osb);
>  
> -	/* Create filecheck sysfile /sys/fs/ocfs2/<devname>/filecheck */
> -	ocfs2_filecheck_create_sysfs(sb);
> +	/* Create sysfs entries */
> +	ocfs2_sysfs_sb_init(sb);
>  
>  	return status;
>  
> @@ -1651,9 +1652,9 @@ static void ocfs2_put_super(struct super_block *sb)
>  {
>  	trace_ocfs2_put_super(sb);
>  
> +	ocfs2_sysfs_sb_exit(sb);
>  	ocfs2_sync_blockdev(sb);
>  	ocfs2_dismount_volume(sb, 0);
> -	ocfs2_filecheck_remove_sysfs(sb);
>  }
>  
>  static int ocfs2_statfs(struct dentry *dentry, struct kstatfs *buf)
> diff --git a/fs/ocfs2/sysfs.c b/fs/ocfs2/sysfs.c
> new file mode 100644
> index 0000000..f0b7435
> --- /dev/null
> +++ b/fs/ocfs2/sysfs.c
> @@ -0,0 +1,92 @@
> +#include "ocfs2.h"
> +#include "sysfs.h"
> +
> +struct ocfs2_sb_attr {
> +	struct attribute attr;
> +	ssize_t (*show)(struct ocfs2_super *, struct ocfs2_sb_attr *,
> +			char *buf);
> +	ssize_t (*store)(struct ocfs2_super *, struct ocfs2_sb_attr *,
> +			const char *buf, size_t count);
> +};
> +
> +#define OCFS2_SB_ATTR(_name, _mode) \
> +struct ocfs2_sb_attr sb_attr_##_name = __ATTR(name, _mode, _show, _store)
> +
> +#define OCFS2_SB_ATTR_RO(_name) \
> +struct ocfs2_sb_attr sb_attr_##_name = __ATTR_RO(_name)
> +
> +static ssize_t ocfs2_sb_attr_show(struct kobject *kobj,
> +		struct attribute *attr, char *buf)
> +{
> +	struct ocfs2_sb_attr *oa =
> +		container_of(attr, struct ocfs2_sb_attr, attr);
> +	struct ocfs2_super *osb = container_of(kobj, struct ocfs2_super, kobj);
> +	if (!oa->show)
> +		return -EIO;
> +
> +	return oa->show(osb, oa, buf);
> +}
> +
> +static ssize_t ocfs2_sb_attr_store(struct kobject *kobj,
> +		struct attribute *attr, const char *buf, size_t count)
> +{
> +	struct ocfs2_sb_attr *oa =
> +		container_of(attr, struct ocfs2_sb_attr, attr);
> +	struct ocfs2_super *osb = container_of(kobj, struct ocfs2_super, kobj);
> +	if (!oa->show)
Should it be "!oa->store" here?

Thanks,
Joseph

> +		return -EIO;
> +
> +	return oa->store(osb, oa, buf, count);
> +}
> +
> +static ssize_t slot_num_show(struct ocfs2_super *osb,
> +			     struct ocfs2_sb_attr *attr,
> +	  		     char *buf)
> +{
> +	return sprintf(buf, "%d\n", osb->slot_num);
> +}
> +
> +static void sb_release(struct kobject *kobj)
> +{
> +	struct ocfs2_super *osb = container_of(kobj, struct ocfs2_super, kobj);
> +	complete(&osb->kobj_unregister);
> +}
> +
> +static const struct sysfs_ops ocfs2_sb_sysfs_ops = {
> +	.show = ocfs2_sb_attr_show,
> +	.store = ocfs2_sb_attr_store,
> +};
> +
> +static OCFS2_SB_ATTR_RO(slot_num);
> +static struct attribute *ocfs2_sb_attrs[] = {
> +	&sb_attr_slot_num.attr,
> +	NULL
> +};
> +
> +static struct kobj_type ocfs2_sb_ktype = {
> +	.sysfs_ops 	= &ocfs2_sb_sysfs_ops,
> +	.default_attrs 	= ocfs2_sb_attrs,
> +	.release	= sb_release,
> +};
> +
> +
> +int ocfs2_sysfs_sb_init(struct super_block *sb)
> +{
> +	int err;
> +	struct ocfs2_super *osb = OCFS2_SB(sb);
> +	init_completion(&osb->kobj_unregister);
> +	osb->kobj.kset = ocfs2_kset;
> +	err = kobject_init_and_add(&osb->kobj, &ocfs2_sb_ktype, NULL, "%s", sb->s_id);
> +	return err;
> +
> +}
> +
> +void ocfs2_sysfs_sb_exit(struct super_block *sb)
> +{
> +	struct ocfs2_super *osb = OCFS2_SB(sb);
> +	kobject_del(&osb->kobj);
> +	kobject_put(&osb->kobj);
> +	wait_for_completion(&osb->kobj_unregister);
> +}
> +
> +
> diff --git a/fs/ocfs2/sysfs.h b/fs/ocfs2/sysfs.h
> new file mode 100644
> index 0000000..d929ac1
> --- /dev/null
> +++ b/fs/ocfs2/sysfs.h
> @@ -0,0 +1,9 @@
> +
> +
> +#ifndef _SYS_H
> +#define _SYS_H
> +
> +int ocfs2_sysfs_sb_init(struct super_block *sb);
> +void ocfs2_sysfs_sb_exit(struct super_block *sb);
> +
> +#endif
> 

[Ocfs2-devel] [PATCH 1/5] ocfs2: Provisions for sysfs entries

[Goldwyn Rodrigues]

On 05/11/2016 05:53 AM, Joseph Qi wrote:
> On 2016/5/11 1:52, Goldwyn Rodrigues wrote:
>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>
>> This adds /sys/fs/ocfs2/<s_id> to the sys filesystem. This
>> is done by adding the kobj into the ocfs2_super. All other
>> files are added in this directory.
>>
>> Introduce ocfs2_sb_attr which encompasses the store() and show() functions.
>> Move all the important data structures with respect to filechecks
>> to ocfs2_super.
>>
>> More superblock information should move in here.
>>
>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>> ---
>>   fs/ocfs2/Makefile |  3 +-
>>   fs/ocfs2/ocfs2.h  |  4 +++
>>   fs/ocfs2/super.c  |  7 +++--
>>   fs/ocfs2/sysfs.c  | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   fs/ocfs2/sysfs.h  |  9 ++++++
>>   5 files changed, 111 insertions(+), 4 deletions(-)
>>   create mode 100644 fs/ocfs2/sysfs.c
>>   create mode 100644 fs/ocfs2/sysfs.h
>>
>> diff --git a/fs/ocfs2/Makefile b/fs/ocfs2/Makefile
>> index e27e652..716ed45 100644
>> --- a/fs/ocfs2/Makefile
>> +++ b/fs/ocfs2/Makefile
>> @@ -41,7 +41,8 @@ ocfs2-objs := \
>>   	quota_local.o		\
>>   	quota_global.o		\
>>   	xattr.o			\
>> -	acl.o	\
>> +	acl.o			\
>> +	sysfs.o			\
>>   	filecheck.o
>>
>>   ocfs2_stackglue-objs := stackglue.o
>> diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
>> index e63af7d..8e66cdf 100644
>> --- a/fs/ocfs2/ocfs2.h
>> +++ b/fs/ocfs2/ocfs2.h
>> @@ -37,6 +37,7 @@
>>   #include <linux/mutex.h>
>>   #include <linux/lockdep.h>
>>   #include <linux/jbd2.h>
>> +#include <linux/kobject.h>
>>
>>   /* For union ocfs2_dlm_lksb */
>>   #include "stackglue.h"
>> @@ -472,6 +473,9 @@ struct ocfs2_super
>>   	 * workqueue and schedule on our own.
>>   	 */
>>   	struct workqueue_struct *ocfs2_wq;
>> +
>> +	struct kobject kobj;
>> +	struct completion kobj_unregister;
>>   };
>>
>>   #define OCFS2_SB(sb)	    ((struct ocfs2_super *)(sb)->s_fs_info)
>> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
>> index d7cae33..96b7a9f 100644
>> --- a/fs/ocfs2/super.c
>> +++ b/fs/ocfs2/super.c
>> @@ -75,6 +75,7 @@
>>
>>   #include "buffer_head_io.h"
>>   #include "filecheck.h"
>> +#include "sysfs.h"
>>
>>   static struct kmem_cache *ocfs2_inode_cachep;
>>   struct kmem_cache *ocfs2_dquot_cachep;
>> @@ -1200,8 +1201,8 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent)
>>   	/* Start this when the mount is almost sure of being successful */
>>   	ocfs2_orphan_scan_start(osb);
>>
>> -	/* Create filecheck sysfile /sys/fs/ocfs2/<devname>/filecheck */
>> -	ocfs2_filecheck_create_sysfs(sb);
>> +	/* Create sysfs entries */
>> +	ocfs2_sysfs_sb_init(sb);
>>
>>   	return status;
>>
>> @@ -1651,9 +1652,9 @@ static void ocfs2_put_super(struct super_block *sb)
>>   {
>>   	trace_ocfs2_put_super(sb);
>>
>> +	ocfs2_sysfs_sb_exit(sb);
>>   	ocfs2_sync_blockdev(sb);
>>   	ocfs2_dismount_volume(sb, 0);
>> -	ocfs2_filecheck_remove_sysfs(sb);
>>   }
>>
>>   static int ocfs2_statfs(struct dentry *dentry, struct kstatfs *buf)
>> diff --git a/fs/ocfs2/sysfs.c b/fs/ocfs2/sysfs.c
>> new file mode 100644
>> index 0000000..f0b7435
>> --- /dev/null
>> +++ b/fs/ocfs2/sysfs.c
>> @@ -0,0 +1,92 @@
>> +#include "ocfs2.h"
>> +#include "sysfs.h"
>> +
>> +struct ocfs2_sb_attr {
>> +	struct attribute attr;
>> +	ssize_t (*show)(struct ocfs2_super *, struct ocfs2_sb_attr *,
>> +			char *buf);
>> +	ssize_t (*store)(struct ocfs2_super *, struct ocfs2_sb_attr *,
>> +			const char *buf, size_t count);
>> +};
>> +
>> +#define OCFS2_SB_ATTR(_name, _mode) \
>> +struct ocfs2_sb_attr sb_attr_##_name = __ATTR(name, _mode, _show, _store)
>> +
>> +#define OCFS2_SB_ATTR_RO(_name) \
>> +struct ocfs2_sb_attr sb_attr_##_name = __ATTR_RO(_name)
>> +
>> +static ssize_t ocfs2_sb_attr_show(struct kobject *kobj,
>> +		struct attribute *attr, char *buf)
>> +{
>> +	struct ocfs2_sb_attr *oa =
>> +		container_of(attr, struct ocfs2_sb_attr, attr);
>> +	struct ocfs2_super *osb = container_of(kobj, struct ocfs2_super, kobj);
>> +	if (!oa->show)
>> +		return -EIO;
>> +
>> +	return oa->show(osb, oa, buf);
>> +}
>> +
>> +static ssize_t ocfs2_sb_attr_store(struct kobject *kobj,
>> +		struct attribute *attr, const char *buf, size_t count)
>> +{
>> +	struct ocfs2_sb_attr *oa =
>> +		container_of(attr, struct ocfs2_sb_attr, attr);
>> +	struct ocfs2_super *osb = container_of(kobj, struct ocfs2_super, kobj);
>> +	if (!oa->show)
> Should it be "!oa->store" here?
>

Yes, you are right. Thanks for pointing it out. I will fix this.

-- 
Goldwyn

[Ocfs2-devel] [PATCH 2/5] Use the sysfs interface for creating filecheck files

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

This reduces the code base and removes unnecessary data structures
for filecheck information since all information is stored in ocfs2_super
now.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/ocfs2/filecheck.c | 440 ++++++---------------------------------------------
 fs/ocfs2/filecheck.h |  10 ++
 fs/ocfs2/ocfs2.h     |   7 +
 fs/ocfs2/super.c     |   7 +
 fs/ocfs2/sysfs.c     |  91 ++++++++++-
 5 files changed, 161 insertions(+), 394 deletions(-)

diff --git a/fs/ocfs2/filecheck.c b/fs/ocfs2/filecheck.c
index 2cabbcf..0b41967 100644
--- a/fs/ocfs2/filecheck.c
+++ b/fs/ocfs2/filecheck.c
@@ -17,15 +17,7 @@
  * General Public License for more details.
  */
 
-#include <linux/list.h>
-#include <linux/spinlock.h>
-#include <linux/module.h>
-#include <linux/slab.h>
-#include <linux/kmod.h>
 #include <linux/fs.h>
-#include <linux/kobject.h>
-#include <linux/sysfs.h>
-#include <linux/sysctl.h>
 #include <cluster/masklog.h>
 
 #include "ocfs2.h"
@@ -53,36 +45,6 @@ static const char * const ocfs2_filecheck_errs[] = {
 	"UNSUPPORTED"
 };
 
-static DEFINE_SPINLOCK(ocfs2_filecheck_sysfs_lock);
-static LIST_HEAD(ocfs2_filecheck_sysfs_list);
-
-struct ocfs2_filecheck {
-	struct list_head fc_head;	/* File check entry list head */
-	spinlock_t fc_lock;
-	unsigned int fc_max;	/* Maximum number of entry in list */
-	unsigned int fc_size;	/* Current entry count in list */
-	unsigned int fc_done;	/* Finished entry count in list */
-};
-
-struct ocfs2_filecheck_sysfs_entry {	/* sysfs entry per mounting */
-	struct list_head fs_list;
-	atomic_t fs_count;
-	struct super_block *fs_sb;
-	struct kset *fs_devicekset;
-	struct kset *fs_fcheckkset;
-	struct ocfs2_filecheck *fs_fcheck;
-};
-
-#define OCFS2_FILECHECK_MAXSIZE		100
-#define OCFS2_FILECHECK_MINSIZE		10
-
-/* File check operation type */
-enum {
-	OCFS2_FILECHECK_TYPE_CHK = 0,	/* Check a file(inode) */
-	OCFS2_FILECHECK_TYPE_FIX,	/* Fix a file(inode) */
-	OCFS2_FILECHECK_TYPE_SET = 100	/* Set entry list maximum size */
-};
-
 struct ocfs2_filecheck_entry {
 	struct list_head fe_list;
 	unsigned long fe_ino;
@@ -91,14 +53,6 @@ struct ocfs2_filecheck_entry {
 	unsigned int fe_status:31;
 };
 
-struct ocfs2_filecheck_args {
-	unsigned int fa_type;
-	union {
-		unsigned long fa_ino;
-		unsigned int fa_len;
-	};
-};
-
 static const char *
 ocfs2_filecheck_error(int errno)
 {
@@ -110,321 +64,51 @@ ocfs2_filecheck_error(int errno)
 	return ocfs2_filecheck_errs[errno - OCFS2_FILECHECK_ERR_START + 1];
 }
 
-static ssize_t ocfs2_filecheck_show(struct kobject *kobj,
-				    struct kobj_attribute *attr,
-				    char *buf);
-static ssize_t ocfs2_filecheck_store(struct kobject *kobj,
-				     struct kobj_attribute *attr,
-				     const char *buf, size_t count);
-static struct kobj_attribute ocfs2_attr_filecheck_chk =
-					__ATTR(check, S_IRUSR | S_IWUSR,
-					ocfs2_filecheck_show,
-					ocfs2_filecheck_store);
-static struct kobj_attribute ocfs2_attr_filecheck_fix =
-					__ATTR(fix, S_IRUSR | S_IWUSR,
-					ocfs2_filecheck_show,
-					ocfs2_filecheck_store);
-static struct kobj_attribute ocfs2_attr_filecheck_set =
-					__ATTR(set, S_IRUSR | S_IWUSR,
-					ocfs2_filecheck_show,
-					ocfs2_filecheck_store);
-
-static int ocfs2_filecheck_sysfs_wait(atomic_t *p)
-{
-	schedule();
-	return 0;
-}
-
-static void
-ocfs2_filecheck_sysfs_free(struct ocfs2_filecheck_sysfs_entry *entry)
-{
-	struct ocfs2_filecheck_entry *p;
-
-	if (!atomic_dec_and_test(&entry->fs_count))
-		wait_on_atomic_t(&entry->fs_count, ocfs2_filecheck_sysfs_wait,
-				 TASK_UNINTERRUPTIBLE);
-
-	spin_lock(&entry->fs_fcheck->fc_lock);
-	while (!list_empty(&entry->fs_fcheck->fc_head)) {
-		p = list_first_entry(&entry->fs_fcheck->fc_head,
-				     struct ocfs2_filecheck_entry, fe_list);
-		list_del(&p->fe_list);
-		BUG_ON(!p->fe_done); /* To free a undone file check entry */
-		kfree(p);
-	}
-	spin_unlock(&entry->fs_fcheck->fc_lock);
-
-	kset_unregister(entry->fs_fcheckkset);
-	kset_unregister(entry->fs_devicekset);
-	kfree(entry->fs_fcheck);
-	kfree(entry);
-}
-
-static void
-ocfs2_filecheck_sysfs_add(struct ocfs2_filecheck_sysfs_entry *entry)
-{
-	spin_lock(&ocfs2_filecheck_sysfs_lock);
-	list_add_tail(&entry->fs_list, &ocfs2_filecheck_sysfs_list);
-	spin_unlock(&ocfs2_filecheck_sysfs_lock);
-}
-
-static int ocfs2_filecheck_sysfs_del(const char *devname)
-{
-	struct ocfs2_filecheck_sysfs_entry *p;
-
-	spin_lock(&ocfs2_filecheck_sysfs_lock);
-	list_for_each_entry(p, &ocfs2_filecheck_sysfs_list, fs_list) {
-		if (!strcmp(p->fs_sb->s_id, devname)) {
-			list_del(&p->fs_list);
-			spin_unlock(&ocfs2_filecheck_sysfs_lock);
-			ocfs2_filecheck_sysfs_free(p);
-			return 0;
-		}
-	}
-	spin_unlock(&ocfs2_filecheck_sysfs_lock);
-	return 1;
-}
-
-static void
-ocfs2_filecheck_sysfs_put(struct ocfs2_filecheck_sysfs_entry *entry)
-{
-	if (atomic_dec_and_test(&entry->fs_count))
-		wake_up_atomic_t(&entry->fs_count);
-}
-
-static struct ocfs2_filecheck_sysfs_entry *
-ocfs2_filecheck_sysfs_get(const char *devname)
-{
-	struct ocfs2_filecheck_sysfs_entry *p = NULL;
-
-	spin_lock(&ocfs2_filecheck_sysfs_lock);
-	list_for_each_entry(p, &ocfs2_filecheck_sysfs_list, fs_list) {
-		if (!strcmp(p->fs_sb->s_id, devname)) {
-			atomic_inc(&p->fs_count);
-			spin_unlock(&ocfs2_filecheck_sysfs_lock);
-			return p;
-		}
-	}
-	spin_unlock(&ocfs2_filecheck_sysfs_lock);
-	return NULL;
-}
-
-int ocfs2_filecheck_create_sysfs(struct super_block *sb)
-{
-	int ret = 0;
-	struct kset *device_kset = NULL;
-	struct kset *fcheck_kset = NULL;
-	struct ocfs2_filecheck *fcheck = NULL;
-	struct ocfs2_filecheck_sysfs_entry *entry = NULL;
-	struct attribute **attrs = NULL;
-	struct attribute_group attrgp;
-
-	if (!ocfs2_kset)
-		return -ENOMEM;
-
-	attrs = kmalloc(sizeof(struct attribute *) * 4, GFP_NOFS);
-	if (!attrs) {
-		ret = -ENOMEM;
-		goto error;
-	} else {
-		attrs[0] = &ocfs2_attr_filecheck_chk.attr;
-		attrs[1] = &ocfs2_attr_filecheck_fix.attr;
-		attrs[2] = &ocfs2_attr_filecheck_set.attr;
-		attrs[3] = NULL;
-		memset(&attrgp, 0, sizeof(attrgp));
-		attrgp.attrs = attrs;
-	}
-
-	fcheck = kmalloc(sizeof(struct ocfs2_filecheck), GFP_NOFS);
-	if (!fcheck) {
-		ret = -ENOMEM;
-		goto error;
-	} else {
-		INIT_LIST_HEAD(&fcheck->fc_head);
-		spin_lock_init(&fcheck->fc_lock);
-		fcheck->fc_max = OCFS2_FILECHECK_MINSIZE;
-		fcheck->fc_size = 0;
-		fcheck->fc_done = 0;
-	}
-
-	if (strlen(sb->s_id) <= 0) {
-		mlog(ML_ERROR,
-		"Cannot get device basename when create filecheck sysfs\n");
-		ret = -ENODEV;
-		goto error;
-	}
-
-	device_kset = kset_create_and_add(sb->s_id, NULL, &ocfs2_kset->kobj);
-	if (!device_kset) {
-		ret = -ENOMEM;
-		goto error;
-	}
-
-	fcheck_kset = kset_create_and_add("filecheck", NULL,
-					  &device_kset->kobj);
-	if (!fcheck_kset) {
-		ret = -ENOMEM;
-		goto error;
-	}
-
-	ret = sysfs_create_group(&fcheck_kset->kobj, &attrgp);
-	if (ret)
-		goto error;
-
-	entry = kmalloc(sizeof(struct ocfs2_filecheck_sysfs_entry), GFP_NOFS);
-	if (!entry) {
-		ret = -ENOMEM;
-		goto error;
-	} else {
-		atomic_set(&entry->fs_count, 1);
-		entry->fs_sb = sb;
-		entry->fs_devicekset = device_kset;
-		entry->fs_fcheckkset = fcheck_kset;
-		entry->fs_fcheck = fcheck;
-		ocfs2_filecheck_sysfs_add(entry);
-	}
-
-	kfree(attrs);
-	return 0;
-
-error:
-	kfree(attrs);
-	kfree(entry);
-	kfree(fcheck);
-	kset_unregister(fcheck_kset);
-	kset_unregister(device_kset);
-	return ret;
-}
-
-int ocfs2_filecheck_remove_sysfs(struct super_block *sb)
-{
-	return ocfs2_filecheck_sysfs_del(sb->s_id);
-}
-
 static int
-ocfs2_filecheck_erase_entries(struct ocfs2_filecheck_sysfs_entry *ent,
+ocfs2_filecheck_erase_entries(struct ocfs2_super *,
 			      unsigned int count);
-static int
-ocfs2_filecheck_adjust_max(struct ocfs2_filecheck_sysfs_entry *ent,
-			   unsigned int len)
+
+int ocfs2_filecheck_set_max_entries(struct ocfs2_super *osb,
+				int len)
 {
 	int ret;
 
 	if ((len < OCFS2_FILECHECK_MINSIZE) || (len > OCFS2_FILECHECK_MAXSIZE))
 		return -EINVAL;
 
-	spin_lock(&ent->fs_fcheck->fc_lock);
-	if (len < (ent->fs_fcheck->fc_size - ent->fs_fcheck->fc_done)) {
+	spin_lock(&osb->fc_lock);
+	if (len < (osb->fc_size - osb->fc_done)) {
 		mlog(ML_ERROR,
 		"Cannot set online file check maximum entry number "
 		"to %u due to too many pending entries(%u)\n",
-		len, ent->fs_fcheck->fc_size - ent->fs_fcheck->fc_done);
+		len, osb->fc_size - osb->fc_done);
 		ret = -EBUSY;
 	} else {
-		if (len < ent->fs_fcheck->fc_size)
-			BUG_ON(!ocfs2_filecheck_erase_entries(ent,
-				ent->fs_fcheck->fc_size - len));
+		if (len < osb->fc_size)
+			BUG_ON(!ocfs2_filecheck_erase_entries(osb,
+				osb->fc_size - len));
 
-		ent->fs_fcheck->fc_max = len;
+		osb->fc_max = len;
 		ret = 0;
 	}
-	spin_unlock(&ent->fs_fcheck->fc_lock);
+	spin_unlock(&osb->fc_lock);
 
 	return ret;
 }
 
-#define OCFS2_FILECHECK_ARGS_LEN	24
-static int
-ocfs2_filecheck_args_get_long(const char *buf, size_t count,
-			      unsigned long *val)
-{
-	char buffer[OCFS2_FILECHECK_ARGS_LEN];
-
-	memcpy(buffer, buf, count);
-	buffer[count] = '\0';
-
-	if (kstrtoul(buffer, 0, val))
-		return 1;
-
-	return 0;
-}
-
-static int
-ocfs2_filecheck_type_parse(const char *name, unsigned int *type)
-{
-	if (!strncmp(name, "fix", 4))
-		*type = OCFS2_FILECHECK_TYPE_FIX;
-	else if (!strncmp(name, "check", 6))
-		*type = OCFS2_FILECHECK_TYPE_CHK;
-	else if (!strncmp(name, "set", 4))
-		*type = OCFS2_FILECHECK_TYPE_SET;
-	else
-		return 1;
-
-	return 0;
-}
-
-static int
-ocfs2_filecheck_args_parse(const char *name, const char *buf, size_t count,
-			   struct ocfs2_filecheck_args *args)
-{
-	unsigned long val = 0;
-	unsigned int type;
-
-	/* too short/long args length */
-	if ((count < 1) || (count >= OCFS2_FILECHECK_ARGS_LEN))
-		return 1;
-
-	if (ocfs2_filecheck_type_parse(name, &type))
-		return 1;
-	if (ocfs2_filecheck_args_get_long(buf, count, &val))
-		return 1;
-
-	if (val <= 0)
-		return 1;
 
-	args->fa_type = type;
-	if (type == OCFS2_FILECHECK_TYPE_SET)
-		args->fa_len = (unsigned int)val;
-	else
-		args->fa_ino = val;
-
-	return 0;
-}
-
-static ssize_t ocfs2_filecheck_show(struct kobject *kobj,
-				    struct kobj_attribute *attr,
+int ocfs2_filecheck_show(struct ocfs2_super *osb, unsigned int type,
 				    char *buf)
 {
 
 	ssize_t ret = 0, total = 0, remain = PAGE_SIZE;
-	unsigned int type;
 	struct ocfs2_filecheck_entry *p;
-	struct ocfs2_filecheck_sysfs_entry *ent;
-
-	if (ocfs2_filecheck_type_parse(attr->attr.name, &type))
-		return -EINVAL;
-
-	ent = ocfs2_filecheck_sysfs_get(kobj->parent->name);
-	if (!ent) {
-		mlog(ML_ERROR,
-		"Cannot get the corresponding entry via device basename %s\n",
-		kobj->name);
-		return -ENODEV;
-	}
-
-	if (type == OCFS2_FILECHECK_TYPE_SET) {
-		spin_lock(&ent->fs_fcheck->fc_lock);
-		total = snprintf(buf, remain, "%u\n", ent->fs_fcheck->fc_max);
-		spin_unlock(&ent->fs_fcheck->fc_lock);
-		goto exit;
-	}
 
 	ret = snprintf(buf, remain, "INO\t\tDONE\tERROR\n");
 	total += ret;
 	remain -= ret;
-	spin_lock(&ent->fs_fcheck->fc_lock);
-	list_for_each_entry(p, &ent->fs_fcheck->fc_head, fe_list) {
+	spin_lock(&osb->fc_lock);
+	list_for_each_entry(p, &osb->file_check_entries, fe_list) {
 		if (p->fe_type != type)
 			continue;
 
@@ -443,24 +127,21 @@ static ssize_t ocfs2_filecheck_show(struct kobject *kobj,
 		total += ret;
 		remain -= ret;
 	}
-	spin_unlock(&ent->fs_fcheck->fc_lock);
-
-exit:
-	ocfs2_filecheck_sysfs_put(ent);
+	spin_unlock(&osb->fc_lock);
 	return total;
 }
 
 static int
-ocfs2_filecheck_erase_entry(struct ocfs2_filecheck_sysfs_entry *ent)
+ocfs2_filecheck_erase_entry(struct ocfs2_super *osb)
 {
 	struct ocfs2_filecheck_entry *p;
 
-	list_for_each_entry(p, &ent->fs_fcheck->fc_head, fe_list) {
+	list_for_each_entry(p, &osb->file_check_entries, fe_list) {
 		if (p->fe_done) {
 			list_del(&p->fe_list);
 			kfree(p);
-			ent->fs_fcheck->fc_size--;
-			ent->fs_fcheck->fc_done--;
+			osb->fc_size--;
+			osb->fc_done--;
 			return 1;
 		}
 	}
@@ -469,14 +150,14 @@ ocfs2_filecheck_erase_entry(struct ocfs2_filecheck_sysfs_entry *ent)
 }
 
 static int
-ocfs2_filecheck_erase_entries(struct ocfs2_filecheck_sysfs_entry *ent,
+ocfs2_filecheck_erase_entries(struct ocfs2_super *osb,
 			      unsigned int count)
 {
 	unsigned int i = 0;
 	unsigned int ret = 0;
 
 	while (i++ < count) {
-		if (ocfs2_filecheck_erase_entry(ent))
+		if (ocfs2_filecheck_erase_entry(osb))
 			ret++;
 		else
 			break;
@@ -486,24 +167,24 @@ ocfs2_filecheck_erase_entries(struct ocfs2_filecheck_sysfs_entry *ent,
 }
 
 static void
-ocfs2_filecheck_done_entry(struct ocfs2_filecheck_sysfs_entry *ent,
+ocfs2_filecheck_done_entry(struct ocfs2_super *osb,
 			   struct ocfs2_filecheck_entry *entry)
 {
 	entry->fe_done = 1;
-	spin_lock(&ent->fs_fcheck->fc_lock);
-	ent->fs_fcheck->fc_done++;
-	spin_unlock(&ent->fs_fcheck->fc_lock);
+	spin_lock(&osb->fc_lock);
+	osb->fc_done++;
+	spin_unlock(&osb->fc_lock);
 }
 
 static unsigned int
-ocfs2_filecheck_handle(struct super_block *sb,
+ocfs2_filecheck_handle(struct ocfs2_super *osb,
 		       unsigned long ino, unsigned int flags)
 {
 	unsigned int ret = OCFS2_FILECHECK_ERR_SUCCESS;
 	struct inode *inode = NULL;
 	int rc;
 
-	inode = ocfs2_iget(OCFS2_SB(sb), ino, flags, 0);
+	inode = ocfs2_iget(osb, ino, flags, 0);
 	if (IS_ERR(inode)) {
 		rc = (int)(-(long)inode);
 		if (rc >= OCFS2_FILECHECK_ERR_START &&
@@ -518,89 +199,64 @@ ocfs2_filecheck_handle(struct super_block *sb,
 }
 
 static void
-ocfs2_filecheck_handle_entry(struct ocfs2_filecheck_sysfs_entry *ent,
+ocfs2_filecheck_handle_entry(struct ocfs2_super *osb,
 			     struct ocfs2_filecheck_entry *entry)
 {
 	if (entry->fe_type == OCFS2_FILECHECK_TYPE_CHK)
-		entry->fe_status = ocfs2_filecheck_handle(ent->fs_sb,
+		entry->fe_status = ocfs2_filecheck_handle(osb,
 				entry->fe_ino, OCFS2_FI_FLAG_FILECHECK_CHK);
 	else if (entry->fe_type == OCFS2_FILECHECK_TYPE_FIX)
-		entry->fe_status = ocfs2_filecheck_handle(ent->fs_sb,
+		entry->fe_status = ocfs2_filecheck_handle(osb,
 				entry->fe_ino, OCFS2_FI_FLAG_FILECHECK_FIX);
 	else
 		entry->fe_status = OCFS2_FILECHECK_ERR_UNSUPPORTED;
 
-	ocfs2_filecheck_done_entry(ent, entry);
+	ocfs2_filecheck_done_entry(osb, entry);
 }
 
-static ssize_t ocfs2_filecheck_store(struct kobject *kobj,
-				     struct kobj_attribute *attr,
-				     const char *buf, size_t count)
+int ocfs2_filecheck_add_inode(struct ocfs2_super *osb,
+				     unsigned long ino)
 {
-	struct ocfs2_filecheck_args args;
 	struct ocfs2_filecheck_entry *entry;
-	struct ocfs2_filecheck_sysfs_entry *ent;
 	ssize_t ret = 0;
 
-	if (count == 0)
-		return count;
-
-	if (ocfs2_filecheck_args_parse(attr->attr.name, buf, count, &args)) {
-		mlog(ML_ERROR, "Invalid arguments for online file check\n");
-		return -EINVAL;
-	}
-
-	ent = ocfs2_filecheck_sysfs_get(kobj->parent->name);
-	if (!ent) {
-		mlog(ML_ERROR,
-		"Cannot get the corresponding entry via device basename %s\n",
-		kobj->parent->name);
-		return -ENODEV;
-	}
-
-	if (args.fa_type == OCFS2_FILECHECK_TYPE_SET) {
-		ret = ocfs2_filecheck_adjust_max(ent, args.fa_len);
-		goto exit;
-	}
-
 	entry = kmalloc(sizeof(struct ocfs2_filecheck_entry), GFP_NOFS);
 	if (!entry) {
 		ret = -ENOMEM;
 		goto exit;
 	}
 
-	spin_lock(&ent->fs_fcheck->fc_lock);
-	if ((ent->fs_fcheck->fc_size >= ent->fs_fcheck->fc_max) &&
-	    (ent->fs_fcheck->fc_done == 0)) {
+	spin_lock(&osb->fc_lock);
+	if ((osb->fc_size >= osb->fc_max) &&
+	    (osb->fc_done == 0)) {
 		mlog(ML_ERROR,
 		"Cannot do more file check "
 		"since file check queue(%u) is full now\n",
-		ent->fs_fcheck->fc_max);
+		osb->fc_max);
 		ret = -EBUSY;
 		kfree(entry);
 	} else {
-		if ((ent->fs_fcheck->fc_size >= ent->fs_fcheck->fc_max) &&
-		    (ent->fs_fcheck->fc_done > 0)) {
+		if ((osb->fc_size >= osb->fc_max) &&
+		    (osb->fc_done > 0)) {
 			/* Delete the oldest entry which was done,
 			 * make sure the entry size in list does
 			 * not exceed maximum value
 			 */
-			BUG_ON(!ocfs2_filecheck_erase_entry(ent));
+			BUG_ON(!ocfs2_filecheck_erase_entry(osb));
 		}
 
-		entry->fe_ino = args.fa_ino;
-		entry->fe_type = args.fa_type;
+		entry->fe_ino = ino;
+		entry->fe_type = OCFS2_FILECHECK_TYPE_CHK;
 		entry->fe_done = 0;
 		entry->fe_status = OCFS2_FILECHECK_ERR_INPROGRESS;
-		list_add_tail(&entry->fe_list, &ent->fs_fcheck->fc_head);
-		ent->fs_fcheck->fc_size++;
+		list_add_tail(&entry->fe_list, &osb->file_check_entries);
+		osb->fc_size++;
 	}
-	spin_unlock(&ent->fs_fcheck->fc_lock);
+	spin_unlock(&osb->fc_lock);
 
 	if (!ret)
-		ocfs2_filecheck_handle_entry(ent, entry);
+		ocfs2_filecheck_handle_entry(osb, entry);
 
 exit:
-	ocfs2_filecheck_sysfs_put(ent);
-	return (!ret ? count : ret);
+	return ret;
 }
diff --git a/fs/ocfs2/filecheck.h b/fs/ocfs2/filecheck.h
index e5cd002..b1a0d8c 100644
--- a/fs/ocfs2/filecheck.h
+++ b/fs/ocfs2/filecheck.h
@@ -42,8 +42,18 @@ enum {
 
 #define OCFS2_FILECHECK_ERR_START	OCFS2_FILECHECK_ERR_FAILED
 #define OCFS2_FILECHECK_ERR_END		OCFS2_FILECHECK_ERR_UNSUPPORTED
+#define OCFS2_FILECHECK_MAXSIZE         100
+#define OCFS2_FILECHECK_MINSIZE         10
+
+/* File check operation type */
+#define OCFS2_FILECHECK_TYPE_CHK  	1   /* Check a file(inode) */
+#define OCFS2_FILECHECK_TYPE_FIX  	2   /* Fix a file(inode) */
 
 int ocfs2_filecheck_create_sysfs(struct super_block *sb);
 int ocfs2_filecheck_remove_sysfs(struct super_block *sb);
+int ocfs2_filefix_inode(struct ocfs2_super *osb, unsigned long ino);
+int ocfs2_filecheck_add_inode(struct ocfs2_super *osb, unsigned long ino);
+int ocfs2_filecheck_set_max_entries(struct ocfs2_super *osb, int num);
+int ocfs2_filecheck_show(struct ocfs2_super *osb, unsigned int type, char *buf);
 
 #endif  /* FILECHECK_H */
diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
index 8e66cdf..9ced543 100644
--- a/fs/ocfs2/ocfs2.h
+++ b/fs/ocfs2/ocfs2.h
@@ -474,6 +474,13 @@ struct ocfs2_super
 	 */
 	struct workqueue_struct *ocfs2_wq;
 
+	/* file check */
+	struct list_head file_check_entries;
+	unsigned int fc_size;
+	unsigned int fc_max;
+	unsigned int fc_done;
+	spinlock_t fc_lock;
+
 	struct kobject kobj;
 	struct completion kobj_unregister;
 };
diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index 96b7a9f..a7791fa 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -2215,6 +2215,13 @@ static int ocfs2_initialize_super(struct super_block *sb,
 
 	get_random_bytes(&osb->s_next_generation, sizeof(u32));
 
+	/* file check information */
+	INIT_LIST_HEAD(&osb->file_check_entries);
+	osb->fc_max = OCFS2_FILECHECK_MINSIZE;
+	osb->fc_size = 0;
+	osb->fc_done = 0;
+	spin_lock_init(&osb->fc_lock);
+
 	/* FIXME
 	 * This should be done in ocfs2_journal_init(), but unknown
 	 * ordering issues will cause the filesystem to crash.
diff --git a/fs/ocfs2/sysfs.c b/fs/ocfs2/sysfs.c
index f0b7435..db4315b 100644
--- a/fs/ocfs2/sysfs.c
+++ b/fs/ocfs2/sysfs.c
@@ -1,5 +1,6 @@
 #include "ocfs2.h"
 #include "sysfs.h"
+#include "filecheck.h"
 
 struct ocfs2_sb_attr {
 	struct attribute attr;
@@ -9,8 +10,12 @@ struct ocfs2_sb_attr {
 			const char *buf, size_t count);
 };
 
-#define OCFS2_SB_ATTR(_name, _mode) \
-struct ocfs2_sb_attr sb_attr_##_name = __ATTR(name, _mode, _show, _store)
+#define OCFS2_SB_ATTR(_name) \
+struct ocfs2_sb_attr sb_attr_##_name = { \
+	.attr = {.name = __stringify(_name), .mode = (S_IWUSR | S_IRUGO)},  \
+	.show = _name##_show, \
+	.store = _name##_store \
+}
 
 #define OCFS2_SB_ATTR_RO(_name) \
 struct ocfs2_sb_attr sb_attr_##_name = __ATTR_RO(_name)
@@ -46,6 +51,81 @@ static ssize_t slot_num_show(struct ocfs2_super *osb,
 	return sprintf(buf, "%d\n", osb->slot_num);
 }
 
+static ssize_t file_check_show(struct ocfs2_super *osb,
+		struct ocfs2_sb_attr *attr,
+		char *buf)
+{
+	return ocfs2_filecheck_show(osb, OCFS2_FILECHECK_TYPE_CHK, buf);
+}
+
+static ssize_t file_check_store(struct ocfs2_super *osb,
+		struct ocfs2_sb_attr *attr,
+		const char *buf, size_t count)
+{
+	unsigned long t;
+	int ret;
+
+	ret = kstrtoul(skip_spaces(buf), 0, &t);
+	if (ret)
+		return ret;
+	ret = ocfs2_filecheck_add_inode(osb, t);
+	if (ret)
+		return ret;
+	return count;
+}
+
+static ssize_t file_fix_show(struct ocfs2_super *osb,
+		struct ocfs2_sb_attr *attr,
+		char *buf)
+{
+	return ocfs2_filecheck_show(osb, OCFS2_FILECHECK_TYPE_FIX, buf);
+}
+
+static ssize_t file_fix_store(struct ocfs2_super *osb,
+		struct ocfs2_sb_attr *attr,
+		const char *buf, size_t count)
+{
+	unsigned long t;
+	int ret;
+
+	ret = kstrtoul(skip_spaces(buf), 0, &t);
+	if (ret)
+		return ret;
+	ret = ocfs2_filecheck_add_inode(osb, t);
+	if (ret)
+		return ret;
+	return count;
+}
+
+static ssize_t file_check_max_entries_show(struct ocfs2_super *osb,
+		struct ocfs2_sb_attr *attr,
+		char *buf)
+{
+	int len = 0;
+	spin_lock(&osb->fc_lock);
+	/* Show done, current size and max */
+	len += sprintf(buf, "%d\t%d\t%d\n", osb->fc_done, osb->fc_size,
+			osb->fc_max);
+	spin_unlock(&osb->fc_lock);
+	return len;
+}
+
+static ssize_t file_check_max_entries_store(struct ocfs2_super *osb,
+		struct ocfs2_sb_attr *attr, const char *buf, size_t count)
+{
+
+	unsigned long t;
+	int ret;
+
+	ret = kstrtoul(skip_spaces(buf), 0, &t);
+	if (ret)
+		return ret;
+	ret = ocfs2_filecheck_set_max_entries(osb, (int)t);
+	if (ret)
+		return ret;
+	return count;
+}
+
 static void sb_release(struct kobject *kobj)
 {
 	struct ocfs2_super *osb = container_of(kobj, struct ocfs2_super, kobj);
@@ -58,8 +138,15 @@ static const struct sysfs_ops ocfs2_sb_sysfs_ops = {
 };
 
 static OCFS2_SB_ATTR_RO(slot_num);
+static OCFS2_SB_ATTR(file_check);
+static OCFS2_SB_ATTR(file_fix);
+static OCFS2_SB_ATTR(file_check_max_entries);
+
 static struct attribute *ocfs2_sb_attrs[] = {
 	&sb_attr_slot_num.attr,
+	&sb_attr_file_check.attr,
+	&sb_attr_file_fix.attr,
+	&sb_attr_file_check_max_entries.attr,
 	NULL
 };
 
-- 
2.6.6

[Ocfs2-devel] [PATCH 3/5] Generate uevents for errors

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Errors are reported to the userspace using the kobject we just added.
This can be used to automate checks and fixes using udev scripts

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/ocfs2/inode.c | 13 +++++++++++++
 fs/ocfs2/sysfs.c | 17 +++++++++++++++++
 fs/ocfs2/sysfs.h |  2 ++
 3 files changed, 32 insertions(+)

diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
index 12f4a9e..a6cfe5d 100644
--- a/fs/ocfs2/inode.c
+++ b/fs/ocfs2/inode.c
@@ -54,6 +54,7 @@
 #include "refcounttree.h"
 #include "ocfs2_trace.h"
 #include "filecheck.h"
+#include "sysfs.h"
 
 #include "buffer_head_io.h"
 
@@ -1412,6 +1413,9 @@ int ocfs2_validate_inode_block(struct super_block *sb,
 		rc = ocfs2_error(sb, "Invalid dinode #%llu: signature = %.*s\n",
 				 (unsigned long long)bh->b_blocknr, 7,
 				 di->i_signature);
+		ocfs2_report_error(OCFS2_SB(sb), (unsigned long long)bh->b_blocknr,
+				(unsigned long long)bh->b_blocknr,
+				rc);
 		goto bail;
 	}
 
@@ -1419,6 +1423,9 @@ int ocfs2_validate_inode_block(struct super_block *sb,
 		rc = ocfs2_error(sb, "Invalid dinode #%llu: i_blkno is %llu\n",
 				 (unsigned long long)bh->b_blocknr,
 				 (unsigned long long)le64_to_cpu(di->i_blkno));
+		ocfs2_report_error(OCFS2_SB(sb), (unsigned long long)bh->b_blocknr,
+				(unsigned long long)bh->b_blocknr,
+				rc);
 		goto bail;
 	}
 
@@ -1426,6 +1433,9 @@ int ocfs2_validate_inode_block(struct super_block *sb,
 		rc = ocfs2_error(sb,
 				 "Invalid dinode #%llu: OCFS2_VALID_FL not set\n",
 				 (unsigned long long)bh->b_blocknr);
+		ocfs2_report_error(OCFS2_SB(sb), (unsigned long long)bh->b_blocknr,
+				(unsigned long long)bh->b_blocknr,
+				rc);
 		goto bail;
 	}
 
@@ -1435,6 +1445,9 @@ int ocfs2_validate_inode_block(struct super_block *sb,
 				 "Invalid dinode #%llu: fs_generation is %u\n",
 				 (unsigned long long)bh->b_blocknr,
 				 le32_to_cpu(di->i_fs_generation));
+		ocfs2_report_error(OCFS2_SB(sb), (unsigned long long)bh->b_blocknr,
+				(unsigned long long)bh->b_blocknr,
+				rc);
 		goto bail;
 	}
 
diff --git a/fs/ocfs2/sysfs.c b/fs/ocfs2/sysfs.c
index db4315b..5632d14 100644
--- a/fs/ocfs2/sysfs.c
+++ b/fs/ocfs2/sysfs.c
@@ -126,6 +126,23 @@ static ssize_t file_check_max_entries_store(struct ocfs2_super *osb,
 	return count;
 }
 
+void ocfs2_report_error(struct ocfs2_super *osb, unsigned long long ino,
+			unsigned long long blkno, int errno)
+{
+	char event_name[] = "EVENT=FS_ERROR";
+	char device[16];
+	char inode_number[16];
+	char error_number[16];
+	char block_number[16];
+	char *envp[] = {event_name, inode_number, error_number, block_number,
+		NULL};
+	snprintf(device, 16, "DEVICE=%s", osb->sb->s_id);
+	snprintf(error_number, 16, "ERROR=%d", errno);
+	snprintf(inode_number, 16, "INODE=%llu", ino);
+	snprintf(block_number, 16, "BLOCK=%llu", blkno);
+	kobject_uevent_env(&osb->kobj, KOBJ_CHANGE, envp);
+}
+
 static void sb_release(struct kobject *kobj)
 {
 	struct ocfs2_super *osb = container_of(kobj, struct ocfs2_super, kobj);
diff --git a/fs/ocfs2/sysfs.h b/fs/ocfs2/sysfs.h
index d929ac1..0383ee3 100644
--- a/fs/ocfs2/sysfs.h
+++ b/fs/ocfs2/sysfs.h
@@ -3,6 +3,8 @@
 #ifndef _SYS_H
 #define _SYS_H
 
+void ocfs2_report_error(struct ocfs2_super *osb, unsigned long long ino,
+		unsigned long long blkno, int error);
 int ocfs2_sysfs_sb_init(struct super_block *sb);
 void ocfs2_sysfs_sb_exit(struct super_block *sb);
 
-- 
2.6.6

[Ocfs2-devel] [PATCH 4/5] ocfs2: Disallow duplicate entries in the list

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

A check to make sure we don't have duplicates in the filecheck list.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/ocfs2/filecheck.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/fs/ocfs2/filecheck.c b/fs/ocfs2/filecheck.c
index 0b41967..006d521 100644
--- a/fs/ocfs2/filecheck.c
+++ b/fs/ocfs2/filecheck.c
@@ -217,14 +217,12 @@ ocfs2_filecheck_handle_entry(struct ocfs2_super *osb,
 int ocfs2_filecheck_add_inode(struct ocfs2_super *osb,
 				     unsigned long ino)
 {
-	struct ocfs2_filecheck_entry *entry;
+	struct ocfs2_filecheck_entry *entry, *p;
 	ssize_t ret = 0;
 
 	entry = kmalloc(sizeof(struct ocfs2_filecheck_entry), GFP_NOFS);
-	if (!entry) {
-		ret = -ENOMEM;
-		goto exit;
-	}
+	if (!entry)
+		return -ENOMEM;
 
 	spin_lock(&osb->fc_lock);
 	if ((osb->fc_size >= osb->fc_max) &&
@@ -244,6 +242,12 @@ int ocfs2_filecheck_add_inode(struct ocfs2_super *osb,
 			 */
 			BUG_ON(!ocfs2_filecheck_erase_entry(osb));
 		}
+		list_for_each_entry(p, &osb->file_check_entries, fe_list)
+			if (p->fe_ino == ino) {
+				ret = -EEXIST;
+				kfree(entry);
+				goto unlock;
+			}
 
 		entry->fe_ino = ino;
 		entry->fe_type = OCFS2_FILECHECK_TYPE_CHK;
@@ -252,11 +256,15 @@ int ocfs2_filecheck_add_inode(struct ocfs2_super *osb,
 		list_add_tail(&entry->fe_list, &osb->file_check_entries);
 		osb->fc_size++;
 	}
+unlock:
 	spin_unlock(&osb->fc_lock);
 
-	if (!ret)
-		ocfs2_filecheck_handle_entry(osb, entry);
+	if (ret)
+		return ret;
 
-exit:
-	return ret;
+	entry->fe_status = ocfs2_filecheck_handle(osb,
+			entry->fe_ino, OCFS2_FI_FLAG_FILECHECK_CHK);
+
+	ocfs2_filecheck_done_entry(osb, entry);
+	return 0;
 }
-- 
2.6.6

[Ocfs2-devel] [PATCH 5/5] Fix files when an inode is written in file_fix

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Check that the entriy exists and has been filed for check.
Also perform some code cleanup to remove ocfs2_filecheck_handle_entry()

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/ocfs2/filecheck.c | 41 +++++++++++++++++++++++++----------------
 fs/ocfs2/filecheck.h |  1 +
 fs/ocfs2/sysfs.c     |  2 +-
 3 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/fs/ocfs2/filecheck.c b/fs/ocfs2/filecheck.c
index 006d521..fc6e183 100644
--- a/fs/ocfs2/filecheck.c
+++ b/fs/ocfs2/filecheck.c
@@ -198,22 +198,6 @@ ocfs2_filecheck_handle(struct ocfs2_super *osb,
 	return ret;
 }
 
-static void
-ocfs2_filecheck_handle_entry(struct ocfs2_super *osb,
-			     struct ocfs2_filecheck_entry *entry)
-{
-	if (entry->fe_type == OCFS2_FILECHECK_TYPE_CHK)
-		entry->fe_status = ocfs2_filecheck_handle(osb,
-				entry->fe_ino, OCFS2_FI_FLAG_FILECHECK_CHK);
-	else if (entry->fe_type == OCFS2_FILECHECK_TYPE_FIX)
-		entry->fe_status = ocfs2_filecheck_handle(osb,
-				entry->fe_ino, OCFS2_FI_FLAG_FILECHECK_FIX);
-	else
-		entry->fe_status = OCFS2_FILECHECK_ERR_UNSUPPORTED;
-
-	ocfs2_filecheck_done_entry(osb, entry);
-}
-
 int ocfs2_filecheck_add_inode(struct ocfs2_super *osb,
 				     unsigned long ino)
 {
@@ -268,3 +252,28 @@ unlock:
 	ocfs2_filecheck_done_entry(osb, entry);
 	return 0;
 }
+
+int ocfs2_filefix_add_inode(struct ocfs2_super *osb,
+				     unsigned long ino)
+{
+	struct ocfs2_filecheck_entry *entry;
+	int ret = -ENOENT;
+
+	spin_lock(&osb->fc_lock);
+	list_for_each_entry(entry, &osb->file_check_entries, fe_list)
+		if (entry->fe_ino == ino) {
+			entry->fe_type = OCFS2_FILECHECK_TYPE_FIX;
+			ret = 0;
+			break;
+		}
+	spin_unlock(&osb->fc_lock);
+	if (ret)
+		return ret;
+
+	entry->fe_status = ocfs2_filecheck_handle(osb,
+			entry->fe_ino, OCFS2_FI_FLAG_FILECHECK_FIX);
+
+	ocfs2_filecheck_done_entry(osb, entry);
+	return 0;
+}
+
diff --git a/fs/ocfs2/filecheck.h b/fs/ocfs2/filecheck.h
index b1a0d8c..d5f81a7 100644
--- a/fs/ocfs2/filecheck.h
+++ b/fs/ocfs2/filecheck.h
@@ -53,6 +53,7 @@ int ocfs2_filecheck_create_sysfs(struct super_block *sb);
 int ocfs2_filecheck_remove_sysfs(struct super_block *sb);
 int ocfs2_filefix_inode(struct ocfs2_super *osb, unsigned long ino);
 int ocfs2_filecheck_add_inode(struct ocfs2_super *osb, unsigned long ino);
+int ocfs2_filefix_add_inode(struct ocfs2_super *osb, unsigned long ino);
 int ocfs2_filecheck_set_max_entries(struct ocfs2_super *osb, int num);
 int ocfs2_filecheck_show(struct ocfs2_super *osb, unsigned int type, char *buf);
 
diff --git a/fs/ocfs2/sysfs.c b/fs/ocfs2/sysfs.c
index 5632d14..daca819 100644
--- a/fs/ocfs2/sysfs.c
+++ b/fs/ocfs2/sysfs.c
@@ -91,7 +91,7 @@ static ssize_t file_fix_store(struct ocfs2_super *osb,
 	ret = kstrtoul(skip_spaces(buf), 0, &t);
 	if (ret)
 		return ret;
-	ret = ocfs2_filecheck_add_inode(osb, t);
+	ret = ocfs2_filefix_add_inode(osb, t);
 	if (ret)
 		return ret;
 	return count;
-- 
2.6.6

[Ocfs2-devel] [PATCH 0/5] ocfs2: sysfs and cleanup

[Gang He]

Hello Goldwyn,

Thank for your efforts on improving online file check.
I was being engaged by some bugs in the past days.

Thanks
Gang


>>> 
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> This adds /sys/fs/ocfs2/<s_id> to the sys filesystem. All other
> files are added in this directory. This is done by:
> 1. Adding kobject in the ocfs2_super structure
> 2. Using ocfs2_sb_attr which encompass all the show() and store() functions
> 
> This also reduce code and does away with unnecessary data structure,
> but adds some in the ocfs2_super for house keeping.
> 
> Hopefully, this would be used to further add configurable parameters
> for the super block.
> 
> Suggestions/Criticism welcome.
> 
> -- 
> Goldwyn

[Ocfs2-devel] [PATCH v2 0/5] ocfs2: sysfs and cleanup

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

This adds /sys/fs/ocfs2/<s_id> to the sys filesystem. All other
files are added in this directory. This is done by:
1. Adding kobject in the ocfs2_super structure
2. Using ocfs2_sb_attr which encompass all the show() and store() functions

This also reduce code and does away with unnecessary data structure,
but adds some in the ocfs2_super for house keeping.

Hopefully, this would be used to further add configurable parameters
for the super block.

Suggestions/Criticism welcome.

Changes since v1:
	- Corrected store() function check

-- 
Goldwyn

[Ocfs2-devel] [PATCH 2/5] Use the sysfs interface for creating filecheck files

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

This reduces the code base and removes unnecessary data structures
for filecheck information since all information is stored in ocfs2_super
now.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/ocfs2/filecheck.c | 440 ++++++---------------------------------------------
 fs/ocfs2/filecheck.h |  10 ++
 fs/ocfs2/ocfs2.h     |   7 +
 fs/ocfs2/super.c     |   7 +
 fs/ocfs2/sysfs.c     |  91 ++++++++++-
 5 files changed, 161 insertions(+), 394 deletions(-)

diff --git a/fs/ocfs2/filecheck.c b/fs/ocfs2/filecheck.c
index 2cabbcf..0b41967 100644
--- a/fs/ocfs2/filecheck.c
+++ b/fs/ocfs2/filecheck.c
@@ -17,15 +17,7 @@
  * General Public License for more details.
  */
 
-#include <linux/list.h>
-#include <linux/spinlock.h>
-#include <linux/module.h>
-#include <linux/slab.h>
-#include <linux/kmod.h>
 #include <linux/fs.h>
-#include <linux/kobject.h>
-#include <linux/sysfs.h>
-#include <linux/sysctl.h>
 #include <cluster/masklog.h>
 
 #include "ocfs2.h"
@@ -53,36 +45,6 @@ static const char * const ocfs2_filecheck_errs[] = {
 	"UNSUPPORTED"
 };
 
-static DEFINE_SPINLOCK(ocfs2_filecheck_sysfs_lock);
-static LIST_HEAD(ocfs2_filecheck_sysfs_list);
-
-struct ocfs2_filecheck {
-	struct list_head fc_head;	/* File check entry list head */
-	spinlock_t fc_lock;
-	unsigned int fc_max;	/* Maximum number of entry in list */
-	unsigned int fc_size;	/* Current entry count in list */
-	unsigned int fc_done;	/* Finished entry count in list */
-};
-
-struct ocfs2_filecheck_sysfs_entry {	/* sysfs entry per mounting */
-	struct list_head fs_list;
-	atomic_t fs_count;
-	struct super_block *fs_sb;
-	struct kset *fs_devicekset;
-	struct kset *fs_fcheckkset;
-	struct ocfs2_filecheck *fs_fcheck;
-};
-
-#define OCFS2_FILECHECK_MAXSIZE		100
-#define OCFS2_FILECHECK_MINSIZE		10
-
-/* File check operation type */
-enum {
-	OCFS2_FILECHECK_TYPE_CHK = 0,	/* Check a file(inode) */
-	OCFS2_FILECHECK_TYPE_FIX,	/* Fix a file(inode) */
-	OCFS2_FILECHECK_TYPE_SET = 100	/* Set entry list maximum size */
-};
-
 struct ocfs2_filecheck_entry {
 	struct list_head fe_list;
 	unsigned long fe_ino;
@@ -91,14 +53,6 @@ struct ocfs2_filecheck_entry {
 	unsigned int fe_status:31;
 };
 
-struct ocfs2_filecheck_args {
-	unsigned int fa_type;
-	union {
-		unsigned long fa_ino;
-		unsigned int fa_len;
-	};
-};
-
 static const char *
 ocfs2_filecheck_error(int errno)
 {
@@ -110,321 +64,51 @@ ocfs2_filecheck_error(int errno)
 	return ocfs2_filecheck_errs[errno - OCFS2_FILECHECK_ERR_START + 1];
 }
 
-static ssize_t ocfs2_filecheck_show(struct kobject *kobj,
-				    struct kobj_attribute *attr,
-				    char *buf);
-static ssize_t ocfs2_filecheck_store(struct kobject *kobj,
-				     struct kobj_attribute *attr,
-				     const char *buf, size_t count);
-static struct kobj_attribute ocfs2_attr_filecheck_chk =
-					__ATTR(check, S_IRUSR | S_IWUSR,
-					ocfs2_filecheck_show,
-					ocfs2_filecheck_store);
-static struct kobj_attribute ocfs2_attr_filecheck_fix =
-					__ATTR(fix, S_IRUSR | S_IWUSR,
-					ocfs2_filecheck_show,
-					ocfs2_filecheck_store);
-static struct kobj_attribute ocfs2_attr_filecheck_set =
-					__ATTR(set, S_IRUSR | S_IWUSR,
-					ocfs2_filecheck_show,
-					ocfs2_filecheck_store);
-
-static int ocfs2_filecheck_sysfs_wait(atomic_t *p)
-{
-	schedule();
-	return 0;
-}
-
-static void
-ocfs2_filecheck_sysfs_free(struct ocfs2_filecheck_sysfs_entry *entry)
-{
-	struct ocfs2_filecheck_entry *p;
-
-	if (!atomic_dec_and_test(&entry->fs_count))
-		wait_on_atomic_t(&entry->fs_count, ocfs2_filecheck_sysfs_wait,
-				 TASK_UNINTERRUPTIBLE);
-
-	spin_lock(&entry->fs_fcheck->fc_lock);
-	while (!list_empty(&entry->fs_fcheck->fc_head)) {
-		p = list_first_entry(&entry->fs_fcheck->fc_head,
-				     struct ocfs2_filecheck_entry, fe_list);
-		list_del(&p->fe_list);
-		BUG_ON(!p->fe_done); /* To free a undone file check entry */
-		kfree(p);
-	}
-	spin_unlock(&entry->fs_fcheck->fc_lock);
-
-	kset_unregister(entry->fs_fcheckkset);
-	kset_unregister(entry->fs_devicekset);
-	kfree(entry->fs_fcheck);
-	kfree(entry);
-}
-
-static void
-ocfs2_filecheck_sysfs_add(struct ocfs2_filecheck_sysfs_entry *entry)
-{
-	spin_lock(&ocfs2_filecheck_sysfs_lock);
-	list_add_tail(&entry->fs_list, &ocfs2_filecheck_sysfs_list);
-	spin_unlock(&ocfs2_filecheck_sysfs_lock);
-}
-
-static int ocfs2_filecheck_sysfs_del(const char *devname)
-{
-	struct ocfs2_filecheck_sysfs_entry *p;
-
-	spin_lock(&ocfs2_filecheck_sysfs_lock);
-	list_for_each_entry(p, &ocfs2_filecheck_sysfs_list, fs_list) {
-		if (!strcmp(p->fs_sb->s_id, devname)) {
-			list_del(&p->fs_list);
-			spin_unlock(&ocfs2_filecheck_sysfs_lock);
-			ocfs2_filecheck_sysfs_free(p);
-			return 0;
-		}
-	}
-	spin_unlock(&ocfs2_filecheck_sysfs_lock);
-	return 1;
-}
-
-static void
-ocfs2_filecheck_sysfs_put(struct ocfs2_filecheck_sysfs_entry *entry)
-{
-	if (atomic_dec_and_test(&entry->fs_count))
-		wake_up_atomic_t(&entry->fs_count);
-}
-
-static struct ocfs2_filecheck_sysfs_entry *
-ocfs2_filecheck_sysfs_get(const char *devname)
-{
-	struct ocfs2_filecheck_sysfs_entry *p = NULL;
-
-	spin_lock(&ocfs2_filecheck_sysfs_lock);
-	list_for_each_entry(p, &ocfs2_filecheck_sysfs_list, fs_list) {
-		if (!strcmp(p->fs_sb->s_id, devname)) {
-			atomic_inc(&p->fs_count);
-			spin_unlock(&ocfs2_filecheck_sysfs_lock);
-			return p;
-		}
-	}
-	spin_unlock(&ocfs2_filecheck_sysfs_lock);
-	return NULL;
-}
-
-int ocfs2_filecheck_create_sysfs(struct super_block *sb)
-{
-	int ret = 0;
-	struct kset *device_kset = NULL;
-	struct kset *fcheck_kset = NULL;
-	struct ocfs2_filecheck *fcheck = NULL;
-	struct ocfs2_filecheck_sysfs_entry *entry = NULL;
-	struct attribute **attrs = NULL;
-	struct attribute_group attrgp;
-
-	if (!ocfs2_kset)
-		return -ENOMEM;
-
-	attrs = kmalloc(sizeof(struct attribute *) * 4, GFP_NOFS);
-	if (!attrs) {
-		ret = -ENOMEM;
-		goto error;
-	} else {
-		attrs[0] = &ocfs2_attr_filecheck_chk.attr;
-		attrs[1] = &ocfs2_attr_filecheck_fix.attr;
-		attrs[2] = &ocfs2_attr_filecheck_set.attr;
-		attrs[3] = NULL;
-		memset(&attrgp, 0, sizeof(attrgp));
-		attrgp.attrs = attrs;
-	}
-
-	fcheck = kmalloc(sizeof(struct ocfs2_filecheck), GFP_NOFS);
-	if (!fcheck) {
-		ret = -ENOMEM;
-		goto error;
-	} else {
-		INIT_LIST_HEAD(&fcheck->fc_head);
-		spin_lock_init(&fcheck->fc_lock);
-		fcheck->fc_max = OCFS2_FILECHECK_MINSIZE;
-		fcheck->fc_size = 0;
-		fcheck->fc_done = 0;
-	}
-
-	if (strlen(sb->s_id) <= 0) {
-		mlog(ML_ERROR,
-		"Cannot get device basename when create filecheck sysfs\n");
-		ret = -ENODEV;
-		goto error;
-	}
-
-	device_kset = kset_create_and_add(sb->s_id, NULL, &ocfs2_kset->kobj);
-	if (!device_kset) {
-		ret = -ENOMEM;
-		goto error;
-	}
-
-	fcheck_kset = kset_create_and_add("filecheck", NULL,
-					  &device_kset->kobj);
-	if (!fcheck_kset) {
-		ret = -ENOMEM;
-		goto error;
-	}
-
-	ret = sysfs_create_group(&fcheck_kset->kobj, &attrgp);
-	if (ret)
-		goto error;
-
-	entry = kmalloc(sizeof(struct ocfs2_filecheck_sysfs_entry), GFP_NOFS);
-	if (!entry) {
-		ret = -ENOMEM;
-		goto error;
-	} else {
-		atomic_set(&entry->fs_count, 1);
-		entry->fs_sb = sb;
-		entry->fs_devicekset = device_kset;
-		entry->fs_fcheckkset = fcheck_kset;
-		entry->fs_fcheck = fcheck;
-		ocfs2_filecheck_sysfs_add(entry);
-	}
-
-	kfree(attrs);
-	return 0;
-
-error:
-	kfree(attrs);
-	kfree(entry);
-	kfree(fcheck);
-	kset_unregister(fcheck_kset);
-	kset_unregister(device_kset);
-	return ret;
-}
-
-int ocfs2_filecheck_remove_sysfs(struct super_block *sb)
-{
-	return ocfs2_filecheck_sysfs_del(sb->s_id);
-}
-
 static int
-ocfs2_filecheck_erase_entries(struct ocfs2_filecheck_sysfs_entry *ent,
+ocfs2_filecheck_erase_entries(struct ocfs2_super *,
 			      unsigned int count);
-static int
-ocfs2_filecheck_adjust_max(struct ocfs2_filecheck_sysfs_entry *ent,
-			   unsigned int len)
+
+int ocfs2_filecheck_set_max_entries(struct ocfs2_super *osb,
+				int len)
 {
 	int ret;
 
 	if ((len < OCFS2_FILECHECK_MINSIZE) || (len > OCFS2_FILECHECK_MAXSIZE))
 		return -EINVAL;
 
-	spin_lock(&ent->fs_fcheck->fc_lock);
-	if (len < (ent->fs_fcheck->fc_size - ent->fs_fcheck->fc_done)) {
+	spin_lock(&osb->fc_lock);
+	if (len < (osb->fc_size - osb->fc_done)) {
 		mlog(ML_ERROR,
 		"Cannot set online file check maximum entry number "
 		"to %u due to too many pending entries(%u)\n",
-		len, ent->fs_fcheck->fc_size - ent->fs_fcheck->fc_done);
+		len, osb->fc_size - osb->fc_done);
 		ret = -EBUSY;
 	} else {
-		if (len < ent->fs_fcheck->fc_size)
-			BUG_ON(!ocfs2_filecheck_erase_entries(ent,
-				ent->fs_fcheck->fc_size - len));
+		if (len < osb->fc_size)
+			BUG_ON(!ocfs2_filecheck_erase_entries(osb,
+				osb->fc_size - len));
 
-		ent->fs_fcheck->fc_max = len;
+		osb->fc_max = len;
 		ret = 0;
 	}
-	spin_unlock(&ent->fs_fcheck->fc_lock);
+	spin_unlock(&osb->fc_lock);
 
 	return ret;
 }
 
-#define OCFS2_FILECHECK_ARGS_LEN	24
-static int
-ocfs2_filecheck_args_get_long(const char *buf, size_t count,
-			      unsigned long *val)
-{
-	char buffer[OCFS2_FILECHECK_ARGS_LEN];
-
-	memcpy(buffer, buf, count);
-	buffer[count] = '\0';
-
-	if (kstrtoul(buffer, 0, val))
-		return 1;
-
-	return 0;
-}
-
-static int
-ocfs2_filecheck_type_parse(const char *name, unsigned int *type)
-{
-	if (!strncmp(name, "fix", 4))
-		*type = OCFS2_FILECHECK_TYPE_FIX;
-	else if (!strncmp(name, "check", 6))
-		*type = OCFS2_FILECHECK_TYPE_CHK;
-	else if (!strncmp(name, "set", 4))
-		*type = OCFS2_FILECHECK_TYPE_SET;
-	else
-		return 1;
-
-	return 0;
-}
-
-static int
-ocfs2_filecheck_args_parse(const char *name, const char *buf, size_t count,
-			   struct ocfs2_filecheck_args *args)
-{
-	unsigned long val = 0;
-	unsigned int type;
-
-	/* too short/long args length */
-	if ((count < 1) || (count >= OCFS2_FILECHECK_ARGS_LEN))
-		return 1;
-
-	if (ocfs2_filecheck_type_parse(name, &type))
-		return 1;
-	if (ocfs2_filecheck_args_get_long(buf, count, &val))
-		return 1;
-
-	if (val <= 0)
-		return 1;
 
-	args->fa_type = type;
-	if (type == OCFS2_FILECHECK_TYPE_SET)
-		args->fa_len = (unsigned int)val;
-	else
-		args->fa_ino = val;
-
-	return 0;
-}
-
-static ssize_t ocfs2_filecheck_show(struct kobject *kobj,
-				    struct kobj_attribute *attr,
+int ocfs2_filecheck_show(struct ocfs2_super *osb, unsigned int type,
 				    char *buf)
 {
 
 	ssize_t ret = 0, total = 0, remain = PAGE_SIZE;
-	unsigned int type;
 	struct ocfs2_filecheck_entry *p;
-	struct ocfs2_filecheck_sysfs_entry *ent;
-
-	if (ocfs2_filecheck_type_parse(attr->attr.name, &type))
-		return -EINVAL;
-
-	ent = ocfs2_filecheck_sysfs_get(kobj->parent->name);
-	if (!ent) {
-		mlog(ML_ERROR,
-		"Cannot get the corresponding entry via device basename %s\n",
-		kobj->name);
-		return -ENODEV;
-	}
-
-	if (type == OCFS2_FILECHECK_TYPE_SET) {
-		spin_lock(&ent->fs_fcheck->fc_lock);
-		total = snprintf(buf, remain, "%u\n", ent->fs_fcheck->fc_max);
-		spin_unlock(&ent->fs_fcheck->fc_lock);
-		goto exit;
-	}
 
 	ret = snprintf(buf, remain, "INO\t\tDONE\tERROR\n");
 	total += ret;
 	remain -= ret;
-	spin_lock(&ent->fs_fcheck->fc_lock);
-	list_for_each_entry(p, &ent->fs_fcheck->fc_head, fe_list) {
+	spin_lock(&osb->fc_lock);
+	list_for_each_entry(p, &osb->file_check_entries, fe_list) {
 		if (p->fe_type != type)
 			continue;
 
@@ -443,24 +127,21 @@ static ssize_t ocfs2_filecheck_show(struct kobject *kobj,
 		total += ret;
 		remain -= ret;
 	}
-	spin_unlock(&ent->fs_fcheck->fc_lock);
-
-exit:
-	ocfs2_filecheck_sysfs_put(ent);
+	spin_unlock(&osb->fc_lock);
 	return total;
 }
 
 static int
-ocfs2_filecheck_erase_entry(struct ocfs2_filecheck_sysfs_entry *ent)
+ocfs2_filecheck_erase_entry(struct ocfs2_super *osb)
 {
 	struct ocfs2_filecheck_entry *p;
 
-	list_for_each_entry(p, &ent->fs_fcheck->fc_head, fe_list) {
+	list_for_each_entry(p, &osb->file_check_entries, fe_list) {
 		if (p->fe_done) {
 			list_del(&p->fe_list);
 			kfree(p);
-			ent->fs_fcheck->fc_size--;
-			ent->fs_fcheck->fc_done--;
+			osb->fc_size--;
+			osb->fc_done--;
 			return 1;
 		}
 	}
@@ -469,14 +150,14 @@ ocfs2_filecheck_erase_entry(struct ocfs2_filecheck_sysfs_entry *ent)
 }
 
 static int
-ocfs2_filecheck_erase_entries(struct ocfs2_filecheck_sysfs_entry *ent,
+ocfs2_filecheck_erase_entries(struct ocfs2_super *osb,
 			      unsigned int count)
 {
 	unsigned int i = 0;
 	unsigned int ret = 0;
 
 	while (i++ < count) {
-		if (ocfs2_filecheck_erase_entry(ent))
+		if (ocfs2_filecheck_erase_entry(osb))
 			ret++;
 		else
 			break;
@@ -486,24 +167,24 @@ ocfs2_filecheck_erase_entries(struct ocfs2_filecheck_sysfs_entry *ent,
 }
 
 static void
-ocfs2_filecheck_done_entry(struct ocfs2_filecheck_sysfs_entry *ent,
+ocfs2_filecheck_done_entry(struct ocfs2_super *osb,
 			   struct ocfs2_filecheck_entry *entry)
 {
 	entry->fe_done = 1;
-	spin_lock(&ent->fs_fcheck->fc_lock);
-	ent->fs_fcheck->fc_done++;
-	spin_unlock(&ent->fs_fcheck->fc_lock);
+	spin_lock(&osb->fc_lock);
+	osb->fc_done++;
+	spin_unlock(&osb->fc_lock);
 }
 
 static unsigned int
-ocfs2_filecheck_handle(struct super_block *sb,
+ocfs2_filecheck_handle(struct ocfs2_super *osb,
 		       unsigned long ino, unsigned int flags)
 {
 	unsigned int ret = OCFS2_FILECHECK_ERR_SUCCESS;
 	struct inode *inode = NULL;
 	int rc;
 
-	inode = ocfs2_iget(OCFS2_SB(sb), ino, flags, 0);
+	inode = ocfs2_iget(osb, ino, flags, 0);
 	if (IS_ERR(inode)) {
 		rc = (int)(-(long)inode);
 		if (rc >= OCFS2_FILECHECK_ERR_START &&
@@ -518,89 +199,64 @@ ocfs2_filecheck_handle(struct super_block *sb,
 }
 
 static void
-ocfs2_filecheck_handle_entry(struct ocfs2_filecheck_sysfs_entry *ent,
+ocfs2_filecheck_handle_entry(struct ocfs2_super *osb,
 			     struct ocfs2_filecheck_entry *entry)
 {
 	if (entry->fe_type == OCFS2_FILECHECK_TYPE_CHK)
-		entry->fe_status = ocfs2_filecheck_handle(ent->fs_sb,
+		entry->fe_status = ocfs2_filecheck_handle(osb,
 				entry->fe_ino, OCFS2_FI_FLAG_FILECHECK_CHK);
 	else if (entry->fe_type == OCFS2_FILECHECK_TYPE_FIX)
-		entry->fe_status = ocfs2_filecheck_handle(ent->fs_sb,
+		entry->fe_status = ocfs2_filecheck_handle(osb,
 				entry->fe_ino, OCFS2_FI_FLAG_FILECHECK_FIX);
 	else
 		entry->fe_status = OCFS2_FILECHECK_ERR_UNSUPPORTED;
 
-	ocfs2_filecheck_done_entry(ent, entry);
+	ocfs2_filecheck_done_entry(osb, entry);
 }
 
-static ssize_t ocfs2_filecheck_store(struct kobject *kobj,
-				     struct kobj_attribute *attr,
-				     const char *buf, size_t count)
+int ocfs2_filecheck_add_inode(struct ocfs2_super *osb,
+				     unsigned long ino)
 {
-	struct ocfs2_filecheck_args args;
 	struct ocfs2_filecheck_entry *entry;
-	struct ocfs2_filecheck_sysfs_entry *ent;
 	ssize_t ret = 0;
 
-	if (count == 0)
-		return count;
-
-	if (ocfs2_filecheck_args_parse(attr->attr.name, buf, count, &args)) {
-		mlog(ML_ERROR, "Invalid arguments for online file check\n");
-		return -EINVAL;
-	}
-
-	ent = ocfs2_filecheck_sysfs_get(kobj->parent->name);
-	if (!ent) {
-		mlog(ML_ERROR,
-		"Cannot get the corresponding entry via device basename %s\n",
-		kobj->parent->name);
-		return -ENODEV;
-	}
-
-	if (args.fa_type == OCFS2_FILECHECK_TYPE_SET) {
-		ret = ocfs2_filecheck_adjust_max(ent, args.fa_len);
-		goto exit;
-	}
-
 	entry = kmalloc(sizeof(struct ocfs2_filecheck_entry), GFP_NOFS);
 	if (!entry) {
 		ret = -ENOMEM;
 		goto exit;
 	}
 
-	spin_lock(&ent->fs_fcheck->fc_lock);
-	if ((ent->fs_fcheck->fc_size >= ent->fs_fcheck->fc_max) &&
-	    (ent->fs_fcheck->fc_done == 0)) {
+	spin_lock(&osb->fc_lock);
+	if ((osb->fc_size >= osb->fc_max) &&
+	    (osb->fc_done == 0)) {
 		mlog(ML_ERROR,
 		"Cannot do more file check "
 		"since file check queue(%u) is full now\n",
-		ent->fs_fcheck->fc_max);
+		osb->fc_max);
 		ret = -EBUSY;
 		kfree(entry);
 	} else {
-		if ((ent->fs_fcheck->fc_size >= ent->fs_fcheck->fc_max) &&
-		    (ent->fs_fcheck->fc_done > 0)) {
+		if ((osb->fc_size >= osb->fc_max) &&
+		    (osb->fc_done > 0)) {
 			/* Delete the oldest entry which was done,
 			 * make sure the entry size in list does
 			 * not exceed maximum value
 			 */
-			BUG_ON(!ocfs2_filecheck_erase_entry(ent));
+			BUG_ON(!ocfs2_filecheck_erase_entry(osb));
 		}
 
-		entry->fe_ino = args.fa_ino;
-		entry->fe_type = args.fa_type;
+		entry->fe_ino = ino;
+		entry->fe_type = OCFS2_FILECHECK_TYPE_CHK;
 		entry->fe_done = 0;
 		entry->fe_status = OCFS2_FILECHECK_ERR_INPROGRESS;
-		list_add_tail(&entry->fe_list, &ent->fs_fcheck->fc_head);
-		ent->fs_fcheck->fc_size++;
+		list_add_tail(&entry->fe_list, &osb->file_check_entries);
+		osb->fc_size++;
 	}
-	spin_unlock(&ent->fs_fcheck->fc_lock);
+	spin_unlock(&osb->fc_lock);
 
 	if (!ret)
-		ocfs2_filecheck_handle_entry(ent, entry);
+		ocfs2_filecheck_handle_entry(osb, entry);
 
 exit:
-	ocfs2_filecheck_sysfs_put(ent);
-	return (!ret ? count : ret);
+	return ret;
 }
diff --git a/fs/ocfs2/filecheck.h b/fs/ocfs2/filecheck.h
index e5cd002..b1a0d8c 100644
--- a/fs/ocfs2/filecheck.h
+++ b/fs/ocfs2/filecheck.h
@@ -42,8 +42,18 @@ enum {
 
 #define OCFS2_FILECHECK_ERR_START	OCFS2_FILECHECK_ERR_FAILED
 #define OCFS2_FILECHECK_ERR_END		OCFS2_FILECHECK_ERR_UNSUPPORTED
+#define OCFS2_FILECHECK_MAXSIZE         100
+#define OCFS2_FILECHECK_MINSIZE         10
+
+/* File check operation type */
+#define OCFS2_FILECHECK_TYPE_CHK  	1   /* Check a file(inode) */
+#define OCFS2_FILECHECK_TYPE_FIX  	2   /* Fix a file(inode) */
 
 int ocfs2_filecheck_create_sysfs(struct super_block *sb);
 int ocfs2_filecheck_remove_sysfs(struct super_block *sb);
+int ocfs2_filefix_inode(struct ocfs2_super *osb, unsigned long ino);
+int ocfs2_filecheck_add_inode(struct ocfs2_super *osb, unsigned long ino);
+int ocfs2_filecheck_set_max_entries(struct ocfs2_super *osb, int num);
+int ocfs2_filecheck_show(struct ocfs2_super *osb, unsigned int type, char *buf);
 
 #endif  /* FILECHECK_H */
diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
index 8e66cdf..9ced543 100644
--- a/fs/ocfs2/ocfs2.h
+++ b/fs/ocfs2/ocfs2.h
@@ -474,6 +474,13 @@ struct ocfs2_super
 	 */
 	struct workqueue_struct *ocfs2_wq;
 
+	/* file check */
+	struct list_head file_check_entries;
+	unsigned int fc_size;
+	unsigned int fc_max;
+	unsigned int fc_done;
+	spinlock_t fc_lock;
+
 	struct kobject kobj;
 	struct completion kobj_unregister;
 };
diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index 96b7a9f..a7791fa 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -2215,6 +2215,13 @@ static int ocfs2_initialize_super(struct super_block *sb,
 
 	get_random_bytes(&osb->s_next_generation, sizeof(u32));
 
+	/* file check information */
+	INIT_LIST_HEAD(&osb->file_check_entries);
+	osb->fc_max = OCFS2_FILECHECK_MINSIZE;
+	osb->fc_size = 0;
+	osb->fc_done = 0;
+	spin_lock_init(&osb->fc_lock);
+
 	/* FIXME
 	 * This should be done in ocfs2_journal_init(), but unknown
 	 * ordering issues will cause the filesystem to crash.
diff --git a/fs/ocfs2/sysfs.c b/fs/ocfs2/sysfs.c
index e21e699..81e3dd4 100644
--- a/fs/ocfs2/sysfs.c
+++ b/fs/ocfs2/sysfs.c
@@ -1,5 +1,6 @@
 #include "ocfs2.h"
 #include "sysfs.h"
+#include "filecheck.h"
 
 struct ocfs2_sb_attr {
 	struct attribute attr;
@@ -9,8 +10,12 @@ struct ocfs2_sb_attr {
 			const char *buf, size_t count);
 };
 
-#define OCFS2_SB_ATTR(_name, _mode) \
-struct ocfs2_sb_attr sb_attr_##_name = __ATTR(name, _mode, _show, _store)
+#define OCFS2_SB_ATTR(_name) \
+struct ocfs2_sb_attr sb_attr_##_name = { \
+	.attr = {.name = __stringify(_name), .mode = (S_IWUSR | S_IRUGO)},  \
+	.show = _name##_show, \
+	.store = _name##_store \
+}
 
 #define OCFS2_SB_ATTR_RO(_name) \
 struct ocfs2_sb_attr sb_attr_##_name = __ATTR_RO(_name)
@@ -46,6 +51,81 @@ static ssize_t slot_num_show(struct ocfs2_super *osb,
 	return sprintf(buf, "%d\n", osb->slot_num);
 }
 
+static ssize_t file_check_show(struct ocfs2_super *osb,
+		struct ocfs2_sb_attr *attr,
+		char *buf)
+{
+	return ocfs2_filecheck_show(osb, OCFS2_FILECHECK_TYPE_CHK, buf);
+}
+
+static ssize_t file_check_store(struct ocfs2_super *osb,
+		struct ocfs2_sb_attr *attr,
+		const char *buf, size_t count)
+{
+	unsigned long t;
+	int ret;
+
+	ret = kstrtoul(skip_spaces(buf), 0, &t);
+	if (ret)
+		return ret;
+	ret = ocfs2_filecheck_add_inode(osb, t);
+	if (ret)
+		return ret;
+	return count;
+}
+
+static ssize_t file_fix_show(struct ocfs2_super *osb,
+		struct ocfs2_sb_attr *attr,
+		char *buf)
+{
+	return ocfs2_filecheck_show(osb, OCFS2_FILECHECK_TYPE_FIX, buf);
+}
+
+static ssize_t file_fix_store(struct ocfs2_super *osb,
+		struct ocfs2_sb_attr *attr,
+		const char *buf, size_t count)
+{
+	unsigned long t;
+	int ret;
+
+	ret = kstrtoul(skip_spaces(buf), 0, &t);
+	if (ret)
+		return ret;
+	ret = ocfs2_filecheck_add_inode(osb, t);
+	if (ret)
+		return ret;
+	return count;
+}
+
+static ssize_t file_check_max_entries_show(struct ocfs2_super *osb,
+		struct ocfs2_sb_attr *attr,
+		char *buf)
+{
+	int len = 0;
+	spin_lock(&osb->fc_lock);
+	/* Show done, current size and max */
+	len += sprintf(buf, "%d\t%d\t%d\n", osb->fc_done, osb->fc_size,
+			osb->fc_max);
+	spin_unlock(&osb->fc_lock);
+	return len;
+}
+
+static ssize_t file_check_max_entries_store(struct ocfs2_super *osb,
+		struct ocfs2_sb_attr *attr, const char *buf, size_t count)
+{
+
+	unsigned long t;
+	int ret;
+
+	ret = kstrtoul(skip_spaces(buf), 0, &t);
+	if (ret)
+		return ret;
+	ret = ocfs2_filecheck_set_max_entries(osb, (int)t);
+	if (ret)
+		return ret;
+	return count;
+}
+
 static void sb_release(struct kobject *kobj)
 {
 	struct ocfs2_super *osb = container_of(kobj, struct ocfs2_super, kobj);
@@ -58,8 +138,15 @@ static const struct sysfs_ops ocfs2_sb_sysfs_ops = {
 };
 
 static OCFS2_SB_ATTR_RO(slot_num);
+static OCFS2_SB_ATTR(file_check);
+static OCFS2_SB_ATTR(file_fix);
+static OCFS2_SB_ATTR(file_check_max_entries);
+
 static struct attribute *ocfs2_sb_attrs[] = {
 	&sb_attr_slot_num.attr,
+	&sb_attr_file_check.attr,
+	&sb_attr_file_fix.attr,
+	&sb_attr_file_check_max_entries.attr,
 	NULL
 };
 
-- 
2.6.6

[Ocfs2-devel] [PATCH 2/5] Use the sysfs interface for creating filecheck files

[Gang He]

Hello Goldwyn,

Please see my comments inline.
I just suggest to re-use the existing code as most as possible, since the code was tested by us.

Thanks
Gang


>>> 
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> This reduces the code base and removes unnecessary data structures
> for filecheck information since all information is stored in ocfs2_super
> now.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/ocfs2/filecheck.c | 440 ++++++---------------------------------------------
>  fs/ocfs2/filecheck.h |  10 ++
>  fs/ocfs2/ocfs2.h     |   7 +
>  fs/ocfs2/super.c     |   7 +
>  fs/ocfs2/sysfs.c     |  91 ++++++++++-
>  5 files changed, 161 insertions(+), 394 deletions(-)
> 
> diff --git a/fs/ocfs2/filecheck.c b/fs/ocfs2/filecheck.c
> index 2cabbcf..0b41967 100644
> --- a/fs/ocfs2/filecheck.c
> +++ b/fs/ocfs2/filecheck.c
> @@ -17,15 +17,7 @@
>   * General Public License for more details.
>   */
>  
> -#include <linux/list.h>
> -#include <linux/spinlock.h>
> -#include <linux/module.h>
> -#include <linux/slab.h>
> -#include <linux/kmod.h>
>  #include <linux/fs.h>
> -#include <linux/kobject.h>
> -#include <linux/sysfs.h>
> -#include <linux/sysctl.h>
>  #include <cluster/masklog.h>
>  
>  #include "ocfs2.h"
> @@ -53,36 +45,6 @@ static const char * const ocfs2_filecheck_errs[] = {
>  	"UNSUPPORTED"
>  };
>  
> -static DEFINE_SPINLOCK(ocfs2_filecheck_sysfs_lock);
> -static LIST_HEAD(ocfs2_filecheck_sysfs_list);
> -

> -struct ocfs2_filecheck {
> -	struct list_head fc_head;	/* File check entry list head */
> -	spinlock_t fc_lock;
> -	unsigned int fc_max;	/* Maximum number of entry in list */
> -	unsigned int fc_size;	/* Current entry count in list */
> -	unsigned int fc_done;	/* Finished entry count in list */
> -};
Gang: we can reuse this structure, please move this structure into struct ocfs2_super, instead of defining a few members in struct ocfs2_super.
We should use a separate structure to handle all the file check related thing, make the code simple and easy to maintain.


> -
> -struct ocfs2_filecheck_sysfs_entry {	/* sysfs entry per mounting */
> -	struct list_head fs_list;
> -	atomic_t fs_count;
> -	struct super_block *fs_sb;
> -	struct kset *fs_devicekset;
> -	struct kset *fs_fcheckkset;
> -	struct ocfs2_filecheck *fs_fcheck;
> -};
Gang: I think that we can not delete this structure directly, some members are still useful.
e.g. fs_count, which is used to track if there are any pending file check entries, can be moved into struct ocfs2_filecheck.
kset members maybe be kept in struct ocfs2_filecheck.

> -
> -#define OCFS2_FILECHECK_MAXSIZE		100
> -#define OCFS2_FILECHECK_MINSIZE		10
Gang: any file will reference these two macro? 
if not, that means these are not interfaces, should be kept in source file.

> -
> -/* File check operation type */
> -enum {
> -	OCFS2_FILECHECK_TYPE_CHK = 0,	/* Check a file(inode) */
> -	OCFS2_FILECHECK_TYPE_FIX,	/* Fix a file(inode) */
> -	OCFS2_FILECHECK_TYPE_SET = 100	/* Set entry list maximum size */
> -};
Gang: delete the member OCFS2_FILECHECK_TYPE_SET, 
Move this enum to the header file if any other source file will use, otherwise should be kept in source file.

> -
>  struct ocfs2_filecheck_entry {
>  	struct list_head fe_list;
>  	unsigned long fe_ino;
> @@ -91,14 +53,6 @@ struct ocfs2_filecheck_entry {
>  	unsigned int fe_status:31;
>  };
>  
> -struct ocfs2_filecheck_args {
> -	unsigned int fa_type;
> -	union {
> -		unsigned long fa_ino;
> -		unsigned int fa_len;
> -	};
> -};
> -
>  static const char *
>  ocfs2_filecheck_error(int errno)
>  {
> @@ -110,321 +64,51 @@ ocfs2_filecheck_error(int errno)
>  	return ocfs2_filecheck_errs[errno - OCFS2_FILECHECK_ERR_START + 1];
>  }
>  
> -static ssize_t ocfs2_filecheck_show(struct kobject *kobj,
> -				    struct kobj_attribute *attr,
> -				    char *buf);
> -static ssize_t ocfs2_filecheck_store(struct kobject *kobj,
> -				     struct kobj_attribute *attr,
> -				     const char *buf, size_t count);
> -static struct kobj_attribute ocfs2_attr_filecheck_chk =
> -					__ATTR(check, S_IRUSR | S_IWUSR,
> -					ocfs2_filecheck_show,
> -					ocfs2_filecheck_store);
> -static struct kobj_attribute ocfs2_attr_filecheck_fix =
> -					__ATTR(fix, S_IRUSR | S_IWUSR,
> -					ocfs2_filecheck_show,
> -					ocfs2_filecheck_store);
> -static struct kobj_attribute ocfs2_attr_filecheck_set =
> -					__ATTR(set, S_IRUSR | S_IWUSR,
> -					ocfs2_filecheck_show,
> -					ocfs2_filecheck_store);
> -
> -static int ocfs2_filecheck_sysfs_wait(atomic_t *p)
> -{
> -	schedule();
> -	return 0;
> -}
> -
> -static void
> -ocfs2_filecheck_sysfs_free(struct ocfs2_filecheck_sysfs_entry *entry)
> -{
> -	struct ocfs2_filecheck_entry *p;
> -
> -	if (!atomic_dec_and_test(&entry->fs_count))
> -		wait_on_atomic_t(&entry->fs_count, ocfs2_filecheck_sysfs_wait,
> -				 TASK_UNINTERRUPTIBLE);
> -
> -	spin_lock(&entry->fs_fcheck->fc_lock);
> -	while (!list_empty(&entry->fs_fcheck->fc_head)) {
> -		p = list_first_entry(&entry->fs_fcheck->fc_head,
> -				     struct ocfs2_filecheck_entry, fe_list);
> -		list_del(&p->fe_list);
> -		BUG_ON(!p->fe_done); /* To free a undone file check entry */
> -		kfree(p);
> -	}
> -	spin_unlock(&entry->fs_fcheck->fc_lock);
> -
> -	kset_unregister(entry->fs_fcheckkset);
> -	kset_unregister(entry->fs_devicekset);
> -	kfree(entry->fs_fcheck);
> -	kfree(entry);
> -}
Gang: we cannot delete the function ocfs2_filecheck_sysfs_free() directly, this will be used to free the file check entries.
Please keep the code, otherwise, it will lead to memory leak.

> -
> -static void
> -ocfs2_filecheck_sysfs_add(struct ocfs2_filecheck_sysfs_entry *entry)
> -{
> -	spin_lock(&ocfs2_filecheck_sysfs_lock);
> -	list_add_tail(&entry->fs_list, &ocfs2_filecheck_sysfs_list);
> -	spin_unlock(&ocfs2_filecheck_sysfs_lock);
> -}
> -
> -static int ocfs2_filecheck_sysfs_del(const char *devname)
> -{
> -	struct ocfs2_filecheck_sysfs_entry *p;
> -
> -	spin_lock(&ocfs2_filecheck_sysfs_lock);
> -	list_for_each_entry(p, &ocfs2_filecheck_sysfs_list, fs_list) {
> -		if (!strcmp(p->fs_sb->s_id, devname)) {
> -			list_del(&p->fs_list);
> -			spin_unlock(&ocfs2_filecheck_sysfs_lock);
> -			ocfs2_filecheck_sysfs_free(p);
> -			return 0;
> -		}
> -	}
> -	spin_unlock(&ocfs2_filecheck_sysfs_lock);
> -	return 1;
> -}
> -
> -static void
> -ocfs2_filecheck_sysfs_put(struct ocfs2_filecheck_sysfs_entry *entry)
> -{
> -	if (atomic_dec_and_test(&entry->fs_count))
> -		wake_up_atomic_t(&entry->fs_count);
> -}
> -
> -static struct ocfs2_filecheck_sysfs_entry *
> -ocfs2_filecheck_sysfs_get(const char *devname)
> -{
> -	struct ocfs2_filecheck_sysfs_entry *p = NULL;
> -
> -	spin_lock(&ocfs2_filecheck_sysfs_lock);
> -	list_for_each_entry(p, &ocfs2_filecheck_sysfs_list, fs_list) {
> -		if (!strcmp(p->fs_sb->s_id, devname)) {
> -			atomic_inc(&p->fs_count);
> -			spin_unlock(&ocfs2_filecheck_sysfs_lock);
> -			return p;
> -		}
> -	}
> -	spin_unlock(&ocfs2_filecheck_sysfs_lock);
> -	return NULL;
> -}
Gang: We can't delete these code directly, fs_count variable is used to track if there is any process to use file check related data.
Please consider this variable before delete this part code.
 

> -int ocfs2_filecheck_create_sysfs(struct super_block *sb)
> -{
> -	int ret = 0;
> -	struct kset *device_kset = NULL;
> -	struct kset *fcheck_kset = NULL;
> -	struct ocfs2_filecheck *fcheck = NULL;
> -	struct ocfs2_filecheck_sysfs_entry *entry = NULL;
> -	struct attribute **attrs = NULL;
> -	struct attribute_group attrgp;
> -
> -	if (!ocfs2_kset)
> -		return -ENOMEM;
> -
> -	attrs = kmalloc(sizeof(struct attribute *) * 4, GFP_NOFS);
> -	if (!attrs) {
> -		ret = -ENOMEM;
> -		goto error;
> -	} else {
> -		attrs[0] = &ocfs2_attr_filecheck_chk.attr;
> -		attrs[1] = &ocfs2_attr_filecheck_fix.attr;
> -		attrs[2] = &ocfs2_attr_filecheck_set.attr;
> -		attrs[3] = NULL;
> -		memset(&attrgp, 0, sizeof(attrgp));
> -		attrgp.attrs = attrs;
> -	}
> -
> -	fcheck = kmalloc(sizeof(struct ocfs2_filecheck), GFP_NOFS);
> -	if (!fcheck) {
> -		ret = -ENOMEM;
> -		goto error;
> -	} else {
> -		INIT_LIST_HEAD(&fcheck->fc_head);
> -		spin_lock_init(&fcheck->fc_lock);
> -		fcheck->fc_max = OCFS2_FILECHECK_MINSIZE;
> -		fcheck->fc_size = 0;
> -		fcheck->fc_done = 0;
> -	}
> -
> -	if (strlen(sb->s_id) <= 0) {
> -		mlog(ML_ERROR,
> -		"Cannot get device basename when create filecheck sysfs\n");
> -		ret = -ENODEV;
> -		goto error;
> -	}
> -
> -	device_kset = kset_create_and_add(sb->s_id, NULL, &ocfs2_kset->kobj);
> -	if (!device_kset) {
> -		ret = -ENOMEM;
> -		goto error;
> -	}
> -
> -	fcheck_kset = kset_create_and_add("filecheck", NULL,
> -					  &device_kset->kobj);
> -	if (!fcheck_kset) {
> -		ret = -ENOMEM;
> -		goto error;
> -	}
> -
> -	ret = sysfs_create_group(&fcheck_kset->kobj, &attrgp);
> -	if (ret)
> -		goto error;
> -
> -	entry = kmalloc(sizeof(struct ocfs2_filecheck_sysfs_entry), GFP_NOFS);
> -	if (!entry) {
> -		ret = -ENOMEM;
> -		goto error;
> -	} else {
> -		atomic_set(&entry->fs_count, 1);
> -		entry->fs_sb = sb;
> -		entry->fs_devicekset = device_kset;
> -		entry->fs_fcheckkset = fcheck_kset;
> -		entry->fs_fcheck = fcheck;
> -		ocfs2_filecheck_sysfs_add(entry);
> -	}
> -
> -	kfree(attrs);
> -	return 0;
> -
> -error:
> -	kfree(attrs);
> -	kfree(entry);
> -	kfree(fcheck);
> -	kset_unregister(fcheck_kset);
> -	kset_unregister(device_kset);
> -	return ret;
> -}
> -
> -int ocfs2_filecheck_remove_sysfs(struct super_block *sb)
> -{
> -	return ocfs2_filecheck_sysfs_del(sb->s_id);
> -}
Gang: this part code can be re-used after a little modification.

> -
>  static int
> -ocfs2_filecheck_erase_entries(struct ocfs2_filecheck_sysfs_entry *ent,
> +ocfs2_filecheck_erase_entries(struct ocfs2_super *,
>  			      unsigned int count);
> -static int
> -ocfs2_filecheck_adjust_max(struct ocfs2_filecheck_sysfs_entry *ent,
> -			   unsigned int len)
> +
> +int ocfs2_filecheck_set_max_entries(struct ocfs2_super *osb,
> +				int len)
>  {
>  	int ret;
>  
>  	if ((len < OCFS2_FILECHECK_MINSIZE) || (len > OCFS2_FILECHECK_MAXSIZE))
>  		return -EINVAL;
>  
> -	spin_lock(&ent->fs_fcheck->fc_lock);
> -	if (len < (ent->fs_fcheck->fc_size - ent->fs_fcheck->fc_done)) {
> +	spin_lock(&osb->fc_lock);
> +	if (len < (osb->fc_size - osb->fc_done)) {
>  		mlog(ML_ERROR,
>  		"Cannot set online file check maximum entry number "
>  		"to %u due to too many pending entries(%u)\n",
> -		len, ent->fs_fcheck->fc_size - ent->fs_fcheck->fc_done);
> +		len, osb->fc_size - osb->fc_done);
>  		ret = -EBUSY;
>  	} else {
> -		if (len < ent->fs_fcheck->fc_size)
> -			BUG_ON(!ocfs2_filecheck_erase_entries(ent,
> -				ent->fs_fcheck->fc_size - len));
> +		if (len < osb->fc_size)
> +			BUG_ON(!ocfs2_filecheck_erase_entries(osb,
> +				osb->fc_size - len));
>  
> -		ent->fs_fcheck->fc_max = len;
> +		osb->fc_max = len;
>  		ret = 0;
>  	}
> -	spin_unlock(&ent->fs_fcheck->fc_lock);
> +	spin_unlock(&osb->fc_lock);
>  
>  	return ret;
>  }
>  
> -#define OCFS2_FILECHECK_ARGS_LEN	24
> -static int
> -ocfs2_filecheck_args_get_long(const char *buf, size_t count,
> -			      unsigned long *val)
> -{
> -	char buffer[OCFS2_FILECHECK_ARGS_LEN];
> -
> -	memcpy(buffer, buf, count);
> -	buffer[count] = '\0';
> -
> -	if (kstrtoul(buffer, 0, val))
> -		return 1;
> -
> -	return 0;
> -}
> -
> -static int
> -ocfs2_filecheck_type_parse(const char *name, unsigned int *type)
> -{
> -	if (!strncmp(name, "fix", 4))
> -		*type = OCFS2_FILECHECK_TYPE_FIX;
> -	else if (!strncmp(name, "check", 6))
> -		*type = OCFS2_FILECHECK_TYPE_CHK;
> -	else if (!strncmp(name, "set", 4))
> -		*type = OCFS2_FILECHECK_TYPE_SET;
> -	else
> -		return 1;
> -
> -	return 0;
> -}
> -
> -static int
> -ocfs2_filecheck_args_parse(const char *name, const char *buf, size_t count,
> -			   struct ocfs2_filecheck_args *args)
> -{
> -	unsigned long val = 0;
> -	unsigned int type;
> -
> -	/* too short/long args length */
> -	if ((count < 1) || (count >= OCFS2_FILECHECK_ARGS_LEN))
> -		return 1;
> -
> -	if (ocfs2_filecheck_type_parse(name, &type))
> -		return 1;
> -	if (ocfs2_filecheck_args_get_long(buf, count, &val))
> -		return 1;
> -
> -	if (val <= 0)
> -		return 1;
>  
> -	args->fa_type = type;
> -	if (type == OCFS2_FILECHECK_TYPE_SET)
> -		args->fa_len = (unsigned int)val;
> -	else
> -		args->fa_ino = val;
> -
> -	return 0;
> -}
> -
Gang: please keep the ocfs2_filecheck_args_parse function, or write a similar argument checking function since the arguments are from user-space, must be check strictly.
Otherwise, this will be a way for malicious users to panic the kernel. 

> -static ssize_t ocfs2_filecheck_show(struct kobject *kobj,
> -				    struct kobj_attribute *attr,
> +int ocfs2_filecheck_show(struct ocfs2_super *osb, unsigned int type,
>  				    char *buf)
>  {
>  
>  	ssize_t ret = 0, total = 0, remain = PAGE_SIZE;
> -	unsigned int type;
>  	struct ocfs2_filecheck_entry *p;
> -	struct ocfs2_filecheck_sysfs_entry *ent;
> -
> -	if (ocfs2_filecheck_type_parse(attr->attr.name, &type))
> -		return -EINVAL;
> -
> -	ent = ocfs2_filecheck_sysfs_get(kobj->parent->name);
Gang: if delete file check data reference count mechanism, how to make sure the resource race condition (e.g. list the check results during the file system umounting.)
Please consider this race condition problem.

> -	if (!ent) {
> -		mlog(ML_ERROR,
> -		"Cannot get the corresponding entry via device basename %s\n",
> -		kobj->name);
> -		return -ENODEV;
> -	}
> -
> -	if (type == OCFS2_FILECHECK_TYPE_SET) {
> -		spin_lock(&ent->fs_fcheck->fc_lock);
> -		total = snprintf(buf, remain, "%u\n", ent->fs_fcheck->fc_max);
> -		spin_unlock(&ent->fs_fcheck->fc_lock);
> -		goto exit;
> -	}
>  
>  	ret = snprintf(buf, remain, "INO\t\tDONE\tERROR\n");
>  	total += ret;
>  	remain -= ret;
> -	spin_lock(&ent->fs_fcheck->fc_lock);
> -	list_for_each_entry(p, &ent->fs_fcheck->fc_head, fe_list) {
> +	spin_lock(&osb->fc_lock);
> +	list_for_each_entry(p, &osb->file_check_entries, fe_list) {
>  		if (p->fe_type != type)
>  			continue;
>  
> @@ -443,24 +127,21 @@ static ssize_t ocfs2_filecheck_show(struct kobject 
> *kobj,
>  		total += ret;
>  		remain -= ret;
>  	}
> -	spin_unlock(&ent->fs_fcheck->fc_lock);
> -
> -exit:
> -	ocfs2_filecheck_sysfs_put(ent);
> +	spin_unlock(&osb->fc_lock);
>  	return total;
>  }
>  
>  static int
> -ocfs2_filecheck_erase_entry(struct ocfs2_filecheck_sysfs_entry *ent)
> +ocfs2_filecheck_erase_entry(struct ocfs2_super *osb)
>  {
>  	struct ocfs2_filecheck_entry *p;
>  
> -	list_for_each_entry(p, &ent->fs_fcheck->fc_head, fe_list) {
> +	list_for_each_entry(p, &osb->file_check_entries, fe_list) {
>  		if (p->fe_done) {
>  			list_del(&p->fe_list);
>  			kfree(p);
> -			ent->fs_fcheck->fc_size--;
> -			ent->fs_fcheck->fc_done--;
> +			osb->fc_size--;
> +			osb->fc_done--;
>  			return 1;
>  		}
>  	}
> @@ -469,14 +150,14 @@ ocfs2_filecheck_erase_entry(struct 
> ocfs2_filecheck_sysfs_entry *ent)
>  }
>  
>  static int
> -ocfs2_filecheck_erase_entries(struct ocfs2_filecheck_sysfs_entry *ent,
> +ocfs2_filecheck_erase_entries(struct ocfs2_super *osb,
>  			      unsigned int count)
>  {
>  	unsigned int i = 0;
>  	unsigned int ret = 0;
>  
>  	while (i++ < count) {
> -		if (ocfs2_filecheck_erase_entry(ent))
> +		if (ocfs2_filecheck_erase_entry(osb))
>  			ret++;
>  		else
>  			break;
> @@ -486,24 +167,24 @@ ocfs2_filecheck_erase_entries(struct 
> ocfs2_filecheck_sysfs_entry *ent,
>  }
>  
>  static void
> -ocfs2_filecheck_done_entry(struct ocfs2_filecheck_sysfs_entry *ent,
> +ocfs2_filecheck_done_entry(struct ocfs2_super *osb,
>  			   struct ocfs2_filecheck_entry *entry)
>  {
>  	entry->fe_done = 1;
> -	spin_lock(&ent->fs_fcheck->fc_lock);
> -	ent->fs_fcheck->fc_done++;
> -	spin_unlock(&ent->fs_fcheck->fc_lock);
> +	spin_lock(&osb->fc_lock);
> +	osb->fc_done++;
> +	spin_unlock(&osb->fc_lock);
>  }
>  
>  static unsigned int
> -ocfs2_filecheck_handle(struct super_block *sb,
> +ocfs2_filecheck_handle(struct ocfs2_super *osb,
>  		       unsigned long ino, unsigned int flags)
>  {
>  	unsigned int ret = OCFS2_FILECHECK_ERR_SUCCESS;
>  	struct inode *inode = NULL;
>  	int rc;
>  
> -	inode = ocfs2_iget(OCFS2_SB(sb), ino, flags, 0);
> +	inode = ocfs2_iget(osb, ino, flags, 0);
>  	if (IS_ERR(inode)) {
>  		rc = (int)(-(long)inode);
>  		if (rc >= OCFS2_FILECHECK_ERR_START &&
> @@ -518,89 +199,64 @@ ocfs2_filecheck_handle(struct super_block *sb,
>  }
>  
>  static void
> -ocfs2_filecheck_handle_entry(struct ocfs2_filecheck_sysfs_entry *ent,
> +ocfs2_filecheck_handle_entry(struct ocfs2_super *osb,
>  			     struct ocfs2_filecheck_entry *entry)
>  {
>  	if (entry->fe_type == OCFS2_FILECHECK_TYPE_CHK)
> -		entry->fe_status = ocfs2_filecheck_handle(ent->fs_sb,
> +		entry->fe_status = ocfs2_filecheck_handle(osb,
>  				entry->fe_ino, OCFS2_FI_FLAG_FILECHECK_CHK);
>  	else if (entry->fe_type == OCFS2_FILECHECK_TYPE_FIX)
> -		entry->fe_status = ocfs2_filecheck_handle(ent->fs_sb,
> +		entry->fe_status = ocfs2_filecheck_handle(osb,
>  				entry->fe_ino, OCFS2_FI_FLAG_FILECHECK_FIX);
>  	else
>  		entry->fe_status = OCFS2_FILECHECK_ERR_UNSUPPORTED;
>  
> -	ocfs2_filecheck_done_entry(ent, entry);
> +	ocfs2_filecheck_done_entry(osb, entry);
>  }
>  
> -static ssize_t ocfs2_filecheck_store(struct kobject *kobj,
> -				     struct kobj_attribute *attr,
> -				     const char *buf, size_t count)
> +int ocfs2_filecheck_add_inode(struct ocfs2_super *osb,
> +				     unsigned long ino)
>  {
> -	struct ocfs2_filecheck_args args;
>  	struct ocfs2_filecheck_entry *entry;
> -	struct ocfs2_filecheck_sysfs_entry *ent;
>  	ssize_t ret = 0;
>  
> -	if (count == 0)
> -		return count;
> -
> -	if (ocfs2_filecheck_args_parse(attr->attr.name, buf, count, &args)) {
> -		mlog(ML_ERROR, "Invalid arguments for online file check\n");
> -		return -EINVAL;
> -	}
> -
> -	ent = ocfs2_filecheck_sysfs_get(kobj->parent->name);
> -	if (!ent) {
> -		mlog(ML_ERROR,
> -		"Cannot get the corresponding entry via device basename %s\n",
> -		kobj->parent->name);
> -		return -ENODEV;
> -	}
> -
> -	if (args.fa_type == OCFS2_FILECHECK_TYPE_SET) {
> -		ret = ocfs2_filecheck_adjust_max(ent, args.fa_len);
> -		goto exit;
> -	}
> -
>  	entry = kmalloc(sizeof(struct ocfs2_filecheck_entry), GFP_NOFS);
>  	if (!entry) {
>  		ret = -ENOMEM;
>  		goto exit;
>  	}
>  
> -	spin_lock(&ent->fs_fcheck->fc_lock);
> -	if ((ent->fs_fcheck->fc_size >= ent->fs_fcheck->fc_max) &&
> -	    (ent->fs_fcheck->fc_done == 0)) {
> +	spin_lock(&osb->fc_lock);
> +	if ((osb->fc_size >= osb->fc_max) &&
> +	    (osb->fc_done == 0)) {
>  		mlog(ML_ERROR,
>  		"Cannot do more file check "
>  		"since file check queue(%u) is full now\n",
> -		ent->fs_fcheck->fc_max);
> +		osb->fc_max);
>  		ret = -EBUSY;
>  		kfree(entry);
>  	} else {
> -		if ((ent->fs_fcheck->fc_size >= ent->fs_fcheck->fc_max) &&
> -		    (ent->fs_fcheck->fc_done > 0)) {
> +		if ((osb->fc_size >= osb->fc_max) &&
> +		    (osb->fc_done > 0)) {
>  			/* Delete the oldest entry which was done,
>  			 * make sure the entry size in list does
>  			 * not exceed maximum value
>  			 */
> -			BUG_ON(!ocfs2_filecheck_erase_entry(ent));
> +			BUG_ON(!ocfs2_filecheck_erase_entry(osb));
>  		}
>  
> -		entry->fe_ino = args.fa_ino;
> -		entry->fe_type = args.fa_type;
> +		entry->fe_ino = ino;
> +		entry->fe_type = OCFS2_FILECHECK_TYPE_CHK;
>  		entry->fe_done = 0;
>  		entry->fe_status = OCFS2_FILECHECK_ERR_INPROGRESS;
> -		list_add_tail(&entry->fe_list, &ent->fs_fcheck->fc_head);
> -		ent->fs_fcheck->fc_size++;
> +		list_add_tail(&entry->fe_list, &osb->file_check_entries);
> +		osb->fc_size++;
>  	}
> -	spin_unlock(&ent->fs_fcheck->fc_lock);
> +	spin_unlock(&osb->fc_lock);
>  
>  	if (!ret)
> -		ocfs2_filecheck_handle_entry(ent, entry);
> +		ocfs2_filecheck_handle_entry(osb, entry);
>  
>  exit:
> -	ocfs2_filecheck_sysfs_put(ent);
> -	return (!ret ? count : ret);
> +	return ret;
>  }
> diff --git a/fs/ocfs2/filecheck.h b/fs/ocfs2/filecheck.h
> index e5cd002..b1a0d8c 100644
> --- a/fs/ocfs2/filecheck.h
> +++ b/fs/ocfs2/filecheck.h
> @@ -42,8 +42,18 @@ enum {
>  
>  #define OCFS2_FILECHECK_ERR_START	OCFS2_FILECHECK_ERR_FAILED
>  #define OCFS2_FILECHECK_ERR_END		OCFS2_FILECHECK_ERR_UNSUPPORTED
> +#define OCFS2_FILECHECK_MAXSIZE         100
> +#define OCFS2_FILECHECK_MINSIZE         10
> +
> +/* File check operation type */
> +#define OCFS2_FILECHECK_TYPE_CHK  	1   /* Check a file(inode) */
> +#define OCFS2_FILECHECK_TYPE_FIX  	2   /* Fix a file(inode) */
Please use enum, not use macro definitions, maybe there will be more type.
OCFS2_FILECHECK_TYPE_CHK should be zero, otherwise this macro is different with previous enum definitions.

>  
>  int ocfs2_filecheck_create_sysfs(struct super_block *sb);
>  int ocfs2_filecheck_remove_sysfs(struct super_block *sb);
> +int ocfs2_filefix_inode(struct ocfs2_super *osb, unsigned long ino);
> +int ocfs2_filecheck_add_inode(struct ocfs2_super *osb, unsigned long ino);
> +int ocfs2_filecheck_set_max_entries(struct ocfs2_super *osb, int num);
> +int ocfs2_filecheck_show(struct ocfs2_super *osb, unsigned int type, char 
> *buf);
>  
>  #endif  /* FILECHECK_H */
> diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
> index 8e66cdf..9ced543 100644
> --- a/fs/ocfs2/ocfs2.h
> +++ b/fs/ocfs2/ocfs2.h
> @@ -474,6 +474,13 @@ struct ocfs2_super
>  	 */
>  	struct workqueue_struct *ocfs2_wq;
>  
> +	/* file check */
> +	struct list_head file_check_entries;
> +	unsigned int fc_size;
> +	unsigned int fc_max;
> +	unsigned int fc_done;
> +	spinlock_t fc_lock;
> +
Gang: please use a separate structure to include all the file check members, easy to read/maintain.

>  	struct kobject kobj;
>  	struct completion kobj_unregister;
>  };
> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
> index 96b7a9f..a7791fa 100644
> --- a/fs/ocfs2/super.c
> +++ b/fs/ocfs2/super.c
> @@ -2215,6 +2215,13 @@ static int ocfs2_initialize_super(struct super_block 
> *sb,
>  
>  	get_random_bytes(&osb->s_next_generation, sizeof(u32));
>  
> +	/* file check information */
> +	INIT_LIST_HEAD(&osb->file_check_entries);
> +	osb->fc_max = OCFS2_FILECHECK_MINSIZE;
> +	osb->fc_size = 0;
> +	osb->fc_done = 0;
> +	spin_lock_init(&osb->fc_lock);
> +
Gang: write a separate funtion in filecheck.c to include these line code, easy to read/maintain.

>  	/* FIXME
>  	 * This should be done in ocfs2_journal_init(), but unknown
>  	 * ordering issues will cause the filesystem to crash.
> diff --git a/fs/ocfs2/sysfs.c b/fs/ocfs2/sysfs.c
> index e21e699..81e3dd4 100644
> --- a/fs/ocfs2/sysfs.c
> +++ b/fs/ocfs2/sysfs.c
> @@ -1,5 +1,6 @@
>  #include "ocfs2.h"
>  #include "sysfs.h"
> +#include "filecheck.h"
>  
>  struct ocfs2_sb_attr {
>  	struct attribute attr;
> @@ -9,8 +10,12 @@ struct ocfs2_sb_attr {
>  			const char *buf, size_t count);
>  };
>  
> -#define OCFS2_SB_ATTR(_name, _mode) \
> -struct ocfs2_sb_attr sb_attr_##_name = __ATTR(name, _mode, _show, _store)
> +#define OCFS2_SB_ATTR(_name) \
> +struct ocfs2_sb_attr sb_attr_##_name = { \
> +	.attr = {.name = __stringify(_name), .mode = (S_IWUSR | S_IRUGO)},  \
> +	.show = _name##_show, \
> +	.store = _name##_store \
> +}
>  
>  #define OCFS2_SB_ATTR_RO(_name) \
>  struct ocfs2_sb_attr sb_attr_##_name = __ATTR_RO(_name)
> @@ -46,6 +51,81 @@ static ssize_t slot_num_show(struct ocfs2_super *osb,
>  	return sprintf(buf, "%d\n", osb->slot_num);
>  }
>  
> +static ssize_t file_check_show(struct ocfs2_super *osb,
> +		struct ocfs2_sb_attr *attr,
> +		char *buf)
> +{
> +	return ocfs2_filecheck_show(osb, OCFS2_FILECHECK_TYPE_CHK, buf);
> +}
> +
> +static ssize_t file_check_store(struct ocfs2_super *osb,
> +		struct ocfs2_sb_attr *attr,
> +		const char *buf, size_t count)
> +{
> +	unsigned long t;
> +	int ret;
> +
> +	ret = kstrtoul(skip_spaces(buf), 0, &t);
> +	if (ret)
> +		return ret;
> +	ret = ocfs2_filecheck_add_inode(osb, t);
> +	if (ret)
> +		return ret;
> +	return count;
> +}
> +
> +static ssize_t file_fix_show(struct ocfs2_super *osb,
> +		struct ocfs2_sb_attr *attr,
> +		char *buf)
> +{
> +	return ocfs2_filecheck_show(osb, OCFS2_FILECHECK_TYPE_FIX, buf);
> +}
> +
> +static ssize_t file_fix_store(struct ocfs2_super *osb,
> +		struct ocfs2_sb_attr *attr,
> +		const char *buf, size_t count)
> +{
> +	unsigned long t;
> +	int ret;
> +
> +	ret = kstrtoul(skip_spaces(buf), 0, &t);
> +	if (ret)
> +		return ret;
> +	ret = ocfs2_filecheck_add_inode(osb, t);
> +	if (ret)
> +		return ret;
> +	return count;
> +}
> +
> +static ssize_t file_check_max_entries_show(struct ocfs2_super *osb,
> +		struct ocfs2_sb_attr *attr,
> +		char *buf)
> +{
> +	int len = 0;
> +	spin_lock(&osb->fc_lock);
> +	/* Show done, current size and max */
> +	len += sprintf(buf, "%d\t%d\t%d\n", osb->fc_done, osb->fc_size,
> +			osb->fc_max);
> +	spin_unlock(&osb->fc_lock);
> +	return len;
> +}
> +
> +static ssize_t file_check_max_entries_store(struct ocfs2_super *osb,
> +		struct ocfs2_sb_attr *attr, const char *buf, size_t count)
> +{
> +
> +	unsigned long t;
> +	int ret;
> +
> +	ret = kstrtoul(skip_spaces(buf), 0, &t);
Gang: please make sure the function kstrtoul does work well since the inputed buf has not terminating null character.
This is why I write a wrapper function ocfs2_filecheck_args_get_long().

> +	if (ret)
> +		return ret;
> +	ret = ocfs2_filecheck_set_max_entries(osb, (int)t);
> +	if (ret)
> +		return ret;
> +	return count;
> +}
> +
>  static void sb_release(struct kobject *kobj)
>  {
>  	struct ocfs2_super *osb = container_of(kobj, struct ocfs2_super, kobj);
> @@ -58,8 +138,15 @@ static const struct sysfs_ops ocfs2_sb_sysfs_ops = {
>  };
>  
>  static OCFS2_SB_ATTR_RO(slot_num);
> +static OCFS2_SB_ATTR(file_check);
> +static OCFS2_SB_ATTR(file_fix);
> +static OCFS2_SB_ATTR(file_check_max_entries);
> +
>  static struct attribute *ocfs2_sb_attrs[] = {
>  	&sb_attr_slot_num.attr,
> +	&sb_attr_file_check.attr,
> +	&sb_attr_file_fix.attr,
> +	&sb_attr_file_check_max_entries.attr,
>  	NULL
>  };
Gang: as I said in the first patch, please make all the file check related attributes in a separate sub directory.
It will let us to be easy to read/debug, especially if there is any reference count leaking problem.

>  
> -- 
> 2.6.6
> 
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com 
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel

[Ocfs2-devel] [PATCH 2/5] Use the sysfs interface for creating filecheck files

[Goldwyn Rodrigues]

On 05/30/2016 04:23 AM, Gang He wrote:
> Hello Goldwyn,
>
> Please see my comments inline.
> I just suggest to re-use the existing code as most as possible, since the code was tested by us.
>
> Thanks
> Gang
>
>
>>>>
>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>
>> This reduces the code base and removes unnecessary data structures
>> for filecheck information since all information is stored in ocfs2_super
>> now.
>>
>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>> ---
>>   fs/ocfs2/filecheck.c | 440 ++++++---------------------------------------------
>>   fs/ocfs2/filecheck.h |  10 ++
>>   fs/ocfs2/ocfs2.h     |   7 +
>>   fs/ocfs2/super.c     |   7 +
>>   fs/ocfs2/sysfs.c     |  91 ++++++++++-
>>   5 files changed, 161 insertions(+), 394 deletions(-)
>>
>> diff --git a/fs/ocfs2/filecheck.c b/fs/ocfs2/filecheck.c
>> index 2cabbcf..0b41967 100644
>> --- a/fs/ocfs2/filecheck.c
>> +++ b/fs/ocfs2/filecheck.c
>> @@ -17,15 +17,7 @@
>>    * General Public License for more details.
>>    */
>>
>> -#include <linux/list.h>
>> -#include <linux/spinlock.h>
>> -#include <linux/module.h>
>> -#include <linux/slab.h>
>> -#include <linux/kmod.h>
>>   #include <linux/fs.h>
>> -#include <linux/kobject.h>
>> -#include <linux/sysfs.h>
>> -#include <linux/sysctl.h>
>>   #include <cluster/masklog.h>
>>
>>   #include "ocfs2.h"
>> @@ -53,36 +45,6 @@ static const char * const ocfs2_filecheck_errs[] = {
>>   	"UNSUPPORTED"
>>   };
>>
>> -static DEFINE_SPINLOCK(ocfs2_filecheck_sysfs_lock);
>> -static LIST_HEAD(ocfs2_filecheck_sysfs_list);
>> -
>
>> -struct ocfs2_filecheck {
>> -	struct list_head fc_head;	/* File check entry list head */
>> -	spinlock_t fc_lock;
>> -	unsigned int fc_max;	/* Maximum number of entry in list */
>> -	unsigned int fc_size;	/* Current entry count in list */
>> -	unsigned int fc_done;	/* Finished entry count in list */
>> -};
> Gang: we can reuse this structure, please move this structure into struct ocfs2_super, instead of defining a few members in struct ocfs2_super.
> We should use a separate structure to handle all the file check related thing, make the code simple and easy to maintain.


Not sure why you are insistent on keeping a separate structure when it 
would not be references from different structures. Keeping a prefix of 
fc will make sure it is for filecheck attributes.

>
>
>> -
>> -struct ocfs2_filecheck_sysfs_entry {	/* sysfs entry per mounting */
>> -	struct list_head fs_list;
>> -	atomic_t fs_count;
>> -	struct super_block *fs_sb;
>> -	struct kset *fs_devicekset;
>> -	struct kset *fs_fcheckkset;
>> -	struct ocfs2_filecheck *fs_fcheck;
>> -};
> Gang: I think that we can not delete this structure directly, some members are still useful.
> e.g. fs_count, which is used to track if there are any pending file check entries, can be moved into struct ocfs2_filecheck.
> kset members maybe be kept in struct ocfs2_filecheck.

Okay, I will fix that.

>
>> -
>> -#define OCFS2_FILECHECK_MAXSIZE		100
>> -#define OCFS2_FILECHECK_MINSIZE		10
> Gang: any file will reference these two macro?
> if not, that means these are not interfaces, should be kept in source file.

Yes, they are referenced by multiple files.

>
>> -
>> -/* File check operation type */
>> -enum {
>> -	OCFS2_FILECHECK_TYPE_CHK = 0,	/* Check a file(inode) */
>> -	OCFS2_FILECHECK_TYPE_FIX,	/* Fix a file(inode) */
>> -	OCFS2_FILECHECK_TYPE_SET = 100	/* Set entry list maximum size */
>> -};
> Gang: delete the member OCFS2_FILECHECK_TYPE_SET,
> Move this enum to the header file if any other source file will use, otherwise should be kept in source file.

The idea of removing this structures was to simplify the code. More 
structures make more complicated code.

>
>> -
>>   struct ocfs2_filecheck_entry {
>>   	struct list_head fe_list;
>>   	unsigned long fe_ino;
>> @@ -91,14 +53,6 @@ struct ocfs2_filecheck_entry {
>>   	unsigned int fe_status:31;
>>   };
>>
>> -struct ocfs2_filecheck_args {
>> -	unsigned int fa_type;
>> -	union {
>> -		unsigned long fa_ino;
>> -		unsigned int fa_len;
>> -	};
>> -};
>> -
>>   static const char *
>>   ocfs2_filecheck_error(int errno)
>>   {
>> @@ -110,321 +64,51 @@ ocfs2_filecheck_error(int errno)
>>   	return ocfs2_filecheck_errs[errno - OCFS2_FILECHECK_ERR_START + 1];
>>   }
>>
>> -static ssize_t ocfs2_filecheck_show(struct kobject *kobj,
>> -				    struct kobj_attribute *attr,
>> -				    char *buf);
>> -static ssize_t ocfs2_filecheck_store(struct kobject *kobj,
>> -				     struct kobj_attribute *attr,
>> -				     const char *buf, size_t count);
>> -static struct kobj_attribute ocfs2_attr_filecheck_chk =
>> -					__ATTR(check, S_IRUSR | S_IWUSR,
>> -					ocfs2_filecheck_show,
>> -					ocfs2_filecheck_store);
>> -static struct kobj_attribute ocfs2_attr_filecheck_fix =
>> -					__ATTR(fix, S_IRUSR | S_IWUSR,
>> -					ocfs2_filecheck_show,
>> -					ocfs2_filecheck_store);
>> -static struct kobj_attribute ocfs2_attr_filecheck_set =
>> -					__ATTR(set, S_IRUSR | S_IWUSR,
>> -					ocfs2_filecheck_show,
>> -					ocfs2_filecheck_store);
>> -
>> -static int ocfs2_filecheck_sysfs_wait(atomic_t *p)
>> -{
>> -	schedule();
>> -	return 0;
>> -}
>> -
>> -static void
>> -ocfs2_filecheck_sysfs_free(struct ocfs2_filecheck_sysfs_entry *entry)
>> -{
>> -	struct ocfs2_filecheck_entry *p;
>> -
>> -	if (!atomic_dec_and_test(&entry->fs_count))
>> -		wait_on_atomic_t(&entry->fs_count, ocfs2_filecheck_sysfs_wait,
>> -				 TASK_UNINTERRUPTIBLE);
>> -
>> -	spin_lock(&entry->fs_fcheck->fc_lock);
>> -	while (!list_empty(&entry->fs_fcheck->fc_head)) {
>> -		p = list_first_entry(&entry->fs_fcheck->fc_head,
>> -				     struct ocfs2_filecheck_entry, fe_list);
>> -		list_del(&p->fe_list);
>> -		BUG_ON(!p->fe_done); /* To free a undone file check entry */
>> -		kfree(p);
>> -	}
>> -	spin_unlock(&entry->fs_fcheck->fc_lock);
>> -
>> -	kset_unregister(entry->fs_fcheckkset);
>> -	kset_unregister(entry->fs_devicekset);
>> -	kfree(entry->fs_fcheck);
>> -	kfree(entry);
>> -}
> Gang: we cannot delete the function ocfs2_filecheck_sysfs_free() directly, this will be used to free the file check entries.
> Please keep the code, otherwise, it will lead to memory leak.

Yes, there should a cleanup routine.

>
>> -
>> -static void
>> -ocfs2_filecheck_sysfs_add(struct ocfs2_filecheck_sysfs_entry *entry)
>> -{
>> -	spin_lock(&ocfs2_filecheck_sysfs_lock);
>> -	list_add_tail(&entry->fs_list, &ocfs2_filecheck_sysfs_list);
>> -	spin_unlock(&ocfs2_filecheck_sysfs_lock);
>> -}
>> -
>> -static int ocfs2_filecheck_sysfs_del(const char *devname)
>> -{
>> -	struct ocfs2_filecheck_sysfs_entry *p;
>> -
>> -	spin_lock(&ocfs2_filecheck_sysfs_lock);
>> -	list_for_each_entry(p, &ocfs2_filecheck_sysfs_list, fs_list) {
>> -		if (!strcmp(p->fs_sb->s_id, devname)) {
>> -			list_del(&p->fs_list);
>> -			spin_unlock(&ocfs2_filecheck_sysfs_lock);
>> -			ocfs2_filecheck_sysfs_free(p);
>> -			return 0;
>> -		}
>> -	}
>> -	spin_unlock(&ocfs2_filecheck_sysfs_lock);
>> -	return 1;
>> -}
>> -
>> -static void
>> -ocfs2_filecheck_sysfs_put(struct ocfs2_filecheck_sysfs_entry *entry)
>> -{
>> -	if (atomic_dec_and_test(&entry->fs_count))
>> -		wake_up_atomic_t(&entry->fs_count);
>> -}
>> -
>> -static struct ocfs2_filecheck_sysfs_entry *
>> -ocfs2_filecheck_sysfs_get(const char *devname)
>> -{
>> -	struct ocfs2_filecheck_sysfs_entry *p = NULL;
>> -
>> -	spin_lock(&ocfs2_filecheck_sysfs_lock);
>> -	list_for_each_entry(p, &ocfs2_filecheck_sysfs_list, fs_list) {
>> -		if (!strcmp(p->fs_sb->s_id, devname)) {
>> -			atomic_inc(&p->fs_count);
>> -			spin_unlock(&ocfs2_filecheck_sysfs_lock);
>> -			return p;
>> -		}
>> -	}
>> -	spin_unlock(&ocfs2_filecheck_sysfs_lock);
>> -	return NULL;
>> -}
> Gang: We can't delete these code directly, fs_count variable is used to track if there is any process to use file check related data.
> Please consider this variable before delete this part code.
>
>
>> -int ocfs2_filecheck_create_sysfs(struct super_block *sb)
>> -{
>> -	int ret = 0;
>> -	struct kset *device_kset = NULL;
>> -	struct kset *fcheck_kset = NULL;
>> -	struct ocfs2_filecheck *fcheck = NULL;
>> -	struct ocfs2_filecheck_sysfs_entry *entry = NULL;
>> -	struct attribute **attrs = NULL;
>> -	struct attribute_group attrgp;
>> -
>> -	if (!ocfs2_kset)
>> -		return -ENOMEM;
>> -
>> -	attrs = kmalloc(sizeof(struct attribute *) * 4, GFP_NOFS);
>> -	if (!attrs) {
>> -		ret = -ENOMEM;
>> -		goto error;
>> -	} else {
>> -		attrs[0] = &ocfs2_attr_filecheck_chk.attr;
>> -		attrs[1] = &ocfs2_attr_filecheck_fix.attr;
>> -		attrs[2] = &ocfs2_attr_filecheck_set.attr;
>> -		attrs[3] = NULL;
>> -		memset(&attrgp, 0, sizeof(attrgp));
>> -		attrgp.attrs = attrs;
>> -	}
>> -
>> -	fcheck = kmalloc(sizeof(struct ocfs2_filecheck), GFP_NOFS);
>> -	if (!fcheck) {
>> -		ret = -ENOMEM;
>> -		goto error;
>> -	} else {
>> -		INIT_LIST_HEAD(&fcheck->fc_head);
>> -		spin_lock_init(&fcheck->fc_lock);
>> -		fcheck->fc_max = OCFS2_FILECHECK_MINSIZE;
>> -		fcheck->fc_size = 0;
>> -		fcheck->fc_done = 0;
>> -	}
>> -
>> -	if (strlen(sb->s_id) <= 0) {
>> -		mlog(ML_ERROR,
>> -		"Cannot get device basename when create filecheck sysfs\n");
>> -		ret = -ENODEV;
>> -		goto error;
>> -	}
>> -
>> -	device_kset = kset_create_and_add(sb->s_id, NULL, &ocfs2_kset->kobj);
>> -	if (!device_kset) {
>> -		ret = -ENOMEM;
>> -		goto error;
>> -	}
>> -
>> -	fcheck_kset = kset_create_and_add("filecheck", NULL,
>> -					  &device_kset->kobj);
>> -	if (!fcheck_kset) {
>> -		ret = -ENOMEM;
>> -		goto error;
>> -	}
>> -
>> -	ret = sysfs_create_group(&fcheck_kset->kobj, &attrgp);
>> -	if (ret)
>> -		goto error;
>> -
>> -	entry = kmalloc(sizeof(struct ocfs2_filecheck_sysfs_entry), GFP_NOFS);
>> -	if (!entry) {
>> -		ret = -ENOMEM;
>> -		goto error;
>> -	} else {
>> -		atomic_set(&entry->fs_count, 1);
>> -		entry->fs_sb = sb;
>> -		entry->fs_devicekset = device_kset;
>> -		entry->fs_fcheckkset = fcheck_kset;
>> -		entry->fs_fcheck = fcheck;
>> -		ocfs2_filecheck_sysfs_add(entry);
>> -	}
>> -
>> -	kfree(attrs);
>> -	return 0;
>> -
>> -error:
>> -	kfree(attrs);
>> -	kfree(entry);
>> -	kfree(fcheck);
>> -	kset_unregister(fcheck_kset);
>> -	kset_unregister(device_kset);
>> -	return ret;
>> -}
>> -
>> -int ocfs2_filecheck_remove_sysfs(struct super_block *sb)
>> -{
>> -	return ocfs2_filecheck_sysfs_del(sb->s_id);
>> -}
> Gang: this part code can be re-used after a little modification.

For what?

>
>> -
>>   static int
>> -ocfs2_filecheck_erase_entries(struct ocfs2_filecheck_sysfs_entry *ent,
>> +ocfs2_filecheck_erase_entries(struct ocfs2_super *,
>>   			      unsigned int count);
>> -static int
>> -ocfs2_filecheck_adjust_max(struct ocfs2_filecheck_sysfs_entry *ent,
>> -			   unsigned int len)
>> +
>> +int ocfs2_filecheck_set_max_entries(struct ocfs2_super *osb,
>> +				int len)
>>   {
>>   	int ret;
>>
>>   	if ((len < OCFS2_FILECHECK_MINSIZE) || (len > OCFS2_FILECHECK_MAXSIZE))
>>   		return -EINVAL;
>>
>> -	spin_lock(&ent->fs_fcheck->fc_lock);
>> -	if (len < (ent->fs_fcheck->fc_size - ent->fs_fcheck->fc_done)) {
>> +	spin_lock(&osb->fc_lock);
>> +	if (len < (osb->fc_size - osb->fc_done)) {
>>   		mlog(ML_ERROR,
>>   		"Cannot set online file check maximum entry number "
>>   		"to %u due to too many pending entries(%u)\n",
>> -		len, ent->fs_fcheck->fc_size - ent->fs_fcheck->fc_done);
>> +		len, osb->fc_size - osb->fc_done);
>>   		ret = -EBUSY;
>>   	} else {
>> -		if (len < ent->fs_fcheck->fc_size)
>> -			BUG_ON(!ocfs2_filecheck_erase_entries(ent,
>> -				ent->fs_fcheck->fc_size - len));
>> +		if (len < osb->fc_size)
>> +			BUG_ON(!ocfs2_filecheck_erase_entries(osb,
>> +				osb->fc_size - len));
>>
>> -		ent->fs_fcheck->fc_max = len;
>> +		osb->fc_max = len;
>>   		ret = 0;
>>   	}
>> -	spin_unlock(&ent->fs_fcheck->fc_lock);
>> +	spin_unlock(&osb->fc_lock);
>>
>>   	return ret;
>>   }
>>
>> -#define OCFS2_FILECHECK_ARGS_LEN	24
>> -static int
>> -ocfs2_filecheck_args_get_long(const char *buf, size_t count,
>> -			      unsigned long *val)
>> -{
>> -	char buffer[OCFS2_FILECHECK_ARGS_LEN];
>> -
>> -	memcpy(buffer, buf, count);
>> -	buffer[count] = '\0';
>> -
>> -	if (kstrtoul(buffer, 0, val))
>> -		return 1;
>> -
>> -	return 0;
>> -}
>> -
>> -static int
>> -ocfs2_filecheck_type_parse(const char *name, unsigned int *type)
>> -{
>> -	if (!strncmp(name, "fix", 4))
>> -		*type = OCFS2_FILECHECK_TYPE_FIX;
>> -	else if (!strncmp(name, "check", 6))
>> -		*type = OCFS2_FILECHECK_TYPE_CHK;
>> -	else if (!strncmp(name, "set", 4))
>> -		*type = OCFS2_FILECHECK_TYPE_SET;
>> -	else
>> -		return 1;
>> -
>> -	return 0;
>> -}
>> -
>> -static int
>> -ocfs2_filecheck_args_parse(const char *name, const char *buf, size_t count,
>> -			   struct ocfs2_filecheck_args *args)
>> -{
>> -	unsigned long val = 0;
>> -	unsigned int type;
>> -
>> -	/* too short/long args length */
>> -	if ((count < 1) || (count >= OCFS2_FILECHECK_ARGS_LEN))
>> -		return 1;
>> -
>> -	if (ocfs2_filecheck_type_parse(name, &type))
>> -		return 1;
>> -	if (ocfs2_filecheck_args_get_long(buf, count, &val))
>> -		return 1;
>> -
>> -	if (val <= 0)
>> -		return 1;
>>
>> -	args->fa_type = type;
>> -	if (type == OCFS2_FILECHECK_TYPE_SET)
>> -		args->fa_len = (unsigned int)val;
>> -	else
>> -		args->fa_ino = val;
>> -
>> -	return 0;
>> -}
>> -
> Gang: please keep the ocfs2_filecheck_args_parse function, or write a similar argument checking function since the arguments are from user-space, must be check strictly.
> Otherwise, this will be a way for malicious users to panic the kernel.

How? Do you have an example?

>
>> -static ssize_t ocfs2_filecheck_show(struct kobject *kobj,
>> -				    struct kobj_attribute *attr,
>> +int ocfs2_filecheck_show(struct ocfs2_super *osb, unsigned int type,
>>   				    char *buf)
>>   {
>>
>>   	ssize_t ret = 0, total = 0, remain = PAGE_SIZE;
>> -	unsigned int type;
>>   	struct ocfs2_filecheck_entry *p;
>> -	struct ocfs2_filecheck_sysfs_entry *ent;
>> -
>> -	if (ocfs2_filecheck_type_parse(attr->attr.name, &type))
>> -		return -EINVAL;
>> -
>> -	ent = ocfs2_filecheck_sysfs_get(kobj->parent->name);
> Gang: if delete file check data reference count mechanism, how to make sure the resource race condition (e.g. list the check results during the file system umounting.)
> Please consider this race condition problem.

Isn't spinlock enough? What is the race?

>
>> -	if (!ent) {
>> -		mlog(ML_ERROR,
>> -		"Cannot get the corresponding entry via device basename %s\n",
>> -		kobj->name);
>> -		return -ENODEV;
>> -	}
>> -
>> -	if (type == OCFS2_FILECHECK_TYPE_SET) {
>> -		spin_lock(&ent->fs_fcheck->fc_lock);
>> -		total = snprintf(buf, remain, "%u\n", ent->fs_fcheck->fc_max);
>> -		spin_unlock(&ent->fs_fcheck->fc_lock);
>> -		goto exit;
>> -	}
>>
>>   	ret = snprintf(buf, remain, "INO\t\tDONE\tERROR\n");
>>   	total += ret;
>>   	remain -= ret;
>> -	spin_lock(&ent->fs_fcheck->fc_lock);
>> -	list_for_each_entry(p, &ent->fs_fcheck->fc_head, fe_list) {
>> +	spin_lock(&osb->fc_lock);
>> +	list_for_each_entry(p, &osb->file_check_entries, fe_list) {
>>   		if (p->fe_type != type)
>>   			continue;
>>
>> @@ -443,24 +127,21 @@ static ssize_t ocfs2_filecheck_show(struct kobject
>> *kobj,
>>   		total += ret;
>>   		remain -= ret;
>>   	}
>> -	spin_unlock(&ent->fs_fcheck->fc_lock);
>> -
>> -exit:
>> -	ocfs2_filecheck_sysfs_put(ent);
>> +	spin_unlock(&osb->fc_lock);
>>   	return total;
>>   }
>>
>>   static int
>> -ocfs2_filecheck_erase_entry(struct ocfs2_filecheck_sysfs_entry *ent)
>> +ocfs2_filecheck_erase_entry(struct ocfs2_super *osb)
>>   {
>>   	struct ocfs2_filecheck_entry *p;
>>
>> -	list_for_each_entry(p, &ent->fs_fcheck->fc_head, fe_list) {
>> +	list_for_each_entry(p, &osb->file_check_entries, fe_list) {
>>   		if (p->fe_done) {
>>   			list_del(&p->fe_list);
>>   			kfree(p);
>> -			ent->fs_fcheck->fc_size--;
>> -			ent->fs_fcheck->fc_done--;
>> +			osb->fc_size--;
>> +			osb->fc_done--;
>>   			return 1;
>>   		}
>>   	}
>> @@ -469,14 +150,14 @@ ocfs2_filecheck_erase_entry(struct
>> ocfs2_filecheck_sysfs_entry *ent)
>>   }
>>
>>   static int
>> -ocfs2_filecheck_erase_entries(struct ocfs2_filecheck_sysfs_entry *ent,
>> +ocfs2_filecheck_erase_entries(struct ocfs2_super *osb,
>>   			      unsigned int count)
>>   {
>>   	unsigned int i = 0;
>>   	unsigned int ret = 0;
>>
>>   	while (i++ < count) {
>> -		if (ocfs2_filecheck_erase_entry(ent))
>> +		if (ocfs2_filecheck_erase_entry(osb))
>>   			ret++;
>>   		else
>>   			break;
>> @@ -486,24 +167,24 @@ ocfs2_filecheck_erase_entries(struct
>> ocfs2_filecheck_sysfs_entry *ent,
>>   }
>>
>>   static void
>> -ocfs2_filecheck_done_entry(struct ocfs2_filecheck_sysfs_entry *ent,
>> +ocfs2_filecheck_done_entry(struct ocfs2_super *osb,
>>   			   struct ocfs2_filecheck_entry *entry)
>>   {
>>   	entry->fe_done = 1;
>> -	spin_lock(&ent->fs_fcheck->fc_lock);
>> -	ent->fs_fcheck->fc_done++;
>> -	spin_unlock(&ent->fs_fcheck->fc_lock);
>> +	spin_lock(&osb->fc_lock);
>> +	osb->fc_done++;
>> +	spin_unlock(&osb->fc_lock);
>>   }
>>
>>   static unsigned int
>> -ocfs2_filecheck_handle(struct super_block *sb,
>> +ocfs2_filecheck_handle(struct ocfs2_super *osb,
>>   		       unsigned long ino, unsigned int flags)
>>   {
>>   	unsigned int ret = OCFS2_FILECHECK_ERR_SUCCESS;
>>   	struct inode *inode = NULL;
>>   	int rc;
>>
>> -	inode = ocfs2_iget(OCFS2_SB(sb), ino, flags, 0);
>> +	inode = ocfs2_iget(osb, ino, flags, 0);
>>   	if (IS_ERR(inode)) {
>>   		rc = (int)(-(long)inode);
>>   		if (rc >= OCFS2_FILECHECK_ERR_START &&
>> @@ -518,89 +199,64 @@ ocfs2_filecheck_handle(struct super_block *sb,
>>   }
>>
>>   static void
>> -ocfs2_filecheck_handle_entry(struct ocfs2_filecheck_sysfs_entry *ent,
>> +ocfs2_filecheck_handle_entry(struct ocfs2_super *osb,
>>   			     struct ocfs2_filecheck_entry *entry)
>>   {
>>   	if (entry->fe_type == OCFS2_FILECHECK_TYPE_CHK)
>> -		entry->fe_status = ocfs2_filecheck_handle(ent->fs_sb,
>> +		entry->fe_status = ocfs2_filecheck_handle(osb,
>>   				entry->fe_ino, OCFS2_FI_FLAG_FILECHECK_CHK);
>>   	else if (entry->fe_type == OCFS2_FILECHECK_TYPE_FIX)
>> -		entry->fe_status = ocfs2_filecheck_handle(ent->fs_sb,
>> +		entry->fe_status = ocfs2_filecheck_handle(osb,
>>   				entry->fe_ino, OCFS2_FI_FLAG_FILECHECK_FIX);
>>   	else
>>   		entry->fe_status = OCFS2_FILECHECK_ERR_UNSUPPORTED;
>>
>> -	ocfs2_filecheck_done_entry(ent, entry);
>> +	ocfs2_filecheck_done_entry(osb, entry);
>>   }
>>
>> -static ssize_t ocfs2_filecheck_store(struct kobject *kobj,
>> -				     struct kobj_attribute *attr,
>> -				     const char *buf, size_t count)
>> +int ocfs2_filecheck_add_inode(struct ocfs2_super *osb,
>> +				     unsigned long ino)
>>   {
>> -	struct ocfs2_filecheck_args args;
>>   	struct ocfs2_filecheck_entry *entry;
>> -	struct ocfs2_filecheck_sysfs_entry *ent;
>>   	ssize_t ret = 0;
>>
>> -	if (count == 0)
>> -		return count;
>> -
>> -	if (ocfs2_filecheck_args_parse(attr->attr.name, buf, count, &args)) {
>> -		mlog(ML_ERROR, "Invalid arguments for online file check\n");
>> -		return -EINVAL;
>> -	}
>> -
>> -	ent = ocfs2_filecheck_sysfs_get(kobj->parent->name);
>> -	if (!ent) {
>> -		mlog(ML_ERROR,
>> -		"Cannot get the corresponding entry via device basename %s\n",
>> -		kobj->parent->name);
>> -		return -ENODEV;
>> -	}
>> -
>> -	if (args.fa_type == OCFS2_FILECHECK_TYPE_SET) {
>> -		ret = ocfs2_filecheck_adjust_max(ent, args.fa_len);
>> -		goto exit;
>> -	}
>> -
>>   	entry = kmalloc(sizeof(struct ocfs2_filecheck_entry), GFP_NOFS);
>>   	if (!entry) {
>>   		ret = -ENOMEM;
>>   		goto exit;
>>   	}
>>
>> -	spin_lock(&ent->fs_fcheck->fc_lock);
>> -	if ((ent->fs_fcheck->fc_size >= ent->fs_fcheck->fc_max) &&
>> -	    (ent->fs_fcheck->fc_done == 0)) {
>> +	spin_lock(&osb->fc_lock);
>> +	if ((osb->fc_size >= osb->fc_max) &&
>> +	    (osb->fc_done == 0)) {
>>   		mlog(ML_ERROR,
>>   		"Cannot do more file check "
>>   		"since file check queue(%u) is full now\n",
>> -		ent->fs_fcheck->fc_max);
>> +		osb->fc_max);
>>   		ret = -EBUSY;
>>   		kfree(entry);
>>   	} else {
>> -		if ((ent->fs_fcheck->fc_size >= ent->fs_fcheck->fc_max) &&
>> -		    (ent->fs_fcheck->fc_done > 0)) {
>> +		if ((osb->fc_size >= osb->fc_max) &&
>> +		    (osb->fc_done > 0)) {
>>   			/* Delete the oldest entry which was done,
>>   			 * make sure the entry size in list does
>>   			 * not exceed maximum value
>>   			 */
>> -			BUG_ON(!ocfs2_filecheck_erase_entry(ent));
>> +			BUG_ON(!ocfs2_filecheck_erase_entry(osb));
>>   		}
>>
>> -		entry->fe_ino = args.fa_ino;
>> -		entry->fe_type = args.fa_type;
>> +		entry->fe_ino = ino;
>> +		entry->fe_type = OCFS2_FILECHECK_TYPE_CHK;
>>   		entry->fe_done = 0;
>>   		entry->fe_status = OCFS2_FILECHECK_ERR_INPROGRESS;
>> -		list_add_tail(&entry->fe_list, &ent->fs_fcheck->fc_head);
>> -		ent->fs_fcheck->fc_size++;
>> +		list_add_tail(&entry->fe_list, &osb->file_check_entries);
>> +		osb->fc_size++;
>>   	}
>> -	spin_unlock(&ent->fs_fcheck->fc_lock);
>> +	spin_unlock(&osb->fc_lock);
>>
>>   	if (!ret)
>> -		ocfs2_filecheck_handle_entry(ent, entry);
>> +		ocfs2_filecheck_handle_entry(osb, entry);
>>
>>   exit:
>> -	ocfs2_filecheck_sysfs_put(ent);
>> -	return (!ret ? count : ret);
>> +	return ret;
>>   }
>> diff --git a/fs/ocfs2/filecheck.h b/fs/ocfs2/filecheck.h
>> index e5cd002..b1a0d8c 100644
>> --- a/fs/ocfs2/filecheck.h
>> +++ b/fs/ocfs2/filecheck.h
>> @@ -42,8 +42,18 @@ enum {
>>
>>   #define OCFS2_FILECHECK_ERR_START	OCFS2_FILECHECK_ERR_FAILED
>>   #define OCFS2_FILECHECK_ERR_END		OCFS2_FILECHECK_ERR_UNSUPPORTED
>> +#define OCFS2_FILECHECK_MAXSIZE         100
>> +#define OCFS2_FILECHECK_MINSIZE         10
>> +
>> +/* File check operation type */
>> +#define OCFS2_FILECHECK_TYPE_CHK  	1   /* Check a file(inode) */
>> +#define OCFS2_FILECHECK_TYPE_FIX  	2   /* Fix a file(inode) */
> Please use enum, not use macro definitions, maybe there will be more type.
> OCFS2_FILECHECK_TYPE_CHK should be zero, otherwise this macro is different with previous enum definitions.
>
>>
>>   int ocfs2_filecheck_create_sysfs(struct super_block *sb);
>>   int ocfs2_filecheck_remove_sysfs(struct super_block *sb);
>> +int ocfs2_filefix_inode(struct ocfs2_super *osb, unsigned long ino);
>> +int ocfs2_filecheck_add_inode(struct ocfs2_super *osb, unsigned long ino);
>> +int ocfs2_filecheck_set_max_entries(struct ocfs2_super *osb, int num);
>> +int ocfs2_filecheck_show(struct ocfs2_super *osb, unsigned int type, char
>> *buf);
>>
>>   #endif  /* FILECHECK_H */
>> diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
>> index 8e66cdf..9ced543 100644
>> --- a/fs/ocfs2/ocfs2.h
>> +++ b/fs/ocfs2/ocfs2.h
>> @@ -474,6 +474,13 @@ struct ocfs2_super
>>   	 */
>>   	struct workqueue_struct *ocfs2_wq;
>>
>> +	/* file check */
>> +	struct list_head file_check_entries;
>> +	unsigned int fc_size;
>> +	unsigned int fc_max;
>> +	unsigned int fc_done;
>> +	spinlock_t fc_lock;
>> +
> Gang: please use a separate structure to include all the file check members, easy to read/maintain.


I would prefer it to be a part of ocfs2_super. It is easier to maintain 
and read.

>
>>   	struct kobject kobj;
>>   	struct completion kobj_unregister;
>>   };
>> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
>> index 96b7a9f..a7791fa 100644
>> --- a/fs/ocfs2/super.c
>> +++ b/fs/ocfs2/super.c
>> @@ -2215,6 +2215,13 @@ static int ocfs2_initialize_super(struct super_block
>> *sb,
>>
>>   	get_random_bytes(&osb->s_next_generation, sizeof(u32));
>>
>> +	/* file check information */
>> +	INIT_LIST_HEAD(&osb->file_check_entries);
>> +	osb->fc_max = OCFS2_FILECHECK_MINSIZE;
>> +	osb->fc_size = 0;
>> +	osb->fc_done = 0;
>> +	spin_lock_init(&osb->fc_lock);
>> +
> Gang: write a separate funtion in filecheck.c to include these line code, easy to read/maintain.

Why? Unless we are starting something specific to filecheck, I don't see 
any need.

>
>>   	/* FIXME
>>   	 * This should be done in ocfs2_journal_init(), but unknown
>>   	 * ordering issues will cause the filesystem to crash.
>> diff --git a/fs/ocfs2/sysfs.c b/fs/ocfs2/sysfs.c
>> index e21e699..81e3dd4 100644
>> --- a/fs/ocfs2/sysfs.c
>> +++ b/fs/ocfs2/sysfs.c
>> @@ -1,5 +1,6 @@
>>   #include "ocfs2.h"
>>   #include "sysfs.h"
>> +#include "filecheck.h"
>>
>>   struct ocfs2_sb_attr {
>>   	struct attribute attr;
>> @@ -9,8 +10,12 @@ struct ocfs2_sb_attr {
>>   			const char *buf, size_t count);
>>   };
>>
>> -#define OCFS2_SB_ATTR(_name, _mode) \
>> -struct ocfs2_sb_attr sb_attr_##_name = __ATTR(name, _mode, _show, _store)
>> +#define OCFS2_SB_ATTR(_name) \
>> +struct ocfs2_sb_attr sb_attr_##_name = { \
>> +	.attr = {.name = __stringify(_name), .mode = (S_IWUSR | S_IRUGO)},  \
>> +	.show = _name##_show, \
>> +	.store = _name##_store \
>> +}
>>
>>   #define OCFS2_SB_ATTR_RO(_name) \
>>   struct ocfs2_sb_attr sb_attr_##_name = __ATTR_RO(_name)
>> @@ -46,6 +51,81 @@ static ssize_t slot_num_show(struct ocfs2_super *osb,
>>   	return sprintf(buf, "%d\n", osb->slot_num);
>>   }
>>
>> +static ssize_t file_check_show(struct ocfs2_super *osb,
>> +		struct ocfs2_sb_attr *attr,
>> +		char *buf)
>> +{
>> +	return ocfs2_filecheck_show(osb, OCFS2_FILECHECK_TYPE_CHK, buf);
>> +}
>> +
>> +static ssize_t file_check_store(struct ocfs2_super *osb,
>> +		struct ocfs2_sb_attr *attr,
>> +		const char *buf, size_t count)
>> +{
>> +	unsigned long t;
>> +	int ret;
>> +
>> +	ret = kstrtoul(skip_spaces(buf), 0, &t);
>> +	if (ret)
>> +		return ret;
>> +	ret = ocfs2_filecheck_add_inode(osb, t);
>> +	if (ret)
>> +		return ret;
>> +	return count;
>> +}
>> +
>> +static ssize_t file_fix_show(struct ocfs2_super *osb,
>> +		struct ocfs2_sb_attr *attr,
>> +		char *buf)
>> +{
>> +	return ocfs2_filecheck_show(osb, OCFS2_FILECHECK_TYPE_FIX, buf);
>> +}
>> +
>> +static ssize_t file_fix_store(struct ocfs2_super *osb,
>> +		struct ocfs2_sb_attr *attr,
>> +		const char *buf, size_t count)
>> +{
>> +	unsigned long t;
>> +	int ret;
>> +
>> +	ret = kstrtoul(skip_spaces(buf), 0, &t);
>> +	if (ret)
>> +		return ret;
>> +	ret = ocfs2_filecheck_add_inode(osb, t);
>> +	if (ret)
>> +		return ret;
>> +	return count;
>> +}
>> +
>> +static ssize_t file_check_max_entries_show(struct ocfs2_super *osb,
>> +		struct ocfs2_sb_attr *attr,
>> +		char *buf)
>> +{
>> +	int len = 0;
>> +	spin_lock(&osb->fc_lock);
>> +	/* Show done, current size and max */
>> +	len += sprintf(buf, "%d\t%d\t%d\n", osb->fc_done, osb->fc_size,
>> +			osb->fc_max);
>> +	spin_unlock(&osb->fc_lock);
>> +	return len;
>> +}
>> +
>> +static ssize_t file_check_max_entries_store(struct ocfs2_super *osb,
>> +		struct ocfs2_sb_attr *attr, const char *buf, size_t count)
>> +{
>> +
>> +	unsigned long t;
>> +	int ret;
>> +
>> +	ret = kstrtoul(skip_spaces(buf), 0, &t);
> Gang: please make sure the function kstrtoul does work well since the inputed buf has not terminating null character.
> This is why I write a wrapper function ocfs2_filecheck_args_get_long().

Can you give an example on how this can be activated? Maybe a check on 
value < 0 may make sense.

>
>> +	if (ret)
>> +		return ret;
>> +	ret = ocfs2_filecheck_set_max_entries(osb, (int)t);
>> +	if (ret)
>> +		return ret;
>> +	return count;
>> +}
>> +
>>   static void sb_release(struct kobject *kobj)
>>   {
>>   	struct ocfs2_super *osb = container_of(kobj, struct ocfs2_super, kobj);
>> @@ -58,8 +138,15 @@ static const struct sysfs_ops ocfs2_sb_sysfs_ops = {
>>   };
>>
>>   static OCFS2_SB_ATTR_RO(slot_num);
>> +static OCFS2_SB_ATTR(file_check);
>> +static OCFS2_SB_ATTR(file_fix);
>> +static OCFS2_SB_ATTR(file_check_max_entries);
>> +
>>   static struct attribute *ocfs2_sb_attrs[] = {
>>   	&sb_attr_slot_num.attr,
>> +	&sb_attr_file_check.attr,
>> +	&sb_attr_file_fix.attr,
>> +	&sb_attr_file_check_max_entries.attr,
>>   	NULL
>>   };
> Gang: as I said in the first patch, please make all the file check related attributes in a separate sub directory.
> It will let us to be easy to read/debug, especially if there is any reference count leaking problem.

The main reason of the code is to remove unnecessary data structures 
which is making the code hard to understand. I don't see a point in 
keeping them in a separate structure besides adding more indirections.


-- 
Goldwyn

[Ocfs2-devel] [PATCH 2/5] Use the sysfs interface for creating filecheck files

[Gang He]

>>> 

> 
> On 05/30/2016 04:23 AM, Gang He wrote:
>> Hello Goldwyn,
>>
>> Please see my comments inline.
>> I just suggest to re-use the existing code as most as possible, since the 
> code was tested by us.
>>
>> Thanks
>> Gang
>>
>>
>>>>>
>>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>>
>>> This reduces the code base and removes unnecessary data structures
>>> for filecheck information since all information is stored in ocfs2_super
>>> now.
>>>
>>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>> ---
>>>   fs/ocfs2/filecheck.c | 440 
> ++++++---------------------------------------------
>>>   fs/ocfs2/filecheck.h |  10 ++
>>>   fs/ocfs2/ocfs2.h     |   7 +
>>>   fs/ocfs2/super.c     |   7 +
>>>   fs/ocfs2/sysfs.c     |  91 ++++++++++-
>>>   5 files changed, 161 insertions(+), 394 deletions(-)
>>>
>>> diff --git a/fs/ocfs2/filecheck.c b/fs/ocfs2/filecheck.c
>>> index 2cabbcf..0b41967 100644
>>> --- a/fs/ocfs2/filecheck.c
>>> +++ b/fs/ocfs2/filecheck.c
>>> @@ -17,15 +17,7 @@
>>>    * General Public License for more details.
>>>    */
>>>
>>> -#include <linux/list.h>
>>> -#include <linux/spinlock.h>
>>> -#include <linux/module.h>
>>> -#include <linux/slab.h>
>>> -#include <linux/kmod.h>
>>>   #include <linux/fs.h>
>>> -#include <linux/kobject.h>
>>> -#include <linux/sysfs.h>
>>> -#include <linux/sysctl.h>
>>>   #include <cluster/masklog.h>
>>>
>>>   #include "ocfs2.h"
>>> @@ -53,36 +45,6 @@ static const char * const ocfs2_filecheck_errs[] = {
>>>   	"UNSUPPORTED"
>>>   };
>>>
>>> -static DEFINE_SPINLOCK(ocfs2_filecheck_sysfs_lock);
>>> -static LIST_HEAD(ocfs2_filecheck_sysfs_list);
>>> -
>>
>>> -struct ocfs2_filecheck {
>>> -	struct list_head fc_head;	/* File check entry list head */
>>> -	spinlock_t fc_lock;
>>> -	unsigned int fc_max;	/* Maximum number of entry in list */
>>> -	unsigned int fc_size;	/* Current entry count in list */
>>> -	unsigned int fc_done;	/* Finished entry count in list */
>>> -};
>> Gang: we can reuse this structure, please move this structure into struct 
> ocfs2_super, instead of defining a few members in struct ocfs2_super.
>> We should use a separate structure to handle all the file check related 
> thing, make the code simple and easy to maintain.
> 
> 
> Not sure why you are insistent on keeping a separate structure when it 
> would not be references from different structures. Keeping a prefix of 
> fc will make sure it is for filecheck attributes.
Gang: I just want to make the feature code more independent in other source files, define a separate data structure to 
make the file check code more easily read for the developer, and keep the code change more less in other source file.
Actually, the existing  struct ocfs2_filecheck can be re-used here.

> 
>>
>>
>>> -
>>> -struct ocfs2_filecheck_sysfs_entry {	/* sysfs entry per mounting */
>>> -	struct list_head fs_list;
>>> -	atomic_t fs_count;
>>> -	struct super_block *fs_sb;
>>> -	struct kset *fs_devicekset;
>>> -	struct kset *fs_fcheckkset;
>>> -	struct ocfs2_filecheck *fs_fcheck;
>>> -};
>> Gang: I think that we can not delete this structure directly, some members 
> are still useful.
>> e.g. fs_count, which is used to track if there are any pending file check 
> entries, can be moved into struct ocfs2_filecheck.
>> kset members maybe be kept in struct ocfs2_filecheck.
> 
> Okay, I will fix that.
> 
>>
>>> -
>>> -#define OCFS2_FILECHECK_MAXSIZE		100
>>> -#define OCFS2_FILECHECK_MINSIZE		10
>> Gang: any file will reference these two macro?
>> if not, that means these are not interfaces, should be kept in source file.
> 
> Yes, they are referenced by multiple files.
> 
>>
>>> -
>>> -/* File check operation type */
>>> -enum {
>>> -	OCFS2_FILECHECK_TYPE_CHK = 0,	/* Check a file(inode) */
>>> -	OCFS2_FILECHECK_TYPE_FIX,	/* Fix a file(inode) */
>>> -	OCFS2_FILECHECK_TYPE_SET = 100	/* Set entry list maximum size */
>>> -};
>> Gang: delete the member OCFS2_FILECHECK_TYPE_SET,
>> Move this enum to the header file if any other source file will use, 
> otherwise should be kept in source file.
> 
> The idea of removing this structures was to simplify the code. More 
> structures make more complicated code.
Gang: that is OK, but please define the OCFS2_FILECHECK_TYPE_CHK to 0, otherwise this replacing is not equivalent.

> 
>>
>>> -
>>>   struct ocfs2_filecheck_entry {
>>>   	struct list_head fe_list;
>>>   	unsigned long fe_ino;
>>> @@ -91,14 +53,6 @@ struct ocfs2_filecheck_entry {
>>>   	unsigned int fe_status:31;
>>>   };
>>>
>>> -struct ocfs2_filecheck_args {
>>> -	unsigned int fa_type;
>>> -	union {
>>> -		unsigned long fa_ino;
>>> -		unsigned int fa_len;
>>> -	};
>>> -};
>>> -
>>>   static const char *
>>>   ocfs2_filecheck_error(int errno)
>>>   {
>>> @@ -110,321 +64,51 @@ ocfs2_filecheck_error(int errno)
>>>   	return ocfs2_filecheck_errs[errno - OCFS2_FILECHECK_ERR_START + 1];
>>>   }
>>>
>>> -static ssize_t ocfs2_filecheck_show(struct kobject *kobj,
>>> -				    struct kobj_attribute *attr,
>>> -				    char *buf);
>>> -static ssize_t ocfs2_filecheck_store(struct kobject *kobj,
>>> -				     struct kobj_attribute *attr,
>>> -				     const char *buf, size_t count);
>>> -static struct kobj_attribute ocfs2_attr_filecheck_chk =
>>> -					__ATTR(check, S_IRUSR | S_IWUSR,
>>> -					ocfs2_filecheck_show,
>>> -					ocfs2_filecheck_store);
>>> -static struct kobj_attribute ocfs2_attr_filecheck_fix =
>>> -					__ATTR(fix, S_IRUSR | S_IWUSR,
>>> -					ocfs2_filecheck_show,
>>> -					ocfs2_filecheck_store);
>>> -static struct kobj_attribute ocfs2_attr_filecheck_set =
>>> -					__ATTR(set, S_IRUSR | S_IWUSR,
>>> -					ocfs2_filecheck_show,
>>> -					ocfs2_filecheck_store);
>>> -
>>> -static int ocfs2_filecheck_sysfs_wait(atomic_t *p)
>>> -{
>>> -	schedule();
>>> -	return 0;
>>> -}
>>> -
>>> -static void
>>> -ocfs2_filecheck_sysfs_free(struct ocfs2_filecheck_sysfs_entry *entry)
>>> -{
>>> -	struct ocfs2_filecheck_entry *p;
>>> -
>>> -	if (!atomic_dec_and_test(&entry->fs_count))
>>> -		wait_on_atomic_t(&entry->fs_count, ocfs2_filecheck_sysfs_wait,
>>> -				 TASK_UNINTERRUPTIBLE);
>>> -
>>> -	spin_lock(&entry->fs_fcheck->fc_lock);
>>> -	while (!list_empty(&entry->fs_fcheck->fc_head)) {
>>> -		p = list_first_entry(&entry->fs_fcheck->fc_head,
>>> -				     struct ocfs2_filecheck_entry, fe_list);
>>> -		list_del(&p->fe_list);
>>> -		BUG_ON(!p->fe_done); /* To free a undone file check entry */
>>> -		kfree(p);
>>> -	}
>>> -	spin_unlock(&entry->fs_fcheck->fc_lock);
>>> -
>>> -	kset_unregister(entry->fs_fcheckkset);
>>> -	kset_unregister(entry->fs_devicekset);
>>> -	kfree(entry->fs_fcheck);
>>> -	kfree(entry);
>>> -}
>> Gang: we cannot delete the function ocfs2_filecheck_sysfs_free() directly, 
> this will be used to free the file check entries.
>> Please keep the code, otherwise, it will lead to memory leak.
> 
> Yes, there should a cleanup routine.
> 
>>
>>> -
>>> -static void
>>> -ocfs2_filecheck_sysfs_add(struct ocfs2_filecheck_sysfs_entry *entry)
>>> -{
>>> -	spin_lock(&ocfs2_filecheck_sysfs_lock);
>>> -	list_add_tail(&entry->fs_list, &ocfs2_filecheck_sysfs_list);
>>> -	spin_unlock(&ocfs2_filecheck_sysfs_lock);
>>> -}
>>> -
>>> -static int ocfs2_filecheck_sysfs_del(const char *devname)
>>> -{
>>> -	struct ocfs2_filecheck_sysfs_entry *p;
>>> -
>>> -	spin_lock(&ocfs2_filecheck_sysfs_lock);
>>> -	list_for_each_entry(p, &ocfs2_filecheck_sysfs_list, fs_list) {
>>> -		if (!strcmp(p->fs_sb->s_id, devname)) {
>>> -			list_del(&p->fs_list);
>>> -			spin_unlock(&ocfs2_filecheck_sysfs_lock);
>>> -			ocfs2_filecheck_sysfs_free(p);
>>> -			return 0;
>>> -		}
>>> -	}
>>> -	spin_unlock(&ocfs2_filecheck_sysfs_lock);
>>> -	return 1;
>>> -}
>>> -
>>> -static void
>>> -ocfs2_filecheck_sysfs_put(struct ocfs2_filecheck_sysfs_entry *entry)
>>> -{
>>> -	if (atomic_dec_and_test(&entry->fs_count))
>>> -		wake_up_atomic_t(&entry->fs_count);
>>> -}
>>> -
>>> -static struct ocfs2_filecheck_sysfs_entry *
>>> -ocfs2_filecheck_sysfs_get(const char *devname)
>>> -{
>>> -	struct ocfs2_filecheck_sysfs_entry *p = NULL;
>>> -
>>> -	spin_lock(&ocfs2_filecheck_sysfs_lock);
>>> -	list_for_each_entry(p, &ocfs2_filecheck_sysfs_list, fs_list) {
>>> -		if (!strcmp(p->fs_sb->s_id, devname)) {
>>> -			atomic_inc(&p->fs_count);
>>> -			spin_unlock(&ocfs2_filecheck_sysfs_lock);
>>> -			return p;
>>> -		}
>>> -	}
>>> -	spin_unlock(&ocfs2_filecheck_sysfs_lock);
>>> -	return NULL;
>>> -}
>> Gang: We can't delete these code directly, fs_count variable is used to 
> track if there is any process to use file check related data.
>> Please consider this variable before delete this part code.
>>
>>
>>> -int ocfs2_filecheck_create_sysfs(struct super_block *sb)
>>> -{
>>> -	int ret = 0;
>>> -	struct kset *device_kset = NULL;
>>> -	struct kset *fcheck_kset = NULL;
>>> -	struct ocfs2_filecheck *fcheck = NULL;
>>> -	struct ocfs2_filecheck_sysfs_entry *entry = NULL;
>>> -	struct attribute **attrs = NULL;
>>> -	struct attribute_group attrgp;
>>> -
>>> -	if (!ocfs2_kset)
>>> -		return -ENOMEM;
>>> -
>>> -	attrs = kmalloc(sizeof(struct attribute *) * 4, GFP_NOFS);
>>> -	if (!attrs) {
>>> -		ret = -ENOMEM;
>>> -		goto error;
>>> -	} else {
>>> -		attrs[0] = &ocfs2_attr_filecheck_chk.attr;
>>> -		attrs[1] = &ocfs2_attr_filecheck_fix.attr;
>>> -		attrs[2] = &ocfs2_attr_filecheck_set.attr;
>>> -		attrs[3] = NULL;
>>> -		memset(&attrgp, 0, sizeof(attrgp));
>>> -		attrgp.attrs = attrs;
>>> -	}
>>> -
>>> -	fcheck = kmalloc(sizeof(struct ocfs2_filecheck), GFP_NOFS);
>>> -	if (!fcheck) {
>>> -		ret = -ENOMEM;
>>> -		goto error;
>>> -	} else {
>>> -		INIT_LIST_HEAD(&fcheck->fc_head);
>>> -		spin_lock_init(&fcheck->fc_lock);
>>> -		fcheck->fc_max = OCFS2_FILECHECK_MINSIZE;
>>> -		fcheck->fc_size = 0;
>>> -		fcheck->fc_done = 0;
>>> -	}
>>> -
>>> -	if (strlen(sb->s_id) <= 0) {
>>> -		mlog(ML_ERROR,
>>> -		"Cannot get device basename when create filecheck sysfs\n");
>>> -		ret = -ENODEV;
>>> -		goto error;
>>> -	}
>>> -
>>> -	device_kset = kset_create_and_add(sb->s_id, NULL, &ocfs2_kset->kobj);
>>> -	if (!device_kset) {
>>> -		ret = -ENOMEM;
>>> -		goto error;
>>> -	}
>>> -
>>> -	fcheck_kset = kset_create_and_add("filecheck", NULL,
>>> -					  &device_kset->kobj);
>>> -	if (!fcheck_kset) {
>>> -		ret = -ENOMEM;
>>> -		goto error;
>>> -	}
>>> -
>>> -	ret = sysfs_create_group(&fcheck_kset->kobj, &attrgp);
>>> -	if (ret)
>>> -		goto error;
>>> -
>>> -	entry = kmalloc(sizeof(struct ocfs2_filecheck_sysfs_entry), GFP_NOFS);
>>> -	if (!entry) {
>>> -		ret = -ENOMEM;
>>> -		goto error;
>>> -	} else {
>>> -		atomic_set(&entry->fs_count, 1);
>>> -		entry->fs_sb = sb;
>>> -		entry->fs_devicekset = device_kset;
>>> -		entry->fs_fcheckkset = fcheck_kset;
>>> -		entry->fs_fcheck = fcheck;
>>> -		ocfs2_filecheck_sysfs_add(entry);
>>> -	}
>>> -
>>> -	kfree(attrs);
>>> -	return 0;
>>> -
>>> -error:
>>> -	kfree(attrs);
>>> -	kfree(entry);
>>> -	kfree(fcheck);
>>> -	kset_unregister(fcheck_kset);
>>> -	kset_unregister(device_kset);
>>> -	return ret;
>>> -}
>>> -
>>> -int ocfs2_filecheck_remove_sysfs(struct super_block *sb)
>>> -{
>>> -	return ocfs2_filecheck_sysfs_del(sb->s_id);
>>> -}
>> Gang: this part code can be re-used after a little modification.
> 
> For what?
Gang: this part code have a complete creating/releasing kset code, have been tested, it will not bring any memory leak.

> 
>>
>>> -
>>>   static int
>>> -ocfs2_filecheck_erase_entries(struct ocfs2_filecheck_sysfs_entry *ent,
>>> +ocfs2_filecheck_erase_entries(struct ocfs2_super *,
>>>   			      unsigned int count);
>>> -static int
>>> -ocfs2_filecheck_adjust_max(struct ocfs2_filecheck_sysfs_entry *ent,
>>> -			   unsigned int len)
>>> +
>>> +int ocfs2_filecheck_set_max_entries(struct ocfs2_super *osb,
>>> +				int len)
>>>   {
>>>   	int ret;
>>>
>>>   	if ((len < OCFS2_FILECHECK_MINSIZE) || (len > OCFS2_FILECHECK_MAXSIZE))
>>>   		return -EINVAL;
>>>
>>> -	spin_lock(&ent->fs_fcheck->fc_lock);
>>> -	if (len < (ent->fs_fcheck->fc_size - ent->fs_fcheck->fc_done)) {
>>> +	spin_lock(&osb->fc_lock);
>>> +	if (len < (osb->fc_size - osb->fc_done)) {
>>>   		mlog(ML_ERROR,
>>>   		"Cannot set online file check maximum entry number "
>>>   		"to %u due to too many pending entries(%u)\n",
>>> -		len, ent->fs_fcheck->fc_size - ent->fs_fcheck->fc_done);
>>> +		len, osb->fc_size - osb->fc_done);
>>>   		ret = -EBUSY;
>>>   	} else {
>>> -		if (len < ent->fs_fcheck->fc_size)
>>> -			BUG_ON(!ocfs2_filecheck_erase_entries(ent,
>>> -				ent->fs_fcheck->fc_size - len));
>>> +		if (len < osb->fc_size)
>>> +			BUG_ON(!ocfs2_filecheck_erase_entries(osb,
>>> +				osb->fc_size - len));
>>>
>>> -		ent->fs_fcheck->fc_max = len;
>>> +		osb->fc_max = len;
>>>   		ret = 0;
>>>   	}
>>> -	spin_unlock(&ent->fs_fcheck->fc_lock);
>>> +	spin_unlock(&osb->fc_lock);
>>>
>>>   	return ret;
>>>   }
>>>
>>> -#define OCFS2_FILECHECK_ARGS_LEN	24
>>> -static int
>>> -ocfs2_filecheck_args_get_long(const char *buf, size_t count,
>>> -			      unsigned long *val)
>>> -{
>>> -	char buffer[OCFS2_FILECHECK_ARGS_LEN];
>>> -
>>> -	memcpy(buffer, buf, count);
>>> -	buffer[count] = '\0';
>>> -
>>> -	if (kstrtoul(buffer, 0, val))
>>> -		return 1;
>>> -
>>> -	return 0;
>>> -}
>>> -
>>> -static int
>>> -ocfs2_filecheck_type_parse(const char *name, unsigned int *type)
>>> -{
>>> -	if (!strncmp(name, "fix", 4))
>>> -		*type = OCFS2_FILECHECK_TYPE_FIX;
>>> -	else if (!strncmp(name, "check", 6))
>>> -		*type = OCFS2_FILECHECK_TYPE_CHK;
>>> -	else if (!strncmp(name, "set", 4))
>>> -		*type = OCFS2_FILECHECK_TYPE_SET;
>>> -	else
>>> -		return 1;
>>> -
>>> -	return 0;
>>> -}
>>> -
>>> -static int
>>> -ocfs2_filecheck_args_parse(const char *name, const char *buf, size_t count,
>>> -			   struct ocfs2_filecheck_args *args)
>>> -{
>>> -	unsigned long val = 0;
>>> -	unsigned int type;
>>> -
>>> -	/* too short/long args length */
>>> -	if ((count < 1) || (count >= OCFS2_FILECHECK_ARGS_LEN))
>>> -		return 1;
>>> -
>>> -	if (ocfs2_filecheck_type_parse(name, &type))
>>> -		return 1;
>>> -	if (ocfs2_filecheck_args_get_long(buf, count, &val))
>>> -		return 1;
>>> -
>>> -	if (val <= 0)
>>> -		return 1;
>>>
>>> -	args->fa_type = type;
>>> -	if (type == OCFS2_FILECHECK_TYPE_SET)
>>> -		args->fa_len = (unsigned int)val;
>>> -	else
>>> -		args->fa_ino = val;
>>> -
>>> -	return 0;
>>> -}
>>> -
>> Gang: please keep the ocfs2_filecheck_args_parse function, or write a 
> similar argument checking function since the arguments are from user-space, 
> must be check strictly.
>> Otherwise, this will be a way for malicious users to panic the kernel.
> 
> How? Do you have an example?
Gang: for example, user input a too long number string, we should return a invalid error directly, not do the further file check operation.
We need a ocfs2_filecheck_args_parse function, add some argument checking rules for now and future (when we find more limitations),
return the error to the user process directly, to avoid bringing the further IO checking efforts. 

> 
>>
>>> -static ssize_t ocfs2_filecheck_show(struct kobject *kobj,
>>> -				    struct kobj_attribute *attr,
>>> +int ocfs2_filecheck_show(struct ocfs2_super *osb, unsigned int type,
>>>   				    char *buf)
>>>   {
>>>
>>>   	ssize_t ret = 0, total = 0, remain = PAGE_SIZE;
>>> -	unsigned int type;
>>>   	struct ocfs2_filecheck_entry *p;
>>> -	struct ocfs2_filecheck_sysfs_entry *ent;
>>> -
>>> -	if (ocfs2_filecheck_type_parse(attr->attr.name, &type))
>>> -		return -EINVAL;
>>> -
>>> -	ent = ocfs2_filecheck_sysfs_get(kobj->parent->name);
>> Gang: if delete file check data reference count mechanism, how to make sure 
> the resource race condition (e.g. list the check results during the file 
> system umounting.)
>> Please consider this race condition problem.
> 
> Isn't spinlock enough? What is the race?
Gang: No, since file check operation is executed without spinlock, we need a reference count to keep the related data, until the last usage is finished.

> 
>>
>>> -	if (!ent) {
>>> -		mlog(ML_ERROR,
>>> -		"Cannot get the corresponding entry via device basename %s\n",
>>> -		kobj->name);
>>> -		return -ENODEV;
>>> -	}
>>> -
>>> -	if (type == OCFS2_FILECHECK_TYPE_SET) {
>>> -		spin_lock(&ent->fs_fcheck->fc_lock);
>>> -		total = snprintf(buf, remain, "%u\n", ent->fs_fcheck->fc_max);
>>> -		spin_unlock(&ent->fs_fcheck->fc_lock);
>>> -		goto exit;
>>> -	}
>>>
>>>   	ret = snprintf(buf, remain, "INO\t\tDONE\tERROR\n");
>>>   	total += ret;
>>>   	remain -= ret;
>>> -	spin_lock(&ent->fs_fcheck->fc_lock);
>>> -	list_for_each_entry(p, &ent->fs_fcheck->fc_head, fe_list) {
>>> +	spin_lock(&osb->fc_lock);
>>> +	list_for_each_entry(p, &osb->file_check_entries, fe_list) {
>>>   		if (p->fe_type != type)
>>>   			continue;
>>>
>>> @@ -443,24 +127,21 @@ static ssize_t ocfs2_filecheck_show(struct kobject
>>> *kobj,
>>>   		total += ret;
>>>   		remain -= ret;
>>>   	}
>>> -	spin_unlock(&ent->fs_fcheck->fc_lock);
>>> -
>>> -exit:
>>> -	ocfs2_filecheck_sysfs_put(ent);
>>> +	spin_unlock(&osb->fc_lock);
>>>   	return total;
>>>   }
>>>
>>>   static int
>>> -ocfs2_filecheck_erase_entry(struct ocfs2_filecheck_sysfs_entry *ent)
>>> +ocfs2_filecheck_erase_entry(struct ocfs2_super *osb)
>>>   {
>>>   	struct ocfs2_filecheck_entry *p;
>>>
>>> -	list_for_each_entry(p, &ent->fs_fcheck->fc_head, fe_list) {
>>> +	list_for_each_entry(p, &osb->file_check_entries, fe_list) {
>>>   		if (p->fe_done) {
>>>   			list_del(&p->fe_list);
>>>   			kfree(p);
>>> -			ent->fs_fcheck->fc_size--;
>>> -			ent->fs_fcheck->fc_done--;
>>> +			osb->fc_size--;
>>> +			osb->fc_done--;
>>>   			return 1;
>>>   		}
>>>   	}
>>> @@ -469,14 +150,14 @@ ocfs2_filecheck_erase_entry(struct
>>> ocfs2_filecheck_sysfs_entry *ent)
>>>   }
>>>
>>>   static int
>>> -ocfs2_filecheck_erase_entries(struct ocfs2_filecheck_sysfs_entry *ent,
>>> +ocfs2_filecheck_erase_entries(struct ocfs2_super *osb,
>>>   			      unsigned int count)
>>>   {
>>>   	unsigned int i = 0;
>>>   	unsigned int ret = 0;
>>>
>>>   	while (i++ < count) {
>>> -		if (ocfs2_filecheck_erase_entry(ent))
>>> +		if (ocfs2_filecheck_erase_entry(osb))
>>>   			ret++;
>>>   		else
>>>   			break;
>>> @@ -486,24 +167,24 @@ ocfs2_filecheck_erase_entries(struct
>>> ocfs2_filecheck_sysfs_entry *ent,
>>>   }
>>>
>>>   static void
>>> -ocfs2_filecheck_done_entry(struct ocfs2_filecheck_sysfs_entry *ent,
>>> +ocfs2_filecheck_done_entry(struct ocfs2_super *osb,
>>>   			   struct ocfs2_filecheck_entry *entry)
>>>   {
>>>   	entry->fe_done = 1;
>>> -	spin_lock(&ent->fs_fcheck->fc_lock);
>>> -	ent->fs_fcheck->fc_done++;
>>> -	spin_unlock(&ent->fs_fcheck->fc_lock);
>>> +	spin_lock(&osb->fc_lock);
>>> +	osb->fc_done++;
>>> +	spin_unlock(&osb->fc_lock);
>>>   }
>>>
>>>   static unsigned int
>>> -ocfs2_filecheck_handle(struct super_block *sb,
>>> +ocfs2_filecheck_handle(struct ocfs2_super *osb,
>>>   		       unsigned long ino, unsigned int flags)
>>>   {
>>>   	unsigned int ret = OCFS2_FILECHECK_ERR_SUCCESS;
>>>   	struct inode *inode = NULL;
>>>   	int rc;
>>>
>>> -	inode = ocfs2_iget(OCFS2_SB(sb), ino, flags, 0);
>>> +	inode = ocfs2_iget(osb, ino, flags, 0);
>>>   	if (IS_ERR(inode)) {
>>>   		rc = (int)(-(long)inode);
>>>   		if (rc >= OCFS2_FILECHECK_ERR_START &&
>>> @@ -518,89 +199,64 @@ ocfs2_filecheck_handle(struct super_block *sb,
>>>   }
>>>
>>>   static void
>>> -ocfs2_filecheck_handle_entry(struct ocfs2_filecheck_sysfs_entry *ent,
>>> +ocfs2_filecheck_handle_entry(struct ocfs2_super *osb,
>>>   			     struct ocfs2_filecheck_entry *entry)
>>>   {
>>>   	if (entry->fe_type == OCFS2_FILECHECK_TYPE_CHK)
>>> -		entry->fe_status = ocfs2_filecheck_handle(ent->fs_sb,
>>> +		entry->fe_status = ocfs2_filecheck_handle(osb,
>>>   				entry->fe_ino, OCFS2_FI_FLAG_FILECHECK_CHK);
>>>   	else if (entry->fe_type == OCFS2_FILECHECK_TYPE_FIX)
>>> -		entry->fe_status = ocfs2_filecheck_handle(ent->fs_sb,
>>> +		entry->fe_status = ocfs2_filecheck_handle(osb,
>>>   				entry->fe_ino, OCFS2_FI_FLAG_FILECHECK_FIX);
>>>   	else
>>>   		entry->fe_status = OCFS2_FILECHECK_ERR_UNSUPPORTED;
>>>
>>> -	ocfs2_filecheck_done_entry(ent, entry);
>>> +	ocfs2_filecheck_done_entry(osb, entry);
>>>   }
>>>
>>> -static ssize_t ocfs2_filecheck_store(struct kobject *kobj,
>>> -				     struct kobj_attribute *attr,
>>> -				     const char *buf, size_t count)
>>> +int ocfs2_filecheck_add_inode(struct ocfs2_super *osb,
>>> +				     unsigned long ino)
>>>   {
>>> -	struct ocfs2_filecheck_args args;
>>>   	struct ocfs2_filecheck_entry *entry;
>>> -	struct ocfs2_filecheck_sysfs_entry *ent;
>>>   	ssize_t ret = 0;
>>>
>>> -	if (count == 0)
>>> -		return count;
>>> -
>>> -	if (ocfs2_filecheck_args_parse(attr->attr.name, buf, count, &args)) {
>>> -		mlog(ML_ERROR, "Invalid arguments for online file check\n");
>>> -		return -EINVAL;
>>> -	}
>>> -
>>> -	ent = ocfs2_filecheck_sysfs_get(kobj->parent->name);
>>> -	if (!ent) {
>>> -		mlog(ML_ERROR,
>>> -		"Cannot get the corresponding entry via device basename %s\n",
>>> -		kobj->parent->name);
>>> -		return -ENODEV;
>>> -	}
>>> -
>>> -	if (args.fa_type == OCFS2_FILECHECK_TYPE_SET) {
>>> -		ret = ocfs2_filecheck_adjust_max(ent, args.fa_len);
>>> -		goto exit;
>>> -	}
>>> -
>>>   	entry = kmalloc(sizeof(struct ocfs2_filecheck_entry), GFP_NOFS);
>>>   	if (!entry) {
>>>   		ret = -ENOMEM;
>>>   		goto exit;
>>>   	}
>>>
>>> -	spin_lock(&ent->fs_fcheck->fc_lock);
>>> -	if ((ent->fs_fcheck->fc_size >= ent->fs_fcheck->fc_max) &&
>>> -	    (ent->fs_fcheck->fc_done == 0)) {
>>> +	spin_lock(&osb->fc_lock);
>>> +	if ((osb->fc_size >= osb->fc_max) &&
>>> +	    (osb->fc_done == 0)) {
>>>   		mlog(ML_ERROR,
>>>   		"Cannot do more file check "
>>>   		"since file check queue(%u) is full now\n",
>>> -		ent->fs_fcheck->fc_max);
>>> +		osb->fc_max);
>>>   		ret = -EBUSY;
>>>   		kfree(entry);
>>>   	} else {
>>> -		if ((ent->fs_fcheck->fc_size >= ent->fs_fcheck->fc_max) &&
>>> -		    (ent->fs_fcheck->fc_done > 0)) {
>>> +		if ((osb->fc_size >= osb->fc_max) &&
>>> +		    (osb->fc_done > 0)) {
>>>   			/* Delete the oldest entry which was done,
>>>   			 * make sure the entry size in list does
>>>   			 * not exceed maximum value
>>>   			 */
>>> -			BUG_ON(!ocfs2_filecheck_erase_entry(ent));
>>> +			BUG_ON(!ocfs2_filecheck_erase_entry(osb));
>>>   		}
>>>
>>> -		entry->fe_ino = args.fa_ino;
>>> -		entry->fe_type = args.fa_type;
>>> +		entry->fe_ino = ino;
>>> +		entry->fe_type = OCFS2_FILECHECK_TYPE_CHK;
>>>   		entry->fe_done = 0;
>>>   		entry->fe_status = OCFS2_FILECHECK_ERR_INPROGRESS;
>>> -		list_add_tail(&entry->fe_list, &ent->fs_fcheck->fc_head);
>>> -		ent->fs_fcheck->fc_size++;
>>> +		list_add_tail(&entry->fe_list, &osb->file_check_entries);
>>> +		osb->fc_size++;
>>>   	}
>>> -	spin_unlock(&ent->fs_fcheck->fc_lock);
>>> +	spin_unlock(&osb->fc_lock);
>>>
>>>   	if (!ret)
>>> -		ocfs2_filecheck_handle_entry(ent, entry);
>>> +		ocfs2_filecheck_handle_entry(osb, entry);
>>>
>>>   exit:
>>> -	ocfs2_filecheck_sysfs_put(ent);
>>> -	return (!ret ? count : ret);
>>> +	return ret;
>>>   }
>>> diff --git a/fs/ocfs2/filecheck.h b/fs/ocfs2/filecheck.h
>>> index e5cd002..b1a0d8c 100644
>>> --- a/fs/ocfs2/filecheck.h
>>> +++ b/fs/ocfs2/filecheck.h
>>> @@ -42,8 +42,18 @@ enum {
>>>
>>>   #define OCFS2_FILECHECK_ERR_START	OCFS2_FILECHECK_ERR_FAILED
>>>   #define OCFS2_FILECHECK_ERR_END		OCFS2_FILECHECK_ERR_UNSUPPORTED
>>> +#define OCFS2_FILECHECK_MAXSIZE         100
>>> +#define OCFS2_FILECHECK_MINSIZE         10
>>> +
>>> +/* File check operation type */
>>> +#define OCFS2_FILECHECK_TYPE_CHK  	1   /* Check a file(inode) */
>>> +#define OCFS2_FILECHECK_TYPE_FIX  	2   /* Fix a file(inode) */
>> Please use enum, not use macro definitions, maybe there will be more type.
>> OCFS2_FILECHECK_TYPE_CHK should be zero, otherwise this macro is different 
> with previous enum definitions.
>>
>>>
>>>   int ocfs2_filecheck_create_sysfs(struct super_block *sb);
>>>   int ocfs2_filecheck_remove_sysfs(struct super_block *sb);
>>> +int ocfs2_filefix_inode(struct ocfs2_super *osb, unsigned long ino);
>>> +int ocfs2_filecheck_add_inode(struct ocfs2_super *osb, unsigned long ino);
>>> +int ocfs2_filecheck_set_max_entries(struct ocfs2_super *osb, int num);
>>> +int ocfs2_filecheck_show(struct ocfs2_super *osb, unsigned int type, char
>>> *buf);
>>>
>>>   #endif  /* FILECHECK_H */
>>> diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
>>> index 8e66cdf..9ced543 100644
>>> --- a/fs/ocfs2/ocfs2.h
>>> +++ b/fs/ocfs2/ocfs2.h
>>> @@ -474,6 +474,13 @@ struct ocfs2_super
>>>   	 */
>>>   	struct workqueue_struct *ocfs2_wq;
>>>
>>> +	/* file check */
>>> +	struct list_head file_check_entries;
>>> +	unsigned int fc_size;
>>> +	unsigned int fc_max;
>>> +	unsigned int fc_done;
>>> +	spinlock_t fc_lock;
>>> +
>> Gang: please use a separate structure to include all the file check members, 
> easy to read/maintain.
> 
> 
> I would prefer it to be a part of ocfs2_super. It is easier to maintain 
> and read.
> 
>>
>>>   	struct kobject kobj;
>>>   	struct completion kobj_unregister;
>>>   };
>>> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
>>> index 96b7a9f..a7791fa 100644
>>> --- a/fs/ocfs2/super.c
>>> +++ b/fs/ocfs2/super.c
>>> @@ -2215,6 +2215,13 @@ static int ocfs2_initialize_super(struct super_block
>>> *sb,
>>>
>>>   	get_random_bytes(&osb->s_next_generation, sizeof(u32));
>>>
>>> +	/* file check information */
>>> +	INIT_LIST_HEAD(&osb->file_check_entries);
>>> +	osb->fc_max = OCFS2_FILECHECK_MINSIZE;
>>> +	osb->fc_size = 0;
>>> +	osb->fc_done = 0;
>>> +	spin_lock_init(&osb->fc_lock);
>>> +
>> Gang: write a separate funtion in filecheck.c to include these line code, 
> easy to read/maintain.
> 
> Why? Unless we are starting something specific to filecheck, I don't see 
> any need.
Gang: I just want to make the file check thing is done in filecheck source file, the other module just call some functions.
Let the code more clear.

> 
>>
>>>   	/* FIXME
>>>   	 * This should be done in ocfs2_journal_init(), but unknown
>>>   	 * ordering issues will cause the filesystem to crash.
>>> diff --git a/fs/ocfs2/sysfs.c b/fs/ocfs2/sysfs.c
>>> index e21e699..81e3dd4 100644
>>> --- a/fs/ocfs2/sysfs.c
>>> +++ b/fs/ocfs2/sysfs.c
>>> @@ -1,5 +1,6 @@
>>>   #include "ocfs2.h"
>>>   #include "sysfs.h"
>>> +#include "filecheck.h"
>>>
>>>   struct ocfs2_sb_attr {
>>>   	struct attribute attr;
>>> @@ -9,8 +10,12 @@ struct ocfs2_sb_attr {
>>>   			const char *buf, size_t count);
>>>   };
>>>
>>> -#define OCFS2_SB_ATTR(_name, _mode) \
>>> -struct ocfs2_sb_attr sb_attr_##_name = __ATTR(name, _mode, _show, _store)
>>> +#define OCFS2_SB_ATTR(_name) \
>>> +struct ocfs2_sb_attr sb_attr_##_name = { \
>>> +	.attr = {.name = __stringify(_name), .mode = (S_IWUSR | S_IRUGO)},  \
>>> +	.show = _name##_show, \
>>> +	.store = _name##_store \
>>> +}
>>>
>>>   #define OCFS2_SB_ATTR_RO(_name) \
>>>   struct ocfs2_sb_attr sb_attr_##_name = __ATTR_RO(_name)
>>> @@ -46,6 +51,81 @@ static ssize_t slot_num_show(struct ocfs2_super *osb,
>>>   	return sprintf(buf, "%d\n", osb->slot_num);
>>>   }
>>>
>>> +static ssize_t file_check_show(struct ocfs2_super *osb,
>>> +		struct ocfs2_sb_attr *attr,
>>> +		char *buf)
>>> +{
>>> +	return ocfs2_filecheck_show(osb, OCFS2_FILECHECK_TYPE_CHK, buf);
>>> +}
>>> +
>>> +static ssize_t file_check_store(struct ocfs2_super *osb,
>>> +		struct ocfs2_sb_attr *attr,
>>> +		const char *buf, size_t count)
>>> +{
>>> +	unsigned long t;
>>> +	int ret;
>>> +
>>> +	ret = kstrtoul(skip_spaces(buf), 0, &t);
>>> +	if (ret)
>>> +		return ret;
>>> +	ret = ocfs2_filecheck_add_inode(osb, t);
>>> +	if (ret)
>>> +		return ret;
>>> +	return count;
>>> +}
>>> +
>>> +static ssize_t file_fix_show(struct ocfs2_super *osb,
>>> +		struct ocfs2_sb_attr *attr,
>>> +		char *buf)
>>> +{
>>> +	return ocfs2_filecheck_show(osb, OCFS2_FILECHECK_TYPE_FIX, buf);
>>> +}
>>> +
>>> +static ssize_t file_fix_store(struct ocfs2_super *osb,
>>> +		struct ocfs2_sb_attr *attr,
>>> +		const char *buf, size_t count)
>>> +{
>>> +	unsigned long t;
>>> +	int ret;
>>> +
>>> +	ret = kstrtoul(skip_spaces(buf), 0, &t);
>>> +	if (ret)
>>> +		return ret;
>>> +	ret = ocfs2_filecheck_add_inode(osb, t);
>>> +	if (ret)
>>> +		return ret;
>>> +	return count;
>>> +}
>>> +
>>> +static ssize_t file_check_max_entries_show(struct ocfs2_super *osb,
>>> +		struct ocfs2_sb_attr *attr,
>>> +		char *buf)
>>> +{
>>> +	int len = 0;
>>> +	spin_lock(&osb->fc_lock);
>>> +	/* Show done, current size and max */
>>> +	len += sprintf(buf, "%d\t%d\t%d\n", osb->fc_done, osb->fc_size,
>>> +			osb->fc_max);
>>> +	spin_unlock(&osb->fc_lock);
>>> +	return len;
>>> +}
>>> +
>>> +static ssize_t file_check_max_entries_store(struct ocfs2_super *osb,
>>> +		struct ocfs2_sb_attr *attr, const char *buf, size_t count)
>>> +{
>>> +
>>> +	unsigned long t;
>>> +	int ret;
>>> +
>>> +	ret = kstrtoul(skip_spaces(buf), 0, &t);
>> Gang: please make sure the function kstrtoul does work well since the 
> inputed buf has not terminating null character.
>> This is why I write a wrapper function ocfs2_filecheck_args_get_long().
> 
> Can you give an example on how this can be activated? Maybe a check on 
> value < 0 may make sense.
Gang: I am not sure if this function works well when the string is inputed without a terminating null character.
need to test all the case if using the function directly. But we test all the case with a terminating null character.


> 
>>
>>> +	if (ret)
>>> +		return ret;
>>> +	ret = ocfs2_filecheck_set_max_entries(osb, (int)t);
>>> +	if (ret)
>>> +		return ret;
>>> +	return count;
>>> +}
>>> +
>>>   static void sb_release(struct kobject *kobj)
>>>   {
>>>   	struct ocfs2_super *osb = container_of(kobj, struct ocfs2_super, kobj);
>>> @@ -58,8 +138,15 @@ static const struct sysfs_ops ocfs2_sb_sysfs_ops = {
>>>   };
>>>
>>>   static OCFS2_SB_ATTR_RO(slot_num);
>>> +static OCFS2_SB_ATTR(file_check);
>>> +static OCFS2_SB_ATTR(file_fix);
>>> +static OCFS2_SB_ATTR(file_check_max_entries);
>>> +
>>>   static struct attribute *ocfs2_sb_attrs[] = {
>>>   	&sb_attr_slot_num.attr,
>>> +	&sb_attr_file_check.attr,
>>> +	&sb_attr_file_fix.attr,
>>> +	&sb_attr_file_check_max_entries.attr,
>>>   	NULL
>>>   };
>> Gang: as I said in the first patch, please make all the file check related 
> attributes in a separate sub directory.
>> It will let us to be easy to read/debug, especially if there is any 
> reference count leaking problem.
> 
> The main reason of the code is to remove unnecessary data structures 
> which is making the code hard to understand. I don't see a point in 
> keeping them in a separate structure besides adding more indirections.
> 
> 
> -- 
> Goldwyn

[Ocfs2-devel] [PATCH 2/5] Use the sysfs interface for creating filecheck files

[Goldwyn Rodrigues]

On 05/31/2016 10:16 PM, Gang He wrote:
>
>
>
>>>>
>
>>
>> On 05/30/2016 04:23 AM, Gang He wrote:
>>> Hello Goldwyn,
>>>
>>> Please see my comments inline.
>>> I just suggest to re-use the existing code as most as possible, since the
>> code was tested by us.
>>>
>>> Thanks
>>> Gang
>>>
>>>
>>>>>>
>>>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>>>
>>>> This reduces the code base and removes unnecessary data structures
>>>> for filecheck information since all information is stored in ocfs2_super
>>>> now.
>>>>
>>>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>>> ---
>>>>    fs/ocfs2/filecheck.c | 440
>> ++++++---------------------------------------------
>>>>    fs/ocfs2/filecheck.h |  10 ++
>>>>    fs/ocfs2/ocfs2.h     |   7 +
>>>>    fs/ocfs2/super.c     |   7 +
>>>>    fs/ocfs2/sysfs.c     |  91 ++++++++++-
>>>>    5 files changed, 161 insertions(+), 394 deletions(-)
>>>>
>>>> diff --git a/fs/ocfs2/filecheck.c b/fs/ocfs2/filecheck.c
>>>> index 2cabbcf..0b41967 100644
>>>> --- a/fs/ocfs2/filecheck.c
>>>> +++ b/fs/ocfs2/filecheck.c
>>>> @@ -17,15 +17,7 @@
>>>>     * General Public License for more details.
>>>>     */
>>>>
>>>> -#include <linux/list.h>
>>>> -#include <linux/spinlock.h>
>>>> -#include <linux/module.h>
>>>> -#include <linux/slab.h>
>>>> -#include <linux/kmod.h>
>>>>    #include <linux/fs.h>
>>>> -#include <linux/kobject.h>
>>>> -#include <linux/sysfs.h>
>>>> -#include <linux/sysctl.h>
>>>>    #include <cluster/masklog.h>
>>>>
>>>>    #include "ocfs2.h"
>>>> @@ -53,36 +45,6 @@ static const char * const ocfs2_filecheck_errs[] = {
>>>>    	"UNSUPPORTED"
>>>>    };
>>>>
>>>> -static DEFINE_SPINLOCK(ocfs2_filecheck_sysfs_lock);
>>>> -static LIST_HEAD(ocfs2_filecheck_sysfs_list);
>>>> -
>>>
>>>> -struct ocfs2_filecheck {
>>>> -	struct list_head fc_head;	/* File check entry list head */
>>>> -	spinlock_t fc_lock;
>>>> -	unsigned int fc_max;	/* Maximum number of entry in list */
>>>> -	unsigned int fc_size;	/* Current entry count in list */
>>>> -	unsigned int fc_done;	/* Finished entry count in list */
>>>> -};
>>> Gang: we can reuse this structure, please move this structure into struct
>> ocfs2_super, instead of defining a few members in struct ocfs2_super.
>>> We should use a separate structure to handle all the file check related
>> thing, make the code simple and easy to maintain.
>>
>>
>> Not sure why you are insistent on keeping a separate structure when it
>> would not be references from different structures. Keeping a prefix of
>> fc will make sure it is for filecheck attributes.
> Gang: I just want to make the feature code more independent in other source files, define a separate data structure to
> make the file check code more easily read for the developer, and keep the code change more less in other source file.
> Actually, the existing  struct ocfs2_filecheck can be re-used here.

Yes, but it is just initialization of the fields. I understand if you 
are doing something more..

>>
>>>
>>>
>>>> -
>>>> -struct ocfs2_filecheck_sysfs_entry {	/* sysfs entry per mounting */
>>>> -	struct list_head fs_list;
>>>> -	atomic_t fs_count;
>>>> -	struct super_block *fs_sb;
>>>> -	struct kset *fs_devicekset;
>>>> -	struct kset *fs_fcheckkset;
>>>> -	struct ocfs2_filecheck *fs_fcheck;
>>>> -};
>>> Gang: I think that we can not delete this structure directly, some members
>> are still useful.
>>> e.g. fs_count, which is used to track if there are any pending file check
>> entries, can be moved into struct ocfs2_filecheck.
>>> kset members maybe be kept in struct ocfs2_filecheck.
>>
>> Okay, I will fix that.

On second though, this may not be required.


>>
>>>
>>>> -
>>>> -#define OCFS2_FILECHECK_MAXSIZE		100
>>>> -#define OCFS2_FILECHECK_MINSIZE		10
>>> Gang: any file will reference these two macro?
>>> if not, that means these are not interfaces, should be kept in source file.
>>
>> Yes, they are referenced by multiple files.
>>
>>>
>>>> -
>>>> -/* File check operation type */
>>>> -enum {
>>>> -	OCFS2_FILECHECK_TYPE_CHK = 0,	/* Check a file(inode) */
>>>> -	OCFS2_FILECHECK_TYPE_FIX,	/* Fix a file(inode) */
>>>> -	OCFS2_FILECHECK_TYPE_SET = 100	/* Set entry list maximum size */
>>>> -};
>>> Gang: delete the member OCFS2_FILECHECK_TYPE_SET,
>>> Move this enum to the header file if any other source file will use,
>> otherwise should be kept in source file.
>>
>> The idea of removing this structures was to simplify the code. More
>> structures make more complicated code.
> Gang: that is OK, but please define the OCFS2_FILECHECK_TYPE_CHK to 0, otherwise this replacing is not equivalent.
>

I am doing away with this in the future posts.

>>
>>>
>>>> -
>>>>    struct ocfs2_filecheck_entry {
>>>>    	struct list_head fe_list;
>>>>    	unsigned long fe_ino;
>>>> @@ -91,14 +53,6 @@ struct ocfs2_filecheck_entry {
>>>>    	unsigned int fe_status:31;
>>>>    };
>>>>
>>>> -struct ocfs2_filecheck_args {
>>>> -	unsigned int fa_type;
>>>> -	union {
>>>> -		unsigned long fa_ino;
>>>> -		unsigned int fa_len;
>>>> -	};
>>>> -};
>>>> -
>>>>    static const char *
>>>>    ocfs2_filecheck_error(int errno)
>>>>    {
>>>> @@ -110,321 +64,51 @@ ocfs2_filecheck_error(int errno)
>>>>    	return ocfs2_filecheck_errs[errno - OCFS2_FILECHECK_ERR_START + 1];
>>>>    }
>>>>
>>>> -static ssize_t ocfs2_filecheck_show(struct kobject *kobj,
>>>> -				    struct kobj_attribute *attr,
>>>> -				    char *buf);
>>>> -static ssize_t ocfs2_filecheck_store(struct kobject *kobj,
>>>> -				     struct kobj_attribute *attr,
>>>> -				     const char *buf, size_t count);
>>>> -static struct kobj_attribute ocfs2_attr_filecheck_chk =
>>>> -					__ATTR(check, S_IRUSR | S_IWUSR,
>>>> -					ocfs2_filecheck_show,
>>>> -					ocfs2_filecheck_store);
>>>> -static struct kobj_attribute ocfs2_attr_filecheck_fix =
>>>> -					__ATTR(fix, S_IRUSR | S_IWUSR,
>>>> -					ocfs2_filecheck_show,
>>>> -					ocfs2_filecheck_store);
>>>> -static struct kobj_attribute ocfs2_attr_filecheck_set =
>>>> -					__ATTR(set, S_IRUSR | S_IWUSR,
>>>> -					ocfs2_filecheck_show,
>>>> -					ocfs2_filecheck_store);
>>>> -
>>>> -static int ocfs2_filecheck_sysfs_wait(atomic_t *p)
>>>> -{
>>>> -	schedule();
>>>> -	return 0;
>>>> -}
>>>> -
>>>> -static void
>>>> -ocfs2_filecheck_sysfs_free(struct ocfs2_filecheck_sysfs_entry *entry)
>>>> -{
>>>> -	struct ocfs2_filecheck_entry *p;
>>>> -
>>>> -	if (!atomic_dec_and_test(&entry->fs_count))
>>>> -		wait_on_atomic_t(&entry->fs_count, ocfs2_filecheck_sysfs_wait,
>>>> -				 TASK_UNINTERRUPTIBLE);
>>>> -
>>>> -	spin_lock(&entry->fs_fcheck->fc_lock);
>>>> -	while (!list_empty(&entry->fs_fcheck->fc_head)) {
>>>> -		p = list_first_entry(&entry->fs_fcheck->fc_head,
>>>> -				     struct ocfs2_filecheck_entry, fe_list);
>>>> -		list_del(&p->fe_list);
>>>> -		BUG_ON(!p->fe_done); /* To free a undone file check entry */
>>>> -		kfree(p);
>>>> -	}
>>>> -	spin_unlock(&entry->fs_fcheck->fc_lock);
>>>> -
>>>> -	kset_unregister(entry->fs_fcheckkset);
>>>> -	kset_unregister(entry->fs_devicekset);
>>>> -	kfree(entry->fs_fcheck);
>>>> -	kfree(entry);
>>>> -}
>>> Gang: we cannot delete the function ocfs2_filecheck_sysfs_free() directly,
>> this will be used to free the file check entries.
>>> Please keep the code, otherwise, it will lead to memory leak.
>>
>> Yes, there should a cleanup routine.
>>
>>>
>>>> -
>>>> -static void
>>>> -ocfs2_filecheck_sysfs_add(struct ocfs2_filecheck_sysfs_entry *entry)
>>>> -{
>>>> -	spin_lock(&ocfs2_filecheck_sysfs_lock);
>>>> -	list_add_tail(&entry->fs_list, &ocfs2_filecheck_sysfs_list);
>>>> -	spin_unlock(&ocfs2_filecheck_sysfs_lock);
>>>> -}
>>>> -
>>>> -static int ocfs2_filecheck_sysfs_del(const char *devname)
>>>> -{
>>>> -	struct ocfs2_filecheck_sysfs_entry *p;
>>>> -
>>>> -	spin_lock(&ocfs2_filecheck_sysfs_lock);
>>>> -	list_for_each_entry(p, &ocfs2_filecheck_sysfs_list, fs_list) {
>>>> -		if (!strcmp(p->fs_sb->s_id, devname)) {
>>>> -			list_del(&p->fs_list);
>>>> -			spin_unlock(&ocfs2_filecheck_sysfs_lock);
>>>> -			ocfs2_filecheck_sysfs_free(p);
>>>> -			return 0;
>>>> -		}
>>>> -	}
>>>> -	spin_unlock(&ocfs2_filecheck_sysfs_lock);
>>>> -	return 1;
>>>> -}
>>>> -
>>>> -static void
>>>> -ocfs2_filecheck_sysfs_put(struct ocfs2_filecheck_sysfs_entry *entry)
>>>> -{
>>>> -	if (atomic_dec_and_test(&entry->fs_count))
>>>> -		wake_up_atomic_t(&entry->fs_count);
>>>> -}
>>>> -
>>>> -static struct ocfs2_filecheck_sysfs_entry *
>>>> -ocfs2_filecheck_sysfs_get(const char *devname)
>>>> -{
>>>> -	struct ocfs2_filecheck_sysfs_entry *p = NULL;
>>>> -
>>>> -	spin_lock(&ocfs2_filecheck_sysfs_lock);
>>>> -	list_for_each_entry(p, &ocfs2_filecheck_sysfs_list, fs_list) {
>>>> -		if (!strcmp(p->fs_sb->s_id, devname)) {
>>>> -			atomic_inc(&p->fs_count);
>>>> -			spin_unlock(&ocfs2_filecheck_sysfs_lock);
>>>> -			return p;
>>>> -		}
>>>> -	}
>>>> -	spin_unlock(&ocfs2_filecheck_sysfs_lock);
>>>> -	return NULL;
>>>> -}
>>> Gang: We can't delete these code directly, fs_count variable is used to
>> track if there is any process to use file check related data.
>>> Please consider this variable before delete this part code.
>>>
>>>
>>>> -int ocfs2_filecheck_create_sysfs(struct super_block *sb)
>>>> -{
>>>> -	int ret = 0;
>>>> -	struct kset *device_kset = NULL;
>>>> -	struct kset *fcheck_kset = NULL;
>>>> -	struct ocfs2_filecheck *fcheck = NULL;
>>>> -	struct ocfs2_filecheck_sysfs_entry *entry = NULL;
>>>> -	struct attribute **attrs = NULL;
>>>> -	struct attribute_group attrgp;
>>>> -
>>>> -	if (!ocfs2_kset)
>>>> -		return -ENOMEM;
>>>> -
>>>> -	attrs = kmalloc(sizeof(struct attribute *) * 4, GFP_NOFS);
>>>> -	if (!attrs) {
>>>> -		ret = -ENOMEM;
>>>> -		goto error;
>>>> -	} else {
>>>> -		attrs[0] = &ocfs2_attr_filecheck_chk.attr;
>>>> -		attrs[1] = &ocfs2_attr_filecheck_fix.attr;
>>>> -		attrs[2] = &ocfs2_attr_filecheck_set.attr;
>>>> -		attrs[3] = NULL;
>>>> -		memset(&attrgp, 0, sizeof(attrgp));
>>>> -		attrgp.attrs = attrs;
>>>> -	}
>>>> -
>>>> -	fcheck = kmalloc(sizeof(struct ocfs2_filecheck), GFP_NOFS);
>>>> -	if (!fcheck) {
>>>> -		ret = -ENOMEM;
>>>> -		goto error;
>>>> -	} else {
>>>> -		INIT_LIST_HEAD(&fcheck->fc_head);
>>>> -		spin_lock_init(&fcheck->fc_lock);
>>>> -		fcheck->fc_max = OCFS2_FILECHECK_MINSIZE;
>>>> -		fcheck->fc_size = 0;
>>>> -		fcheck->fc_done = 0;
>>>> -	}
>>>> -
>>>> -	if (strlen(sb->s_id) <= 0) {
>>>> -		mlog(ML_ERROR,
>>>> -		"Cannot get device basename when create filecheck sysfs\n");
>>>> -		ret = -ENODEV;
>>>> -		goto error;
>>>> -	}
>>>> -
>>>> -	device_kset = kset_create_and_add(sb->s_id, NULL, &ocfs2_kset->kobj);
>>>> -	if (!device_kset) {
>>>> -		ret = -ENOMEM;
>>>> -		goto error;
>>>> -	}
>>>> -
>>>> -	fcheck_kset = kset_create_and_add("filecheck", NULL,
>>>> -					  &device_kset->kobj);
>>>> -	if (!fcheck_kset) {
>>>> -		ret = -ENOMEM;
>>>> -		goto error;
>>>> -	}
>>>> -
>>>> -	ret = sysfs_create_group(&fcheck_kset->kobj, &attrgp);
>>>> -	if (ret)
>>>> -		goto error;
>>>> -
>>>> -	entry = kmalloc(sizeof(struct ocfs2_filecheck_sysfs_entry), GFP_NOFS);
>>>> -	if (!entry) {
>>>> -		ret = -ENOMEM;
>>>> -		goto error;
>>>> -	} else {
>>>> -		atomic_set(&entry->fs_count, 1);
>>>> -		entry->fs_sb = sb;
>>>> -		entry->fs_devicekset = device_kset;
>>>> -		entry->fs_fcheckkset = fcheck_kset;
>>>> -		entry->fs_fcheck = fcheck;
>>>> -		ocfs2_filecheck_sysfs_add(entry);
>>>> -	}
>>>> -
>>>> -	kfree(attrs);
>>>> -	return 0;
>>>> -
>>>> -error:
>>>> -	kfree(attrs);
>>>> -	kfree(entry);
>>>> -	kfree(fcheck);
>>>> -	kset_unregister(fcheck_kset);
>>>> -	kset_unregister(device_kset);
>>>> -	return ret;
>>>> -}
>>>> -
>>>> -int ocfs2_filecheck_remove_sysfs(struct super_block *sb)
>>>> -{
>>>> -	return ocfs2_filecheck_sysfs_del(sb->s_id);
>>>> -}
>>> Gang: this part code can be re-used after a little modification.
>>
>> For what?
> Gang: this part code have a complete creating/releasing kset code, have been tested, it will not bring any memory leak.
>
>>
>>>
>>>> -
>>>>    static int
>>>> -ocfs2_filecheck_erase_entries(struct ocfs2_filecheck_sysfs_entry *ent,
>>>> +ocfs2_filecheck_erase_entries(struct ocfs2_super *,
>>>>    			      unsigned int count);
>>>> -static int
>>>> -ocfs2_filecheck_adjust_max(struct ocfs2_filecheck_sysfs_entry *ent,
>>>> -			   unsigned int len)
>>>> +
>>>> +int ocfs2_filecheck_set_max_entries(struct ocfs2_super *osb,
>>>> +				int len)
>>>>    {
>>>>    	int ret;
>>>>
>>>>    	if ((len < OCFS2_FILECHECK_MINSIZE) || (len > OCFS2_FILECHECK_MAXSIZE))
>>>>    		return -EINVAL;
>>>>
>>>> -	spin_lock(&ent->fs_fcheck->fc_lock);
>>>> -	if (len < (ent->fs_fcheck->fc_size - ent->fs_fcheck->fc_done)) {
>>>> +	spin_lock(&osb->fc_lock);
>>>> +	if (len < (osb->fc_size - osb->fc_done)) {
>>>>    		mlog(ML_ERROR,
>>>>    		"Cannot set online file check maximum entry number "
>>>>    		"to %u due to too many pending entries(%u)\n",
>>>> -		len, ent->fs_fcheck->fc_size - ent->fs_fcheck->fc_done);
>>>> +		len, osb->fc_size - osb->fc_done);
>>>>    		ret = -EBUSY;
>>>>    	} else {
>>>> -		if (len < ent->fs_fcheck->fc_size)
>>>> -			BUG_ON(!ocfs2_filecheck_erase_entries(ent,
>>>> -				ent->fs_fcheck->fc_size - len));
>>>> +		if (len < osb->fc_size)
>>>> +			BUG_ON(!ocfs2_filecheck_erase_entries(osb,
>>>> +				osb->fc_size - len));
>>>>
>>>> -		ent->fs_fcheck->fc_max = len;
>>>> +		osb->fc_max = len;
>>>>    		ret = 0;
>>>>    	}
>>>> -	spin_unlock(&ent->fs_fcheck->fc_lock);
>>>> +	spin_unlock(&osb->fc_lock);
>>>>
>>>>    	return ret;
>>>>    }
>>>>
>>>> -#define OCFS2_FILECHECK_ARGS_LEN	24
>>>> -static int
>>>> -ocfs2_filecheck_args_get_long(const char *buf, size_t count,
>>>> -			      unsigned long *val)
>>>> -{
>>>> -	char buffer[OCFS2_FILECHECK_ARGS_LEN];
>>>> -
>>>> -	memcpy(buffer, buf, count);
>>>> -	buffer[count] = '\0';
>>>> -
>>>> -	if (kstrtoul(buffer, 0, val))
>>>> -		return 1;
>>>> -
>>>> -	return 0;
>>>> -}
>>>> -
>>>> -static int
>>>> -ocfs2_filecheck_type_parse(const char *name, unsigned int *type)
>>>> -{
>>>> -	if (!strncmp(name, "fix", 4))
>>>> -		*type = OCFS2_FILECHECK_TYPE_FIX;
>>>> -	else if (!strncmp(name, "check", 6))
>>>> -		*type = OCFS2_FILECHECK_TYPE_CHK;
>>>> -	else if (!strncmp(name, "set", 4))
>>>> -		*type = OCFS2_FILECHECK_TYPE_SET;
>>>> -	else
>>>> -		return 1;
>>>> -
>>>> -	return 0;
>>>> -}
>>>> -
>>>> -static int
>>>> -ocfs2_filecheck_args_parse(const char *name, const char *buf, size_t count,
>>>> -			   struct ocfs2_filecheck_args *args)
>>>> -{
>>>> -	unsigned long val = 0;
>>>> -	unsigned int type;
>>>> -
>>>> -	/* too short/long args length */
>>>> -	if ((count < 1) || (count >= OCFS2_FILECHECK_ARGS_LEN))
>>>> -		return 1;
>>>> -
>>>> -	if (ocfs2_filecheck_type_parse(name, &type))
>>>> -		return 1;
>>>> -	if (ocfs2_filecheck_args_get_long(buf, count, &val))
>>>> -		return 1;
>>>> -
>>>> -	if (val <= 0)
>>>> -		return 1;
>>>>
>>>> -	args->fa_type = type;
>>>> -	if (type == OCFS2_FILECHECK_TYPE_SET)
>>>> -		args->fa_len = (unsigned int)val;
>>>> -	else
>>>> -		args->fa_ino = val;
>>>> -
>>>> -	return 0;
>>>> -}
>>>> -
>>> Gang: please keep the ocfs2_filecheck_args_parse function, or write a
>> similar argument checking function since the arguments are from user-space,
>> must be check strictly.
>>> Otherwise, this will be a way for malicious users to panic the kernel.
>>
>> How? Do you have an example?
> Gang: for example, user input a too long number string, we should return a invalid error directly, not do the further file check operation.
> We need a ocfs2_filecheck_args_parse function, add some argument checking rules for now and future (when we find more limitations),
> return the error to the user process directly, to avoid bringing the further IO checking efforts.
>
>>
>>>
>>>> -static ssize_t ocfs2_filecheck_show(struct kobject *kobj,
>>>> -				    struct kobj_attribute *attr,
>>>> +int ocfs2_filecheck_show(struct ocfs2_super *osb, unsigned int type,
>>>>    				    char *buf)
>>>>    {
>>>>
>>>>    	ssize_t ret = 0, total = 0, remain = PAGE_SIZE;
>>>> -	unsigned int type;
>>>>    	struct ocfs2_filecheck_entry *p;
>>>> -	struct ocfs2_filecheck_sysfs_entry *ent;
>>>> -
>>>> -	if (ocfs2_filecheck_type_parse(attr->attr.name, &type))
>>>> -		return -EINVAL;
>>>> -
>>>> -	ent = ocfs2_filecheck_sysfs_get(kobj->parent->name);
>>> Gang: if delete file check data reference count mechanism, how to make sure
>> the resource race condition (e.g. list the check results during the file
>> system umounting.)
>>> Please consider this race condition problem.



>>
>> Isn't spinlock enough? What is the race?
> Gang: No, since file check operation is executed without spinlock, we need a reference count to keep the related data, until the last usage is finished.


Filesystem unmonting is a different game. It evicts inodes. Could you 
clarify more?

Anyways, doing away with fe_done and using the status instead should help.

>
>>
>>>
>>>> -	if (!ent) {
>>>> -		mlog(ML_ERROR,
>>>> -		"Cannot get the corresponding entry via device basename %s\n",
>>>> -		kobj->name);
>>>> -		return -ENODEV;
>>>> -	}
>>>> -
>>>> -	if (type == OCFS2_FILECHECK_TYPE_SET) {
>>>> -		spin_lock(&ent->fs_fcheck->fc_lock);
>>>> -		total = snprintf(buf, remain, "%u\n", ent->fs_fcheck->fc_max);
>>>> -		spin_unlock(&ent->fs_fcheck->fc_lock);
>>>> -		goto exit;
>>>> -	}
>>>>
>>>>    	ret = snprintf(buf, remain, "INO\t\tDONE\tERROR\n");
>>>>    	total += ret;
>>>>    	remain -= ret;
>>>> -	spin_lock(&ent->fs_fcheck->fc_lock);
>>>> -	list_for_each_entry(p, &ent->fs_fcheck->fc_head, fe_list) {
>>>> +	spin_lock(&osb->fc_lock);
>>>> +	list_for_each_entry(p, &osb->file_check_entries, fe_list) {
>>>>    		if (p->fe_type != type)
>>>>    			continue;
>>>>
>>>> @@ -443,24 +127,21 @@ static ssize_t ocfs2_filecheck_show(struct kobject
>>>> *kobj,
>>>>    		total += ret;
>>>>    		remain -= ret;
>>>>    	}
>>>> -	spin_unlock(&ent->fs_fcheck->fc_lock);
>>>> -
>>>> -exit:
>>>> -	ocfs2_filecheck_sysfs_put(ent);
>>>> +	spin_unlock(&osb->fc_lock);
>>>>    	return total;
>>>>    }
>>>>
>>>>    static int
>>>> -ocfs2_filecheck_erase_entry(struct ocfs2_filecheck_sysfs_entry *ent)
>>>> +ocfs2_filecheck_erase_entry(struct ocfs2_super *osb)
>>>>    {
>>>>    	struct ocfs2_filecheck_entry *p;
>>>>
>>>> -	list_for_each_entry(p, &ent->fs_fcheck->fc_head, fe_list) {
>>>> +	list_for_each_entry(p, &osb->file_check_entries, fe_list) {
>>>>    		if (p->fe_done) {
>>>>    			list_del(&p->fe_list);
>>>>    			kfree(p);
>>>> -			ent->fs_fcheck->fc_size--;
>>>> -			ent->fs_fcheck->fc_done--;
>>>> +			osb->fc_size--;
>>>> +			osb->fc_done--;
>>>>    			return 1;
>>>>    		}
>>>>    	}
>>>> @@ -469,14 +150,14 @@ ocfs2_filecheck_erase_entry(struct
>>>> ocfs2_filecheck_sysfs_entry *ent)
>>>>    }
>>>>
>>>>    static int
>>>> -ocfs2_filecheck_erase_entries(struct ocfs2_filecheck_sysfs_entry *ent,
>>>> +ocfs2_filecheck_erase_entries(struct ocfs2_super *osb,
>>>>    			      unsigned int count)
>>>>    {
>>>>    	unsigned int i = 0;
>>>>    	unsigned int ret = 0;
>>>>
>>>>    	while (i++ < count) {
>>>> -		if (ocfs2_filecheck_erase_entry(ent))
>>>> +		if (ocfs2_filecheck_erase_entry(osb))
>>>>    			ret++;
>>>>    		else
>>>>    			break;
>>>> @@ -486,24 +167,24 @@ ocfs2_filecheck_erase_entries(struct
>>>> ocfs2_filecheck_sysfs_entry *ent,
>>>>    }
>>>>
>>>>    static void
>>>> -ocfs2_filecheck_done_entry(struct ocfs2_filecheck_sysfs_entry *ent,
>>>> +ocfs2_filecheck_done_entry(struct ocfs2_super *osb,
>>>>    			   struct ocfs2_filecheck_entry *entry)
>>>>    {
>>>>    	entry->fe_done = 1;
>>>> -	spin_lock(&ent->fs_fcheck->fc_lock);
>>>> -	ent->fs_fcheck->fc_done++;
>>>> -	spin_unlock(&ent->fs_fcheck->fc_lock);
>>>> +	spin_lock(&osb->fc_lock);
>>>> +	osb->fc_done++;
>>>> +	spin_unlock(&osb->fc_lock);
>>>>    }
>>>>
>>>>    static unsigned int
>>>> -ocfs2_filecheck_handle(struct super_block *sb,
>>>> +ocfs2_filecheck_handle(struct ocfs2_super *osb,
>>>>    		       unsigned long ino, unsigned int flags)
>>>>    {
>>>>    	unsigned int ret = OCFS2_FILECHECK_ERR_SUCCESS;
>>>>    	struct inode *inode = NULL;
>>>>    	int rc;
>>>>
>>>> -	inode = ocfs2_iget(OCFS2_SB(sb), ino, flags, 0);
>>>> +	inode = ocfs2_iget(osb, ino, flags, 0);
>>>>    	if (IS_ERR(inode)) {
>>>>    		rc = (int)(-(long)inode);
>>>>    		if (rc >= OCFS2_FILECHECK_ERR_START &&
>>>> @@ -518,89 +199,64 @@ ocfs2_filecheck_handle(struct super_block *sb,
>>>>    }
>>>>
>>>>    static void
>>>> -ocfs2_filecheck_handle_entry(struct ocfs2_filecheck_sysfs_entry *ent,
>>>> +ocfs2_filecheck_handle_entry(struct ocfs2_super *osb,
>>>>    			     struct ocfs2_filecheck_entry *entry)
>>>>    {
>>>>    	if (entry->fe_type == OCFS2_FILECHECK_TYPE_CHK)
>>>> -		entry->fe_status = ocfs2_filecheck_handle(ent->fs_sb,
>>>> +		entry->fe_status = ocfs2_filecheck_handle(osb,
>>>>    				entry->fe_ino, OCFS2_FI_FLAG_FILECHECK_CHK);
>>>>    	else if (entry->fe_type == OCFS2_FILECHECK_TYPE_FIX)
>>>> -		entry->fe_status = ocfs2_filecheck_handle(ent->fs_sb,
>>>> +		entry->fe_status = ocfs2_filecheck_handle(osb,
>>>>    				entry->fe_ino, OCFS2_FI_FLAG_FILECHECK_FIX);
>>>>    	else
>>>>    		entry->fe_status = OCFS2_FILECHECK_ERR_UNSUPPORTED;
>>>>
>>>> -	ocfs2_filecheck_done_entry(ent, entry);
>>>> +	ocfs2_filecheck_done_entry(osb, entry);
>>>>    }
>>>>
>>>> -static ssize_t ocfs2_filecheck_store(struct kobject *kobj,
>>>> -				     struct kobj_attribute *attr,
>>>> -				     const char *buf, size_t count)
>>>> +int ocfs2_filecheck_add_inode(struct ocfs2_super *osb,
>>>> +				     unsigned long ino)
>>>>    {
>>>> -	struct ocfs2_filecheck_args args;
>>>>    	struct ocfs2_filecheck_entry *entry;
>>>> -	struct ocfs2_filecheck_sysfs_entry *ent;
>>>>    	ssize_t ret = 0;
>>>>
>>>> -	if (count == 0)
>>>> -		return count;
>>>> -
>>>> -	if (ocfs2_filecheck_args_parse(attr->attr.name, buf, count, &args)) {
>>>> -		mlog(ML_ERROR, "Invalid arguments for online file check\n");
>>>> -		return -EINVAL;
>>>> -	}
>>>> -
>>>> -	ent = ocfs2_filecheck_sysfs_get(kobj->parent->name);
>>>> -	if (!ent) {
>>>> -		mlog(ML_ERROR,
>>>> -		"Cannot get the corresponding entry via device basename %s\n",
>>>> -		kobj->parent->name);
>>>> -		return -ENODEV;
>>>> -	}
>>>> -
>>>> -	if (args.fa_type == OCFS2_FILECHECK_TYPE_SET) {
>>>> -		ret = ocfs2_filecheck_adjust_max(ent, args.fa_len);
>>>> -		goto exit;
>>>> -	}
>>>> -
>>>>    	entry = kmalloc(sizeof(struct ocfs2_filecheck_entry), GFP_NOFS);
>>>>    	if (!entry) {
>>>>    		ret = -ENOMEM;
>>>>    		goto exit;
>>>>    	}
>>>>
>>>> -	spin_lock(&ent->fs_fcheck->fc_lock);
>>>> -	if ((ent->fs_fcheck->fc_size >= ent->fs_fcheck->fc_max) &&
>>>> -	    (ent->fs_fcheck->fc_done == 0)) {
>>>> +	spin_lock(&osb->fc_lock);
>>>> +	if ((osb->fc_size >= osb->fc_max) &&
>>>> +	    (osb->fc_done == 0)) {
>>>>    		mlog(ML_ERROR,
>>>>    		"Cannot do more file check "
>>>>    		"since file check queue(%u) is full now\n",
>>>> -		ent->fs_fcheck->fc_max);
>>>> +		osb->fc_max);
>>>>    		ret = -EBUSY;
>>>>    		kfree(entry);
>>>>    	} else {
>>>> -		if ((ent->fs_fcheck->fc_size >= ent->fs_fcheck->fc_max) &&
>>>> -		    (ent->fs_fcheck->fc_done > 0)) {
>>>> +		if ((osb->fc_size >= osb->fc_max) &&
>>>> +		    (osb->fc_done > 0)) {
>>>>    			/* Delete the oldest entry which was done,
>>>>    			 * make sure the entry size in list does
>>>>    			 * not exceed maximum value
>>>>    			 */
>>>> -			BUG_ON(!ocfs2_filecheck_erase_entry(ent));
>>>> +			BUG_ON(!ocfs2_filecheck_erase_entry(osb));
>>>>    		}
>>>>
>>>> -		entry->fe_ino = args.fa_ino;
>>>> -		entry->fe_type = args.fa_type;
>>>> +		entry->fe_ino = ino;
>>>> +		entry->fe_type = OCFS2_FILECHECK_TYPE_CHK;
>>>>    		entry->fe_done = 0;
>>>>    		entry->fe_status = OCFS2_FILECHECK_ERR_INPROGRESS;
>>>> -		list_add_tail(&entry->fe_list, &ent->fs_fcheck->fc_head);
>>>> -		ent->fs_fcheck->fc_size++;
>>>> +		list_add_tail(&entry->fe_list, &osb->file_check_entries);
>>>> +		osb->fc_size++;
>>>>    	}
>>>> -	spin_unlock(&ent->fs_fcheck->fc_lock);
>>>> +	spin_unlock(&osb->fc_lock);
>>>>
>>>>    	if (!ret)
>>>> -		ocfs2_filecheck_handle_entry(ent, entry);
>>>> +		ocfs2_filecheck_handle_entry(osb, entry);
>>>>
>>>>    exit:
>>>> -	ocfs2_filecheck_sysfs_put(ent);
>>>> -	return (!ret ? count : ret);
>>>> +	return ret;
>>>>    }
>>>> diff --git a/fs/ocfs2/filecheck.h b/fs/ocfs2/filecheck.h
>>>> index e5cd002..b1a0d8c 100644
>>>> --- a/fs/ocfs2/filecheck.h
>>>> +++ b/fs/ocfs2/filecheck.h
>>>> @@ -42,8 +42,18 @@ enum {
>>>>
>>>>    #define OCFS2_FILECHECK_ERR_START	OCFS2_FILECHECK_ERR_FAILED
>>>>    #define OCFS2_FILECHECK_ERR_END		OCFS2_FILECHECK_ERR_UNSUPPORTED
>>>> +#define OCFS2_FILECHECK_MAXSIZE         100
>>>> +#define OCFS2_FILECHECK_MINSIZE         10
>>>> +
>>>> +/* File check operation type */
>>>> +#define OCFS2_FILECHECK_TYPE_CHK  	1   /* Check a file(inode) */
>>>> +#define OCFS2_FILECHECK_TYPE_FIX  	2   /* Fix a file(inode) */
>>> Please use enum, not use macro definitions, maybe there will be more type.
>>> OCFS2_FILECHECK_TYPE_CHK should be zero, otherwise this macro is different
>> with previous enum definitions.
>>>
>>>>
>>>>    int ocfs2_filecheck_create_sysfs(struct super_block *sb);
>>>>    int ocfs2_filecheck_remove_sysfs(struct super_block *sb);
>>>> +int ocfs2_filefix_inode(struct ocfs2_super *osb, unsigned long ino);
>>>> +int ocfs2_filecheck_add_inode(struct ocfs2_super *osb, unsigned long ino);
>>>> +int ocfs2_filecheck_set_max_entries(struct ocfs2_super *osb, int num);
>>>> +int ocfs2_filecheck_show(struct ocfs2_super *osb, unsigned int type, char
>>>> *buf);
>>>>
>>>>    #endif  /* FILECHECK_H */
>>>> diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
>>>> index 8e66cdf..9ced543 100644
>>>> --- a/fs/ocfs2/ocfs2.h
>>>> +++ b/fs/ocfs2/ocfs2.h
>>>> @@ -474,6 +474,13 @@ struct ocfs2_super
>>>>    	 */
>>>>    	struct workqueue_struct *ocfs2_wq;
>>>>
>>>> +	/* file check */
>>>> +	struct list_head file_check_entries;
>>>> +	unsigned int fc_size;
>>>> +	unsigned int fc_max;
>>>> +	unsigned int fc_done;
>>>> +	spinlock_t fc_lock;
>>>> +
>>> Gang: please use a separate structure to include all the file check members,
>> easy to read/maintain.
>>
>>
>> I would prefer it to be a part of ocfs2_super. It is easier to maintain
>> and read.
>>
>>>
>>>>    	struct kobject kobj;
>>>>    	struct completion kobj_unregister;
>>>>    };
>>>> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
>>>> index 96b7a9f..a7791fa 100644
>>>> --- a/fs/ocfs2/super.c
>>>> +++ b/fs/ocfs2/super.c
>>>> @@ -2215,6 +2215,13 @@ static int ocfs2_initialize_super(struct super_block
>>>> *sb,
>>>>
>>>>    	get_random_bytes(&osb->s_next_generation, sizeof(u32));
>>>>
>>>> +	/* file check information */
>>>> +	INIT_LIST_HEAD(&osb->file_check_entries);
>>>> +	osb->fc_max = OCFS2_FILECHECK_MINSIZE;
>>>> +	osb->fc_size = 0;
>>>> +	osb->fc_done = 0;
>>>> +	spin_lock_init(&osb->fc_lock);
>>>> +
>>> Gang: write a separate funtion in filecheck.c to include these line code,
>> easy to read/maintain.
>>
>> Why? Unless we are starting something specific to filecheck, I don't see
>> any need.
> Gang: I just want to make the file check thing is done in filecheck source file, the other module just call some functions.
> Let the code more clear.
>
>>
>>>
>>>>    	/* FIXME
>>>>    	 * This should be done in ocfs2_journal_init(), but unknown
>>>>    	 * ordering issues will cause the filesystem to crash.
>>>> diff --git a/fs/ocfs2/sysfs.c b/fs/ocfs2/sysfs.c
>>>> index e21e699..81e3dd4 100644
>>>> --- a/fs/ocfs2/sysfs.c
>>>> +++ b/fs/ocfs2/sysfs.c
>>>> @@ -1,5 +1,6 @@
>>>>    #include "ocfs2.h"
>>>>    #include "sysfs.h"
>>>> +#include "filecheck.h"
>>>>
>>>>    struct ocfs2_sb_attr {
>>>>    	struct attribute attr;
>>>> @@ -9,8 +10,12 @@ struct ocfs2_sb_attr {
>>>>    			const char *buf, size_t count);
>>>>    };
>>>>
>>>> -#define OCFS2_SB_ATTR(_name, _mode) \
>>>> -struct ocfs2_sb_attr sb_attr_##_name = __ATTR(name, _mode, _show, _store)
>>>> +#define OCFS2_SB_ATTR(_name) \
>>>> +struct ocfs2_sb_attr sb_attr_##_name = { \
>>>> +	.attr = {.name = __stringify(_name), .mode = (S_IWUSR | S_IRUGO)},  \
>>>> +	.show = _name##_show, \
>>>> +	.store = _name##_store \
>>>> +}
>>>>
>>>>    #define OCFS2_SB_ATTR_RO(_name) \
>>>>    struct ocfs2_sb_attr sb_attr_##_name = __ATTR_RO(_name)
>>>> @@ -46,6 +51,81 @@ static ssize_t slot_num_show(struct ocfs2_super *osb,
>>>>    	return sprintf(buf, "%d\n", osb->slot_num);
>>>>    }
>>>>
>>>> +static ssize_t file_check_show(struct ocfs2_super *osb,
>>>> +		struct ocfs2_sb_attr *attr,
>>>> +		char *buf)
>>>> +{
>>>> +	return ocfs2_filecheck_show(osb, OCFS2_FILECHECK_TYPE_CHK, buf);
>>>> +}
>>>> +
>>>> +static ssize_t file_check_store(struct ocfs2_super *osb,
>>>> +		struct ocfs2_sb_attr *attr,
>>>> +		const char *buf, size_t count)
>>>> +{
>>>> +	unsigned long t;
>>>> +	int ret;
>>>> +
>>>> +	ret = kstrtoul(skip_spaces(buf), 0, &t);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +	ret = ocfs2_filecheck_add_inode(osb, t);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +	return count;
>>>> +}
>>>> +
>>>> +static ssize_t file_fix_show(struct ocfs2_super *osb,
>>>> +		struct ocfs2_sb_attr *attr,
>>>> +		char *buf)
>>>> +{
>>>> +	return ocfs2_filecheck_show(osb, OCFS2_FILECHECK_TYPE_FIX, buf);
>>>> +}
>>>> +
>>>> +static ssize_t file_fix_store(struct ocfs2_super *osb,
>>>> +		struct ocfs2_sb_attr *attr,
>>>> +		const char *buf, size_t count)
>>>> +{
>>>> +	unsigned long t;
>>>> +	int ret;
>>>> +
>>>> +	ret = kstrtoul(skip_spaces(buf), 0, &t);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +	ret = ocfs2_filecheck_add_inode(osb, t);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +	return count;
>>>> +}
>>>> +
>>>> +static ssize_t file_check_max_entries_show(struct ocfs2_super *osb,
>>>> +		struct ocfs2_sb_attr *attr,
>>>> +		char *buf)
>>>> +{
>>>> +	int len = 0;
>>>> +	spin_lock(&osb->fc_lock);
>>>> +	/* Show done, current size and max */
>>>> +	len += sprintf(buf, "%d\t%d\t%d\n", osb->fc_done, osb->fc_size,
>>>> +			osb->fc_max);
>>>> +	spin_unlock(&osb->fc_lock);
>>>> +	return len;
>>>> +}
>>>> +
>>>> +static ssize_t file_check_max_entries_store(struct ocfs2_super *osb,
>>>> +		struct ocfs2_sb_attr *attr, const char *buf, size_t count)
>>>> +{
>>>> +
>>>> +	unsigned long t;
>>>> +	int ret;
>>>> +
>>>> +	ret = kstrtoul(skip_spaces(buf), 0, &t);
>>> Gang: please make sure the function kstrtoul does work well since the
>> inputed buf has not terminating null character.
>>> This is why I write a wrapper function ocfs2_filecheck_args_get_long().
>>
>> Can you give an example on how this can be activated? Maybe a check on
>> value < 0 may make sense.
> Gang: I am not sure if this function works well when the string is inputed without a terminating null character.
> need to test all the case if using the function directly. But we test all the case with a terminating null character.
>

So, do you have a testcase?

>
>>
>>>
>>>> +	if (ret)
>>>> +		return ret;
>>>> +	ret = ocfs2_filecheck_set_max_entries(osb, (int)t);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +	return count;
>>>> +}
>>>> +
>>>>    static void sb_release(struct kobject *kobj)
>>>>    {
>>>>    	struct ocfs2_super *osb = container_of(kobj, struct ocfs2_super, kobj);
>>>> @@ -58,8 +138,15 @@ static const struct sysfs_ops ocfs2_sb_sysfs_ops = {
>>>>    };
>>>>
>>>>    static OCFS2_SB_ATTR_RO(slot_num);
>>>> +static OCFS2_SB_ATTR(file_check);
>>>> +static OCFS2_SB_ATTR(file_fix);
>>>> +static OCFS2_SB_ATTR(file_check_max_entries);
>>>> +
>>>>    static struct attribute *ocfs2_sb_attrs[] = {
>>>>    	&sb_attr_slot_num.attr,
>>>> +	&sb_attr_file_check.attr,
>>>> +	&sb_attr_file_fix.attr,
>>>> +	&sb_attr_file_check_max_entries.attr,
>>>>    	NULL
>>>>    };
>>> Gang: as I said in the first patch, please make all the file check related
>> attributes in a separate sub directory.
>>> It will let us to be easy to read/debug, especially if there is any
>> reference count leaking problem.
>>
>> The main reason of the code is to remove unnecessary data structures
>> which is making the code hard to understand. I don't see a point in
>> keeping them in a separate structure besides adding more indirections.
>>
>>
>> --
>> Goldwyn

-- 
Goldwyn

[Ocfs2-devel] [PATCH 2/5] Use the sysfs interface for creating filecheck files

[Gang He]

>>> 

> 
> On 05/31/2016 10:16 PM, Gang He wrote:
>>
>>
>>
>>>>>
>>
>>>
>>> On 05/30/2016 04:23 AM, Gang He wrote:
>>>> Hello Goldwyn,
>>>>
>>>> Please see my comments inline.
>>>> I just suggest to re-use the existing code as most as possible, since the
>>> code was tested by us.
>>>>
>>>> Thanks
>>>> Gang
>>>>
>>>>
>>>>>>>
>>>>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>>>>
>>>>> This reduces the code base and removes unnecessary data structures
>>>>> for filecheck information since all information is stored in ocfs2_super
>>>>> now.
>>>>>
>>>>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>>>> ---
>>>>>    fs/ocfs2/filecheck.c | 440
>>> ++++++---------------------------------------------
>>>>>    fs/ocfs2/filecheck.h |  10 ++
>>>>>    fs/ocfs2/ocfs2.h     |   7 +
>>>>>    fs/ocfs2/super.c     |   7 +
>>>>>    fs/ocfs2/sysfs.c     |  91 ++++++++++-
>>>>>    5 files changed, 161 insertions(+), 394 deletions(-)
>>>>>
>>>>> diff --git a/fs/ocfs2/filecheck.c b/fs/ocfs2/filecheck.c
>>>>> index 2cabbcf..0b41967 100644
>>>>> --- a/fs/ocfs2/filecheck.c
>>>>> +++ b/fs/ocfs2/filecheck.c
>>>>> @@ -17,15 +17,7 @@
>>>>>     * General Public License for more details.
>>>>>     */
>>>>>
>>>>> -#include <linux/list.h>
>>>>> -#include <linux/spinlock.h>
>>>>> -#include <linux/module.h>
>>>>> -#include <linux/slab.h>
>>>>> -#include <linux/kmod.h>
>>>>>    #include <linux/fs.h>
>>>>> -#include <linux/kobject.h>
>>>>> -#include <linux/sysfs.h>
>>>>> -#include <linux/sysctl.h>
>>>>>    #include <cluster/masklog.h>
>>>>>
>>>>>    #include "ocfs2.h"
>>>>> @@ -53,36 +45,6 @@ static const char * const ocfs2_filecheck_errs[] = {
>>>>>    	"UNSUPPORTED"
>>>>>    };
>>>>>
>>>>> -static DEFINE_SPINLOCK(ocfs2_filecheck_sysfs_lock);
>>>>> -static LIST_HEAD(ocfs2_filecheck_sysfs_list);
>>>>> -
>>>>
>>>>> -struct ocfs2_filecheck {
>>>>> -	struct list_head fc_head;	/* File check entry list head */
>>>>> -	spinlock_t fc_lock;
>>>>> -	unsigned int fc_max;	/* Maximum number of entry in list */
>>>>> -	unsigned int fc_size;	/* Current entry count in list */
>>>>> -	unsigned int fc_done;	/* Finished entry count in list */
>>>>> -};
>>>> Gang: we can reuse this structure, please move this structure into struct
>>> ocfs2_super, instead of defining a few members in struct ocfs2_super.
>>>> We should use a separate structure to handle all the file check related
>>> thing, make the code simple and easy to maintain.
>>>
>>>
>>> Not sure why you are insistent on keeping a separate structure when it
>>> would not be references from different structures. Keeping a prefix of
>>> fc will make sure it is for filecheck attributes.
>> Gang: I just want to make the feature code more independent in other source 
> files, define a separate data structure to
>> make the file check code more easily read for the developer, and keep the 
> code change more less in other source file.
>> Actually, the existing  struct ocfs2_filecheck can be re-used here.
> 
> Yes, but it is just initialization of the fields. I understand if you 
> are doing something more..
> 
>>>
>>>>
>>>>
>>>>> -
>>>>> -struct ocfs2_filecheck_sysfs_entry {	/* sysfs entry per mounting */
>>>>> -	struct list_head fs_list;
>>>>> -	atomic_t fs_count;
>>>>> -	struct super_block *fs_sb;
>>>>> -	struct kset *fs_devicekset;
>>>>> -	struct kset *fs_fcheckkset;
>>>>> -	struct ocfs2_filecheck *fs_fcheck;
>>>>> -};
>>>> Gang: I think that we can not delete this structure directly, some members
>>> are still useful.
>>>> e.g. fs_count, which is used to track if there are any pending file check
>>> entries, can be moved into struct ocfs2_filecheck.
>>>> kset members maybe be kept in struct ocfs2_filecheck.
>>>
>>> Okay, I will fix that.
> 
> On second though, this may not be required.
Gang: This should need a reference count to track if the related data is still needed by others before we delete in the umount stage.
You know, file check entry operation is running in the kernel without holding the list spin_lock (actually can not hold), 
if we try to release the list during umounting, how to make sure all the filecheck IO operation is done? 

> 
> 
>>>
>>>>
>>>>> -
>>>>> -#define OCFS2_FILECHECK_MAXSIZE		100
>>>>> -#define OCFS2_FILECHECK_MINSIZE		10
>>>> Gang: any file will reference these two macro?
>>>> if not, that means these are not interfaces, should be kept in source file.
>>>
>>> Yes, they are referenced by multiple files.
>>>
>>>>
>>>>> -
>>>>> -/* File check operation type */
>>>>> -enum {
>>>>> -	OCFS2_FILECHECK_TYPE_CHK = 0,	/* Check a file(inode) */
>>>>> -	OCFS2_FILECHECK_TYPE_FIX,	/* Fix a file(inode) */
>>>>> -	OCFS2_FILECHECK_TYPE_SET = 100	/* Set entry list maximum size */
>>>>> -};
>>>> Gang: delete the member OCFS2_FILECHECK_TYPE_SET,
>>>> Move this enum to the header file if any other source file will use,
>>> otherwise should be kept in source file.
>>>
>>> The idea of removing this structures was to simplify the code. More
>>> structures make more complicated code.
>> Gang: that is OK, but please define the OCFS2_FILECHECK_TYPE_CHK to 0, 
> otherwise this replacing is not equivalent.
>>
> 
> I am doing away with this in the future posts.
> 
>>>
>>>>
>>>>> -
>>>>>    struct ocfs2_filecheck_entry {
>>>>>    	struct list_head fe_list;
>>>>>    	unsigned long fe_ino;
>>>>> @@ -91,14 +53,6 @@ struct ocfs2_filecheck_entry {
>>>>>    	unsigned int fe_status:31;
>>>>>    };
>>>>>
>>>>> -struct ocfs2_filecheck_args {
>>>>> -	unsigned int fa_type;
>>>>> -	union {
>>>>> -		unsigned long fa_ino;
>>>>> -		unsigned int fa_len;
>>>>> -	};
>>>>> -};
>>>>> -
>>>>>    static const char *
>>>>>    ocfs2_filecheck_error(int errno)
>>>>>    {
>>>>> @@ -110,321 +64,51 @@ ocfs2_filecheck_error(int errno)
>>>>>    	return ocfs2_filecheck_errs[errno - OCFS2_FILECHECK_ERR_START + 1];
>>>>>    }
>>>>>
>>>>> -static ssize_t ocfs2_filecheck_show(struct kobject *kobj,
>>>>> -				    struct kobj_attribute *attr,
>>>>> -				    char *buf);
>>>>> -static ssize_t ocfs2_filecheck_store(struct kobject *kobj,
>>>>> -				     struct kobj_attribute *attr,
>>>>> -				     const char *buf, size_t count);
>>>>> -static struct kobj_attribute ocfs2_attr_filecheck_chk =
>>>>> -					__ATTR(check, S_IRUSR | S_IWUSR,
>>>>> -					ocfs2_filecheck_show,
>>>>> -					ocfs2_filecheck_store);
>>>>> -static struct kobj_attribute ocfs2_attr_filecheck_fix =
>>>>> -					__ATTR(fix, S_IRUSR | S_IWUSR,
>>>>> -					ocfs2_filecheck_show,
>>>>> -					ocfs2_filecheck_store);
>>>>> -static struct kobj_attribute ocfs2_attr_filecheck_set =
>>>>> -					__ATTR(set, S_IRUSR | S_IWUSR,
>>>>> -					ocfs2_filecheck_show,
>>>>> -					ocfs2_filecheck_store);
>>>>> -
>>>>> -static int ocfs2_filecheck_sysfs_wait(atomic_t *p)
>>>>> -{
>>>>> -	schedule();
>>>>> -	return 0;
>>>>> -}
>>>>> -
>>>>> -static void
>>>>> -ocfs2_filecheck_sysfs_free(struct ocfs2_filecheck_sysfs_entry *entry)
>>>>> -{
>>>>> -	struct ocfs2_filecheck_entry *p;
>>>>> -
>>>>> -	if (!atomic_dec_and_test(&entry->fs_count))
>>>>> -		wait_on_atomic_t(&entry->fs_count, ocfs2_filecheck_sysfs_wait,
>>>>> -				 TASK_UNINTERRUPTIBLE);
>>>>> -
>>>>> -	spin_lock(&entry->fs_fcheck->fc_lock);
>>>>> -	while (!list_empty(&entry->fs_fcheck->fc_head)) {
>>>>> -		p = list_first_entry(&entry->fs_fcheck->fc_head,
>>>>> -				     struct ocfs2_filecheck_entry, fe_list);
>>>>> -		list_del(&p->fe_list);
>>>>> -		BUG_ON(!p->fe_done); /* To free a undone file check entry */
>>>>> -		kfree(p);
>>>>> -	}
>>>>> -	spin_unlock(&entry->fs_fcheck->fc_lock);
>>>>> -
>>>>> -	kset_unregister(entry->fs_fcheckkset);
>>>>> -	kset_unregister(entry->fs_devicekset);
>>>>> -	kfree(entry->fs_fcheck);
>>>>> -	kfree(entry);
>>>>> -}
>>>> Gang: we cannot delete the function ocfs2_filecheck_sysfs_free() directly,
>>> this will be used to free the file check entries.
>>>> Please keep the code, otherwise, it will lead to memory leak.
>>>
>>> Yes, there should a cleanup routine.
>>>
>>>>
>>>>> -
>>>>> -static void
>>>>> -ocfs2_filecheck_sysfs_add(struct ocfs2_filecheck_sysfs_entry *entry)
>>>>> -{
>>>>> -	spin_lock(&ocfs2_filecheck_sysfs_lock);
>>>>> -	list_add_tail(&entry->fs_list, &ocfs2_filecheck_sysfs_list);
>>>>> -	spin_unlock(&ocfs2_filecheck_sysfs_lock);
>>>>> -}
>>>>> -
>>>>> -static int ocfs2_filecheck_sysfs_del(const char *devname)
>>>>> -{
>>>>> -	struct ocfs2_filecheck_sysfs_entry *p;
>>>>> -
>>>>> -	spin_lock(&ocfs2_filecheck_sysfs_lock);
>>>>> -	list_for_each_entry(p, &ocfs2_filecheck_sysfs_list, fs_list) {
>>>>> -		if (!strcmp(p->fs_sb->s_id, devname)) {
>>>>> -			list_del(&p->fs_list);
>>>>> -			spin_unlock(&ocfs2_filecheck_sysfs_lock);
>>>>> -			ocfs2_filecheck_sysfs_free(p);
>>>>> -			return 0;
>>>>> -		}
>>>>> -	}
>>>>> -	spin_unlock(&ocfs2_filecheck_sysfs_lock);
>>>>> -	return 1;
>>>>> -}
>>>>> -
>>>>> -static void
>>>>> -ocfs2_filecheck_sysfs_put(struct ocfs2_filecheck_sysfs_entry *entry)
>>>>> -{
>>>>> -	if (atomic_dec_and_test(&entry->fs_count))
>>>>> -		wake_up_atomic_t(&entry->fs_count);
>>>>> -}
>>>>> -
>>>>> -static struct ocfs2_filecheck_sysfs_entry *
>>>>> -ocfs2_filecheck_sysfs_get(const char *devname)
>>>>> -{
>>>>> -	struct ocfs2_filecheck_sysfs_entry *p = NULL;
>>>>> -
>>>>> -	spin_lock(&ocfs2_filecheck_sysfs_lock);
>>>>> -	list_for_each_entry(p, &ocfs2_filecheck_sysfs_list, fs_list) {
>>>>> -		if (!strcmp(p->fs_sb->s_id, devname)) {
>>>>> -			atomic_inc(&p->fs_count);
>>>>> -			spin_unlock(&ocfs2_filecheck_sysfs_lock);
>>>>> -			return p;
>>>>> -		}
>>>>> -	}
>>>>> -	spin_unlock(&ocfs2_filecheck_sysfs_lock);
>>>>> -	return NULL;
>>>>> -}
>>>> Gang: We can't delete these code directly, fs_count variable is used to
>>> track if there is any process to use file check related data.
>>>> Please consider this variable before delete this part code.
>>>>
>>>>
>>>>> -int ocfs2_filecheck_create_sysfs(struct super_block *sb)
>>>>> -{
>>>>> -	int ret = 0;
>>>>> -	struct kset *device_kset = NULL;
>>>>> -	struct kset *fcheck_kset = NULL;
>>>>> -	struct ocfs2_filecheck *fcheck = NULL;
>>>>> -	struct ocfs2_filecheck_sysfs_entry *entry = NULL;
>>>>> -	struct attribute **attrs = NULL;
>>>>> -	struct attribute_group attrgp;
>>>>> -
>>>>> -	if (!ocfs2_kset)
>>>>> -		return -ENOMEM;
>>>>> -
>>>>> -	attrs = kmalloc(sizeof(struct attribute *) * 4, GFP_NOFS);
>>>>> -	if (!attrs) {
>>>>> -		ret = -ENOMEM;
>>>>> -		goto error;
>>>>> -	} else {
>>>>> -		attrs[0] = &ocfs2_attr_filecheck_chk.attr;
>>>>> -		attrs[1] = &ocfs2_attr_filecheck_fix.attr;
>>>>> -		attrs[2] = &ocfs2_attr_filecheck_set.attr;
>>>>> -		attrs[3] = NULL;
>>>>> -		memset(&attrgp, 0, sizeof(attrgp));
>>>>> -		attrgp.attrs = attrs;
>>>>> -	}
>>>>> -
>>>>> -	fcheck = kmalloc(sizeof(struct ocfs2_filecheck), GFP_NOFS);
>>>>> -	if (!fcheck) {
>>>>> -		ret = -ENOMEM;
>>>>> -		goto error;
>>>>> -	} else {
>>>>> -		INIT_LIST_HEAD(&fcheck->fc_head);
>>>>> -		spin_lock_init(&fcheck->fc_lock);
>>>>> -		fcheck->fc_max = OCFS2_FILECHECK_MINSIZE;
>>>>> -		fcheck->fc_size = 0;
>>>>> -		fcheck->fc_done = 0;
>>>>> -	}
>>>>> -
>>>>> -	if (strlen(sb->s_id) <= 0) {
>>>>> -		mlog(ML_ERROR,
>>>>> -		"Cannot get device basename when create filecheck sysfs\n");
>>>>> -		ret = -ENODEV;
>>>>> -		goto error;
>>>>> -	}
>>>>> -
>>>>> -	device_kset = kset_create_and_add(sb->s_id, NULL, &ocfs2_kset->kobj);
>>>>> -	if (!device_kset) {
>>>>> -		ret = -ENOMEM;
>>>>> -		goto error;
>>>>> -	}
>>>>> -
>>>>> -	fcheck_kset = kset_create_and_add("filecheck", NULL,
>>>>> -					  &device_kset->kobj);
>>>>> -	if (!fcheck_kset) {
>>>>> -		ret = -ENOMEM;
>>>>> -		goto error;
>>>>> -	}
>>>>> -
>>>>> -	ret = sysfs_create_group(&fcheck_kset->kobj, &attrgp);
>>>>> -	if (ret)
>>>>> -		goto error;
>>>>> -
>>>>> -	entry = kmalloc(sizeof(struct ocfs2_filecheck_sysfs_entry), GFP_NOFS);
>>>>> -	if (!entry) {
>>>>> -		ret = -ENOMEM;
>>>>> -		goto error;
>>>>> -	} else {
>>>>> -		atomic_set(&entry->fs_count, 1);
>>>>> -		entry->fs_sb = sb;
>>>>> -		entry->fs_devicekset = device_kset;
>>>>> -		entry->fs_fcheckkset = fcheck_kset;
>>>>> -		entry->fs_fcheck = fcheck;
>>>>> -		ocfs2_filecheck_sysfs_add(entry);
>>>>> -	}
>>>>> -
>>>>> -	kfree(attrs);
>>>>> -	return 0;
>>>>> -
>>>>> -error:
>>>>> -	kfree(attrs);
>>>>> -	kfree(entry);
>>>>> -	kfree(fcheck);
>>>>> -	kset_unregister(fcheck_kset);
>>>>> -	kset_unregister(device_kset);
>>>>> -	return ret;
>>>>> -}
>>>>> -
>>>>> -int ocfs2_filecheck_remove_sysfs(struct super_block *sb)
>>>>> -{
>>>>> -	return ocfs2_filecheck_sysfs_del(sb->s_id);
>>>>> -}
>>>> Gang: this part code can be re-used after a little modification.
>>>
>>> For what?
>> Gang: this part code have a complete creating/releasing kset code, have been 
> tested, it will not bring any memory leak.
>>
>>>
>>>>
>>>>> -
>>>>>    static int
>>>>> -ocfs2_filecheck_erase_entries(struct ocfs2_filecheck_sysfs_entry *ent,
>>>>> +ocfs2_filecheck_erase_entries(struct ocfs2_super *,
>>>>>    			      unsigned int count);
>>>>> -static int
>>>>> -ocfs2_filecheck_adjust_max(struct ocfs2_filecheck_sysfs_entry *ent,
>>>>> -			   unsigned int len)
>>>>> +
>>>>> +int ocfs2_filecheck_set_max_entries(struct ocfs2_super *osb,
>>>>> +				int len)
>>>>>    {
>>>>>    	int ret;
>>>>>
>>>>>    	if ((len < OCFS2_FILECHECK_MINSIZE) || (len > OCFS2_FILECHECK_MAXSIZE))
>>>>>    		return -EINVAL;
>>>>>
>>>>> -	spin_lock(&ent->fs_fcheck->fc_lock);
>>>>> -	if (len < (ent->fs_fcheck->fc_size - ent->fs_fcheck->fc_done)) {
>>>>> +	spin_lock(&osb->fc_lock);
>>>>> +	if (len < (osb->fc_size - osb->fc_done)) {
>>>>>    		mlog(ML_ERROR,
>>>>>    		"Cannot set online file check maximum entry number "
>>>>>    		"to %u due to too many pending entries(%u)\n",
>>>>> -		len, ent->fs_fcheck->fc_size - ent->fs_fcheck->fc_done);
>>>>> +		len, osb->fc_size - osb->fc_done);
>>>>>    		ret = -EBUSY;
>>>>>    	} else {
>>>>> -		if (len < ent->fs_fcheck->fc_size)
>>>>> -			BUG_ON(!ocfs2_filecheck_erase_entries(ent,
>>>>> -				ent->fs_fcheck->fc_size - len));
>>>>> +		if (len < osb->fc_size)
>>>>> +			BUG_ON(!ocfs2_filecheck_erase_entries(osb,
>>>>> +				osb->fc_size - len));
>>>>>
>>>>> -		ent->fs_fcheck->fc_max = len;
>>>>> +		osb->fc_max = len;
>>>>>    		ret = 0;
>>>>>    	}
>>>>> -	spin_unlock(&ent->fs_fcheck->fc_lock);
>>>>> +	spin_unlock(&osb->fc_lock);
>>>>>
>>>>>    	return ret;
>>>>>    }
>>>>>
>>>>> -#define OCFS2_FILECHECK_ARGS_LEN	24
>>>>> -static int
>>>>> -ocfs2_filecheck_args_get_long(const char *buf, size_t count,
>>>>> -			      unsigned long *val)
>>>>> -{
>>>>> -	char buffer[OCFS2_FILECHECK_ARGS_LEN];
>>>>> -
>>>>> -	memcpy(buffer, buf, count);
>>>>> -	buffer[count] = '\0';
>>>>> -
>>>>> -	if (kstrtoul(buffer, 0, val))
>>>>> -		return 1;
>>>>> -
>>>>> -	return 0;
>>>>> -}
>>>>> -
>>>>> -static int
>>>>> -ocfs2_filecheck_type_parse(const char *name, unsigned int *type)
>>>>> -{
>>>>> -	if (!strncmp(name, "fix", 4))
>>>>> -		*type = OCFS2_FILECHECK_TYPE_FIX;
>>>>> -	else if (!strncmp(name, "check", 6))
>>>>> -		*type = OCFS2_FILECHECK_TYPE_CHK;
>>>>> -	else if (!strncmp(name, "set", 4))
>>>>> -		*type = OCFS2_FILECHECK_TYPE_SET;
>>>>> -	else
>>>>> -		return 1;
>>>>> -
>>>>> -	return 0;
>>>>> -}
>>>>> -
>>>>> -static int
>>>>> -ocfs2_filecheck_args_parse(const char *name, const char *buf, size_t count,
>>>>> -			   struct ocfs2_filecheck_args *args)
>>>>> -{
>>>>> -	unsigned long val = 0;
>>>>> -	unsigned int type;
>>>>> -
>>>>> -	/* too short/long args length */
>>>>> -	if ((count < 1) || (count >= OCFS2_FILECHECK_ARGS_LEN))
>>>>> -		return 1;
>>>>> -
>>>>> -	if (ocfs2_filecheck_type_parse(name, &type))
>>>>> -		return 1;
>>>>> -	if (ocfs2_filecheck_args_get_long(buf, count, &val))
>>>>> -		return 1;
>>>>> -
>>>>> -	if (val <= 0)
>>>>> -		return 1;
>>>>>
>>>>> -	args->fa_type = type;
>>>>> -	if (type == OCFS2_FILECHECK_TYPE_SET)
>>>>> -		args->fa_len = (unsigned int)val;
>>>>> -	else
>>>>> -		args->fa_ino = val;
>>>>> -
>>>>> -	return 0;
>>>>> -}
>>>>> -
>>>> Gang: please keep the ocfs2_filecheck_args_parse function, or write a
>>> similar argument checking function since the arguments are from user-space,
>>> must be check strictly.
>>>> Otherwise, this will be a way for malicious users to panic the kernel.
>>>
>>> How? Do you have an example?
>> Gang: for example, user input a too long number string, we should return a 
> invalid error directly, not do the further file check operation.
>> We need a ocfs2_filecheck_args_parse function, add some argument checking 
> rules for now and future (when we find more limitations),
>> return the error to the user process directly, to avoid bringing the further 
> IO checking efforts.
>>
>>>
>>>>
>>>>> -static ssize_t ocfs2_filecheck_show(struct kobject *kobj,
>>>>> -				    struct kobj_attribute *attr,
>>>>> +int ocfs2_filecheck_show(struct ocfs2_super *osb, unsigned int type,
>>>>>    				    char *buf)
>>>>>    {
>>>>>
>>>>>    	ssize_t ret = 0, total = 0, remain = PAGE_SIZE;
>>>>> -	unsigned int type;
>>>>>    	struct ocfs2_filecheck_entry *p;
>>>>> -	struct ocfs2_filecheck_sysfs_entry *ent;
>>>>> -
>>>>> -	if (ocfs2_filecheck_type_parse(attr->attr.name, &type))
>>>>> -		return -EINVAL;
>>>>> -
>>>>> -	ent = ocfs2_filecheck_sysfs_get(kobj->parent->name);
>>>> Gang: if delete file check data reference count mechanism, how to make sure
>>> the resource race condition (e.g. list the check results during the file
>>> system umounting.)
>>>> Please consider this race condition problem.
> 
> 
> 
>>>
>>> Isn't spinlock enough? What is the race?
>> Gang: No, since file check operation is executed without spinlock, we need a 
> reference count to keep the related data, until the last usage is finished.
> 
> 
> Filesystem unmonting is a different game. It evicts inodes. Could you 
> clarify more?
> 
> Anyways, doing away with fe_done and using the status instead should help.
Gang: the file check entry is on behalf of one file check user trigger, is not related with inode object in the kernel.
When we get to handle one inode object, we need to follow all the locks of inode.

> 
>>
>>>
>>>>
>>>>> -	if (!ent) {
>>>>> -		mlog(ML_ERROR,
>>>>> -		"Cannot get the corresponding entry via device basename %s\n",
>>>>> -		kobj->name);
>>>>> -		return -ENODEV;
>>>>> -	}
>>>>> -
>>>>> -	if (type == OCFS2_FILECHECK_TYPE_SET) {
>>>>> -		spin_lock(&ent->fs_fcheck->fc_lock);
>>>>> -		total = snprintf(buf, remain, "%u\n", ent->fs_fcheck->fc_max);
>>>>> -		spin_unlock(&ent->fs_fcheck->fc_lock);
>>>>> -		goto exit;
>>>>> -	}
>>>>>
>>>>>    	ret = snprintf(buf, remain, "INO\t\tDONE\tERROR\n");
>>>>>    	total += ret;
>>>>>    	remain -= ret;
>>>>> -	spin_lock(&ent->fs_fcheck->fc_lock);
>>>>> -	list_for_each_entry(p, &ent->fs_fcheck->fc_head, fe_list) {
>>>>> +	spin_lock(&osb->fc_lock);
>>>>> +	list_for_each_entry(p, &osb->file_check_entries, fe_list) {
>>>>>    		if (p->fe_type != type)
>>>>>    			continue;
>>>>>
>>>>> @@ -443,24 +127,21 @@ static ssize_t ocfs2_filecheck_show(struct kobject
>>>>> *kobj,
>>>>>    		total += ret;
>>>>>    		remain -= ret;
>>>>>    	}
>>>>> -	spin_unlock(&ent->fs_fcheck->fc_lock);
>>>>> -
>>>>> -exit:
>>>>> -	ocfs2_filecheck_sysfs_put(ent);
>>>>> +	spin_unlock(&osb->fc_lock);
>>>>>    	return total;
>>>>>    }
>>>>>
>>>>>    static int
>>>>> -ocfs2_filecheck_erase_entry(struct ocfs2_filecheck_sysfs_entry *ent)
>>>>> +ocfs2_filecheck_erase_entry(struct ocfs2_super *osb)
>>>>>    {
>>>>>    	struct ocfs2_filecheck_entry *p;
>>>>>
>>>>> -	list_for_each_entry(p, &ent->fs_fcheck->fc_head, fe_list) {
>>>>> +	list_for_each_entry(p, &osb->file_check_entries, fe_list) {
>>>>>    		if (p->fe_done) {
>>>>>    			list_del(&p->fe_list);
>>>>>    			kfree(p);
>>>>> -			ent->fs_fcheck->fc_size--;
>>>>> -			ent->fs_fcheck->fc_done--;
>>>>> +			osb->fc_size--;
>>>>> +			osb->fc_done--;
>>>>>    			return 1;
>>>>>    		}
>>>>>    	}
>>>>> @@ -469,14 +150,14 @@ ocfs2_filecheck_erase_entry(struct
>>>>> ocfs2_filecheck_sysfs_entry *ent)
>>>>>    }
>>>>>
>>>>>    static int
>>>>> -ocfs2_filecheck_erase_entries(struct ocfs2_filecheck_sysfs_entry *ent,
>>>>> +ocfs2_filecheck_erase_entries(struct ocfs2_super *osb,
>>>>>    			      unsigned int count)
>>>>>    {
>>>>>    	unsigned int i = 0;
>>>>>    	unsigned int ret = 0;
>>>>>
>>>>>    	while (i++ < count) {
>>>>> -		if (ocfs2_filecheck_erase_entry(ent))
>>>>> +		if (ocfs2_filecheck_erase_entry(osb))
>>>>>    			ret++;
>>>>>    		else
>>>>>    			break;
>>>>> @@ -486,24 +167,24 @@ ocfs2_filecheck_erase_entries(struct
>>>>> ocfs2_filecheck_sysfs_entry *ent,
>>>>>    }
>>>>>
>>>>>    static void
>>>>> -ocfs2_filecheck_done_entry(struct ocfs2_filecheck_sysfs_entry *ent,
>>>>> +ocfs2_filecheck_done_entry(struct ocfs2_super *osb,
>>>>>    			   struct ocfs2_filecheck_entry *entry)
>>>>>    {
>>>>>    	entry->fe_done = 1;
>>>>> -	spin_lock(&ent->fs_fcheck->fc_lock);
>>>>> -	ent->fs_fcheck->fc_done++;
>>>>> -	spin_unlock(&ent->fs_fcheck->fc_lock);
>>>>> +	spin_lock(&osb->fc_lock);
>>>>> +	osb->fc_done++;
>>>>> +	spin_unlock(&osb->fc_lock);
>>>>>    }
>>>>>
>>>>>    static unsigned int
>>>>> -ocfs2_filecheck_handle(struct super_block *sb,
>>>>> +ocfs2_filecheck_handle(struct ocfs2_super *osb,
>>>>>    		       unsigned long ino, unsigned int flags)
>>>>>    {
>>>>>    	unsigned int ret = OCFS2_FILECHECK_ERR_SUCCESS;
>>>>>    	struct inode *inode = NULL;
>>>>>    	int rc;
>>>>>
>>>>> -	inode = ocfs2_iget(OCFS2_SB(sb), ino, flags, 0);
>>>>> +	inode = ocfs2_iget(osb, ino, flags, 0);
>>>>>    	if (IS_ERR(inode)) {
>>>>>    		rc = (int)(-(long)inode);
>>>>>    		if (rc >= OCFS2_FILECHECK_ERR_START &&
>>>>> @@ -518,89 +199,64 @@ ocfs2_filecheck_handle(struct super_block *sb,
>>>>>    }
>>>>>
>>>>>    static void
>>>>> -ocfs2_filecheck_handle_entry(struct ocfs2_filecheck_sysfs_entry *ent,
>>>>> +ocfs2_filecheck_handle_entry(struct ocfs2_super *osb,
>>>>>    			     struct ocfs2_filecheck_entry *entry)
>>>>>    {
>>>>>    	if (entry->fe_type == OCFS2_FILECHECK_TYPE_CHK)
>>>>> -		entry->fe_status = ocfs2_filecheck_handle(ent->fs_sb,
>>>>> +		entry->fe_status = ocfs2_filecheck_handle(osb,
>>>>>    				entry->fe_ino, OCFS2_FI_FLAG_FILECHECK_CHK);
>>>>>    	else if (entry->fe_type == OCFS2_FILECHECK_TYPE_FIX)
>>>>> -		entry->fe_status = ocfs2_filecheck_handle(ent->fs_sb,
>>>>> +		entry->fe_status = ocfs2_filecheck_handle(osb,
>>>>>    				entry->fe_ino, OCFS2_FI_FLAG_FILECHECK_FIX);
>>>>>    	else
>>>>>    		entry->fe_status = OCFS2_FILECHECK_ERR_UNSUPPORTED;
>>>>>
>>>>> -	ocfs2_filecheck_done_entry(ent, entry);
>>>>> +	ocfs2_filecheck_done_entry(osb, entry);
>>>>>    }
>>>>>
>>>>> -static ssize_t ocfs2_filecheck_store(struct kobject *kobj,
>>>>> -				     struct kobj_attribute *attr,
>>>>> -				     const char *buf, size_t count)
>>>>> +int ocfs2_filecheck_add_inode(struct ocfs2_super *osb,
>>>>> +				     unsigned long ino)
>>>>>    {
>>>>> -	struct ocfs2_filecheck_args args;
>>>>>    	struct ocfs2_filecheck_entry *entry;
>>>>> -	struct ocfs2_filecheck_sysfs_entry *ent;
>>>>>    	ssize_t ret = 0;
>>>>>
>>>>> -	if (count == 0)
>>>>> -		return count;
>>>>> -
>>>>> -	if (ocfs2_filecheck_args_parse(attr->attr.name, buf, count, &args)) {
>>>>> -		mlog(ML_ERROR, "Invalid arguments for online file check\n");
>>>>> -		return -EINVAL;
>>>>> -	}
>>>>> -
>>>>> -	ent = ocfs2_filecheck_sysfs_get(kobj->parent->name);
>>>>> -	if (!ent) {
>>>>> -		mlog(ML_ERROR,
>>>>> -		"Cannot get the corresponding entry via device basename %s\n",
>>>>> -		kobj->parent->name);
>>>>> -		return -ENODEV;
>>>>> -	}
>>>>> -
>>>>> -	if (args.fa_type == OCFS2_FILECHECK_TYPE_SET) {
>>>>> -		ret = ocfs2_filecheck_adjust_max(ent, args.fa_len);
>>>>> -		goto exit;
>>>>> -	}
>>>>> -
>>>>>    	entry = kmalloc(sizeof(struct ocfs2_filecheck_entry), GFP_NOFS);
>>>>>    	if (!entry) {
>>>>>    		ret = -ENOMEM;
>>>>>    		goto exit;
>>>>>    	}
>>>>>
>>>>> -	spin_lock(&ent->fs_fcheck->fc_lock);
>>>>> -	if ((ent->fs_fcheck->fc_size >= ent->fs_fcheck->fc_max) &&
>>>>> -	    (ent->fs_fcheck->fc_done == 0)) {
>>>>> +	spin_lock(&osb->fc_lock);
>>>>> +	if ((osb->fc_size >= osb->fc_max) &&
>>>>> +	    (osb->fc_done == 0)) {
>>>>>    		mlog(ML_ERROR,
>>>>>    		"Cannot do more file check "
>>>>>    		"since file check queue(%u) is full now\n",
>>>>> -		ent->fs_fcheck->fc_max);
>>>>> +		osb->fc_max);
>>>>>    		ret = -EBUSY;
>>>>>    		kfree(entry);
>>>>>    	} else {
>>>>> -		if ((ent->fs_fcheck->fc_size >= ent->fs_fcheck->fc_max) &&
>>>>> -		    (ent->fs_fcheck->fc_done > 0)) {
>>>>> +		if ((osb->fc_size >= osb->fc_max) &&
>>>>> +		    (osb->fc_done > 0)) {
>>>>>    			/* Delete the oldest entry which was done,
>>>>>    			 * make sure the entry size in list does
>>>>>    			 * not exceed maximum value
>>>>>    			 */
>>>>> -			BUG_ON(!ocfs2_filecheck_erase_entry(ent));
>>>>> +			BUG_ON(!ocfs2_filecheck_erase_entry(osb));
>>>>>    		}
>>>>>
>>>>> -		entry->fe_ino = args.fa_ino;
>>>>> -		entry->fe_type = args.fa_type;
>>>>> +		entry->fe_ino = ino;
>>>>> +		entry->fe_type = OCFS2_FILECHECK_TYPE_CHK;
>>>>>    		entry->fe_done = 0;
>>>>>    		entry->fe_status = OCFS2_FILECHECK_ERR_INPROGRESS;
>>>>> -		list_add_tail(&entry->fe_list, &ent->fs_fcheck->fc_head);
>>>>> -		ent->fs_fcheck->fc_size++;
>>>>> +		list_add_tail(&entry->fe_list, &osb->file_check_entries);
>>>>> +		osb->fc_size++;
>>>>>    	}
>>>>> -	spin_unlock(&ent->fs_fcheck->fc_lock);
>>>>> +	spin_unlock(&osb->fc_lock);
>>>>>
>>>>>    	if (!ret)
>>>>> -		ocfs2_filecheck_handle_entry(ent, entry);
>>>>> +		ocfs2_filecheck_handle_entry(osb, entry);
>>>>>
>>>>>    exit:
>>>>> -	ocfs2_filecheck_sysfs_put(ent);
>>>>> -	return (!ret ? count : ret);
>>>>> +	return ret;
>>>>>    }
>>>>> diff --git a/fs/ocfs2/filecheck.h b/fs/ocfs2/filecheck.h
>>>>> index e5cd002..b1a0d8c 100644
>>>>> --- a/fs/ocfs2/filecheck.h
>>>>> +++ b/fs/ocfs2/filecheck.h
>>>>> @@ -42,8 +42,18 @@ enum {
>>>>>
>>>>>    #define OCFS2_FILECHECK_ERR_START	OCFS2_FILECHECK_ERR_FAILED
>>>>>    #define OCFS2_FILECHECK_ERR_END		OCFS2_FILECHECK_ERR_UNSUPPORTED
>>>>> +#define OCFS2_FILECHECK_MAXSIZE         100
>>>>> +#define OCFS2_FILECHECK_MINSIZE         10
>>>>> +
>>>>> +/* File check operation type */
>>>>> +#define OCFS2_FILECHECK_TYPE_CHK  	1   /* Check a file(inode) */
>>>>> +#define OCFS2_FILECHECK_TYPE_FIX  	2   /* Fix a file(inode) */
>>>> Please use enum, not use macro definitions, maybe there will be more type.
>>>> OCFS2_FILECHECK_TYPE_CHK should be zero, otherwise this macro is different
>>> with previous enum definitions.
>>>>
>>>>>
>>>>>    int ocfs2_filecheck_create_sysfs(struct super_block *sb);
>>>>>    int ocfs2_filecheck_remove_sysfs(struct super_block *sb);
>>>>> +int ocfs2_filefix_inode(struct ocfs2_super *osb, unsigned long ino);
>>>>> +int ocfs2_filecheck_add_inode(struct ocfs2_super *osb, unsigned long ino);
>>>>> +int ocfs2_filecheck_set_max_entries(struct ocfs2_super *osb, int num);
>>>>> +int ocfs2_filecheck_show(struct ocfs2_super *osb, unsigned int type, char
>>>>> *buf);
>>>>>
>>>>>    #endif  /* FILECHECK_H */
>>>>> diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
>>>>> index 8e66cdf..9ced543 100644
>>>>> --- a/fs/ocfs2/ocfs2.h
>>>>> +++ b/fs/ocfs2/ocfs2.h
>>>>> @@ -474,6 +474,13 @@ struct ocfs2_super
>>>>>    	 */
>>>>>    	struct workqueue_struct *ocfs2_wq;
>>>>>
>>>>> +	/* file check */
>>>>> +	struct list_head file_check_entries;
>>>>> +	unsigned int fc_size;
>>>>> +	unsigned int fc_max;
>>>>> +	unsigned int fc_done;
>>>>> +	spinlock_t fc_lock;
>>>>> +
>>>> Gang: please use a separate structure to include all the file check members,
>>> easy to read/maintain.
>>>
>>>
>>> I would prefer it to be a part of ocfs2_super. It is easier to maintain
>>> and read.
>>>
>>>>
>>>>>    	struct kobject kobj;
>>>>>    	struct completion kobj_unregister;
>>>>>    };
>>>>> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
>>>>> index 96b7a9f..a7791fa 100644
>>>>> --- a/fs/ocfs2/super.c
>>>>> +++ b/fs/ocfs2/super.c
>>>>> @@ -2215,6 +2215,13 @@ static int ocfs2_initialize_super(struct super_block
>>>>> *sb,
>>>>>
>>>>>    	get_random_bytes(&osb->s_next_generation, sizeof(u32));
>>>>>
>>>>> +	/* file check information */
>>>>> +	INIT_LIST_HEAD(&osb->file_check_entries);
>>>>> +	osb->fc_max = OCFS2_FILECHECK_MINSIZE;
>>>>> +	osb->fc_size = 0;
>>>>> +	osb->fc_done = 0;
>>>>> +	spin_lock_init(&osb->fc_lock);
>>>>> +
>>>> Gang: write a separate funtion in filecheck.c to include these line code,
>>> easy to read/maintain.
>>>
>>> Why? Unless we are starting something specific to filecheck, I don't see
>>> any need.
>> Gang: I just want to make the file check thing is done in filecheck source 
> file, the other module just call some functions.
>> Let the code more clear.
>>
>>>
>>>>
>>>>>    	/* FIXME
>>>>>    	 * This should be done in ocfs2_journal_init(), but unknown
>>>>>    	 * ordering issues will cause the filesystem to crash.
>>>>> diff --git a/fs/ocfs2/sysfs.c b/fs/ocfs2/sysfs.c
>>>>> index e21e699..81e3dd4 100644
>>>>> --- a/fs/ocfs2/sysfs.c
>>>>> +++ b/fs/ocfs2/sysfs.c
>>>>> @@ -1,5 +1,6 @@
>>>>>    #include "ocfs2.h"
>>>>>    #include "sysfs.h"
>>>>> +#include "filecheck.h"
>>>>>
>>>>>    struct ocfs2_sb_attr {
>>>>>    	struct attribute attr;
>>>>> @@ -9,8 +10,12 @@ struct ocfs2_sb_attr {
>>>>>    			const char *buf, size_t count);
>>>>>    };
>>>>>
>>>>> -#define OCFS2_SB_ATTR(_name, _mode) \
>>>>> -struct ocfs2_sb_attr sb_attr_##_name = __ATTR(name, _mode, _show, _store)
>>>>> +#define OCFS2_SB_ATTR(_name) \
>>>>> +struct ocfs2_sb_attr sb_attr_##_name = { \
>>>>> +	.attr = {.name = __stringify(_name), .mode = (S_IWUSR | S_IRUGO)},  \
>>>>> +	.show = _name##_show, \
>>>>> +	.store = _name##_store \
>>>>> +}
>>>>>
>>>>>    #define OCFS2_SB_ATTR_RO(_name) \
>>>>>    struct ocfs2_sb_attr sb_attr_##_name = __ATTR_RO(_name)
>>>>> @@ -46,6 +51,81 @@ static ssize_t slot_num_show(struct ocfs2_super *osb,
>>>>>    	return sprintf(buf, "%d\n", osb->slot_num);
>>>>>    }
>>>>>
>>>>> +static ssize_t file_check_show(struct ocfs2_super *osb,
>>>>> +		struct ocfs2_sb_attr *attr,
>>>>> +		char *buf)
>>>>> +{
>>>>> +	return ocfs2_filecheck_show(osb, OCFS2_FILECHECK_TYPE_CHK, buf);
>>>>> +}
>>>>> +
>>>>> +static ssize_t file_check_store(struct ocfs2_super *osb,
>>>>> +		struct ocfs2_sb_attr *attr,
>>>>> +		const char *buf, size_t count)
>>>>> +{
>>>>> +	unsigned long t;
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = kstrtoul(skip_spaces(buf), 0, &t);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +	ret = ocfs2_filecheck_add_inode(osb, t);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +	return count;
>>>>> +}
>>>>> +
>>>>> +static ssize_t file_fix_show(struct ocfs2_super *osb,
>>>>> +		struct ocfs2_sb_attr *attr,
>>>>> +		char *buf)
>>>>> +{
>>>>> +	return ocfs2_filecheck_show(osb, OCFS2_FILECHECK_TYPE_FIX, buf);
>>>>> +}
>>>>> +
>>>>> +static ssize_t file_fix_store(struct ocfs2_super *osb,
>>>>> +		struct ocfs2_sb_attr *attr,
>>>>> +		const char *buf, size_t count)
>>>>> +{
>>>>> +	unsigned long t;
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = kstrtoul(skip_spaces(buf), 0, &t);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +	ret = ocfs2_filecheck_add_inode(osb, t);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +	return count;
>>>>> +}
>>>>> +
>>>>> +static ssize_t file_check_max_entries_show(struct ocfs2_super *osb,
>>>>> +		struct ocfs2_sb_attr *attr,
>>>>> +		char *buf)
>>>>> +{
>>>>> +	int len = 0;
>>>>> +	spin_lock(&osb->fc_lock);
>>>>> +	/* Show done, current size and max */
>>>>> +	len += sprintf(buf, "%d\t%d\t%d\n", osb->fc_done, osb->fc_size,
>>>>> +			osb->fc_max);
>>>>> +	spin_unlock(&osb->fc_lock);
>>>>> +	return len;
>>>>> +}
>>>>> +
>>>>> +static ssize_t file_check_max_entries_store(struct ocfs2_super *osb,
>>>>> +		struct ocfs2_sb_attr *attr, const char *buf, size_t count)
>>>>> +{
>>>>> +
>>>>> +	unsigned long t;
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = kstrtoul(skip_spaces(buf), 0, &t);
>>>> Gang: please make sure the function kstrtoul does work well since the
>>> inputed buf has not terminating null character.
>>>> This is why I write a wrapper function ocfs2_filecheck_args_get_long().
>>>
>>> Can you give an example on how this can be activated? Maybe a check on
>>> value < 0 may make sense.
>> Gang: I am not sure if this function works well when the string is inputed 
> without a terminating null character.
>> need to test all the case if using the function directly. But we test all 
> the case with a terminating null character.
>>
> 
> So, do you have a testcase?
Gang: Eric tested this manually in various possible cases, not sure if he wrote these cases in the scripts.

> 
>>
>>>
>>>>
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +	ret = ocfs2_filecheck_set_max_entries(osb, (int)t);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +	return count;
>>>>> +}
>>>>> +
>>>>>    static void sb_release(struct kobject *kobj)
>>>>>    {
>>>>>    	struct ocfs2_super *osb = container_of(kobj, struct ocfs2_super, kobj);
>>>>> @@ -58,8 +138,15 @@ static const struct sysfs_ops ocfs2_sb_sysfs_ops = {
>>>>>    };
>>>>>
>>>>>    static OCFS2_SB_ATTR_RO(slot_num);
>>>>> +static OCFS2_SB_ATTR(file_check);
>>>>> +static OCFS2_SB_ATTR(file_fix);
>>>>> +static OCFS2_SB_ATTR(file_check_max_entries);
>>>>> +
>>>>>    static struct attribute *ocfs2_sb_attrs[] = {
>>>>>    	&sb_attr_slot_num.attr,
>>>>> +	&sb_attr_file_check.attr,
>>>>> +	&sb_attr_file_fix.attr,
>>>>> +	&sb_attr_file_check_max_entries.attr,
>>>>>    	NULL
>>>>>    };
>>>> Gang: as I said in the first patch, please make all the file check related
>>> attributes in a separate sub directory.
>>>> It will let us to be easy to read/debug, especially if there is any
>>> reference count leaking problem.
>>>
>>> The main reason of the code is to remove unnecessary data structures
>>> which is making the code hard to understand. I don't see a point in
>>> keeping them in a separate structure besides adding more indirections.
>>>
>>>
>>> --
>>> Goldwyn
> 
> -- 
> Goldwyn