[PATCH v4 0/2] ceph: add debugfs entries signifying new mount syntax support

[PATCH v4 0/2] ceph: add debugfs entries signifying new mount syntax support

[Venky Shankar]

v4:
  - use mount_syntax_v1,.. as file names

[This is based on top of new mount syntax series]

Patrick proposed the idea of having debugfs entries to signify if
kernel supports the new (v2) mount syntax. The primary use of this
information is to catch any bugs in the new syntax implementation.

This would be done as follows::

The userspace mount helper tries to mount using the new mount syntax
and fallsback to using old syntax if the mount using new syntax fails.
However, a bug in the new mount syntax implementation can silently
result in the mount helper switching to old syntax.

So, the debugfs entries can be relied upon by the mount helper to
check if the kernel supports the new mount syntax. Cases when the
mount using the new syntax fails, but the kernel does support the
new mount syntax, the mount helper could probably log before switching
to the old syntax (or fail the mount altogether when run in test mode).

Debugfs entries are as follows::

    /sys/kernel/debug/ceph/
    ....
    ....
    /sys/kernel/debug/ceph/meta
    /sys/kernel/debug/ceph/meta/client_features
    /sys/kernel/debug/ceph/meta/client_features/mount_syntax_v2
    /sys/kernel/debug/ceph/meta/client_features/mount_syntax_v1
    ....
    ....

Venky Shankar (2):
  libceph: export ceph_debugfs_dir for use in ceph.ko
  ceph: add debugfs entries for mount syntax support

 fs/ceph/debugfs.c            | 41 ++++++++++++++++++++++++++++++++++++
 fs/ceph/super.c              |  3 +++
 fs/ceph/super.h              |  2 ++
 include/linux/ceph/debugfs.h |  2 ++
 net/ceph/debugfs.c           |  3 ++-
 5 files changed, 50 insertions(+), 1 deletion(-)

-- 
2.27.0

[PATCH v4 1/2] libceph: export ceph_debugfs_dir for use in ceph.ko

[Venky Shankar]

Signed-off-by: Venky Shankar <vshankar@redhat.com>
---
 include/linux/ceph/debugfs.h | 2 ++
 net/ceph/debugfs.c           | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/ceph/debugfs.h b/include/linux/ceph/debugfs.h
index 8b3a1a7a953a..464c7dfced87 100644
--- a/include/linux/ceph/debugfs.h
+++ b/include/linux/ceph/debugfs.h
@@ -4,6 +4,8 @@
 
 #include <linux/ceph/types.h>
 
+extern struct dentry *ceph_debugfs_dir;
+
 /* debugfs.c */
 extern void ceph_debugfs_init(void);
 extern void ceph_debugfs_cleanup(void);
diff --git a/net/ceph/debugfs.c b/net/ceph/debugfs.c
index 2110439f8a24..774e0c0fd18a 100644
--- a/net/ceph/debugfs.c
+++ b/net/ceph/debugfs.c
@@ -29,7 +29,8 @@
  *      .../bdi         - symlink to ../../bdi/something
  */
 
