Re: [lustre-devel] [PATCH] posix acls: Move namespace conversion into filesystem / xattr handlers
> On 2016/05/23, 15:06, "James Simmons" wrote:
> >
> >> Currently, getxattr() and setxattr() check for the xattr names
> >> "system.posix_acl_{access,default}" and perform in-place UID / GID
> >> namespace mappings in the xattr values. Filesystems then again check for
> >> the same xattr names to handle those attributes, almost always using the
> >> standard posix_acl_{access,default}_xattr_handler handlers. This is
> >> unnecessary overhead; move the namespace conversion into the xattr
> >> handlers instead.
> >>
> >> For filesystems that use the POSIX ACL xattr handlers, no change is
> >> required. Filesystems that don't use those handlers (cifs and lustre)
> >> need to take care of the namespace mapping themselves now.
> >>
> >> The only user left of the posix_acl_fix_xattr_{from,to}_user helpers is
> >> lustre, so this patch moves them into lustre.
> >>
> >> Signed-off-by: Andreas Gruenbacher
> >> ---
> >>
> >> I'm reasonably confident about the core and cifs changes in this patch.
> >> The lustre code is pretty weird though, so could I please get a careful
> >> review on the changes there?
> >
> >Nak on the Lustre changes. POSIX ACLs are also handled in mdc_request.c.
> >Besides POSIX ACLs lustre has created a extended ACL as well that is
> >grouped in with posix ACL handling. This extended ACL is like POSIX acls
> >except it also contains the state (deleted, modified, ...) of the ACL.
> >Besides normal local access control list handling Lustre manages remote
> >access control list handling. You can read about this handling is in
> >llite_rmtacl.c. This code was written long before I became involved with
> >lustre so the exact details are fuzzy to me.
>
> James,
> the remote ACL code never found any usage in the field and can be
> deleted. Please see http://review.whamcloud.com/19789 for details.
That is a huge cleanup which will make Greg very happy. The tools and test
are going to be updated so the landing has to be just right so we
don't have a flood of test failures. Gruenbacher with the 19789 patch
Dilger pointed out Lustre's POSIX ACL code just becomes ordinary which
means we can use the default POSIS xattr handler. You wouldn't have to
keep posix_acl.c around with these changes.
> Cheers, Andreas
>
> > The guts of this are handled is located at:
> >
> >drivers/staging/lustre/lustre/obdclass/acl.c
> >
> >P.S
> >
> >A you probably have noticed Lustre has had an uptick in development
> >which means the code is now changing all the time in staging. How should
> >we handle the changes you are working in your own trees verses what is
> >happing in staging. For example I'm looking at moving lustre to the
> >xattr_handles. Should I push it to you and wait until it works it way
> >into Greg's tree. What do the merge schedules look like. Lastly the
> >a_refcount for the POSIX acl changed with your xattr updates which now
> >causes lustre to crash :-(
> >
> >> Thanks,
> >> Andreas
> >>
> >> drivers/staging/lustre/lustre/llite/Makefile | 1 +
> >> .../staging/lustre/lustre/llite/llite_internal.h | 3 ++
> >> drivers/staging/lustre/lustre/llite/posix_acl.c| 62
> >> ++
> >> drivers/staging/lustre/lustre/llite/xattr.c| 8 ++-
> >> fs/cifs/cifssmb.c | 41 ++
> >> fs/posix_acl.c | 62
> >> +-
> >> fs/xattr.c | 7 ---
> >> include/linux/posix_acl_xattr.h| 12 -
> >> 8 files changed, 107 insertions(+), 89 deletions(-)
> >> create mode 100644 drivers/staging/lustre/lustre/llite/posix_acl.c
> >>
> >> diff --git a/drivers/staging/lustre/lustre/llite/Makefile
> >> b/drivers/staging/lustre/lustre/llite/Makefile
> >> index 2ce10ff..67125f8 100644
> >> --- a/drivers/staging/lustre/lustre/llite/Makefile
> >> +++ b/drivers/staging/lustre/lustre/llite/Makefile
> >> @@ -7,5 +7,6 @@ lustre-y := dcache.o dir.o file.o llite_close.o
> >> llite_lib.o llite_nfs.o \
> >>glimpse.o lcommon_cl.o lcommon_misc.o \
> >>vvp_dev.o vvp_page.o vvp_lock.o vvp_io.o vvp_object.o vvp_req.o \
> >>lproc_llite.o
> >> +lustre-$(CONFIG_FS_POSIX_ACL) += posix_acl.o
> >>
> >> llite_lloop-y := lloop.o
> >> diff --git a/drivers/staging/lustre/lustre/llite/llite_internal.h
> >> b/drivers/staging/lustre/lustre/llite/llite_internal.h
> >> index ce1f949..d454dfb 100644
> >> --- a/drivers/staging/lustre/lustre/llite/llite_internal.h
> >> +++ b/drivers/staging/lustre/lustre/llite/llite_internal.h
> >> @@ -1402,4 +1402,7 @@ int cl_local_size(struct inode *inode);
> >> __u64 cl_fid_build_ino(const struct lu_fid *fid, int api32);
> >> __u32 cl_fid_build_gen(const struct lu_fid *fid);
> >>
> >> +void posix_acl_fix_xattr_from_user(void *value, size_t size);
> >> +void posix_acl_fix_xattr_to_user(void *value, size_t size);
> >> +
> >> #endif /* LLITE_INTERNAL_H */
> >> diff --git a/drivers/stag
Re: [lustre-devel] [PATCH] posix acls: Move namespace conversion into filesystem / xattr handlers
On Tue, May 24, 2016 at 10:35 PM, James Simmons wrote: > That is a huge cleanup which will make Greg very happy. The tools and test > are going to be updated so the landing has to be just right so we > don't have a flood of test failures. Gruenbacher with the 19789 patch > Dilger pointed out Lustre's POSIX ACL code just becomes ordinary which > means we can use the default POSIS xattr handler. You wouldn't have to > keep posix_acl.c around with these changes. Change http://review.whamcloud.com/19789 doesn't switch Lustre over to using xattr handlers. As long as Lustre doesn't use those (i.e., use posix_acl_access_xattr_handler and posix_acl_default_xattr_handler), it will still need the uid/gid namespace conversion code in ll_getxattr and ll_setxattr for this patch to be correct. The merge conflict between this patch and http://review.whamcloud.com/19789 is easily resolved though. Andreas ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [lustre-devel] [PATCH] posix acls: Move namespace conversion into filesystem / xattr handlers
On 2016/05/23, 15:06, "James Simmons" wrote:
>
>> Currently, getxattr() and setxattr() check for the xattr names
>> "system.posix_acl_{access,default}" and perform in-place UID / GID
>> namespace mappings in the xattr values. Filesystems then again check for
>> the same xattr names to handle those attributes, almost always using the
>> standard posix_acl_{access,default}_xattr_handler handlers. This is
>> unnecessary overhead; move the namespace conversion into the xattr
>> handlers instead.
>>
>> For filesystems that use the POSIX ACL xattr handlers, no change is
>> required. Filesystems that don't use those handlers (cifs and lustre)
>> need to take care of the namespace mapping themselves now.
>>
>> The only user left of the posix_acl_fix_xattr_{from,to}_user helpers is
>> lustre, so this patch moves them into lustre.
>>
>> Signed-off-by: Andreas Gruenbacher
>> ---
>>
>> I'm reasonably confident about the core and cifs changes in this patch.
>> The lustre code is pretty weird though, so could I please get a careful
>> review on the changes there?
>
>Nak on the Lustre changes. POSIX ACLs are also handled in mdc_request.c.
>Besides POSIX ACLs lustre has created a extended ACL as well that is
>grouped in with posix ACL handling. This extended ACL is like POSIX acls
>except it also contains the state (deleted, modified, ...) of the ACL.
>Besides normal local access control list handling Lustre manages remote
>access control list handling. You can read about this handling is in
>llite_rmtacl.c. This code was written long before I became involved with
>lustre so the exact details are fuzzy to me.
James,
the remote ACL code never found any usage in the field and can be
deleted. Please see http://review.whamcloud.com/19789 for details.
Cheers, Andreas
> The guts of this are handled is located at:
>
>drivers/staging/lustre/lustre/obdclass/acl.c
>
>P.S
>
>A you probably have noticed Lustre has had an uptick in development
>which means the code is now changing all the time in staging. How should
>we handle the changes you are working in your own trees verses what is
>happing in staging. For example I'm looking at moving lustre to the
>xattr_handles. Should I push it to you and wait until it works it way
>into Greg's tree. What do the merge schedules look like. Lastly the
>a_refcount for the POSIX acl changed with your xattr updates which now
>causes lustre to crash :-(
>
>> Thanks,
>> Andreas
>>
>> drivers/staging/lustre/lustre/llite/Makefile | 1 +
>> .../staging/lustre/lustre/llite/llite_internal.h | 3 ++
>> drivers/staging/lustre/lustre/llite/posix_acl.c| 62
>> ++
>> drivers/staging/lustre/lustre/llite/xattr.c| 8 ++-
>> fs/cifs/cifssmb.c | 41 ++
>> fs/posix_acl.c | 62
>> +-
>> fs/xattr.c | 7 ---
>> include/linux/posix_acl_xattr.h| 12 -
>> 8 files changed, 107 insertions(+), 89 deletions(-)
>> create mode 100644 drivers/staging/lustre/lustre/llite/posix_acl.c
>>
>> diff --git a/drivers/staging/lustre/lustre/llite/Makefile
>> b/drivers/staging/lustre/lustre/llite/Makefile
>> index 2ce10ff..67125f8 100644
>> --- a/drivers/staging/lustre/lustre/llite/Makefile
>> +++ b/drivers/staging/lustre/lustre/llite/Makefile
>> @@ -7,5 +7,6 @@ lustre-y := dcache.o dir.o file.o llite_close.o llite_lib.o
>> llite_nfs.o \
>> glimpse.o lcommon_cl.o lcommon_misc.o \
>> vvp_dev.o vvp_page.o vvp_lock.o vvp_io.o vvp_object.o vvp_req.o \
>> lproc_llite.o
>> +lustre-$(CONFIG_FS_POSIX_ACL) += posix_acl.o
>>
>> llite_lloop-y := lloop.o
>> diff --git a/drivers/staging/lustre/lustre/llite/llite_internal.h
>> b/drivers/staging/lustre/lustre/llite/llite_internal.h
>> index ce1f949..d454dfb 100644
>> --- a/drivers/staging/lustre/lustre/llite/llite_internal.h
>> +++ b/drivers/staging/lustre/lustre/llite/llite_internal.h
>> @@ -1402,4 +1402,7 @@ int cl_local_size(struct inode *inode);
>> __u64 cl_fid_build_ino(const struct lu_fid *fid, int api32);
>> __u32 cl_fid_build_gen(const struct lu_fid *fid);
>>
>> +void posix_acl_fix_xattr_from_user(void *value, size_t size);
>> +void posix_acl_fix_xattr_to_user(void *value, size_t size);
>> +
>> #endif /* LLITE_INTERNAL_H */
>> diff --git a/drivers/staging/lustre/lustre/llite/posix_acl.c
>> b/drivers/staging/lustre/lustre/llite/posix_acl.c
>> new file mode 100644
>> index 000..4fabd0f
>> --- /dev/null
>> +++ b/drivers/staging/lustre/lustre/llite/posix_acl.c
>> @@ -0,0 +1,62 @@
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +/*
>> + * Fix up the uids and gids in posix acl extended attributes in place.
>> + */
>> +static void posix_acl_fix_xattr_userns(
>> +struct user_namespace *to, struct user_namespace *from,
>> +void *value, size_t size)
>> +{
>> +posix_acl_xattr_header *header = (posix_acl_xa
Re: [lustre-devel] [PATCH] posix acls: Move namespace conversion into filesystem / xattr handlers
2016-05-23 23:06 GMT+02:00 James Simmons :
> Nak on the Lustre changes. POSIX ACLs are also handled in mdc_request.c.
> Besides POSIX ACLs lustre has created a extended ACL as well that is
> grouped in with posix ACL handling. This extended ACL is like POSIX acls
> except it also contains the state (deleted, modified, ...) of the ACL.
> Besides normal local access control list handling Lustre manages remote
> access control list handling. You can read about this handling is in
> llite_rmtacl.c. This code was written long before I became involved with
> lustre so the exact details are fuzzy to me. The guts of this are handled
> is located at:
>
> drivers/staging/lustre/lustre/obdclass/acl.c
I'm not sure we're talking about the same thing here. As far as I can
see, the only code path from getxattr(2) and settxattr(2) into lustre
is through ll_getxattr and ll_setxattr. So far, the vfs performed the
uid/gid mappings before calling into the filesystem. With this patch,
the filesystem performs this mapping instead. In the lustre case,
pretty much directly after entering ll_getxattr and immediately before
leaving ll_setxattr. The rest of the code has and still does operate
in the "init uid/gid namespace". Did I miss anything?
> A you probably have noticed Lustre has had an uptick in development
> which means the code is now changing all the time in staging. How should
> we handle the changes you are working in your own trees verses what is
> happing in staging. For example I'm looking at moving lustre to the
> xattr_handles.
Okay, great.
> Should I push it to you and wait until it works it way
> into Greg's tree.
Changing lustre to use xattr handlers does not dependent on any of my
pending xattr changes, only the other way around. Please copy me for
review and merge through Greg's tree as usual. Once I have your
changes on a public branch, I can merge that into my xattr inode
operation removal branch.
> What do the merge schedules look like.
I hope the {get,set,remove}xattr inode operations will go away in the
next merge window, so it would be good to have the lustre xattr
handler convestion on a public branch relatively soon if possible.
> Lastly the a_refcount for the POSIX acl changed with your xattr updates which
> now
> causes lustre to crash :-(
You mean commit b8a7a3a6 "posix_acl: Inode acl caching fixes"? It
seems a "forget_cached_acl(inode, type);" before returning the acl in
ll_get_acl is missing -- but lustre still shouldn't crash because of
that.
Thanks,
Andreas
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [lustre-devel] [PATCH] posix acls: Move namespace conversion into filesystem / xattr handlers
> Currently, getxattr() and setxattr() check for the xattr names
> "system.posix_acl_{access,default}" and perform in-place UID / GID
> namespace mappings in the xattr values. Filesystems then again check for
> the same xattr names to handle those attributes, almost always using the
> standard posix_acl_{access,default}_xattr_handler handlers. This is
> unnecessary overhead; move the namespace conversion into the xattr
> handlers instead.
>
> For filesystems that use the POSIX ACL xattr handlers, no change is
> required. Filesystems that don't use those handlers (cifs and lustre)
> need to take care of the namespace mapping themselves now.
>
> The only user left of the posix_acl_fix_xattr_{from,to}_user helpers is
> lustre, so this patch moves them into lustre.
>
> Signed-off-by: Andreas Gruenbacher
> ---
>
> I'm reasonably confident about the core and cifs changes in this patch.
> The lustre code is pretty weird though, so could I please get a careful
> review on the changes there?
Nak on the Lustre changes. POSIX ACLs are also handled in mdc_request.c.
Besides POSIX ACLs lustre has created a extended ACL as well that is
grouped in with posix ACL handling. This extended ACL is like POSIX acls
except it also contains the state (deleted, modified, ...) of the ACL.
Besides normal local access control list handling Lustre manages remote
access control list handling. You can read about this handling is in
llite_rmtacl.c. This code was written long before I became involved with
lustre so the exact details are fuzzy to me. The guts of this are handled
is located at:
drivers/staging/lustre/lustre/obdclass/acl.c
P.S
A you probably have noticed Lustre has had an uptick in development
which means the code is now changing all the time in staging. How should
we handle the changes you are working in your own trees verses what is
happing in staging. For example I'm looking at moving lustre to the
xattr_handles. Should I push it to you and wait until it works it way
into Greg's tree. What do the merge schedules look like. Lastly the
a_refcount for the POSIX acl changed with your xattr updates which now
causes lustre to crash :-(
> Thanks,
> Andreas
>
> drivers/staging/lustre/lustre/llite/Makefile | 1 +
> .../staging/lustre/lustre/llite/llite_internal.h | 3 ++
> drivers/staging/lustre/lustre/llite/posix_acl.c| 62
> ++
> drivers/staging/lustre/lustre/llite/xattr.c| 8 ++-
> fs/cifs/cifssmb.c | 41 ++
> fs/posix_acl.c | 62
> +-
> fs/xattr.c | 7 ---
> include/linux/posix_acl_xattr.h| 12 -
> 8 files changed, 107 insertions(+), 89 deletions(-)
> create mode 100644 drivers/staging/lustre/lustre/llite/posix_acl.c
>
> diff --git a/drivers/staging/lustre/lustre/llite/Makefile
> b/drivers/staging/lustre/lustre/llite/Makefile
> index 2ce10ff..67125f8 100644
> --- a/drivers/staging/lustre/lustre/llite/Makefile
> +++ b/drivers/staging/lustre/lustre/llite/Makefile
> @@ -7,5 +7,6 @@ lustre-y := dcache.o dir.o file.o llite_close.o llite_lib.o
> llite_nfs.o \
> glimpse.o lcommon_cl.o lcommon_misc.o \
> vvp_dev.o vvp_page.o vvp_lock.o vvp_io.o vvp_object.o vvp_req.o \
> lproc_llite.o
> +lustre-$(CONFIG_FS_POSIX_ACL) += posix_acl.o
>
> llite_lloop-y := lloop.o
> diff --git a/drivers/staging/lustre/lustre/llite/llite_internal.h
> b/drivers/staging/lustre/lustre/llite/llite_internal.h
> index ce1f949..d454dfb 100644
> --- a/drivers/staging/lustre/lustre/llite/llite_internal.h
> +++ b/drivers/staging/lustre/lustre/llite/llite_internal.h
> @@ -1402,4 +1402,7 @@ int cl_local_size(struct inode *inode);
> __u64 cl_fid_build_ino(const struct lu_fid *fid, int api32);
> __u32 cl_fid_build_gen(const struct lu_fid *fid);
>
> +void posix_acl_fix_xattr_from_user(void *value, size_t size);
> +void posix_acl_fix_xattr_to_user(void *value, size_t size);
> +
> #endif /* LLITE_INTERNAL_H */
> diff --git a/drivers/staging/lustre/lustre/llite/posix_acl.c
> b/drivers/staging/lustre/lustre/llite/posix_acl.c
> new file mode 100644
> index 000..4fabd0f
> --- /dev/null
> +++ b/drivers/staging/lustre/lustre/llite/posix_acl.c
> @@ -0,0 +1,62 @@
> +#include
> +#include
> +#include
> +#include
> +
> +/*
> + * Fix up the uids and gids in posix acl extended attributes in place.
> + */
> +static void posix_acl_fix_xattr_userns(
> + struct user_namespace *to, struct user_namespace *from,
> + void *value, size_t size)
> +{
> + posix_acl_xattr_header *header = (posix_acl_xattr_header *)value;
> + posix_acl_xattr_entry *entry = (posix_acl_xattr_entry *)(header+1),
> *end;
> + int count;
> + kuid_t uid;
> + kgid_t gid;
> +
> + if (!value)
> + return;
> + if (size < sizeof(posix_acl_xattr_header))
> + return;
> + if (header->a
