ocfs2: sysfs and cleanup

Quoted 1 line

Quoted 1 line

From: Goldwyn Rodrigues <[email protected]>

This adds /sys/fs/ocfs2/<s_id> to the sys filesystem. This

fs/ocfs2/sysfs.h new file +9 -0

@@ -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

Signature · 7 lines

2.6.6


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

Hello Goldwyn,

Thanks for your code, my comments are inline.

Quoted 3 lines

Quoted 1 line

Quoted 1 line

From: Goldwyn Rodrigues <[email protected]>

Quoted 1 line

This adds /sys/fs/ocfs2/<s_id> to the sys filesystem. This

Gang: My suggestion is to create a sysfs directory device_name under /sys/fs/ocfs2 per each file system, The file system generic attributes should be located under this top sub directory /sys/fs/ocfs2/<s_id>, but we should create a separate sub directory (e.g. filecheck) under this top sub directory for file check feature, then, all the file check related attributes should be located under that directory /sys/fs/ocfs2/<s_id>/filecheck. You know, in the future, we maybe add more attributes/sub directories under /sys/fs/ocfs2/<s_id>,
I should make sure the file check feature is not mixed with other things, it is easy to read/debug for the users/developers.

fs/ocfs2/sysfs.c

Quoted 97 lines

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 <[email protected]>

---
  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

fs/ocfs2/Makefile +2 -1

index e27e652..716ed45 100644
@@ -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

fs/ocfs2/ocfs2.h +4 -0

index e63af7d..8e66cdf 100644
@@ -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)

fs/ocfs2/super.c +4 -3

index d7cae33..96b7a9f 100644
@@ -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)

fs/ocfs2/sysfs.c new file +0 -0

new file mode 100644
index 0000000..e21e699

Gang: please add source file comments like other source files.

Quoted 91 lines

fs/ocfs2/sysfs.c new file +90 -0

@@ -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->store)
+		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);
+}

fs/ocfs2/sysfs.h

Quoted 7 lines

fs/ocfs2/sysfs.c new file +2 -0

+
+

fs/ocfs2/sysfs.h new file +0 -0

new file mode 100644
index 0000000..d929ac1

Gang: please add header file comments like other header files.

Quoted 18 lines

fs/ocfs2/sysfs.h new file +9 -1

@@ -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 mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

Quoted 4 lines

Quoted 4 lines

Quoted 10 lines · Gang He

Hello Goldwyn,

Thanks for your code, my comments are inline.

Quoted 4 lines

Quoted 1 line

Quoted 1 line

From: Goldwyn Rodrigues <[email protected]>

This adds /sys/fs/ocfs2/<s_id> to the sys filesystem. This

Gang: My suggestion is to create a sysfs directory device_name under

/sys/fs/ocfs2 per each file system,

Quoted 1 line

The file system generic attributes should be located under this top sub

directory /sys/fs/ocfs2/<s_id>,

Quoted 1 line

but we should create a separate sub directory (e.g. filecheck) under this

top sub directory for file check feature,

Quoted 1 line

then, all the file check related attributes should be located under that

directory /sys/fs/ocfs2/<s_id>/filecheck.

Quoted 1 line

You know, in the future, we maybe add more attributes/sub directories under

/sys/fs/ocfs2/<s_id>,

Quoted 1 line

I should make sure the file check feature is not mixed with other things, it

is easy to read/debug for the users/developers.

Quoted 1 line

I don't think keeping it separate is important. We would need a separate kset for incorporating all of this. Besides, mulitple redirections will mess it up more than keep it simple. However, we should document what each portion is used for.

I don't see a point in using multiple structures when they would not be references at multiple points, at least in this case.

Quoted 192 lines

fs/ocfs2/sysfs.c

Quoted 99 lines

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 <[email protected]>

---
  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

fs/ocfs2/Makefile +2 -1

index e27e652..716ed45 100644
@@ -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

fs/ocfs2/ocfs2.h +4 -0

index e63af7d..8e66cdf 100644
@@ -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)

fs/ocfs2/super.c +4 -3

index d7cae33..96b7a9f 100644
@@ -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)

fs/ocfs2/sysfs.c new file +0 -0

new file mode 100644
index 0000000..e21e699

Gang: please add source file comments like other source files.

Quoted 91 lines

fs/ocfs2/sysfs.c new file +90 -0

@@ -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->store)
+		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);
+}

Quoted 9 lines

fs/ocfs2/sysfs.h

Quoted 7 lines

fs/ocfs2/sysfs.c new file +2 -0

+
+

fs/ocfs2/sysfs.h new file +0 -0

new file mode 100644
index 0000000..d929ac1

Gang: please add header file comments like other header files.

Agree.

Quoted 18 lines

Quoted 18 lines

fs/ocfs2/sysfs.h new file +9 -1

@@ -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 mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

Signature · 1 line

Goldwyn

Quoted 1 line

Quoted 1 line