-static struct dentry *ceph_debugfs_dir;
+struct dentry *ceph_debugfs_dir;
+EXPORT_SYMBOL(ceph_debugfs_dir);
 
 static int monmap_show(struct seq_file *s, void *p)
 {
-- 
2.27.0

[PATCH v4 2/2] ceph: add debugfs entries for mount syntax support

[Venky Shankar]

Signed-off-by: Venky Shankar <vshankar@redhat.com>
---
 fs/ceph/debugfs.c | 41 +++++++++++++++++++++++++++++++++++++++++
 fs/ceph/super.c   |  3 +++
 fs/ceph/super.h   |  2 ++
 3 files changed, 46 insertions(+)

diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
index 66989c880adb..d1610393c213 100644
--- a/fs/ceph/debugfs.c
+++ b/fs/ceph/debugfs.c
@@ -22,6 +22,16 @@
 #include "mds_client.h"
 #include "metric.h"
 
+#define META_INFO_DIR_NAME         "meta"
+#define CLIENT_FEATURES_DIR_NAME   "client_features"
+#define MOUNT_DEVICE_V1_SUPPORT_FILE_NAME "mount_syntax_v1"
+#define MOUNT_DEVICE_V2_SUPPORT_FILE_NAME "mount_syntax_v2"
+
+static struct dentry *ceph_client_meta_dir;
+static struct dentry *ceph_client_features_dir;
+static struct dentry *ceph_mount_device_v1_support;
+static struct dentry *ceph_mount_device_v2_support;
+
 static int mdsmap_show(struct seq_file *s, void *p)
 {
 	int i;
@@ -416,6 +426,29 @@ void ceph_fs_debugfs_init(struct ceph_fs_client *fsc)
 						  &status_fops);
 }
 
+void ceph_fs_debugfs_client_features_init(void)
+{
+	ceph_client_meta_dir = debugfs_create_dir(META_INFO_DIR_NAME,
+						  ceph_debugfs_dir);
+	ceph_client_features_dir = debugfs_create_dir(CLIENT_FEATURES_DIR_NAME,
+						      ceph_client_meta_dir);
+	ceph_mount_device_v1_support = debugfs_create_file(MOUNT_DEVICE_V1_SUPPORT_FILE_NAME,
+							   0400,
+							   ceph_client_features_dir,
+							   NULL, NULL);
+	ceph_mount_device_v2_support = debugfs_create_file(MOUNT_DEVICE_V2_SUPPORT_FILE_NAME,
+							   0400,
+							   ceph_client_features_dir,
+							   NULL, NULL);
+}
+
+void ceph_fs_debugfs_client_features_cleanup(void)
+{
+	debugfs_remove(ceph_mount_device_v1_support);
+	debugfs_remove(ceph_mount_device_v2_support);
+	debugfs_remove(ceph_client_features_dir);
+	debugfs_remove(ceph_client_meta_dir);
+}
 
 #else  /* CONFIG_DEBUG_FS */
 
@@ -427,4 +460,12 @@ void ceph_fs_debugfs_cleanup(struct ceph_fs_client *fsc)
 {
 }
 
+void ceph_fs_debugfs_client_features_init(void)
+{
+}
+
+void ceph_fs_debugfs_client_features_cleanup(void)
+{
+}
+
 #endif  /* CONFIG_DEBUG_FS */
diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 609ffc8c2d78..21d59deb042d 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -1404,6 +1404,8 @@ static int __init init_ceph(void)
 	if (ret)
 		goto out_caches;
 
+	ceph_fs_debugfs_client_features_init();
+
 	pr_info("loaded (mds proto %d)\n", CEPH_MDSC_PROTOCOL);
 
 	return 0;
