| Message ID | 20211001050037.497199-1-vshankar@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ceph: add debugfs entries signifying new mount syntax support | expand |
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".
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.
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,
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> >
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
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.
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 >
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 > > > >
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> >
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(-)