From: Goldwyn Rodrigues <[email protected]>

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 <[email protected]>

---
 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(-)

fs/ocfs2/filecheck.c +0 -9

index 2cabbcf..0b41967 100644
@@ -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);

fs/ocfs2/filecheck.c +0 -1

-
 struct ocfs2_filecheck_entry {
 	struct list_head fe_list;

fs/ocfs2/filecheck.c +0 -47

 	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);

fs/ocfs2/filecheck.c +0 -39

-
-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);

fs/ocfs2/filecheck.c +0 -81

-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);

fs/ocfs2/filecheck.c +12 -62

-
 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;

fs/ocfs2/filecheck.c +1 -2

-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)
 {

fs/ocfs2/filecheck.c +7 -23

-	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;
 		}

fs/ocfs2/filecheck.c +2 -2

 	}

@@ -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

fs/ocfs2/filecheck.c +6 -6

 			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);

fs/ocfs2/filecheck.c +20 -45

 		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;
 }

fs/ocfs2/filecheck.h +0 -0

index e5cd002..b1a0d8c 100644

fs/ocfs2/filecheck.h +4 -0

 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 */

fs/ocfs2/ocfs2.h +0 -0

index 8e66cdf..9ced543 100644

fs/ocfs2/ocfs2.h +0 -0

 	struct kobject kobj;
 	struct completion kobj_unregister;
 };

fs/ocfs2/super.c +0 -0