@@ -1417,6 +1419,7 @@ static int __init init_ceph(void)
 static void __exit exit_ceph(void)
 {
 	dout("exit_ceph\n");
+	ceph_fs_debugfs_client_features_cleanup();
 	unregister_filesystem(&ceph_fs_type);
 	destroy_caches();
 }
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index 8f71184b7c85..7e7b140cab5d 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -1231,6 +1231,8 @@ extern int ceph_locks_to_pagelist(struct ceph_filelock *flocks,
 /* debugfs.c */
 extern void ceph_fs_debugfs_init(struct ceph_fs_client *client);
 extern void ceph_fs_debugfs_cleanup(struct ceph_fs_client *client);
+extern void ceph_fs_debugfs_client_features_init(void);
+extern void ceph_fs_debugfs_client_features_cleanup(void);
 
 /* quota.c */
 static inline bool __ceph_has_any_quota(struct ceph_inode_info *ci)
-- 
2.27.0

Re: [PATCH v4 0/2] ceph: add debugfs entries signifying new mount syntax support

[Jeff Layton]

On Fri, 2021-10-01 at 10:30 +0530, Venky Shankar wrote:
> v4:
>   - use mount_syntax_v1,.. as file names
> 
> [This is based on top of new mount syntax series]
> 
> Patrick proposed the idea of having debugfs entries to signify if
> kernel supports the new (v2) mount syntax. The primary use of this
> information is to catch any bugs in the new syntax implementation.
> 
> This would be done as follows::
> 
> The userspace mount helper tries to mount using the new mount syntax
> and fallsback to using old syntax if the mount using new syntax fails.
> However, a bug in the new mount syntax implementation can silently
> result in the mount helper switching to old syntax.
> 
> So, the debugfs entries can be relied upon by the mount helper to
> check if the kernel supports the new mount syntax. Cases when the
> mount using the new syntax fails, but the kernel does support the
> new mount syntax, the mount helper could probably log before switching
> to the old syntax (or fail the mount altogether when run in test mode).
> 
> Debugfs entries are as follows::
> 
>     /sys/kernel/debug/ceph/
>     ....
>     ....
>     /sys/kernel/debug/ceph/meta
>     /sys/kernel/debug/ceph/meta/client_features
>     /sys/kernel/debug/ceph/meta/client_features/mount_syntax_v2
>     /sys/kernel/debug/ceph/meta/client_features/mount_syntax_v1
>     ....
>     ....
> 
> Venky Shankar (2):
>   libceph: export ceph_debugfs_dir for use in ceph.ko
>   ceph: add debugfs entries for mount syntax support
> 
>  fs/ceph/debugfs.c            | 41 ++++++++++++++++++++++++++++++++++++
>  fs/ceph/super.c              |  3 +++
>  fs/ceph/super.h              |  2 ++
>  include/linux/ceph/debugfs.h |  2 ++
>  net/ceph/debugfs.c           |  3 ++-
>  5 files changed, 50 insertions(+), 1 deletion(-)
> 

This looks good to me. Merged into testing branch.

Note that there is a non-zero chance that this will break teuthology in
some wa. In particular, looking at qa/tasks/cephfs/kernel_mount.py, it
does this in _get_global_id:

            pyscript = dedent("""
                import glob
                import os
                import json

                def get_id_to_dir():
                    result = {}
                    for dir in glob.glob("/sys/kernel/debug/ceph/*"):
                        mds_sessions_lines = open(os.path.join(dir, "mds_sessions")).readlines()
                        global_id = mds_sessions_lines[0].split()[1].strip('"')
                        client_id = mds_sessions_lines[1].split()[1].strip('"')
                        result[client_id] = global_id
                    return result
                print(json.dumps(get_id_to_dir()))
            """)


What happens when this hits the "meta" directory? Is that a problem?

We may need to fix up some places like this. Maybe the open there needs
some error handling? Or we could just skip directories called "meta".
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v4 0/2] ceph: add debugfs entries signifying new mount syntax support

[Patrick Donnelly]

On Fri, Oct 1, 2021 at 12:24 PM Jeff Layton <jlayton@redhat.com> wrote:
> Note that there is a non-zero chance that this will break teuthology in
> some wa. In particular, looking at qa/tasks/cephfs/kernel_mount.py, it
> does this in _get_global_id:
>
>             pyscript = dedent("""
>                 import glob
>                 import os
>                 import json
>
>                 def get_id_to_dir():
>                     result = {}
>                     for dir in glob.glob("/sys/kernel/debug/ceph/*"):
>                         mds_sessions_lines = open(os.path.join(dir, "mds_sessions")).readlines()
>                         global_id = mds_sessions_lines[0].split()[1].strip('"')
>                         client_id = mds_sessions_lines[1].split()[1].strip('"')
>                         result[client_id] = global_id
>                     return result
>                 print(json.dumps(get_id_to_dir()))
>             """)
>
>
> What happens when this hits the "meta" directory? Is that a problem?
>
> We may need to fix up some places like this. Maybe the open there needs
> some error handling? Or we could just skip directories called "meta".

