[PATCH v3] f2fs: add symbolic link to kobject in sysfs

[PATCH v3] f2fs: add symbolic link to kobject in sysfs

[Daeho Jeong]

From: Daeho Jeong <daehojeong@google.com>

Added a symbolic link to directory of sysfs. It will
create a symbolic link such as "mount_0" and "mount_1" to
each f2fs mount in the order of mounting filesystem. But
once one mount point was umounted, that sequential number
@x in "mount_@x" could be reused by later newly mounted
point. It will provide easy access to sysfs node even if
not knowing the specific device node name like sda19 and
dm-3.

Signed-off-by: Daeho Jeong <daehojeong@google.com>
---
 Documentation/filesystems/f2fs.rst | 13 ++++++++++++-
 fs/f2fs/f2fs.h                     |  4 ++++
 fs/f2fs/sysfs.c                    | 31 ++++++++++++++++++++++++++----
 3 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/Documentation/filesystems/f2fs.rst b/Documentation/filesystems/f2fs.rst
index 535021c46260..916956d433b2 100644
--- a/Documentation/filesystems/f2fs.rst
+++ b/Documentation/filesystems/f2fs.rst
@@ -279,7 +279,18 @@ Sysfs Entries
 
 Information about mounted f2fs file systems can be found in
 /sys/fs/f2fs.  Each mounted filesystem will have a directory in
-/sys/fs/f2fs based on its device name (i.e., /sys/fs/f2fs/sda).
+/sys/fs/f2fs based on its device name (i.e., /sys/fs/f2fs/sda),
+or mount_#x (#x is the number representing the order of mounting).
+But once one mount point was umounted, that sequential number @x
+in "mount_@x" could be reused by later newly mounted point.
+
+Here is an example of this.
+
+mount dev0 mount0 (mount_0 -> dev0)
+mount dev1 mount1 (mount_1 -> dev1)
+umount mount0
+mount dev2 (mount_0 -> dev2)
+
 The files in each per-device directory are shown in table below.
 
 Files in /sys/fs/f2fs/<devname>
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 4b28fd42fdbc..7d6c5f8ce16b 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1419,6 +1419,8 @@ struct decompress_io_ctx {
 #define MAX_COMPRESS_LOG_SIZE		8
 #define MAX_COMPRESS_WINDOW_SIZE	((PAGE_SIZE) << MAX_COMPRESS_LOG_SIZE)
 
+#define MOUNT_NAME_SIZE			20
+
 struct f2fs_sb_info {
 	struct super_block *sb;			/* pointer to VFS super block */
 	struct proc_dir_entry *s_proc;		/* proc entry */
@@ -1599,6 +1601,8 @@ struct f2fs_sb_info {
 	/* For sysfs suppport */
 	struct kobject s_kobj;
 	struct completion s_kobj_unregister;
+	int s_mount_id;
+	char s_mount_name[MOUNT_NAME_SIZE];
 
 	/* For shrinker support */
 	struct list_head s_list;
diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
index 2a140657fc4d..703d9f460d03 100644
--- a/fs/f2fs/sysfs.c
+++ b/fs/f2fs/sysfs.c
@@ -18,6 +18,7 @@
 #include <trace/events/f2fs.h>
 
 static struct proc_dir_entry *f2fs_proc_root;
+static struct ida f2fs_mount_ida;
 
 /* Sysfs support for f2fs */
 enum {
@@ -908,6 +909,9 @@ int __init f2fs_init_sysfs(void)
 	} else {
 		f2fs_proc_root = proc_mkdir("fs/f2fs", NULL);
 	}
+
+	ida_init(&f2fs_mount_ida);
+
 	return ret;
 }
 
@@ -917,6 +921,7 @@ void f2fs_exit_sysfs(void)
 	kset_unregister(&f2fs_kset);
 	remove_proc_entry("fs/f2fs", NULL);
 	f2fs_proc_root = NULL;
+	ida_destroy(&f2fs_mount_ida);
 }
 
 int f2fs_register_sysfs(struct f2fs_sb_info *sbi)
@@ -928,12 +933,22 @@ int f2fs_register_sysfs(struct f2fs_sb_info *sbi)
 	init_completion(&sbi->s_kobj_unregister);
 	err = kobject_init_and_add(&sbi->s_kobj, &f2fs_sb_ktype, NULL,
 				"%s", sb->s_id);
-	if (err) {
-		kobject_put(&sbi->s_kobj);
-		wait_for_completion(&sbi->s_kobj_unregister);
-		return err;
+	if (err)
+		goto err1;
+
+	sbi->s_mount_id = ida_simple_get(&f2fs_mount_ida, 0, 0, GFP_KERNEL);
+	if (sbi->s_mount_id < 0) {
+		err = sbi->s_mount_id;
+		goto err1;
 	}
 
+	snprintf(sbi->s_mount_name, MOUNT_NAME_SIZE, "mount_%d",
+			sbi->s_mount_id);
+	err = sysfs_create_link(&f2fs_kset.kobj, &sbi->s_kobj,
+			sbi->s_mount_name);
+	if (err)
+		goto err2;
+
 	if (f2fs_proc_root)
 		sbi->s_proc = proc_mkdir(sb->s_id, f2fs_proc_root);
 
@@ -948,6 +963,12 @@ int f2fs_register_sysfs(struct f2fs_sb_info *sbi)
 				victim_bits_seq_show, sb);
 	}
 	return 0;
