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

From: Jeff Layton <jlayton@redhat.com>
To: Venky Shankar <vshankar@redhat.com>, pdonnell@redhat.com
Cc: ceph-devel@vger.kernel.org
Subject: Re: [PATCH v4 0/2] ceph: add debugfs entries signifying new mount syntax support
Date: Fri, 01 Oct 2021 12:24:09 -0400	[thread overview]
Message-ID: <e0f529e2e17cb886bd6a906541fb978be45e0e4e.camel@redhat.com> (raw)
In-Reply-To: <20211001050037.497199-1-vshankar@redhat.com>

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>