Yes, this will likely break all the kernel tests. It must be fixed
before this can be merged into testing.

-- 
Patrick Donnelly, Ph.D.
He / Him / His
Principal Software Engineer
Red Hat Sunnyvale, CA
GPG: 19F28A586F808C2402351B93C3301A3E258DD79D

Re: [PATCH v4 0/2] ceph: add debugfs entries signifying new mount syntax support

[Jeff Layton]

On Fri, 2021-10-01 at 16:18 -0400, Patrick Donnelly wrote:
> On Fri, Oct 1, 2021 at 12:24 PM Jeff Layton <jlayton@redhat.com> wrote:
> > Note that there is a non-zero chance that this will break teuthology in
> > some wa. In particular, looking at qa/tasks/cephfs/kernel_mount.py, it
> > does this in _get_global_id:
> > 
> >             pyscript = dedent("""
> >                 import glob
> >                 import os
> >                 import json
> > 
> >                 def get_id_to_dir():
> >                     result = {}
> >                     for dir in glob.glob("/sys/kernel/debug/ceph/*"):
> >                         mds_sessions_lines = open(os.path.join(dir, "mds_sessions")).readlines()
> >                         global_id = mds_sessions_lines[0].split()[1].strip('"')
> >                         client_id = mds_sessions_lines[1].split()[1].strip('"')
> >                         result[client_id] = global_id
> >                     return result
> >                 print(json.dumps(get_id_to_dir()))
> >             """)
> > 
> > 
> > What happens when this hits the "meta" directory? Is that a problem?
> > 
> > We may need to fix up some places like this. Maybe the open there needs
> > some error handling? Or we could just skip directories called "meta".
> 
> Yes, this will likely break all the kernel tests. It must be fixed
> before this can be merged into testing.
> 

Ok, I'll drop these patches for now. Let me know when it's clear to
merge them again, and I'll do so.

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v4 0/2] ceph: add debugfs entries signifying new mount syntax support

[Venky Shankar]

On Sat, Oct 2, 2021 at 1:54 AM Jeff Layton <jlayton@redhat.com> wrote:
>
> On Fri, 2021-10-01 at 16:18 -0400, Patrick Donnelly wrote:
> > On Fri, Oct 1, 2021 at 12:24 PM Jeff Layton <jlayton@redhat.com> wrote:
> > > Note that there is a non-zero chance that this will break teuthology in
> > > some wa. In particular, looking at qa/tasks/cephfs/kernel_mount.py, it
> > > does this in _get_global_id:
> > >
> > >             pyscript = dedent("""
> > >                 import glob
> > >                 import os
> > >                 import json
> > >
> > >                 def get_id_to_dir():
> > >                     result = {}
> > >                     for dir in glob.glob("/sys/kernel/debug/ceph/*"):
> > >                         mds_sessions_lines = open(os.path.join(dir, "mds_sessions")).readlines()
> > >                         global_id = mds_sessions_lines[0].split()[1].strip('"')
> > >                         client_id = mds_sessions_lines[1].split()[1].strip('"')
> > >                         result[client_id] = global_id
> > >                     return result
> > >                 print(json.dumps(get_id_to_dir()))
> > >             """)
> > >
> > >
> > > What happens when this hits the "meta" directory? Is that a problem?
> > >
> > > We may need to fix up some places like this. Maybe the open there needs
> > > some error handling? Or we could just skip directories called "meta".

That seems to be the only place where dir entries are fetched.
Skipping the meta dir should suffice.

I'll push a change for this fix.

> >
> > Yes, this will likely break all the kernel tests. It must be fixed
> > before this can be merged into testing.
> >
>
> Ok, I'll drop these patches for now. Let me know when it's clear to
> merge them again, and I'll do so.
>
> Thanks,
> --
> Jeff Layton <jlayton@redhat.com>
>


-- 
Cheers,
Venky