+err2:
+	ida_simple_remove(&f2fs_mount_ida, sbi->s_mount_id);
+err1:
+	kobject_put(&sbi->s_kobj);
+	wait_for_completion(&sbi->s_kobj_unregister);
+	return err;
 }
 
 void f2fs_unregister_sysfs(struct f2fs_sb_info *sbi)
@@ -959,6 +980,8 @@ void f2fs_unregister_sysfs(struct f2fs_sb_info *sbi)
 		remove_proc_entry("victim_bits", sbi->s_proc);
 		remove_proc_entry(sbi->sb->s_id, f2fs_proc_root);
 	}
+	sysfs_remove_link(&f2fs_kset.kobj, sbi->s_mount_name);
+	ida_simple_remove(&f2fs_mount_ida, sbi->s_mount_id);
 	kobject_del(&sbi->s_kobj);
 	kobject_put(&sbi->s_kobj);
 }
-- 
2.27.0.383.g050319c2ae-goog

Re: [f2fs-dev] [PATCH v3] f2fs: add symbolic link to kobject in sysfs

[Chao Yu]

On 2020/7/3 14:54, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
> 
> Added a symbolic link to directory of sysfs. It will
> create a symbolic link such as "mount_0" and "mount_1" to
> each f2fs mount in the order of mounting filesystem. But
> once one mount point was umounted, that sequential number
> @x in "mount_@x" could be reused by later newly mounted
> point. It will provide easy access to sysfs node even if
> not knowing the specific device node name like sda19 and
> dm-3.
> 
> Signed-off-by: Daeho Jeong <daehojeong@google.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

Re: [PATCH v3] f2fs: add symbolic link to kobject in sysfs

[Greg KH]

On Fri, Jul 03, 2020 at 03:54:20PM +0900, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
> 
> Added a symbolic link to directory of sysfs. It will
> create a symbolic link such as "mount_0" and "mount_1" to
> each f2fs mount in the order of mounting filesystem. But
> once one mount point was umounted, that sequential number
> @x in "mount_@x" could be reused by later newly mounted
> point. It will provide easy access to sysfs node even if
> not knowing the specific device node name like sda19 and
> dm-3.
> 
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> ---
>  Documentation/filesystems/f2fs.rst | 13 ++++++++++++-

No Documentation/ABI/ entry for your new sysfs file/link?

And what does this help with?

If it's really needed, why don't we do this for all filesystem types?

thanks,

greg k-h

Re: [PATCH v3] f2fs: add symbolic link to kobject in sysfs

[Daeho Jeong]

> No Documentation/ABI/ entry for your new sysfs file/link?

This is for adding a symbolic link to a pre-existed
/sys/fs/f2fs/<disk> directory and it means /sys/fs/f2fs/<mount> points
to /sys/fs/f2fs/<disk>. I already added the description of this in
Documentation/filesystems/f2fs.rst.

>
> And what does this help with?

Some system daemons in Android access with the pre-defined sysfs entry
name in the json file. So whenever the project changes and the
partition layout is changed, we have to follow up the changes and
modify /sys/fs/f2fs/<disk> name in the json file accordingly.

This will help them access all the f2fs sysfs entries consistently
without requiring to know those changes.

>
> If it's really needed, why don't we do this for all filesystem types?

