| Message ID | 20211129221257.2536146-1-shr@fb.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | io_uring: add xattr support | expand |
On Mon, Nov 29 2021 at 14:12:52 -0800, Stefan Roesch quoth thus: > This adds the xattr support to io_uring. The intent is to have a more > complete support for file operations in io_uring. > > This change adds support for the following functions to io_uring: > - fgetxattr > - fsetxattr > - getxattr > - setxattr You may wish to consider the following. Patching for these functions makes for an excellent opportunity to provide a better interface. Rather than implement fXetattr at all, you could enable io_uring to use functions like: int Xetxattr(int dfd, const char *path, const char *name, [const] void *value, size_t size, int flags); Not only does this simplify the io_uring interface down to two functions, but modernizes and fixes a deficit in usability. In terms of io_uring, this is just changing internal interfaces. Although unnecessary for io_uring, it would be nice to at least consider what parts of this code could be leveraged for future Xetxattr2 syscalls. > Patch 1: fs: make user_path_at_empty() take a struct filename > The user_path_at_empty filename parameter has been changed > from a const char user pointer to a filename struct. io_uring > operates on filenames. > In addition also the functions that call user_path_at_empty > in namei.c and stat.c have been modified for this change. > > Patch 2: fs: split off setxattr_setup function from setxattr > Split off the setup part of the setxattr function > > Patch 3: fs: split off the vfs_getxattr from getxattr > Split of the vfs_getxattr part from getxattr. This will > allow to invoke it from io_uring. > > Patch 4: io_uring: add fsetxattr and setxattr support > This adds new functions to support the fsetxattr and setxattr > functions. > > Patch 5: io_uring: add fgetxattr and getxattr support > This adds new functions to support the fgetxattr and getxattr > functions. > > > There are two additional patches: > liburing: Add support for xattr api's. > This also includes the tests for the new code. > xfstests: Add support for io_uring xattr support. > > > Stefan Roesch (5): > fs: make user_path_at_empty() take a struct filename > fs: split off setxattr_setup function from setxattr > fs: split off the vfs_getxattr from getxattr > io_uring: add fsetxattr and setxattr support > io_uring: add fgetxattr and getxattr support > > fs/internal.h | 23 +++ > fs/io_uring.c | 325 ++++++++++++++++++++++++++++++++++ > fs/namei.c | 5 +- > fs/stat.c | 7 +- > fs/xattr.c | 114 +++++++----- > include/linux/namei.h | 4 +- > include/uapi/linux/io_uring.h | 8 +- > 7 files changed, 439 insertions(+), 47 deletions(-) > > > Signed-off-by: Stefan Roesch <shr@fb.com> > base-commit: c2626d30f312afc341158e07bf088f5a23b4eeeb > -- > 2.30.2
> On Nov 29, 2021, at 6:08 PM, Clay Harris <bugs@claycon.org> wrote: > > On Mon, Nov 29 2021 at 14:12:52 -0800, Stefan Roesch quoth thus: > >> This adds the xattr support to io_uring. The intent is to have a more >> complete support for file operations in io_uring. >> >> This change adds support for the following functions to io_uring: >> - fgetxattr >> - fsetxattr >> - getxattr >> - setxattr > > You may wish to consider the following. > > Patching for these functions makes for an excellent opportunity > to provide a better interface. Rather than implement fXetattr > at all, you could enable io_uring to use functions like: > > int Xetxattr(int dfd, const char *path, const char *name, > [const] void *value, size_t size, int flags); This would naturally be named "...xattrat()"? > Not only does this simplify the io_uring interface down to two > functions, but modernizes and fixes a deficit in usability. > In terms of io_uring, this is just changing internal interfaces. Even better would be the ability to get/set an array of xattrs in one call, to avoid repeated path lookups in the common case of handling multiple xattrs on a single file. > Although unnecessary for io_uring, it would be nice to at least > consider what parts of this code could be leveraged for future > Xetxattr2 syscalls. > >> Patch 1: fs: make user_path_at_empty() take a struct filename >> The user_path_at_empty filename parameter has been changed >> from a const char user pointer to a filename struct. io_uring >> operates on filenames. >> In addition also the functions that call user_path_at_empty >> in namei.c and stat.c have been modified for this change. >> >> Patch 2: fs: split off setxattr_setup function from setxattr >> Split off the setup part of the setxattr function >> >> Patch 3: fs: split off the vfs_getxattr from getxattr >> Split of the vfs_getxattr part from getxattr. This will >> allow to invoke it from io_uring. >> >> Patch 4: io_uring: add fsetxattr and setxattr support >> This adds new functions to support the fsetxattr and setxattr >> functions. >> >> Patch 5: io_uring: add fgetxattr and getxattr support >> This adds new functions to support the fgetxattr and getxattr >> functions. >> >> >> There are two additional patches: >> liburing: Add support for xattr api's. >> This also includes the tests for the new code. >> xfstests: Add support for io_uring xattr support. >> >> >> Stefan Roesch (5): >> fs: make user_path_at_empty() take a struct filename >> fs: split off setxattr_setup function from setxattr >> fs: split off the vfs_getxattr from getxattr >> io_uring: add fsetxattr and setxattr support >> io_uring: add fgetxattr and getxattr support >> >> fs/internal.h | 23 +++ >> fs/io_uring.c | 325 ++++++++++++++++++++++++++++++++++ >> fs/namei.c | 5 +- >> fs/stat.c | 7 +- >> fs/xattr.c | 114 +++++++----- >> include/linux/namei.h | 4 +- >> include/uapi/linux/io_uring.h | 8 +- >> 7 files changed, 439 insertions(+), 47 deletions(-) >> >> >> Signed-off-by: Stefan Roesch <shr@fb.com> >> base-commit: c2626d30f312afc341158e07bf088f5a23b4eeeb >> -- >> 2.30.2 Cheers, Andreas
On Mon, Nov 29 2021 at 20:16:02 -0700, Andreas Dilger quoth thus: > > > On Nov 29, 2021, at 6:08 PM, Clay Harris <bugs@claycon.org> wrote: > > > > On Mon, Nov 29 2021 at 14:12:52 -0800, Stefan Roesch quoth thus: > > > >> This adds the xattr support to io_uring. The intent is to have a more > >> complete support for file operations in io_uring. > >> > >> This change adds support for the following functions to io_uring: > >> - fgetxattr > >> - fsetxattr > >> - getxattr > >> - setxattr > > > > You may wish to consider the following. > > > > Patching for these functions makes for an excellent opportunity > > to provide a better interface. Rather than implement fXetattr > > at all, you could enable io_uring to use functions like: > > > > int Xetxattr(int dfd, const char *path, const char *name, > > [const] void *value, size_t size, int flags); > > This would naturally be named "...xattrat()"? Indeed! > > Not only does this simplify the io_uring interface down to two > > functions, but modernizes and fixes a deficit in usability. > > In terms of io_uring, this is just changing internal interfaces. > > Even better would be the ability to get/set an array of xattrs in > one call, to avoid repeated path lookups in the common case of > handling multiple xattrs on a single file. True. > > Although unnecessary for io_uring, it would be nice to at least > > consider what parts of this code could be leveraged for future > > Xetxattr2 syscalls. s/Xetxattr2/Xetxattrat/ > > > >> Patch 1: fs: make user_path_at_empty() take a struct filename > >> The user_path_at_empty filename parameter has been changed > >> from a const char user pointer to a filename struct. io_uring > >> operates on filenames. > >> In addition also the functions that call user_path_at_empty > >> in namei.c and stat.c have been modified for this change. > >> > >> Patch 2: fs: split off setxattr_setup function from setxattr > >> Split off the setup part of the setxattr function > >> > >> Patch 3: fs: split off the vfs_getxattr from getxattr > >> Split of the vfs_getxattr part from getxattr. This will > >> allow to invoke it from io_uring. > >> > >> Patch 4: io_uring: add fsetxattr and setxattr support > >> This adds new functions to support the fsetxattr and setxattr > >> functions. > >> > >> Patch 5: io_uring: add fgetxattr and getxattr support > >> This adds new functions to support the fgetxattr and getxattr > >> functions. > >> > >> > >> There are two additional patches: > >> liburing: Add support for xattr api's. > >> This also includes the tests for the new code. > >> xfstests: Add support for io_uring xattr support. > >> > >> > >> Stefan Roesch (5): > >> fs: make user_path_at_empty() take a struct filename > >> fs: split off setxattr_setup function from setxattr > >> fs: split off the vfs_getxattr from getxattr > >> io_uring: add fsetxattr and setxattr support > >> io_uring: add fgetxattr and getxattr support > >> > >> fs/internal.h | 23 +++ > >> fs/io_uring.c | 325 ++++++++++++++++++++++++++++++++++ > >> fs/namei.c | 5 +- > >> fs/stat.c | 7 +- > >> fs/xattr.c | 114 +++++++----- > >> include/linux/namei.h | 4 +- > >> include/uapi/linux/io_uring.h | 8 +- > >> 7 files changed, 439 insertions(+), 47 deletions(-) > >> > >> > >> Signed-off-by: Stefan Roesch <shr@fb.com> > >> base-commit: c2626d30f312afc341158e07bf088f5a23b4eeeb > >> -- > >> 2.30.2 > > > Cheers, Andreas > > > > >
On Tue, Nov 30 2021 at 00:37:03 -0600, Clay Harris quoth thus: > On Mon, Nov 29 2021 at 20:16:02 -0700, Andreas Dilger quoth thus: > > > > > > On Nov 29, 2021, at 6:08 PM, Clay Harris <bugs@claycon.org> wrote: > > > > > > On Mon, Nov 29 2021 at 14:12:52 -0800, Stefan Roesch quoth thus: > > > > > >> This adds the xattr support to io_uring. The intent is to have a more > > >> complete support for file operations in io_uring. > > >> > > >> This change adds support for the following functions to io_uring: > > >> - fgetxattr > > >> - fsetxattr > > >> - getxattr > > >> - setxattr > > > > > > You may wish to consider the following. > > > > > > Patching for these functions makes for an excellent opportunity > > > to provide a better interface. Rather than implement fXetattr > > > at all, you could enable io_uring to use functions like: > > > > > > int Xetxattr(int dfd, const char *path, const char *name, > > > [const] void *value, size_t size, int flags); > > > > This would naturally be named "...xattrat()"? > > Indeed! > > > > Not only does this simplify the io_uring interface down to two > > > functions, but modernizes and fixes a deficit in usability. > > > In terms of io_uring, this is just changing internal interfaces. > > > > Even better would be the ability to get/set an array of xattrs in > > one call, to avoid repeated path lookups in the common case of > > handling multiple xattrs on a single file. > > True. > > > > Although unnecessary for io_uring, it would be nice to at least > > > consider what parts of this code could be leveraged for future > > > Xetxattr2 syscalls. > s/Xetxattr2/Xetxattrat/ I forgot to mention a final thought about the interface. Unless there is a really good reason (security auditing??), there is no reason to have a removexattr() function. That seems much better handled by passing NULL for value and specifying a remove flag in flags to setxattrat(). > > > > > >> Patch 1: fs: make user_path_at_empty() take a struct filename > > >> The user_path_at_empty filename parameter has been changed > > >> from a const char user pointer to a filename struct. io_uring > > >> operates on filenames. > > >> In addition also the functions that call user_path_at_empty > > >> in namei.c and stat.c have been modified for this change. > > >> > > >> Patch 2: fs: split off setxattr_setup function from setxattr > > >> Split off the setup part of the setxattr function > > >> > > >> Patch 3: fs: split off the vfs_getxattr from getxattr > > >> Split of the vfs_getxattr part from getxattr. This will > > >> allow to invoke it from io_uring. > > >> > > >> Patch 4: io_uring: add fsetxattr and setxattr support > > >> This adds new functions to support the fsetxattr and setxattr > > >> functions. > > >> > > >> Patch 5: io_uring: add fgetxattr and getxattr support > > >> This adds new functions to support the fgetxattr and getxattr > > >> functions. > > >> > > >> > > >> There are two additional patches: > > >> liburing: Add support for xattr api's. > > >> This also includes the tests for the new code. > > >> xfstests: Add support for io_uring xattr support. > > >> > > >> > > >> Stefan Roesch (5): > > >> fs: make user_path_at_empty() take a struct filename > > >> fs: split off setxattr_setup function from setxattr > > >> fs: split off the vfs_getxattr from getxattr > > >> io_uring: add fsetxattr and setxattr support > > >> io_uring: add fgetxattr and getxattr support > > >> > > >> fs/internal.h | 23 +++ > > >> fs/io_uring.c | 325 ++++++++++++++++++++++++++++++++++ > > >> fs/namei.c | 5 +- > > >> fs/stat.c | 7 +- > > >> fs/xattr.c | 114 +++++++----- > > >> include/linux/namei.h | 4 +- > > >> include/uapi/linux/io_uring.h | 8 +- > > >> 7 files changed, 439 insertions(+), 47 deletions(-) > > >> > > >> > > >> Signed-off-by: Stefan Roesch <shr@fb.com> > > >> base-commit: c2626d30f312afc341158e07bf088f5a23b4eeeb > > >> -- > > >> 2.30.2 > > > > > > Cheers, Andreas > > > > > > > > > > >
On Mon, Nov 29, 2021 at 08:16:02PM -0700, Andreas Dilger wrote: > > > On Nov 29, 2021, at 6:08 PM, Clay Harris <bugs@claycon.org> wrote: > > > > On Mon, Nov 29 2021 at 14:12:52 -0800, Stefan Roesch quoth thus: > > > >> This adds the xattr support to io_uring. The intent is to have a more > >> complete support for file operations in io_uring. > >> > >> This change adds support for the following functions to io_uring: > >> - fgetxattr > >> - fsetxattr > >> - getxattr > >> - setxattr > > > > You may wish to consider the following. > > > > Patching for these functions makes for an excellent opportunity > > to provide a better interface. Rather than implement fXetattr > > at all, you could enable io_uring to use functions like: > > > > int Xetxattr(int dfd, const char *path, const char *name, > > [const] void *value, size_t size, int flags); > > This would naturally be named "...xattrat()"? > > > Not only does this simplify the io_uring interface down to two > > functions, but modernizes and fixes a deficit in usability. > > In terms of io_uring, this is just changing internal interfaces. > > Even better would be the ability to get/set an array of xattrs in > one call, to avoid repeated path lookups in the common case of > handling multiple xattrs on a single file. Been around for since the mid 1990s IIRC. XFS brought them to Linux from Irix and they are used by xfsdump/restore via libhandle. API documented here: $ man 3 attr_multi And they are implemented through XFS ioctls. Cheers, Dave.
On Tue, Nov 30 2021 at 00:53:45 -0600, Clay Harris quoth thus: > On Tue, Nov 30 2021 at 00:37:03 -0600, Clay Harris quoth thus: > > > On Mon, Nov 29 2021 at 20:16:02 -0700, Andreas Dilger quoth thus: > > > > > > > > > On Nov 29, 2021, at 6:08 PM, Clay Harris <bugs@claycon.org> wrote: > > > > > > > > On Mon, Nov 29 2021 at 14:12:52 -0800, Stefan Roesch quoth thus: > > > > > > > >> This adds the xattr support to io_uring. The intent is to have a more > > > >> complete support for file operations in io_uring. > > > >> > > > >> This change adds support for the following functions to io_uring: > > > >> - fgetxattr > > > >> - fsetxattr > > > >> - getxattr > > > >> - setxattr > > > > > > > > You may wish to consider the following. > > > > > > > > Patching for these functions makes for an excellent opportunity > > > > to provide a better interface. Rather than implement fXetattr > > > > at all, you could enable io_uring to use functions like: > > > > > > > > int Xetxattr(int dfd, const char *path, const char *name, > > > > [const] void *value, size_t size, int flags); > > > > > > This would naturally be named "...xattrat()"? > > > > Indeed! > > > > > > Not only does this simplify the io_uring interface down to two > > > > functions, but modernizes and fixes a deficit in usability. > > > > In terms of io_uring, this is just changing internal interfaces. One more reason, it would be very desirable if io_uring called a *etxattrat-like interface, is that the old f*etxattr calls require an fd open for reading (fget*) or writing (fset*). So, you're out of luck if you have an execute-only file or just an O_PATH descriptor! In those cases, you're forced to use a pathname for every call. Not very efficient for people who choose to use the highly optimized io_uring interface. > > > Even better would be the ability to get/set an array of xattrs in > > > one call, to avoid repeated path lookups in the common case of > > > handling multiple xattrs on a single file. > > > > True. > > > > > > Although unnecessary for io_uring, it would be nice to at least > > > > consider what parts of this code could be leveraged for future > > > > Xetxattr2 syscalls. > > s/Xetxattr2/Xetxattrat/ > > I forgot to mention a final thought about the interface. > Unless there is a really good reason (security auditing??), there > is no reason to have a removexattr() function. That seems much > better handled by passing NULL for value and specifying a remove > flag in flags to setxattrat(). > > > > > > > > >> Patch 1: fs: make user_path_at_empty() take a struct filename > > > >> The user_path_at_empty filename parameter has been changed > > > >> from a const char user pointer to a filename struct. io_uring > > > >> operates on filenames. > > > >> In addition also the functions that call user_path_at_empty > > > >> in namei.c and stat.c have been modified for this change. > > > >> > > > >> Patch 2: fs: split off setxattr_setup function from setxattr > > > >> Split off the setup part of the setxattr function > > > >> > > > >> Patch 3: fs: split off the vfs_getxattr from getxattr > > > >> Split of the vfs_getxattr part from getxattr. This will > > > >> allow to invoke it from io_uring. > > > >> > > > >> Patch 4: io_uring: add fsetxattr and setxattr support > > > >> This adds new functions to support the fsetxattr and setxattr > > > >> functions. > > > >> > > > >> Patch 5: io_uring: add fgetxattr and getxattr support > > > >> This adds new functions to support the fgetxattr and getxattr > > > >> functions. > > > >> > > > >> > > > >> There are two additional patches: > > > >> liburing: Add support for xattr api's. > > > >> This also includes the tests for the new code. > > > >> xfstests: Add support for io_uring xattr support. > > > >> > > > >> > > > >> Stefan Roesch (5): > > > >> fs: make user_path_at_empty() take a struct filename > > > >> fs: split off setxattr_setup function from setxattr > > > >> fs: split off the vfs_getxattr from getxattr > > > >> io_uring: add fsetxattr and setxattr support > > > >> io_uring: add fgetxattr and getxattr support > > > >> > > > >> fs/internal.h | 23 +++ > > > >> fs/io_uring.c | 325 ++++++++++++++++++++++++++++++++++ > > > >> fs/namei.c | 5 +- > > > >> fs/stat.c | 7 +- > > > >> fs/xattr.c | 114 +++++++----- > > > >> include/linux/namei.h | 4 +- > > > >> include/uapi/linux/io_uring.h | 8 +- > > > >> 7 files changed, 439 insertions(+), 47 deletions(-) > > > >> > > > >> > > > >> Signed-off-by: Stefan Roesch <shr@fb.com> > > > >> base-commit: c2626d30f312afc341158e07bf088f5a23b4eeeb > > > >> -- > > > >> 2.30.2 > > > > > > > > > Cheers, Andreas > > > > > > > > > > > > > > > > >
On 11/29/21 5:08 PM, Clay Harris wrote: > On Mon, Nov 29 2021 at 14:12:52 -0800, Stefan Roesch quoth thus: > >> This adds the xattr support to io_uring. The intent is to have a more >> complete support for file operations in io_uring. >> >> This change adds support for the following functions to io_uring: >> - fgetxattr >> - fsetxattr >> - getxattr >> - setxattr > > You may wish to consider the following. > > Patching for these functions makes for an excellent opportunity > to provide a better interface. Rather than implement fXetattr > at all, you could enable io_uring to use functions like: > > int Xetxattr(int dfd, const char *path, const char *name, > [const] void *value, size_t size, int flags); > > Not only does this simplify the io_uring interface down to two > functions, but modernizes and fixes a deficit in usability. > In terms of io_uring, this is just changing internal interfaces. > > Although unnecessary for io_uring, it would be nice to at least > consider what parts of this code could be leveraged for future > Xetxattr2 syscalls. Clay, while we can reduce the number of calls to 2, providing 4 calls will ease the adoption of the interface. If you look at the userspace interface in liburing, you can see the following function signature: static inline void io_uring_prep_fgetxattr(struct io_uring_sqe *sqe, int fd, const char *name, const char *value, size_t len) This is very similar to what you proposed. > >> Patch 1: fs: make user_path_at_empty() take a struct filename >> The user_path_at_empty filename parameter has been changed >> from a const char user pointer to a filename struct. io_uring >> operates on filenames. >> In addition also the functions that call user_path_at_empty >> in namei.c and stat.c have been modified for this change. >> >> Patch 2: fs: split off setxattr_setup function from setxattr >> Split off the setup part of the setxattr function >> >> Patch 3: fs: split off the vfs_getxattr from getxattr >> Split of the vfs_getxattr part from getxattr. This will >> allow to invoke it from io_uring. >> >> Patch 4: io_uring: add fsetxattr and setxattr support >> This adds new functions to support the fsetxattr and setxattr >> functions. >> >> Patch 5: io_uring: add fgetxattr and getxattr support >> This adds new functions to support the fgetxattr and getxattr >> functions. >> >> >> There are two additional patches: >> liburing: Add support for xattr api's. >> This also includes the tests for the new code. >> xfstests: Add support for io_uring xattr support. >> >> >> Stefan Roesch (5): >> fs: make user_path_at_empty() take a struct filename >> fs: split off setxattr_setup function from setxattr >> fs: split off the vfs_getxattr from getxattr >> io_uring: add fsetxattr and setxattr support >> io_uring: add fgetxattr and getxattr support >> >> fs/internal.h | 23 +++ >> fs/io_uring.c | 325 ++++++++++++++++++++++++++++++++++ >> fs/namei.c | 5 +- >> fs/stat.c | 7 +- >> fs/xattr.c | 114 +++++++----- >> include/linux/namei.h | 4 +- >> include/uapi/linux/io_uring.h | 8 +- >> 7 files changed, 439 insertions(+), 47 deletions(-) >> >> >> Signed-off-by: Stefan Roesch <shr@fb.com> >> base-commit: c2626d30f312afc341158e07bf088f5a23b4eeeb >> -- >> 2.30.2
On 11/29/21 7:16 PM, Andreas Dilger wrote: > >> On Nov 29, 2021, at 6:08 PM, Clay Harris <bugs@claycon.org> wrote: >> >> On Mon, Nov 29 2021 at 14:12:52 -0800, Stefan Roesch quoth thus: >> >>> This adds the xattr support to io_uring. The intent is to have a more >>> complete support for file operations in io_uring. >>> >>> This change adds support for the following functions to io_uring: >>> - fgetxattr >>> - fsetxattr >>> - getxattr >>> - setxattr >> >> You may wish to consider the following. >> >> Patching for these functions makes for an excellent opportunity >> to provide a better interface. Rather than implement fXetattr >> at all, you could enable io_uring to use functions like: >> >> int Xetxattr(int dfd, const char *path, const char *name, >> [const] void *value, size_t size, int flags); > > This would naturally be named "...xattrat()"? > >> Not only does this simplify the io_uring interface down to two >> functions, but modernizes and fixes a deficit in usability. >> In terms of io_uring, this is just changing internal interfaces. > > Even better would be the ability to get/set an array of xattrs in > one call, to avoid repeated path lookups in the common case of > handling multiple xattrs on a single file. > You are proposing a new API. However that API has its challenges: - How do you implement error handling? What if only some requests fail. - It will make the code considerably more complicated (for user-space as well as kernel) Instead the user can do the following: - io_uring already has support for the following: - io_uring already has the ability to prepare several SQE's at once - These SQE's can be submitted in one operation - The SQE's can also be linked and waited for as a unit. - Allows to map each individual CQE to its request. >> Although unnecessary for io_uring, it would be nice to at least >> consider what parts of this code could be leveraged for future >> Xetxattr2 syscalls. > >> >>> Patch 1: fs: make user_path_at_empty() take a struct filename >>> The user_path_at_empty filename parameter has been changed >>> from a const char user pointer to a filename struct. io_uring >>> operates on filenames. >>> In addition also the functions that call user_path_at_empty >>> in namei.c and stat.c have been modified for this change. >>> >>> Patch 2: fs: split off setxattr_setup function from setxattr >>> Split off the setup part of the setxattr function >>> >>> Patch 3: fs: split off the vfs_getxattr from getxattr >>> Split of the vfs_getxattr part from getxattr. This will >>> allow to invoke it from io_uring. >>> >>> Patch 4: io_uring: add fsetxattr and setxattr support >>> This adds new functions to support the fsetxattr and setxattr >>> functions. >>> >>> Patch 5: io_uring: add fgetxattr and getxattr support >>> This adds new functions to support the fgetxattr and getxattr >>> functions. >>> >>> >>> There are two additional patches: >>> liburing: Add support for xattr api's. >>> This also includes the tests for the new code. >>> xfstests: Add support for io_uring xattr support. >>> >>> >>> Stefan Roesch (5): >>> fs: make user_path_at_empty() take a struct filename >>> fs: split off setxattr_setup function from setxattr >>> fs: split off the vfs_getxattr from getxattr >>> io_uring: add fsetxattr and setxattr support >>> io_uring: add fgetxattr and getxattr support >>> >>> fs/internal.h | 23 +++ >>> fs/io_uring.c | 325 ++++++++++++++++++++++++++++++++++ >>> fs/namei.c | 5 +- >>> fs/stat.c | 7 +- >>> fs/xattr.c | 114 +++++++----- >>> include/linux/namei.h | 4 +- >>> include/uapi/linux/io_uring.h | 8 +- >>> 7 files changed, 439 insertions(+), 47 deletions(-) >>> >>> >>> Signed-off-by: Stefan Roesch <shr@fb.com> >>> base-commit: c2626d30f312afc341158e07bf088f5a23b4eeeb >>> -- >>> 2.30.2 > > > Cheers, Andreas > > > > >
On Tue, Nov 30 2021 at 22:07:47 -0800, Stefan Roesch quoth thus: > > > On 11/29/21 5:08 PM, Clay Harris wrote: > > On Mon, Nov 29 2021 at 14:12:52 -0800, Stefan Roesch quoth thus: > > > >> This adds the xattr support to io_uring. The intent is to have a more > >> complete support for file operations in io_uring. > >> > >> This change adds support for the following functions to io_uring: > >> - fgetxattr > >> - fsetxattr > >> - getxattr > >> - setxattr > > > > You may wish to consider the following. > > > > Patching for these functions makes for an excellent opportunity > > to provide a better interface. Rather than implement fXetattr > > at all, you could enable io_uring to use functions like: > > > > int Xetxattr(int dfd, const char *path, const char *name, > > [const] void *value, size_t size, int flags); > > > > Not only does this simplify the io_uring interface down to two > > functions, but modernizes and fixes a deficit in usability. > > In terms of io_uring, this is just changing internal interfaces. > > > > Although unnecessary for io_uring, it would be nice to at least > > consider what parts of this code could be leveraged for future > > Xetxattr2 syscalls. > I may have become a little over-excited when I saw someone was thinking about new code associated with these interfaces. It's just that, to be very kind, the existing interfaces have so much room for improvement. I'm aware that changes in this area can be a non-trivial amount of work, due to specific xattr keys being handled by different security module hooks. > Clay, > > while we can reduce the number of calls to 2, providing 4 calls will > ease the adoption of the interface. Well, there's removexattr(), but who's counting? I believe people use the other *at() interfaces without ever looking back at the old calls and that there is little point in io_uring reproducing all of the old baggage. > If you look at the userspace interface in liburing, you can see the > following function signature: > > static inline void io_uring_prep_fgetxattr(struct io_uring_sqe *sqe, > int fd, > const char *name, > const char *value, > size_t len) > > This is very similar to what you proposed. Even though these functions desperately need updating, and as super nice as it would be, I don't expect you to implement getxattrat() and setxattrat(). If I were to name a single thing that would most increase the usability of these functions, it would be: Make the fXetxattr() functions (at least the io_uring versions) work with an O_PATH descriptor. That one thing would at least provide most of the desired functionality at the cost of an extra openat() call. > > > > >> Patch 1: fs: make user_path_at_empty() take a struct filename > >> The user_path_at_empty filename parameter has been changed > >> from a const char user pointer to a filename struct. io_uring > >> operates on filenames. > >> In addition also the functions that call user_path_at_empty > >> in namei.c and stat.c have been modified for this change. > >> > >> Patch 2: fs: split off setxattr_setup function from setxattr > >> Split off the setup part of the setxattr function > >> > >> Patch 3: fs: split off the vfs_getxattr from getxattr > >> Split of the vfs_getxattr part from getxattr. This will > >> allow to invoke it from io_uring. > >> > >> Patch 4: io_uring: add fsetxattr and setxattr support > >> This adds new functions to support the fsetxattr and setxattr > >> functions. > >> > >> Patch 5: io_uring: add fgetxattr and getxattr support > >> This adds new functions to support the fgetxattr and getxattr > >> functions. > >> > >> > >> There are two additional patches: > >> liburing: Add support for xattr api's. > >> This also includes the tests for the new code. > >> xfstests: Add support for io_uring xattr support. > >> > >> > >> Stefan Roesch (5): > >> fs: make user_path_at_empty() take a struct filename > >> fs: split off setxattr_setup function from setxattr > >> fs: split off the vfs_getxattr from getxattr > >> io_uring: add fsetxattr and setxattr support > >> io_uring: add fgetxattr and getxattr support > >> > >> fs/internal.h | 23 +++ > >> fs/io_uring.c | 325 ++++++++++++++++++++++++++++++++++ > >> fs/namei.c | 5 +- > >> fs/stat.c | 7 +- > >> fs/xattr.c | 114 +++++++----- > >> include/linux/namei.h | 4 +- > >> include/uapi/linux/io_uring.h | 8 +- > >> 7 files changed, 439 insertions(+), 47 deletions(-) > >> > >> > >> Signed-off-by: Stefan Roesch <shr@fb.com> > >> base-commit: c2626d30f312afc341158e07bf088f5a23b4eeeb > >> -- > >> 2.30.2
Hi Stefan, > On 11/29/21 5:08 PM, Clay Harris wrote: >> On Mon, Nov 29 2021 at 14:12:52 -0800, Stefan Roesch quoth thus: >> >>> This adds the xattr support to io_uring. The intent is to have a more >>> complete support for file operations in io_uring. >>> >>> This change adds support for the following functions to io_uring: >>> - fgetxattr >>> - fsetxattr >>> - getxattr >>> - setxattr >> >> You may wish to consider the following. >> >> Patching for these functions makes for an excellent opportunity >> to provide a better interface. Rather than implement fXetattr >> at all, you could enable io_uring to use functions like: >> >> int Xetxattr(int dfd, const char *path, const char *name, >> [const] void *value, size_t size, int flags); >> >> Not only does this simplify the io_uring interface down to two >> functions, but modernizes and fixes a deficit in usability. >> In terms of io_uring, this is just changing internal interfaces. >> >> Although unnecessary for io_uring, it would be nice to at least >> consider what parts of this code could be leveraged for future >> Xetxattr2 syscalls. > > Clay, > > while we can reduce the number of calls to 2, providing 4 calls will > ease the adoption of the interface. > > If you look at the userspace interface in liburing, you can see the > following function signature: > > static inline void io_uring_prep_fgetxattr(struct io_uring_sqe *sqe, > int fd, > const char *name, > const char *value, > size_t len) > > This is very similar to what you proposed. What's with lsetxattr and lgetxattr, why are they missing. I'd assume that even 6 helper functions in liburing would be able to use just 2 low level iouring opcodes. *listxattr is also missing, are there plans for them? metze
On Wed, Dec 01, 2021 at 01:46:21AM -0600, Clay Harris wrote: > On Tue, Nov 30 2021 at 22:07:47 -0800, Stefan Roesch quoth thus: > > > > > > > On 11/29/21 5:08 PM, Clay Harris wrote: > > > On Mon, Nov 29 2021 at 14:12:52 -0800, Stefan Roesch quoth thus: > > > > > >> This adds the xattr support to io_uring. The intent is to have a more > > >> complete support for file operations in io_uring. > > >> > > >> This change adds support for the following functions to io_uring: > > >> - fgetxattr > > >> - fsetxattr > > >> - getxattr > > >> - setxattr > > > > > > You may wish to consider the following. > > > > > > Patching for these functions makes for an excellent opportunity > > > to provide a better interface. Rather than implement fXetattr > > > at all, you could enable io_uring to use functions like: > > > > > > int Xetxattr(int dfd, const char *path, const char *name, > > > [const] void *value, size_t size, int flags); > > > > > > Not only does this simplify the io_uring interface down to two > > > functions, but modernizes and fixes a deficit in usability. > > > In terms of io_uring, this is just changing internal interfaces. > > > > > > Although unnecessary for io_uring, it would be nice to at least > > > consider what parts of this code could be leveraged for future > > > Xetxattr2 syscalls. > > > > I may have become a little over-excited when I saw someone was thinking > about new code associated with these interfaces. It's just that, to be > very kind, the existing interfaces have so much room for improvement. > I'm aware that changes in this area can be a non-trivial amount of > work, due to specific xattr keys being handled by different security > module hooks. > > > Clay, > > > > while we can reduce the number of calls to 2, providing 4 calls will > > ease the adoption of the interface. > > Well, there's removexattr(), but who's counting? > I believe people use the other *at() interfaces without ever looking > back at the old calls and that there is little point in io_uring reproducing > all of the old baggage. > > > If you look at the userspace interface in liburing, you can see the > > following function signature: > > > > static inline void io_uring_prep_fgetxattr(struct io_uring_sqe *sqe, > > int fd, > > const char *name, > > const char *value, > > size_t len) > > > > This is very similar to what you proposed. > > Even though these functions desperately need updating, and as super nice This code could use some serious cleanup as it is super hard to follow right now imho. It often gives the impression of forming loops when following callchains down into the filesystem. None of this is by design of course. I just happened to grow that way, I guess. However, for maintenance this is quite painful. I also don't like that the relationship between xattr and acls and the .set_acl inode methods is rather opaque in the code. I have a vague plan to cleanup some of that since I had to mess with this code not too long ago. But that'll be a bigger chunk of work. Christian
On Wed, Dec 01 2021 at 13:19:03 +0100, Stefan Metzmacher quoth thus: > Hi Stefan, > > > On 11/29/21 5:08 PM, Clay Harris wrote: > >> On Mon, Nov 29 2021 at 14:12:52 -0800, Stefan Roesch quoth thus: > >> > >>> This adds the xattr support to io_uring. The intent is to have a more > >>> complete support for file operations in io_uring. > >>> > >>> This change adds support for the following functions to io_uring: > >>> - fgetxattr > >>> - fsetxattr > >>> - getxattr > >>> - setxattr > >> > >> You may wish to consider the following. > >> > >> Patching for these functions makes for an excellent opportunity > >> to provide a better interface. Rather than implement fXetattr > >> at all, you could enable io_uring to use functions like: > >> > >> int Xetxattr(int dfd, const char *path, const char *name, > >> [const] void *value, size_t size, int flags); > >> > >> Not only does this simplify the io_uring interface down to two > >> functions, but modernizes and fixes a deficit in usability. > >> In terms of io_uring, this is just changing internal interfaces. > >> > >> Although unnecessary for io_uring, it would be nice to at least > >> consider what parts of this code could be leveraged for future > >> Xetxattr2 syscalls. > > > > Clay, > > > > while we can reduce the number of calls to 2, providing 4 calls will > > ease the adoption of the interface. > > > > If you look at the userspace interface in liburing, you can see the > > following function signature: > > > > static inline void io_uring_prep_fgetxattr(struct io_uring_sqe *sqe, > > int fd, > > const char *name, > > const char *value, > > size_t len) > > > > This is very similar to what you proposed. > > What's with lsetxattr and lgetxattr, why are they missing. Do any filesystems even support xattrs on symbolic links? > I'd assume that even 6 helper functions in liburing would be able > to use just 2 low level iouring opcodes. > > *listxattr is also missing, are there plans for them? > > metze
On Dec 1, 2021, at 12:52 PM, Clay Harris <bugs@claycon.org> wrote: > > On Wed, Dec 01 2021 at 13:19:03 +0100, Stefan Metzmacher quoth thus: >> >> What's with lsetxattr and lgetxattr, why are they missing. > Do any filesystems even support xattrs on symbolic links? Yes, ext4 does. There are definitely security xattrs on symlinks that need to be accessed and changed. Cheers, Andreas
Hi Stefan, On 12/1/21 4:19 AM, Stefan Metzmacher wrote: > Hi Stefan, > >> On 11/29/21 5:08 PM, Clay Harris wrote: >>> On Mon, Nov 29 2021 at 14:12:52 -0800, Stefan Roesch quoth thus: >>> >>>> This adds the xattr support to io_uring. The intent is to have a more >>>> complete support for file operations in io_uring. >>>> >>>> This change adds support for the following functions to io_uring: >>>> - fgetxattr >>>> - fsetxattr >>>> - getxattr >>>> - setxattr >>> >>> You may wish to consider the following. >>> >>> Patching for these functions makes for an excellent opportunity >>> to provide a better interface. Rather than implement fXetattr >>> at all, you could enable io_uring to use functions like: >>> >>> int Xetxattr(int dfd, const char *path, const char *name, >>> [const] void *value, size_t size, int flags); >>> >>> Not only does this simplify the io_uring interface down to two >>> functions, but modernizes and fixes a deficit in usability. >>> In terms of io_uring, this is just changing internal interfaces. >>> >>> Although unnecessary for io_uring, it would be nice to at least >>> consider what parts of this code could be leveraged for future >>> Xetxattr2 syscalls. >> >> Clay, >> >> while we can reduce the number of calls to 2, providing 4 calls will >> ease the adoption of the interface. >> >> If you look at the userspace interface in liburing, you can see the >> following function signature: >> >> static inline void io_uring_prep_fgetxattr(struct io_uring_sqe *sqe, >> int fd, >> const char *name, >> const char *value, >> size_t len) >> >> This is very similar to what you proposed. > > What's with lsetxattr and lgetxattr, why are they missing. > > I'd assume that even 6 helper functions in liburing would be able > to use just 2 low level iouring opcodes. > I'm open to also adding lsetxattr and lgetxattr. Do you have a use case in mind? > *listxattr is also missing, are there plans for them? > *listxattr is currently not planned. > metze >
This adds the xattr support to io_uring. The intent is to have a more complete support for file operations in io_uring. This change adds support for the following functions to io_uring: - fgetxattr - fsetxattr - getxattr - setxattr Patch 1: fs: make user_path_at_empty() take a struct filename The user_path_at_empty filename parameter has been changed from a const char user pointer to a filename struct. io_uring operates on filenames. In addition also the functions that call user_path_at_empty in namei.c and stat.c have been modified for this change. Patch 2: fs: split off setxattr_setup function from setxattr Split off the setup part of the setxattr function Patch 3: fs: split off the vfs_getxattr from getxattr Split of the vfs_getxattr part from getxattr. This will allow to invoke it from io_uring. Patch 4: io_uring: add fsetxattr and setxattr support This adds new functions to support the fsetxattr and setxattr functions. Patch 5: io_uring: add fgetxattr and getxattr support This adds new functions to support the fgetxattr and getxattr functions. There are two additional patches: liburing: Add support for xattr api's. This also includes the tests for the new code. xfstests: Add support for io_uring xattr support. Stefan Roesch (5): fs: make user_path_at_empty() take a struct filename fs: split off setxattr_setup function from setxattr fs: split off the vfs_getxattr from getxattr io_uring: add fsetxattr and setxattr support io_uring: add fgetxattr and getxattr support fs/internal.h | 23 +++ fs/io_uring.c | 325 ++++++++++++++++++++++++++++++++++ fs/namei.c | 5 +- fs/stat.c | 7 +- fs/xattr.c | 114 +++++++----- include/linux/namei.h | 4 +- include/uapi/linux/io_uring.h | 8 +- 7 files changed, 439 insertions(+), 47 deletions(-) Signed-off-by: Stefan Roesch <shr@fb.com> base-commit: c2626d30f312afc341158e07bf088f5a23b4eeeb