Re: [PATCH v4 0/2] ceph: add debugfs entries signifying new mount syntax support

[Ilya Dryomov]

On Fri, Oct 1, 2021 at 7:05 AM Venky Shankar <vshankar@redhat.com> wrote:
>
> v4:
>   - use mount_syntax_v1,.. as file names
>
> [This is based on top of new mount syntax series]
>
> Patrick proposed the idea of having debugfs entries to signify if
> kernel supports the new (v2) mount syntax. The primary use of this
> information is to catch any bugs in the new syntax implementation.
>
> This would be done as follows::
>
> The userspace mount helper tries to mount using the new mount syntax
> and fallsback to using old syntax if the mount using new syntax fails.
> However, a bug in the new mount syntax implementation can silently
> result in the mount helper switching to old syntax.
>
> So, the debugfs entries can be relied upon by the mount helper to
> check if the kernel supports the new mount syntax. Cases when the
> mount using the new syntax fails, but the kernel does support the
> new mount syntax, the mount helper could probably log before switching
> to the old syntax (or fail the mount altogether when run in test mode).
>
> Debugfs entries are as follows::
>
>     /sys/kernel/debug/ceph/
>     ....
>     ....
>     /sys/kernel/debug/ceph/meta
>     /sys/kernel/debug/ceph/meta/client_features
>     /sys/kernel/debug/ceph/meta/client_features/mount_syntax_v2
>     /sys/kernel/debug/ceph/meta/client_features/mount_syntax_v1
>     ....
>     ....

Hi Venky, Jeff,

If this is supposed to be used in the wild and not just in teuthology,
I would be wary of going with debugfs.  debugfs isn't always available
(it is actually compiled out in some configurations, it may or may not
be mounted, etc).  With the new mount syntax feature it is not a big
deal because the mount helper should do just fine without it but with
other features we may find ourselves in a situation where the mount
helper (or something else) just *has* to know whether the feature is
supported or not and falling back to "no" if debugfs is not available
is undesirable or too much work.

I don't have a great suggestion though.  When I needed to do this in
the past for RADOS feature bits, I went with a read-only kernel module
parameter [1].  They are exported via sysfs which is guaranteed to be
available.  Perhaps we should do the same for mount_syntax -- have it
be either 1 or 2, allowing it to be revved in the future?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d6a3408a77807037872892c2a2034180fcc08d12

Thanks,

                Ilya

Re: [PATCH v4 0/2] ceph: add debugfs entries signifying new mount syntax support

[Jeff Layton]

On Tue, 2021-10-19 at 10:31 +0200, Ilya Dryomov wrote:
> On Fri, Oct 1, 2021 at 7:05 AM Venky Shankar <vshankar@redhat.com> wrote:
> > 
> > v4:
> >   - use mount_syntax_v1,.. as file names
> > 
> > [This is based on top of new mount syntax series]
> > 
> > Patrick proposed the idea of having debugfs entries to signify if
> > kernel supports the new (v2) mount syntax. The primary use of this
> > information is to catch any bugs in the new syntax implementation.
> > 
> > This would be done as follows::
> > 
> > The userspace mount helper tries to mount using the new mount syntax
> > and fallsback to using old syntax if the mount using new syntax fails.
> > However, a bug in the new mount syntax implementation can silently
> > result in the mount helper switching to old syntax.
> > 
> > So, the debugfs entries can be relied upon by the mount helper to
> > check if the kernel supports the new mount syntax. Cases when the
> > mount using the new syntax fails, but the kernel does support the
> > new mount syntax, the mount helper could probably log before switching
> > to the old syntax (or fail the mount altogether when run in test mode).
> > 
> > Debugfs entries are as follows::
> > 
> >     /sys/kernel/debug/ceph/
> >     ....
> >     ....
> >     /sys/kernel/debug/ceph/meta
> >     /sys/kernel/debug/ceph/meta/client_features
> >     /sys/kernel/debug/ceph/meta/client_features/mount_syntax_v2
> >     /sys/kernel/debug/ceph/meta/client_features/mount_syntax_v1
> >     ....
> >     ....
> 
> Hi Venky, Jeff,
> 
> If this is supposed to be used in the wild and not just in teuthology,
> I would be wary of going with debugfs.  debugfs isn't always available
> (it is actually compiled out in some configurations, it may or may not
> be mounted, etc).  With the new mount syntax feature it is not a big
> deal because the mount helper should do just fine without it but with
> other features we may find ourselves in a situation where the mount
> helper (or something else) just *has* to know whether the feature is
> supported or not and falling back to "no" if debugfs is not available
> is undesirable or too much work.
> 