This is for the daemon to change the mode of only F2FS with the power
hint of Android.

>
> thanks,
>
> greg k-h

Re: [PATCH v3] f2fs: add symbolic link to kobject in sysfs

[Greg KH]

On Mon, Jul 06, 2020 at 08:47:07AM +0900, Daeho Jeong wrote:
> > No Documentation/ABI/ entry for your new sysfs file/link?
> 
> This is for adding a symbolic link to a pre-existed
> /sys/fs/f2fs/<disk> directory and it means /sys/fs/f2fs/<mount> points
> to /sys/fs/f2fs/<disk>. I already added the description of this in
> Documentation/filesystems/f2fs.rst.

Ok, but that's not the standard location for sysfs file documentation.

> > And what does this help with?
> 
> Some system daemons in Android access with the pre-defined sysfs entry
> name in the json file. So whenever the project changes and the
> partition layout is changed, we have to follow up the changes and
> modify /sys/fs/f2fs/<disk> name in the json file accordingly.

That's what partition names are for, you should never depend on a
"random number".

> This will help them access all the f2fs sysfs entries consistently
> without requiring to know those changes.

No, please use a partition name, that is the only way to always know you
are mounting the correct partition.  You have created a random number
here that might, or might not, change between boots depending on the
order of the filesystem being mounted.  It is not persistant or
deterministic at all, please do not treat it as such.

> > If it's really needed, why don't we do this for all filesystem types?
> 
> This is for the daemon to change the mode of only F2FS with the power
> hint of Android.

Again, the point is that a filesystem type is not unique, this, if
really really needed, should be an attribute for ALL filesystem types,
f2fs is not special here at all.

Please do not rely on this number ever being the same across boots,
because your code is such that you can not guarantee that.

And again, if you really want to know the partition you are mounting
really is the partition you think you are mounting, use the partition
label name, that is what it is there for, and is why we have been
relying on that for decades.  A new special per-filesystem-attribute
that is semi-random is not the correct solution for the problem you are
describing as far as I can determine.

thanks,

greg k-h

Re: [PATCH v3] f2fs: add symbolic link to kobject in sysfs

[Daeho Jeong]

I thought it's working for our system. But as you said, it might be
not true for others. I got your point. Partition label would be a
great idea for us.

Thank you for your comment!

2020년 7월 8일 (수) 오후 7:05, Greg KH <gregkh@linuxfoundation.org>님이 작성:
>
> On Mon, Jul 06, 2020 at 08:47:07AM +0900, Daeho Jeong wrote:
> > > No Documentation/ABI/ entry for your new sysfs file/link?
> >
> > This is for adding a symbolic link to a pre-existed
> > /sys/fs/f2fs/<disk> directory and it means /sys/fs/f2fs/<mount> points
> > to /sys/fs/f2fs/<disk>. I already added the description of this in
> > Documentation/filesystems/f2fs.rst.
>
> Ok, but that's not the standard location for sysfs file documentation.
>
> > > And what does this help with?
> >
> > Some system daemons in Android access with the pre-defined sysfs entry
> > name in the json file. So whenever the project changes and the
> > partition layout is changed, we have to follow up the changes and
> > modify /sys/fs/f2fs/<disk> name in the json file accordingly.
>
> That's what partition names are for, you should never depend on a
> "random number".
>
> > This will help them access all the f2fs sysfs entries consistently
> > without requiring to know those changes.
>
> No, please use a partition name, that is the only way to always know you
> are mounting the correct partition.  You have created a random number
> here that might, or might not, change between boots depending on the
> order of the filesystem being mounted.  It is not persistant or
> deterministic at all, please do not treat it as such.
>
> > > If it's really needed, why don't we do this for all filesystem types?
> >
> > This is for the daemon to change the mode of only F2FS with the power
> > hint of Android.
>
> Again, the point is that a filesystem type is not unique, this, if
> really really needed, should be an attribute for ALL filesystem types,
> f2fs is not special here at all.
>
> Please do not rely on this number ever being the same across boots,
> because your code is such that you can not guarantee that.
>
> And again, if you really want to know the partition you are mounting
> really is the partition you think you are mounting, use the partition
> label name, that is what it is there for, and is why we have been
> relying on that for decades.  A new special per-filesystem-attribute
> that is semi-random is not the correct solution for the problem you are
> describing as far as I can determine.
>
> thanks,
>
> greg k-h