| Message ID | 87tupuya6t.fsf@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [EXPERIMENT] new mount API verbose errors from userspace | expand |
Oops, sent the wrong set of patches. Resending right one.
On Tue, Mar 2, 2021 at 5:03 AM Aurélien Aptel <aaptel@suse.com> wrote: > > Hi, > > I have some code to use the new mount API from user space. > > The kernel changes are just making the code use the fs_context logging > feature. > > The sample userspace prog (fsopen.c) is just a PoC showing how mounting > is done and how the mount errors are read. > > If you change the prog to use a wrong version for example (vers=4.0) you > get this: > > $ gcc -o fsopen fsopen.c > $ ./fsopen > fsconfig(sfd, FSCONFIG_SET_STRING, "vers", "4.0", 0): Invalid argument > kernel mount errors: > e Unknown vers= option specified: 4.0 > > There are some cons to using this API, mainly that mount.cifs will have > to encode more knowledge and processing of the user-provided option > before passing it to the kernel. > > Where previously we would pass most options as-is to the kernel, making > it easier to add new ones without patching mount.cifs; we know have to > know if the option is a key=string, or key=int, or boolean (key/nokey) > and call the appropriate FSCONFIG_SET_{STRING,FLAG,PATH,...}. > > The pros are that we process one option at a time and we can fail early > with verbose, helpful messages to the user. I like this, this is very nice. But, as you touch upon, it requires we know in mount.c what type each argument is. Which is problematic because the list of mount arguments in cifs.ko has a fair amount of crunch and I think it would be unworkable to keep cifs-utils and cifs.ko in lockstep for every release where we modify a mount argument. What I think we should have is a ioctl(), system-call, /proc/fs/cifs/options, where we can query the kernel/file-system module for "give me a list of all recognized mount options and their type" i.e. basically a way to fetch the "struct fs_parameter_spec" to userspace. This is probably something that would not be specific to cifs, but would apply to all filesystem modules. regards ronnie sahlberg > > > Cheers, > -- > Aurélien Aptel / SUSE Labs Samba Team > GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
ronnie sahlberg <ronniesahlberg@gmail.com> writes: > I like this, this is very nice. > But, as you touch upon, it requires we know in mount.c what type each > argument is. > Which is problematic because the list of mount arguments in cifs.ko > has a fair amount of crunch > and I think it would be unworkable to keep cifs-utils and cifs.ko in > lockstep for every release where > we modify a mount argument. We could do some basic pattern matching: like if has no '=', assume boolean flag. If value only made of number, assume int. String for everything else. But this might fail on numbers which are actually string (e.g. password=123456). > What I think we should have is a ioctl(), system-call, > /proc/fs/cifs/options, where we can query the kernel/file-system > module > for "give me a list of all recognized mount options and their type" > i.e. basically a way to fetch the "struct fs_parameter_spec" to userspace. > > This is probably something that would not be specific to cifs, but > would apply to all filesystem modules. That sounds like a good idea yes. I wonder if David Howells has considered this. We can already sort-of do this actually. We can have a special boolean option "help" which on the kernel side would log the list of options&types in the fs_context log (as "i" (info) messages). Cheers,
Aurélien Aptel <aaptel@suse.com> wrote: > > What I think we should have is a ioctl(), system-call, > > /proc/fs/cifs/options, where we can query the kernel/file-system > > module > > for "give me a list of all recognized mount options and their type" > > i.e. basically a way to fetch the "struct fs_parameter_spec" to userspace. > > > > This is probably something that would not be specific to cifs, but > > would apply to all filesystem modules. > > That sounds like a good idea yes. I wonder if David Howells has > considered this. I had this. Al disliked it and removed it before the patches got upstream. Also Linus hated the fsinfo() syscall that was the way to get this (amongst other things). David
David Howells <dhowells@redhat.com> writes: > I had this. Al disliked it and removed it before the patches got upstream. > Also Linus hated the fsinfo() syscall that was the way to get this (amongst > other things). I see, that's too bad :/ thanks for trying anyway. Cheers,
Since there's no standard VFS way to get the supported mount options
from userspace, I thought I would do what Ronnie suggested and export
them from a cifs /proc file.
That's the only change since v1, in the 4th patch.
David, maybe this can give your arguments for the need for fsinfo() if
we end up using this in cifs-utils.
I have added some dumb code in userspace to parse it and see if the
option exists and what type it is. This removes the requirement of
having to keep cifs-utils and kernel updated at the same time to use new
options.
Previous intro
=======================================================================
I have some code to use the new mount API from user space.
The kernel changes are just making the code use the fs_context logging
feature.
The sample userspace prog (fsopen.c attached) is just a PoC showing how
mounting is done and how the mount errors are read.
If you change the prog to use a wrong version for example (vers=4.0) you
get this:
$ gcc -o fsopen fsopen.c
$ ./fsopen
fsconfig(sfd, FSCONFIG_SET_STRING, "vers", "4.0", 0): Invalid argument
kernel mount errors:
e Unknown vers= option specified: 4.0
The pros are that we process one option at a time and we can fail early
with verbose, helpful messages to the user.
=======================================================================
Cheers,
Tentatively merged into cifs-2.6.git for-next but would like more feedback on other's thoughts on this. Getting more verbose error information back on mount errors (to userspace returning something more than a primitive small set of return codes, and a message logged to dmesg) is critical, and this approach seems reasonable at first glance but if there are better ways ... On Thu, Mar 18, 2021 at 8:12 AM Aurélien Aptel <aaptel@suse.com> wrote: > > > Since there's no standard VFS way to get the supported mount options > from userspace, I thought I would do what Ronnie suggested and export > them from a cifs /proc file. > That's the only change since v1, in the 4th patch. > > David, maybe this can give your arguments for the need for fsinfo() if > we end up using this in cifs-utils. > > I have added some dumb code in userspace to parse it and see if the > option exists and what type it is. This removes the requirement of > having to keep cifs-utils and kernel updated at the same time to use new > options. > > Previous intro > ======================================================================= > I have some code to use the new mount API from user space. > > The kernel changes are just making the code use the fs_context logging > feature. > > The sample userspace prog (fsopen.c attached) is just a PoC showing how > mounting is done and how the mount errors are read. > > If you change the prog to use a wrong version for example (vers=4.0) you > get this: > > $ gcc -o fsopen fsopen.c > $ ./fsopen > fsconfig(sfd, FSCONFIG_SET_STRING, "vers", "4.0", 0): Invalid argument > kernel mount errors: > e Unknown vers= option specified: 4.0 > > The pros are that we process one option at a time and we can fail early > with verbose, helpful messages to the user. > ======================================================================= > > > > Cheers, > -- > Aurélien Aptel / SUSE Labs Samba Team > GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
Steve French <smfrench@gmail.com> writes: > Tentatively merged into cifs-2.6.git for-next but would like more > feedback on other's thoughts on this. Getting more verbose error > information back on mount errors (to userspace returning something > more than a primitive small set of return codes, and a message logged > to dmesg) is critical, and this approach seems reasonable at first > glance but if there are better ways ... Yes more feedback would be reasonable. I've basically redone a barebone version of the missing fsinfo() call just for cifs. New patch version attached. Changes since v2: - add missing call to remove_proc_entry() (fix splat on rmmod) Cheers,
From fe07bfda2fb9cdef8a4d4008a409bb02f35f1bd8 Mon Sep 17 00:00:00 2001 From: Linus Torvalds <torvalds@linux-foundation.org> Date: Sun, 28 Feb 2021 16:05:19 -0800 Subject: [PATCH 1/3] Linux 5.12-rc1 --- Makefile | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index b0e1bb472202..f9b54da2fca0 100644 --- a/Makefile +++ b/Makefile @@ -1,9 +1,9 @@ # SPDX-License-Identifier: GPL-2.0 VERSION = 5 -PATCHLEVEL = 11 +PATCHLEVEL = 12 SUBLEVEL = 0 -EXTRAVERSION = -NAME =
Hi, I have some code to use the new mount API from user space. The kernel changes are just making the code use the fs_context logging feature. The sample userspace prog (fsopen.c) is just a PoC showing how mounting is done and how the mount errors are read. If you change the prog to use a wrong version for example (vers=4.0) you get this: $ gcc -o fsopen fsopen.c $ ./fsopen fsconfig(sfd, FSCONFIG_SET_STRING, "vers", "4.0", 0): Invalid argument kernel mount errors: e Unknown vers= option specified: 4.0 There are some cons to using this API, mainly that mount.cifs will have to encode more knowledge and processing of the user-provided option before passing it to the kernel. Where previously we would pass most options as-is to the kernel, making it easier to add new ones without patching mount.cifs; we know have to know if the option is a key=string, or key=int, or boolean (key/nokey) and call the appropriate FSCONFIG_SET_{STRING,FLAG,PATH,...}. The pros are that we process one option at a time and we can fail early with verbose, helpful messages to the user. Cheers,