I made this same point earlier, and the response was that this was
basically specifically for certain teuthology tests (mostly for testing
different mount syntax handling), and so debugfs should be available for
those.

> I don't have a great suggestion though.  When I needed to do this in
> the past for RADOS feature bits, I went with a read-only kernel module
> parameter [1].  They are exported via sysfs which is guaranteed to be
> available.  Perhaps we should do the same for mount_syntax -- have it
> be either 1 or 2, allowing it to be revved in the future?
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d6a3408a77807037872892c2a2034180fcc08d12
> 


That's not a bad idea either, and this info _is_ read-only. I don't have
a particular preference, but this approach would be fine with me as
well, and there is already some precedent for it.
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v4 0/2] ceph: add debugfs entries signifying new mount syntax support

[Venky Shankar]

On Tue, Oct 19, 2021 at 2:02 PM Ilya Dryomov <idryomov@gmail.com> wrote:
>
> On Fri, Oct 1, 2021 at 7:05 AM Venky Shankar <vshankar@redhat.com> wrote:
> >
> > v4:
> >   - use mount_syntax_v1,.. as file names
> >
> > [This is based on top of new mount syntax series]
> >
> > Patrick proposed the idea of having debugfs entries to signify if
> > kernel supports the new (v2) mount syntax. The primary use of this
> > information is to catch any bugs in the new syntax implementation.
> >
> > This would be done as follows::
> >
> > The userspace mount helper tries to mount using the new mount syntax
> > and fallsback to using old syntax if the mount using new syntax fails.
> > However, a bug in the new mount syntax implementation can silently
> > result in the mount helper switching to old syntax.
> >
> > So, the debugfs entries can be relied upon by the mount helper to
> > check if the kernel supports the new mount syntax. Cases when the
> > mount using the new syntax fails, but the kernel does support the
> > new mount syntax, the mount helper could probably log before switching
> > to the old syntax (or fail the mount altogether when run in test mode).
> >
> > Debugfs entries are as follows::
> >
> >     /sys/kernel/debug/ceph/
> >     ....
> >     ....
> >     /sys/kernel/debug/ceph/meta
> >     /sys/kernel/debug/ceph/meta/client_features
> >     /sys/kernel/debug/ceph/meta/client_features/mount_syntax_v2
> >     /sys/kernel/debug/ceph/meta/client_features/mount_syntax_v1
> >     ....
> >     ....
>
> Hi Venky, Jeff,
>
> If this is supposed to be used in the wild and not just in teuthology,
> I would be wary of going with debugfs.  debugfs isn't always available
> (it is actually compiled out in some configurations, it may or may not
> be mounted, etc).  With the new mount syntax feature it is not a big
> deal because the mount helper should do just fine without it but with
> other features we may find ourselves in a situation where the mount
> helper (or something else) just *has* to know whether the feature is
> supported or not and falling back to "no" if debugfs is not available
> is undesirable or too much work.
>
> I don't have a great suggestion though.  When I needed to do this in
> the past for RADOS feature bits, I went with a read-only kernel module
> parameter [1].  They are exported via sysfs which is guaranteed to be
> available.  Perhaps we should do the same for mount_syntax -- have it
> be either 1 or 2, allowing it to be revved in the future?

