| Message ID | 20211221164959.174480-4-shr@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | io_uring: add xattr support | expand |
On Tue, Dec 21, 2021 at 8:50 AM Stefan Roesch <shr@fb.com> wrote: > > This splits off do_getxattr function from the getxattr > function. This will allow io_uring to call it from its > io worker. Hmm. My reaction to this one is "Why isn't do_getxattr() using 'struct xattr_ctx' for its context?" As far as I can tell, that's *exactly* what it wants, and it would be logical to match up with the setxattr side. Yeah, yeah, setxattr has a 'const void __user *value' while getxattr obviously has just a 'void __user *value'. But if the cost of having a unified interface is that you lose the 'const' part for the setxattr, I think that's still a good thing. Yes? No? Comments? Linus
On 12/21/21 9:22 AM, Linus Torvalds wrote: > On Tue, Dec 21, 2021 at 8:50 AM Stefan Roesch <shr@fb.com> wrote: >> >> This splits off do_getxattr function from the getxattr >> function. This will allow io_uring to call it from its >> io worker. > > Hmm. > > My reaction to this one is > > "Why isn't do_getxattr() using 'struct xattr_ctx' for its context?" > > As far as I can tell, that's *exactly* what it wants, and it would be > logical to match up with the setxattr side. > > Yeah, yeah, setxattr has a 'const void __user *value' while getxattr > obviously has just a 'void __user *value'. But if the cost of having a > unified interface is that you lose the 'const' part for the setxattr, > I think that's still a good thing. > > Yes? No? Comments? Linus, if we remove the constness, then we either need to cast away the constness (the system call is defined as const) or change the definition of the system call. > > Linus >
On Tue, Dec 21, 2021 at 11:15 AM Stefan Roesch <shr@fb.com> wrote: > > Linus, if we remove the constness, then we either need to cast away the constness (the system call > is defined as const) or change the definition of the system call. You could also do it as union { const void __user *setxattr_value; void __user *getxattr_value; }; if you wanted to.. Linus
On 12/21/21 11:18 AM, Linus Torvalds wrote: > On Tue, Dec 21, 2021 at 11:15 AM Stefan Roesch <shr@fb.com> wrote: >> >> Linus, if we remove the constness, then we either need to cast away the constness (the system call >> is defined as const) or change the definition of the system call. > > You could also do it as > > union { > const void __user *setxattr_value; > void __user *getxattr_value; > }; > Pavel brought up a very good point. By adding the kname array into the xarray_ctx we increase the size of io_xattr structure too much. In addition this will also increase the size of the io_kiocb structure. The original solution did not increase the size. Per opcode we limit the storage space to 64 bytes. However the array itself requires 256 bytes. > if you wanted to.. > > Linus >
On 12/21/21 2:59 PM, Stefan Roesch wrote: > > > On 12/21/21 11:18 AM, Linus Torvalds wrote: >> On Tue, Dec 21, 2021 at 11:15 AM Stefan Roesch <shr@fb.com> wrote: >>> >>> Linus, if we remove the constness, then we either need to cast away the constness (the system call >>> is defined as const) or change the definition of the system call. >> >> You could also do it as >> >> union { >> const void __user *setxattr_value; >> void __user *getxattr_value; >> }; >> > > Pavel brought up a very good point. By adding the kname array into the > xarray_ctx we increase the size of io_xattr structure too much. In > addition this will also increase the size of the io_kiocb structure. > The original solution did not increase the size. > > Per opcode we limit the storage space to 64 bytes. However the array > itself requires 256 bytes. Just to expand on that a bit - part of struct io_kiocb is per-command data, and we try pretty hard to keep that at 64-bytes as that's the largest one we currently have. If we add the array to the io_xattr structure, then that will increase the whole io_kiocb from 224 bytes to more than twice that. So there are really two options here: 1) The xattr_ctx structure goes into the async data that a command has to allocate for deferred execution. This isn't a _huge_ deal as we have to defer the xattr commands for now anyway, as the VFS doesn't support a nonblocking version of that yet. But it would still be nice not to have to do that. 2) We keep the original interface that Stefan proposed, leaving the xattr_ctx bits embedded as they fit fine like that. #2 would be a bit more efficient, but I don't feel that strongly about it for this particular case. Comments?
diff --git a/fs/internal.h b/fs/internal.h index d13b9f13df09..efd2c4f7a536 100644 --- a/fs/internal.h +++ b/fs/internal.h @@ -207,6 +207,12 @@ struct xattr_ctx { unsigned int flags; }; +ssize_t do_getxattr(struct user_namespace *mnt_userns, + struct dentry *d, + const char *kname, + void __user *value, + size_t size); + void *setxattr_setup(struct user_namespace *mnt_userns, const char __user *name, struct xattr_ctx *data); diff --git a/fs/xattr.c b/fs/xattr.c index a4b59523bd0e..1d9795bc8be6 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -663,19 +663,12 @@ SYSCALL_DEFINE5(fsetxattr, int, fd, const char __user *, name, /* * Extended attribute GET operations */ -static ssize_t -getxattr(struct user_namespace *mnt_userns, struct dentry *d, - const char __user *name, void __user *value, size_t size) +ssize_t +do_getxattr(struct user_namespace *mnt_userns, struct dentry *d, + const char *kname, void __user *value, size_t size) { - ssize_t error; void *kvalue = NULL; - char kname[XATTR_NAME_MAX + 1]; - - error = strncpy_from_user(kname, name, sizeof(kname)); - if (error == 0 || error == sizeof(kname)) - error = -ERANGE; - if (error < 0) - return error; + ssize_t error; if (size) { if (size > XATTR_SIZE_MAX) @@ -703,6 +696,22 @@ getxattr(struct user_namespace *mnt_userns, struct dentry *d, return error; } +static ssize_t +getxattr(struct user_namespace *mnt_userns, struct dentry *d, + const char __user *name, void __user *value, size_t size) +{ + ssize_t error; + char kname[XATTR_NAME_MAX + 1]; + + error = strncpy_from_user(kname, name, sizeof(kname)); + if (error == 0 || error == sizeof(kname)) + error = -ERANGE; + if (error < 0) + return error; + + return do_getxattr(mnt_userns, d, kname, value, size); +} + static ssize_t path_getxattr(const char __user *pathname, const char __user *name, void __user *value, size_t size, unsigned int lookup_flags)
This splits off do_getxattr function from the getxattr function. This will allow io_uring to call it from its io worker. Signed-off-by: Stefan Roesch <shr@fb.com> --- fs/internal.h | 6 ++++++ fs/xattr.c | 31 ++++++++++++++++++++----------- 2 files changed, 26 insertions(+), 11 deletions(-)