index 96b7a9f..a7791fa 100644
@@ -2215,6 +2215,13 @@ static int ocfs2_initialize_super(struct super_block 
 *sb,

fs/ocfs2/super.c +0 -0

 	/* FIXME
 	 * This should be done in ocfs2_journal_init(), but unknown

fs/ocfs2/super.c +0 -0

 	 * ordering issues will cause the filesystem to crash.

fs/ocfs2/sysfs.c +64 -2

index e21e699..81e3dd4 100644
@@ -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;

fs/ocfs2/sysfs.c +9 -0

+	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);

Signature · 8 lines

-- 
2.6.6


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

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

Quoted 43 lines

Quoted 1 line

Quoted 1 line

From: Goldwyn Rodrigues <[email protected]>

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 <[email protected]>

---
  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(-)

fs/ocfs2/filecheck.c +0 -11

index 2cabbcf..0b41967 100644
@@ -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);
-

fs/ocfs2/filecheck.c static const char * const ocfs2_filecheck_errs[] = { · line 65

Quoted 7 lines

fs/ocfs2/filecheck.c +0 -7

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

fs/ocfs2/filecheck.c static const char * const ocfs2_filecheck_errs[] = { · line 74

Quoted 11 lines

fs/ocfs2/filecheck.c +0 -9

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

fs/ocfs2/filecheck.c static const char * const ocfs2_filecheck_errs[] = { · line 77

Quoted 4 lines

fs/ocfs2/filecheck.c +0 -3

-
-#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.

fs/ocfs2/filecheck.c static const char * const ocfs2_filecheck_errs[] = { · line 84

Quoted 8 lines

fs/ocfs2/filecheck.c +0 -7

-
-/* 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.

Quoted 67 lines

fs/ocfs2/filecheck.c +0 -52

-
  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);

fs/ocfs2/filecheck.c ocfs2_filecheck_error(int errno) · line 161

Quoted 6 lines

fs/ocfs2/filecheck.c +0 -6

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

Quoted 40 lines

fs/ocfs2/filecheck.c +0 -39

-
-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);

fs/ocfs2/filecheck.c ocfs2_filecheck_error(int errno) · line 210

Quoted 10 lines

fs/ocfs2/filecheck.c +0 -10

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

Quoted 84 lines

fs/ocfs2/filecheck.c +0 -84

-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);

fs/ocfs2/filecheck.c ocfs2_filecheck_error(int errno) · line 302

Quoted 7 lines

fs/ocfs2/filecheck.c +0 -7

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

Quoted 96 lines

fs/ocfs2/filecheck.c +12 -62

-
  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;
 

fs/ocfs2/filecheck.c ocfs2_filecheck_error(int errno) · line 394

Quoted 9 lines

fs/ocfs2/filecheck.c +0 -9

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

Quoted 7 lines

fs/ocfs2/filecheck.c +1 -2

-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)
  {
 

fs/ocfs2/filecheck.c ocfs2_filecheck_error(int errno) · line 408

Quoted 9 lines

fs/ocfs2/filecheck.c +0 -7

  	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.

Quoted 221 lines

fs/ocfs2/filecheck.c +7 -23

-	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;
  		}

  	}

fs/ocfs2/filecheck.c +2 -2

@@ -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;

fs/ocfs2/filecheck.c +6 -6

@@ -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 &&

fs/ocfs2/filecheck.c +20 -45

@@ -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;
  }

fs/ocfs2/filecheck.h +0 -0

index e5cd002..b1a0d8c 100644

fs/ocfs2/filecheck.h enum { · line 50

Quoted 9 lines

fs/ocfs2/filecheck.h +6 -0

@@ -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.

Quoted 18 lines

fs/ocfs2/filecheck.h +4 -0

  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 */

fs/ocfs2/ocfs2.h +0 -0

index 8e66cdf..9ced543 100644
@@ -474,6 +474,13 @@ struct ocfs2_super
  	 */
  	struct workqueue_struct *ocfs2_wq;
 

fs/ocfs2/ocfs2.h struct ocfs2_super · line 483

Quoted 7 lines

fs/ocfs2/ocfs2.h +7 -0

+	/* 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.

Quoted 13 lines

fs/ocfs2/ocfs2.h +0 -0

  	struct kobject kobj;
  	struct completion kobj_unregister;
  };

fs/ocfs2/super.c +0 -0

index 96b7a9f..a7791fa 100644
@@ -2215,6 +2215,13 @@ static int ocfs2_initialize_super(struct super_block
 *sb,
 
  	get_random_bytes(&osb->s_next_generation, sizeof(u32));
 

fs/ocfs2/super.c static int ocfs2_initialize_super(struct super_block *sb, · line 2224

Quoted 7 lines

fs/ocfs2/super.c +7 -0

+	/* 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.

Quoted 95 lines

fs/ocfs2/super.c +0 -0

  	/* FIXME
  	 * This should be done in ocfs2_journal_init(), but unknown

* ordering issues will cause the filesystem to crash.

fs/ocfs2/sysfs.c +68 -2

index e21e699..81e3dd4 100644
@@ -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)

fs/ocfs2/sysfs.c static ssize_t slot_num_show(struct ocfs2_super *osb, · line 68

Quoted 6 lines

fs/ocfs2/sysfs.c +6 -0

+{
+
+	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().

Quoted 15 lines

fs/ocfs2/sysfs.c +8 -0

+	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 = {
  };
 

fs/ocfs2/sysfs.c static const struct sysfs_ops ocfs2_sb_sysfs_ops = { · line 151

Quoted 12 lines

fs/ocfs2/sysfs.c +7 -0

  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.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines · 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.

Quoted 15 lines

Thanks
Gang

Quoted 10 lines

Quoted 1 line

Quoted 1 line

From: Goldwyn Rodrigues <[email protected]>

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 <[email protected]>

---
  fs/ocfs2/filecheck.c | 440 

++++++---------------------------------------------

Quoted 34 lines

Quoted 33 lines

  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(-)

fs/ocfs2/filecheck.c +0 -11

index 2cabbcf..0b41967 100644
@@ -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);
-

Quoted 8 lines

fs/ocfs2/filecheck.c static const char * const ocfs2_filecheck_errs[] = { · line 65

Quoted 7 lines

fs/ocfs2/filecheck.c +0 -7

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

Quoted 1 line

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.

Quoted 13 lines

fs/ocfs2/filecheck.c static const char * const ocfs2_filecheck_errs[] = { · line 74

Quoted 12 lines

fs/ocfs2/filecheck.c +0 -9

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

Quoted 1 line

e.g. fs_count, which is used to track if there are any pending file check

entries, can be moved into struct ocfs2_filecheck.

Quoted 1 line

kset members maybe be kept in struct ocfs2_filecheck.

Okay, I will fix that.

Quoted 6 lines

fs/ocfs2/filecheck.c static const char * const ocfs2_filecheck_errs[] = { · line 77

Quoted 4 lines

fs/ocfs2/filecheck.c +0 -3

-
-#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.

Quoted 10 lines

fs/ocfs2/filecheck.c static const char * const ocfs2_filecheck_errs[] = { · line 84

Quoted 8 lines

fs/ocfs2/filecheck.c +0 -7

-
-/* 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.

Quoted 67 lines

Quoted 67 lines

fs/ocfs2/filecheck.c +0 -51

-
  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);
-	}

Quoted 8 lines

fs/ocfs2/filecheck.c ocfs2_filecheck_error(int errno) · line 161

Quoted 7 lines

fs/ocfs2/filecheck.c +0 -7

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

Quoted 1 line

Please keep the code, otherwise, it will lead to memory leak.

Yes, there should a cleanup routine.

Quoted 43 lines

Quoted 43 lines

fs/ocfs2/filecheck.c +0 -42

-
-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);

Quoted 8 lines

fs/ocfs2/filecheck.c ocfs2_filecheck_error(int errno) · line 210

Quoted 7 lines

fs/ocfs2/filecheck.c +0 -7

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

Quoted 87 lines

Please consider this variable before delete this part code.

Quoted 84 lines

fs/ocfs2/filecheck.c +0 -84

-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);

Quoted 8 lines

fs/ocfs2/filecheck.c ocfs2_filecheck_error(int errno) · line 302

Quoted 7 lines

fs/ocfs2/filecheck.c +0 -7

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