I'm ok with exporting via sysfs (since it's guaranteed). My only ask
here would be to have the mount support information present itself as
files rather than file contents to avoid writing parsing stuff in
userspace, which is ok, however, relying on stat() is nicer.

>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d6a3408a77807037872892c2a2034180fcc08d12
>
> Thanks,
>
>                 Ilya
>


-- 
Cheers,
Venky

Re: [PATCH v4 0/2] ceph: add debugfs entries signifying new mount syntax support

[Jeff Layton]

On Wed, 2021-10-20 at 18:04 +0530, Venky Shankar wrote:
> On Tue, Oct 19, 2021 at 2:02 PM Ilya Dryomov <idryomov@gmail.com> wrote:
> > 
> > On Fri, Oct 1, 2021 at 7:05 AM Venky Shankar <vshankar@redhat.com> wrote:
> > > 
> > > v4:
> > >   - use mount_syntax_v1,.. as file names
> > > 
> > > [This is based on top of new mount syntax series]
> > > 
> > > Patrick proposed the idea of having debugfs entries to signify if
> > > kernel supports the new (v2) mount syntax. The primary use of this
> > > information is to catch any bugs in the new syntax implementation.
> > > 
> > > This would be done as follows::
> > > 
> > > The userspace mount helper tries to mount using the new mount syntax
> > > and fallsback to using old syntax if the mount using new syntax fails.
> > > However, a bug in the new mount syntax implementation can silently
> > > result in the mount helper switching to old syntax.
> > > 
> > > So, the debugfs entries can be relied upon by the mount helper to
> > > check if the kernel supports the new mount syntax. Cases when the
> > > mount using the new syntax fails, but the kernel does support the
> > > new mount syntax, the mount helper could probably log before switching
> > > to the old syntax (or fail the mount altogether when run in test mode).
> > > 
> > > Debugfs entries are as follows::
> > > 
> > >     /sys/kernel/debug/ceph/
> > >     ....
> > >     ....
> > >     /sys/kernel/debug/ceph/meta
> > >     /sys/kernel/debug/ceph/meta/client_features
> > >     /sys/kernel/debug/ceph/meta/client_features/mount_syntax_v2
> > >     /sys/kernel/debug/ceph/meta/client_features/mount_syntax_v1
> > >     ....
> > >     ....
> > 
> > Hi Venky, Jeff,
> > 
> > If this is supposed to be used in the wild and not just in teuthology,
> > I would be wary of going with debugfs.  debugfs isn't always available
> > (it is actually compiled out in some configurations, it may or may not
> > be mounted, etc).  With the new mount syntax feature it is not a big
> > deal because the mount helper should do just fine without it but with
> > other features we may find ourselves in a situation where the mount
> > helper (or something else) just *has* to know whether the feature is
> > supported or not and falling back to "no" if debugfs is not available
> > is undesirable or too much work.
> > 
> > I don't have a great suggestion though.  When I needed to do this in
> > the past for RADOS feature bits, I went with a read-only kernel module
> > parameter [1].  They are exported via sysfs which is guaranteed to be
> > available.  Perhaps we should do the same for mount_syntax -- have it
> > be either 1 or 2, allowing it to be revved in the future?
> 
> I'm ok with exporting via sysfs (since it's guaranteed). My only ask
> here would be to have the mount support information present itself as
> files rather than file contents to avoid writing parsing stuff in
> userspace, which is ok, however, relying on stat() is nicer.
> 

You should be able to do that by just making a read-only parameter
called "mount_syntax_v2", and then you can test for it by doing
something like:

    # stat /sys/module/ceph/parameters/mount_syntax_v2

The contents of the file can be blank (or just return Y or something).

> > 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d6a3408a77807037872892c2a2034180fcc08d12
> > 
> > Thanks,
> > 
> >                 Ilya
> > 
> 
> 

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v4 0/2] ceph: add debugfs entries signifying new mount syntax support

[Venky Shankar]

On Wed, Oct 20, 2021 at 6:13 PM Jeff Layton <jlayton@redhat.com> wrote:
>
> On Wed, 2021-10-20 at 18:04 +0530, Venky Shankar wrote:
> > On Tue, Oct 19, 2021 at 2:02 PM Ilya Dryomov <idryomov@gmail.com> wrote:
> > >
> > > On Fri, Oct 1, 2021 at 7:05 AM Venky Shankar <vshankar@redhat.com> wrote:
> > > >
> > > > v4:
> > > >   - use mount_syntax_v1,.. as file names
> > > >
> > > > [This is based on top of new mount syntax series]
> > > >
> > > > Patrick proposed the idea of having debugfs entries to signify if
> > > > kernel supports the new (v2) mount syntax. The primary use of this
> > > > information is to catch any bugs in the new syntax implementation.
> > > >
> > > > This would be done as follows::
> > > >
> > > > The userspace mount helper tries to mount using the new mount syntax
> > > > and fallsback to using old syntax if the mount using new syntax fails.
> > > > However, a bug in the new mount syntax implementation can silently
> > > > result in the mount helper switching to old syntax.
> > > >
> > > > So, the debugfs entries can be relied upon by the mount helper to
> > > > check if the kernel supports the new mount syntax. Cases when the
> > > > mount using the new syntax fails, but the kernel does support the
> > > > new mount syntax, the mount helper could probably log before switching
> > > > to the old syntax (or fail the mount altogether when run in test mode).
> > > >
> > > > Debugfs entries are as follows::
> > > >
> > > >     /sys/kernel/debug/ceph/
> > > >     ....
> > > >     ....
> > > >     /sys/kernel/debug/ceph/meta
> > > >     /sys/kernel/debug/ceph/meta/client_features
> > > >     /sys/kernel/debug/ceph/meta/client_features/mount_syntax_v2
> > > >     /sys/kernel/debug/ceph/meta/client_features/mount_syntax_v1
> > > >     ....
> > > >     ....
> > >
> > > Hi Venky, Jeff,
> > >
> > > If this is supposed to be used in the wild and not just in teuthology,
> > > I would be wary of going with debugfs.  debugfs isn't always available
> > > (it is actually compiled out in some configurations, it may or may not
> > > be mounted, etc).  With the new mount syntax feature it is not a big
> > > deal because the mount helper should do just fine without it but with
> > > other features we may find ourselves in a situation where the mount
> > > helper (or something else) just *has* to know whether the feature is
> > > supported or not and falling back to "no" if debugfs is not available
> > > is undesirable or too much work.
> > >
> > > I don't have a great suggestion though.  When I needed to do this in
> > > the past for RADOS feature bits, I went with a read-only kernel module
> > > parameter [1].  They are exported via sysfs which is guaranteed to be
> > > available.  Perhaps we should do the same for mount_syntax -- have it
> > > be either 1 or 2, allowing it to be revved in the future?
> >
> > I'm ok with exporting via sysfs (since it's guaranteed). My only ask
> > here would be to have the mount support information present itself as
> > files rather than file contents to avoid writing parsing stuff in
> > userspace, which is ok, however, relying on stat() is nicer.
> >
>
> You should be able to do that by just making a read-only parameter
> called "mount_syntax_v2", and then you can test for it by doing
> something like:
>
>     # stat /sys/module/ceph/parameters/mount_syntax_v2
>
> The contents of the file can be blank (or just return Y or something).

Yep. That's fine.

So, we no longer have these entries in subdirectories
(meta/client_features/...) and those end up under parameters?

I do see subdirectories under other modules, so it's probably doable with sysfs.

>
> > >
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d6a3408a77807037872892c2a2034180fcc08d12
> > >
> > > Thanks,
> > >
> > >                 Ilya
> > >
> >
> >
>
> --
> Jeff Layton <jlayton@redhat.com>
>


-- 
Cheers,
Venky