Fixed the bugs in the set_acl patch pointed out by Dan Carpenter.
Rebased the next patch 'remove unused parameter..." on the parent
patch. Added newer xattr fixes that were recently pushed.
Andrew Perepechko (1):
staging: lustre: mdc: excessive memory consumption by the xattr cache
Dmitry Eremin (1):
staging: lustre: llite: add support set_acl method in inode operations
Fan Yong (1):
staging: lustre: acl: increase ACL entries limitation
John L. Hammond (2):
staging: lustre: llite: remove unused parameters from md_{get,set}xattr()
staging: lustre: mdc: use large xattr buffers for old servers
.../lustre/include/uapi/linux/lustre/lustre_idl.h | 2 +-
drivers/staging/lustre/lustre/include/lustre_acl.h | 7 ++-
drivers/staging/lustre/lustre/include/obd.h | 7 +--
drivers/staging/lustre/lustre/include/obd_class.h | 21 +++----
drivers/staging/lustre/lustre/llite/file.c | 65 +++++++++++++++++++++-
.../staging/lustre/lustre/llite/llite_internal.h | 4 ++
drivers/staging/lustre/lustre/llite/llite_lib.c | 3 +-
drivers/staging/lustre/lustre/llite/namei.c | 10 +++-
drivers/staging/lustre/lustre/llite/xattr.c | 6 +-
drivers/staging/lustre/lustre/lmv/lmv_obd.c | 22 ++++----
drivers/staging/lustre/lustre/mdc/mdc_locks.c | 42 +++++++++++---
drivers/staging/lustre/lustre/mdc/mdc_reint.c | 2 +
drivers/staging/lustre/lustre/mdc/mdc_request.c | 38 ++++++++-----
drivers/staging/lustre/lustre/ptlrpc/layout.c | 4 +-
drivers/staging/lustre/lustre/ptlrpc/wiretest.c | 4 +-
15 files changed, 171 insertions(+), 66 deletions(-)
--
1.8.3.1
From: Andrew Perepechko <[email protected]>
The refill operation of the xattr cache does not know the
reply size in advance, so it makes a guess based on
the maxeasize value returned by the MDS.
In practice, it allocates 16 KiB for the common case and
4 MiB for the large xattr case. However, a typical reply
is just a few hundred bytes.
If we follow the conservative approach, we can prepare a
single memory page for the reply. It is large enough for
any reasonable xattr set and, at the same time, it does
not require multiple page memory reclaim, which can be
costly.
If, for a specific file, the reply is larger than a single
page, the client is prepared to handle that and will fall back
to non-cached xattr code. Indeed, if this happens often and
xattrs are often used to store large values, it makes sense to
disable the xattr cache at all since it wasn't designed for
such [mis]use.
Signed-off-by: Andrew Perepechko <[email protected]>
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-9417
Reviewed-on: https://review.whamcloud.com/26887
Reviewed-by: Fan Yong <[email protected]>
Reviewed-by: Ben Evans <[email protected]>
Reviewed-by: Oleg Drokin <[email protected]>
Signed-off-by: James Simmons <[email protected]>
---
drivers/staging/lustre/lustre/mdc/mdc_locks.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/lustre/lustre/mdc/mdc_locks.c b/drivers/staging/lustre/lustre/mdc/mdc_locks.c
index 65a5341..a8aa0fa 100644
--- a/drivers/staging/lustre/lustre/mdc/mdc_locks.c
+++ b/drivers/staging/lustre/lustre/mdc/mdc_locks.c
@@ -315,6 +315,10 @@ static void mdc_realloc_openmsg(struct ptlrpc_request *req,
return req;
}
+#define GA_DEFAULT_EA_NAME_LEN 20
+#define GA_DEFAULT_EA_VAL_LEN 250
+#define GA_DEFAULT_EA_NUM 10
+
static struct ptlrpc_request *
mdc_intent_getxattr_pack(struct obd_export *exp,
struct lookup_intent *it,
@@ -323,7 +327,6 @@ static void mdc_realloc_openmsg(struct ptlrpc_request *req,
struct ptlrpc_request *req;
struct ldlm_intent *lit;
int rc, count = 0;
- u32 maxdata;
LIST_HEAD(cancels);
req = ptlrpc_request_alloc(class_exp2cliimp(exp),
@@ -341,20 +344,20 @@ static void mdc_realloc_openmsg(struct ptlrpc_request *req,
lit = req_capsule_client_get(&req->rq_pill, &RMF_LDLM_INTENT);
lit->opc = IT_GETXATTR;
- maxdata = class_exp2cliimp(exp)->imp_connect_data.ocd_max_easize;
-
/* pack the intended request */
- mdc_pack_body(req, &op_data->op_fid1, op_data->op_valid, maxdata, -1,
- 0);
+ mdc_pack_body(req, &op_data->op_fid1, op_data->op_valid,
+ GA_DEFAULT_EA_NAME_LEN * GA_DEFAULT_EA_NUM, -1, 0);
- req_capsule_set_size(&req->rq_pill, &RMF_EADATA, RCL_SERVER, maxdata);
+ req_capsule_set_size(&req->rq_pill, &RMF_EADATA, RCL_SERVER,
+ GA_DEFAULT_EA_NAME_LEN * GA_DEFAULT_EA_NUM);
- req_capsule_set_size(&req->rq_pill, &RMF_EAVALS, RCL_SERVER, maxdata);
+ req_capsule_set_size(&req->rq_pill, &RMF_EAVALS, RCL_SERVER,
+ GA_DEFAULT_EA_NAME_LEN * GA_DEFAULT_EA_NUM);
- req_capsule_set_size(&req->rq_pill, &RMF_EAVALS_LENS,
- RCL_SERVER, maxdata);
+ req_capsule_set_size(&req->rq_pill, &RMF_EAVALS_LENS, RCL_SERVER,
+ sizeof(u32) * GA_DEFAULT_EA_NUM);
- req_capsule_set_size(&req->rq_pill, &RMF_ACL, RCL_SERVER, maxdata);
+ req_capsule_set_size(&req->rq_pill, &RMF_ACL, RCL_SERVER, 0);
ptlrpc_request_set_replen(req);
--
1.8.3.1
From: "John L. Hammond" <[email protected]>
Pre 2.10.1 MDTs will crash when they receive a listxattr (MDS_GETXATTR
with OBD_MD_FLXATTRLS) RPC for an orphan or dead object. So for
clients connected to these older MDTs, try to avoid sending listxattr
RPCs by making the bulk getxattr (MDS_GETXATTR with OBD_MD_FLXATTRALL)
more likely to succeed and thereby reducing the chances of falling
back to listxattr.
Signed-off-by: John L. Hammond <[email protected]>
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-10912
Reviewed-on: https://review.whamcloud.com/31990
Reviewed-by: Andreas Dilger <[email protected]>
Reviewed-by: Fan Yong <[email protected]>
Reviewed-by: Oleg Drokin <[email protected]>
Signed-off-by: James Simmons <[email protected]>
---
drivers/staging/lustre/lustre/mdc/mdc_locks.c | 31 +++++++++++++++++++++------
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/lustre/lustre/mdc/mdc_locks.c b/drivers/staging/lustre/lustre/mdc/mdc_locks.c
index a8aa0fa..b991c6f 100644
--- a/drivers/staging/lustre/lustre/mdc/mdc_locks.c
+++ b/drivers/staging/lustre/lustre/mdc/mdc_locks.c
@@ -326,8 +326,10 @@ static void mdc_realloc_openmsg(struct ptlrpc_request *req,
{
struct ptlrpc_request *req;
struct ldlm_intent *lit;
+ u32 min_buf_size = 0;
int rc, count = 0;
LIST_HEAD(cancels);
+ u32 buf_size = 0;
req = ptlrpc_request_alloc(class_exp2cliimp(exp),
&RQF_LDLM_INTENT_GETXATTR);
@@ -344,18 +346,33 @@ static void mdc_realloc_openmsg(struct ptlrpc_request *req,
lit = req_capsule_client_get(&req->rq_pill, &RMF_LDLM_INTENT);
lit->opc = IT_GETXATTR;
+#if LUSTRE_VERSION_CODE < OBD_OCD_VERSION(3, 0, 53, 0)
+ /* If the supplied buffer is too small then the server will
+ * return -ERANGE and llite will fallback to using non cached
+ * xattr operations. On servers before 2.10.1 a (non-cached)
+ * listxattr RPC for an orphan or dead file causes an oops. So
+ * let's try to avoid sending too small a buffer to too old a
+ * server. This is effectively undoing the memory conservation
+ * of LU-9417 when it would be *more* likely to crash the
+ * server. See LU-9856.
+ */
+ if (exp->exp_connect_data.ocd_version < OBD_OCD_VERSION(2, 10, 1, 0))
+ min_buf_size = exp->exp_connect_data.ocd_max_easize;
+#endif
+ buf_size = max_t(u32, min_buf_size,
+ GA_DEFAULT_EA_NAME_LEN * GA_DEFAULT_EA_NUM);
+
/* pack the intended request */
- mdc_pack_body(req, &op_data->op_fid1, op_data->op_valid,
- GA_DEFAULT_EA_NAME_LEN * GA_DEFAULT_EA_NUM, -1, 0);
+ mdc_pack_body(req, &op_data->op_fid1, op_data->op_valid, buf_size,
+ -1, 0);
- req_capsule_set_size(&req->rq_pill, &RMF_EADATA, RCL_SERVER,
- GA_DEFAULT_EA_NAME_LEN * GA_DEFAULT_EA_NUM);
+ req_capsule_set_size(&req->rq_pill, &RMF_EADATA, RCL_SERVER, buf_size);
- req_capsule_set_size(&req->rq_pill, &RMF_EAVALS, RCL_SERVER,
- GA_DEFAULT_EA_NAME_LEN * GA_DEFAULT_EA_NUM);
+ req_capsule_set_size(&req->rq_pill, &RMF_EAVALS, RCL_SERVER, buf_size);
req_capsule_set_size(&req->rq_pill, &RMF_EAVALS_LENS, RCL_SERVER,
- sizeof(u32) * GA_DEFAULT_EA_NUM);
+ max_t(u32, min_buf_size,
+ sizeof(u32) * GA_DEFAULT_EA_NUM));
req_capsule_set_size(&req->rq_pill, &RMF_ACL, RCL_SERVER, 0);
--
1.8.3.1
From: "John L. Hammond" <[email protected]>
md_getxattr() and md_setxattr() each have several unused
parameters. Remove them and improve the naming or remaining
parameters.
Signed-off-by: John L. Hammond <[email protected]>
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-10792
Reviewed-on: https://review.whamcloud.com/
Reviewed-by: Dmitry Eremin <[email protected]>
Reviewed-by: James Simmons <[email protected]>
Signed-off-by: James Simmons <[email protected]>
---
Changelog:
v1) Initial patch ported to staging tree
v2) Rebased on fixed parent patch
drivers/staging/lustre/lustre/include/obd.h | 7 ++---
drivers/staging/lustre/lustre/include/obd_class.h | 21 ++++++--------
drivers/staging/lustre/lustre/llite/file.c | 5 ++--
drivers/staging/lustre/lustre/llite/xattr.c | 6 ++--
drivers/staging/lustre/lustre/lmv/lmv_obd.c | 22 +++++++--------
drivers/staging/lustre/lustre/mdc/mdc_request.c | 34 +++++++++++++----------
6 files changed, 46 insertions(+), 49 deletions(-)
diff --git a/drivers/staging/lustre/lustre/include/obd.h b/drivers/staging/lustre/lustre/include/obd.h
index fe21987..a69564d 100644
--- a/drivers/staging/lustre/lustre/include/obd.h
+++ b/drivers/staging/lustre/lustre/include/obd.h
@@ -940,12 +940,11 @@ struct md_ops {
struct ptlrpc_request **);
int (*setxattr)(struct obd_export *, const struct lu_fid *,
- u64, const char *, const char *, int, int, int, __u32,
- struct ptlrpc_request **);
+ u64, const char *, const void *, size_t, unsigned int,
+ u32, struct ptlrpc_request **);
int (*getxattr)(struct obd_export *, const struct lu_fid *,
- u64, const char *, const char *, int, int, int,
- struct ptlrpc_request **);
+ u64, const char *, size_t, struct ptlrpc_request **);
int (*init_ea_size)(struct obd_export *, u32, u32);
diff --git a/drivers/staging/lustre/lustre/include/obd_class.h b/drivers/staging/lustre/lustre/include/obd_class.h
index a76f016..0081578 100644
--- a/drivers/staging/lustre/lustre/include/obd_class.h
+++ b/drivers/staging/lustre/lustre/include/obd_class.h
@@ -1385,29 +1385,26 @@ static inline int md_merge_attr(struct obd_export *exp,
}
static inline int md_setxattr(struct obd_export *exp, const struct lu_fid *fid,
- u64 valid, const char *name,
- const char *input, int input_size,
- int output_size, int flags, __u32 suppgid,
+ u64 obd_md_valid, const char *name,
+ const char *value, size_t value_size,
+ unsigned int xattr_flags, u32 suppgid,
struct ptlrpc_request **request)
{
EXP_CHECK_MD_OP(exp, setxattr);
EXP_MD_COUNTER_INCREMENT(exp, setxattr);
- return MDP(exp->exp_obd, setxattr)(exp, fid, valid, name, input,
- input_size, output_size, flags,
+ return MDP(exp->exp_obd, setxattr)(exp, fid, obd_md_valid, name,
+ value, value_size, xattr_flags,
suppgid, request);
}
static inline int md_getxattr(struct obd_export *exp, const struct lu_fid *fid,
- u64 valid, const char *name,
- const char *input, int input_size,
- int output_size, int flags,
- struct ptlrpc_request **request)
+ u64 obd_md_valid, const char *name,
+ size_t buf_size, struct ptlrpc_request **req)
{
EXP_CHECK_MD_OP(exp, getxattr);
EXP_MD_COUNTER_INCREMENT(exp, getxattr);
- return MDP(exp->exp_obd, getxattr)(exp, fid, valid, name, input,
- input_size, output_size, flags,
- request);
+ return MDP(exp->exp_obd, getxattr)(exp, fid, obd_md_valid, name,
+ buf_size, req);
}
static inline int md_set_open_replay_data(struct obd_export *exp,
diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c
index 64a5698..de30df2 100644
--- a/drivers/staging/lustre/lustre/llite/file.c
+++ b/drivers/staging/lustre/lustre/llite/file.c
@@ -3088,7 +3088,7 @@ int ll_set_acl(struct inode *inode, struct posix_acl *acl, int type)
rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
value ? OBD_MD_FLXATTR : OBD_MD_FLXATTRRM,
- name, value, value_size, 0, 0, 0, &req);
+ name, value, value_size, 0, 0, &req);
ptlrpc_req_finished(req);
out_value:
@@ -3400,8 +3400,7 @@ static int ll_layout_fetch(struct inode *inode, struct ldlm_lock *lock)
rc = ll_get_default_mdsize(sbi, &lmmsize);
if (rc == 0)
rc = md_getxattr(sbi->ll_md_exp, ll_inode2fid(inode),
- OBD_MD_FLXATTR, XATTR_NAME_LOV, NULL, 0,
- lmmsize, 0, &req);
+ OBD_MD_FLXATTR, XATTR_NAME_LOV, lmmsize, &req);
if (rc < 0)
return rc;
diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
index 1a597a6..7fa0a41 100644
--- a/drivers/staging/lustre/lustre/llite/xattr.c
+++ b/drivers/staging/lustre/lustre/llite/xattr.c
@@ -145,7 +145,7 @@ static int ll_xattr_set_common(const struct xattr_handler *handler,
return -ENOMEM;
rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode), valid, fullname,
- pv, size, 0, flags, ll_i2suppgid(inode), &req);
+ pv, size, flags, ll_i2suppgid(inode), &req);
kfree(fullname);
if (rc) {
if (rc == -EOPNOTSUPP && handler->flags == XATTR_USER_T) {
@@ -344,8 +344,8 @@ int ll_xattr_list(struct inode *inode, const char *name, int type, void *buffer,
}
} else {
getxattr_nocache:
- rc = md_getxattr(sbi->ll_md_exp, ll_inode2fid(inode),
- valid, name, NULL, 0, size, 0, &req);
+ rc = md_getxattr(sbi->ll_md_exp, ll_inode2fid(inode), valid,
+ name, size, &req);
if (rc < 0)
goto out_xattr;
diff --git a/drivers/staging/lustre/lustre/lmv/lmv_obd.c b/drivers/staging/lustre/lustre/lmv/lmv_obd.c
index 7198a63..1ec42e2 100644
--- a/drivers/staging/lustre/lustre/lmv/lmv_obd.c
+++ b/drivers/staging/lustre/lustre/lmv/lmv_obd.c
@@ -1398,9 +1398,8 @@ static int lmv_getstatus(struct obd_export *exp,
}
static int lmv_getxattr(struct obd_export *exp, const struct lu_fid *fid,
- u64 valid, const char *name,
- const char *input, int input_size, int output_size,
- int flags, struct ptlrpc_request **request)
+ u64 obd_md_valid, const char *name, size_t buf_size,
+ struct ptlrpc_request **req)
{
struct obd_device *obd = exp->exp_obd;
struct lmv_obd *lmv = &obd->u.lmv;
@@ -1410,15 +1409,15 @@ static int lmv_getxattr(struct obd_export *exp, const struct lu_fid *fid,
if (IS_ERR(tgt))
return PTR_ERR(tgt);
- return md_getxattr(tgt->ltd_exp, fid, valid, name, input,
- input_size, output_size, flags, request);
+ return md_getxattr(tgt->ltd_exp, fid, obd_md_valid, name, buf_size,
+ req);
}
static int lmv_setxattr(struct obd_export *exp, const struct lu_fid *fid,
- u64 valid, const char *name,
- const char *input, int input_size, int output_size,
- int flags, __u32 suppgid,
- struct ptlrpc_request **request)
+ u64 obd_md_valid, const char *name,
+ const void *value, size_t value_size,
+ unsigned int xattr_flags, u32 suppgid,
+ struct ptlrpc_request **req)
{
struct obd_device *obd = exp->exp_obd;
struct lmv_obd *lmv = &obd->u.lmv;
@@ -1428,9 +1427,8 @@ static int lmv_setxattr(struct obd_export *exp, const struct lu_fid *fid,
if (IS_ERR(tgt))
return PTR_ERR(tgt);
- return md_setxattr(tgt->ltd_exp, fid, valid, name, input,
- input_size, output_size, flags, suppgid,
- request);
+ return md_setxattr(tgt->ltd_exp, fid, obd_md_valid, name,
+ value, value_size, xattr_flags, suppgid, req);
}
static int lmv_getattr(struct obd_export *exp, struct md_op_data *op_data,
diff --git a/drivers/staging/lustre/lustre/mdc/mdc_request.c b/drivers/staging/lustre/lustre/mdc/mdc_request.c
index 7d577bf..a90647a 100644
--- a/drivers/staging/lustre/lustre/mdc/mdc_request.c
+++ b/drivers/staging/lustre/lustre/mdc/mdc_request.c
@@ -347,26 +347,30 @@ static int mdc_xattr_common(struct obd_export *exp,
}
static int mdc_setxattr(struct obd_export *exp, const struct lu_fid *fid,
- u64 valid, const char *xattr_name,
- const char *input, int input_size, int output_size,
- int flags, __u32 suppgid,
- struct ptlrpc_request **request)
+ u64 obd_md_valid, const char *name,
+ const void *value, size_t value_size,
+ unsigned int xattr_flags, u32 suppgid,
+ struct ptlrpc_request **req)
{
+ LASSERT(obd_md_valid == OBD_MD_FLXATTR ||
+ obd_md_valid == OBD_MD_FLXATTRRM);
+
return mdc_xattr_common(exp, &RQF_MDS_REINT_SETXATTR,
- fid, MDS_REINT, valid, xattr_name,
- input, input_size, output_size, flags,
- suppgid, request);
+ fid, MDS_REINT, obd_md_valid, name,
+ value, value_size, 0, xattr_flags, suppgid,
+ req);
}
static int mdc_getxattr(struct obd_export *exp, const struct lu_fid *fid,
- u64 valid, const char *xattr_name,
- const char *input, int input_size, int output_size,
- int flags, struct ptlrpc_request **request)
-{
- return mdc_xattr_common(exp, &RQF_MDS_GETXATTR,
- fid, MDS_GETXATTR, valid, xattr_name,
- input, input_size, output_size, flags,
- -1, request);
+ u64 obd_md_valid, const char *name, size_t buf_size,
+ struct ptlrpc_request **req)
+{
+ LASSERT(obd_md_valid == OBD_MD_FLXATTR ||
+ obd_md_valid == OBD_MD_FLXATTRLS);
+
+ return mdc_xattr_common(exp, &RQF_MDS_GETXATTR, fid, MDS_GETXATTR,
+ obd_md_valid, name, NULL, 0, buf_size, 0, -1,
+ req);
}
#ifdef CONFIG_FS_POSIX_ACL
--
1.8.3.1
From: Fan Yong <[email protected]>
Originally, the limitation of ACL entries is 32, that is not
enough for some use cases. In fact, restricting ACL entries
count is mainly for preparing the RPC reply buffer to receive
the ACL data. So we cannot make the ACL entries count to be
unlimited. But we can enlarge the RPC reply buffer to hold
more ACL entries. On the other hand, MDT backend filesystem
has its own EA size limitation. For example, for ldiskfs case,
if large EA enable, then the max ACL size is 1048492 bytes;
otherwise, it is 4012 bytes. For ZFS backend, such value is
32768 bytes. With such hard limitation, we can calculate how
many ACL entries we can have at most. This patch increases
the RPC reply buffer to match such hard limitation. For old
client, to avoid buffer overflow because of large ACL data
(more than 32 ACL entries), the MDT will forbid the old client
to access the file with large ACL data. As for how to know
whether it is old client or new, a new connection flag
OBD_CONNECT_LARGE_ACL is used for that.
Signed-off-by: Fan Yong <[email protected]>
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-7473
Reviewed-on: https://review.whamcloud.com/19790
Reviewed-by: Andreas Dilger <[email protected]>
Reviewed-by: Li Xi <[email protected]>
Reviewed-by: Lai Siyao <[email protected]>
Reviewed-by: Oleg Drokin <[email protected]>
Signed-off-by: James Simmons <[email protected]>
---
drivers/staging/lustre/include/uapi/linux/lustre/lustre_idl.h | 2 +-
drivers/staging/lustre/lustre/include/lustre_acl.h | 7 ++++++-
drivers/staging/lustre/lustre/llite/llite_lib.c | 3 ++-
drivers/staging/lustre/lustre/mdc/mdc_locks.c | 6 ++++++
drivers/staging/lustre/lustre/mdc/mdc_reint.c | 2 ++
drivers/staging/lustre/lustre/mdc/mdc_request.c | 4 ++++
drivers/staging/lustre/lustre/ptlrpc/layout.c | 4 +---
drivers/staging/lustre/lustre/ptlrpc/wiretest.c | 4 ++--
8 files changed, 24 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/lustre/include/uapi/linux/lustre/lustre_idl.h b/drivers/staging/lustre/include/uapi/linux/lustre/lustre_idl.h
index aac98db..8778c6f 100644
--- a/drivers/staging/lustre/include/uapi/linux/lustre/lustre_idl.h
+++ b/drivers/staging/lustre/include/uapi/linux/lustre/lustre_idl.h
@@ -615,7 +615,7 @@ struct ptlrpc_body_v2 {
#define OBD_CONNECT_REQPORTAL 0x40ULL /*Separate non-IO req portal */
#define OBD_CONNECT_ACL 0x80ULL /*access control lists */
#define OBD_CONNECT_XATTR 0x100ULL /*client use extended attr */
-#define OBD_CONNECT_CROW 0x200ULL /*MDS+OST create obj on write*/
+#define OBD_CONNECT_LARGE_ACL 0x200ULL /* more than 32 ACL entries */
#define OBD_CONNECT_TRUNCLOCK 0x400ULL /*locks on server for punch */
#define OBD_CONNECT_TRANSNO 0x800ULL /*replay sends init transno */
#define OBD_CONNECT_IBITS 0x1000ULL /*support for inodebits locks*/
diff --git a/drivers/staging/lustre/lustre/include/lustre_acl.h b/drivers/staging/lustre/lustre/include/lustre_acl.h
index 35ff61c..e7575a1 100644
--- a/drivers/staging/lustre/lustre/include/lustre_acl.h
+++ b/drivers/staging/lustre/lustre/include/lustre_acl.h
@@ -36,11 +36,16 @@
#include <linux/fs.h>
#include <linux/dcache.h>
+#ifdef CONFIG_FS_POSIX_ACL
#include <linux/posix_acl_xattr.h>
#define LUSTRE_POSIX_ACL_MAX_ENTRIES 32
-#define LUSTRE_POSIX_ACL_MAX_SIZE \
+#define LUSTRE_POSIX_ACL_MAX_SIZE_OLD \
(sizeof(struct posix_acl_xattr_header) + \
LUSTRE_POSIX_ACL_MAX_ENTRIES * sizeof(struct posix_acl_xattr_entry))
+#else /* ! CONFIG_FS_POSIX_ACL */
+#define LUSTRE_POSIX_ACL_MAX_SIZE_OLD 0
+#endif /* CONFIG_FS_POSIX_ACL */
+
#endif
diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c b/drivers/staging/lustre/lustre/llite/llite_lib.c
index 83eb2da..b5c287b 100644
--- a/drivers/staging/lustre/lustre/llite/llite_lib.c
+++ b/drivers/staging/lustre/lustre/llite/llite_lib.c
@@ -198,7 +198,8 @@ static int client_common_fill_super(struct super_block *sb, char *md, char *dt)
if (sbi->ll_flags & LL_SBI_LRU_RESIZE)
data->ocd_connect_flags |= OBD_CONNECT_LRU_RESIZE;
#ifdef CONFIG_FS_POSIX_ACL
- data->ocd_connect_flags |= OBD_CONNECT_ACL | OBD_CONNECT_UMASK;
+ data->ocd_connect_flags |= OBD_CONNECT_ACL | OBD_CONNECT_UMASK |
+ OBD_CONNECT_LARGE_ACL;
#endif
if (OBD_FAIL_CHECK(OBD_FAIL_MDC_LIGHTWEIGHT))
diff --git a/drivers/staging/lustre/lustre/mdc/mdc_locks.c b/drivers/staging/lustre/lustre/mdc/mdc_locks.c
index 253a545..65a5341 100644
--- a/drivers/staging/lustre/lustre/mdc/mdc_locks.c
+++ b/drivers/staging/lustre/lustre/mdc/mdc_locks.c
@@ -308,6 +308,8 @@ static void mdc_realloc_openmsg(struct ptlrpc_request *req,
req_capsule_set_size(&req->rq_pill, &RMF_MDT_MD, RCL_SERVER,
obddev->u.cli.cl_max_mds_easize);
+ req_capsule_set_size(&req->rq_pill, &RMF_ACL, RCL_SERVER,
+ req->rq_import->imp_connect_data.ocd_max_easize);
ptlrpc_request_set_replen(req);
return req;
@@ -352,6 +354,8 @@ static void mdc_realloc_openmsg(struct ptlrpc_request *req,
req_capsule_set_size(&req->rq_pill, &RMF_EAVALS_LENS,
RCL_SERVER, maxdata);
+ req_capsule_set_size(&req->rq_pill, &RMF_ACL, RCL_SERVER, maxdata);
+
ptlrpc_request_set_replen(req);
return req;
@@ -433,6 +437,8 @@ static struct ptlrpc_request *mdc_intent_getattr_pack(struct obd_export *exp,
mdc_getattr_pack(req, valid, it->it_flags, op_data, easize);
req_capsule_set_size(&req->rq_pill, &RMF_MDT_MD, RCL_SERVER, easize);
+ req_capsule_set_size(&req->rq_pill, &RMF_ACL, RCL_SERVER,
+ req->rq_import->imp_connect_data.ocd_max_easize);
ptlrpc_request_set_replen(req);
return req;
}
diff --git a/drivers/staging/lustre/lustre/mdc/mdc_reint.c b/drivers/staging/lustre/lustre/mdc/mdc_reint.c
index 488b980..e1e26cb 100644
--- a/drivers/staging/lustre/lustre/mdc/mdc_reint.c
+++ b/drivers/staging/lustre/lustre/mdc/mdc_reint.c
@@ -134,6 +134,8 @@ int mdc_setattr(struct obd_export *exp, struct md_op_data *op_data,
LTIME_S(op_data->op_attr.ia_ctime));
mdc_setattr_pack(req, op_data, ea, ealen);
+ req_capsule_set_size(&req->rq_pill, &RMF_ACL, RCL_SERVER,
+ req->rq_import->imp_connect_data.ocd_max_easize);
ptlrpc_request_set_replen(req);
rc = mdc_reint(req, LUSTRE_IMP_FULL);
diff --git a/drivers/staging/lustre/lustre/mdc/mdc_request.c b/drivers/staging/lustre/lustre/mdc/mdc_request.c
index a90647a..77cb699 100644
--- a/drivers/staging/lustre/lustre/mdc/mdc_request.c
+++ b/drivers/staging/lustre/lustre/mdc/mdc_request.c
@@ -181,6 +181,8 @@ static int mdc_getattr(struct obd_export *exp, struct md_op_data *op_data,
mdc_pack_body(req, &op_data->op_fid1, op_data->op_valid,
op_data->op_mode, -1, 0);
+ req_capsule_set_size(&req->rq_pill, &RMF_ACL, RCL_SERVER,
+ req->rq_import->imp_connect_data.ocd_max_easize);
req_capsule_set_size(&req->rq_pill, &RMF_MDT_MD, RCL_SERVER,
op_data->op_mode);
ptlrpc_request_set_replen(req);
@@ -227,6 +229,8 @@ static int mdc_getattr_name(struct obd_export *exp, struct md_op_data *op_data,
req_capsule_set_size(&req->rq_pill, &RMF_MDT_MD, RCL_SERVER,
op_data->op_mode);
+ req_capsule_set_size(&req->rq_pill, &RMF_ACL, RCL_SERVER,
+ req->rq_import->imp_connect_data.ocd_max_easize);
ptlrpc_request_set_replen(req);
rc = mdc_getattr_common(exp, req);
diff --git a/drivers/staging/lustre/lustre/ptlrpc/layout.c b/drivers/staging/lustre/lustre/ptlrpc/layout.c
index 2855f38..417d4a1 100644
--- a/drivers/staging/lustre/lustre/ptlrpc/layout.c
+++ b/drivers/staging/lustre/lustre/ptlrpc/layout.c
@@ -992,9 +992,7 @@ struct req_msg_field RMF_EADATA = DEFINE_MSGF("eadata", 0, -1,
struct req_msg_field RMF_EAVALS = DEFINE_MSGF("eavals", 0, -1, NULL, NULL);
EXPORT_SYMBOL(RMF_EAVALS);
-struct req_msg_field RMF_ACL =
- DEFINE_MSGF("acl", RMF_F_NO_SIZE_CHECK,
- LUSTRE_POSIX_ACL_MAX_SIZE, NULL, NULL);
+struct req_msg_field RMF_ACL = DEFINE_MSGF("acl", 0, -1, NULL, NULL);
EXPORT_SYMBOL(RMF_ACL);
/* FIXME: this should be made to use RMF_F_STRUCT_ARRAY */
diff --git a/drivers/staging/lustre/lustre/ptlrpc/wiretest.c b/drivers/staging/lustre/lustre/ptlrpc/wiretest.c
index 2f64eb4..f9394c3 100644
--- a/drivers/staging/lustre/lustre/ptlrpc/wiretest.c
+++ b/drivers/staging/lustre/lustre/ptlrpc/wiretest.c
@@ -1010,8 +1010,8 @@ void lustre_assert_wire_constants(void)
OBD_CONNECT_ACL);
LASSERTF(OBD_CONNECT_XATTR == 0x100ULL, "found 0x%.16llxULL\n",
OBD_CONNECT_XATTR);
- LASSERTF(OBD_CONNECT_CROW == 0x200ULL, "found 0x%.16llxULL\n",
- OBD_CONNECT_CROW);
+ LASSERTF(OBD_CONNECT_LARGE_ACL == 0x200ULL, "found 0x%.16llxULL\n",
+ OBD_CONNECT_LARGE_ACL);
LASSERTF(OBD_CONNECT_TRUNCLOCK == 0x400ULL, "found 0x%.16llxULL\n",
OBD_CONNECT_TRUNCLOCK);
LASSERTF(OBD_CONNECT_TRANSNO == 0x800ULL, "found 0x%.16llxULL\n",
--
1.8.3.1
From: Dmitry Eremin <[email protected]>
Linux kernel v3.14 adds set_acl method to inode operations.
This patch adds support to Lustre for proper acl management.
Signed-off-by: Dmitry Eremin <[email protected]>
Signed-off-by: John L. Hammond <[email protected]>
Signed-off-by: James Simmons <[email protected]>
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-9183
Reviewed-on: https://review.whamcloud.com/25965
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-10541
Reviewed-on: https://review.whamcloud.com/31588
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-10926
Reviewed-on: https://review.whamcloud.com/32045
Reviewed-by: Bob Glossman <[email protected]>
Reviewed-by: James Simmons <[email protected]>
Reviewed-by: Andreas Dilger <[email protected]>
Reviewed-by: Dmitry Eremin <[email protected]>
Reviewed-by: Oleg Drokin <[email protected]>
Signed-off-by: James Simmons <[email protected]>
---
Changelog:
v1) Initial patch ported to staging tree
v2) Fixed up goto handling and avoid BUG() when calling
forget_cached_acl()with invalid type as pointed out by Dan Carpenter
drivers/staging/lustre/lustre/llite/file.c | 62 ++++++++++++++++++++++
.../staging/lustre/lustre/llite/llite_internal.h | 4 ++
drivers/staging/lustre/lustre/llite/namei.c | 10 +++-
3 files changed, 74 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c
index 0026fde..64a5698 100644
--- a/drivers/staging/lustre/lustre/llite/file.c
+++ b/drivers/staging/lustre/lustre/llite/file.c
@@ -3030,6 +3030,7 @@ static int ll_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
return rc;
}
+#ifdef CONFIG_FS_POSIX_ACL
struct posix_acl *ll_get_acl(struct inode *inode, int type)
{
struct ll_inode_info *lli = ll_i2info(inode);
@@ -3043,6 +3044,64 @@ struct posix_acl *ll_get_acl(struct inode *inode, int type)
return acl;
}
+int ll_set_acl(struct inode *inode, struct posix_acl *acl, int type)
+{
+ struct ll_sb_info *sbi = ll_i2sbi(inode);
+ struct ptlrpc_request *req = NULL;
+ const char *name = NULL;
+ size_t value_size = 0;
+ char *value = NULL;
+ int rc;
+
+ switch (type) {
+ case ACL_TYPE_ACCESS:
+ name = XATTR_NAME_POSIX_ACL_ACCESS;
+ if (acl)
+ rc = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+ break;
+
+ case ACL_TYPE_DEFAULT:
+ name = XATTR_NAME_POSIX_ACL_DEFAULT;
+ if (!S_ISDIR(inode->i_mode))
+ rc = acl ? -EACCES : 0;
+ break;
+
+ default:
+ rc = -EINVAL;
+ break;
+ }
+ if (rc)
+ return rc;
+
+ if (acl) {
+ value_size = posix_acl_xattr_size(acl->a_count);
+ value = kmalloc(value_size, GFP_NOFS);
+ if (!value) {
+ rc = -ENOMEM;
+ goto out;
+ }
+
+ rc = posix_acl_to_xattr(&init_user_ns, acl, value, value_size);
+ if (rc < 0)
+ goto out_value;
+ }
+
+ rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
+ value ? OBD_MD_FLXATTR : OBD_MD_FLXATTRRM,
+ name, value, value_size, 0, 0, 0, &req);
+
+ ptlrpc_req_finished(req);
+out_value:
+ kfree(value);
+out:
+ if (rc)
+ forget_cached_acl(inode, type);
+ else
+ set_cached_acl(inode, type, acl);
+ return rc;
+}
+#endif /* CONFIG_FS_POSIX_ACL */
+
int ll_inode_permission(struct inode *inode, int mask)
{
struct ll_sb_info *sbi;
@@ -3164,7 +3223,10 @@ int ll_inode_permission(struct inode *inode, int mask)
.permission = ll_inode_permission,
.listxattr = ll_listxattr,
.fiemap = ll_fiemap,
+#ifdef CONFIG_FS_POSIX_ACL
.get_acl = ll_get_acl,
+ .set_acl = ll_set_acl,
+#endif
};
/* dynamic ioctl number support routines */
diff --git a/drivers/staging/lustre/lustre/llite/llite_internal.h b/drivers/staging/lustre/lustre/llite/llite_internal.h
index 6504850..2280327 100644
--- a/drivers/staging/lustre/lustre/llite/llite_internal.h
+++ b/drivers/staging/lustre/lustre/llite/llite_internal.h
@@ -754,7 +754,11 @@ enum ldlm_mode ll_take_md_lock(struct inode *inode, __u64 bits,
int ll_md_real_close(struct inode *inode, fmode_t fmode);
int ll_getattr(const struct path *path, struct kstat *stat,
u32 request_mask, unsigned int flags);
+#ifdef CONFIG_FS_POSIX_ACL
struct posix_acl *ll_get_acl(struct inode *inode, int type);
+int ll_set_acl(struct inode *inode, struct posix_acl *acl, int type);
+#endif /* CONFIG_FS_POSIX_ACL */
+
int ll_migrate(struct inode *parent, struct file *file, int mdtidx,
const char *name, int namelen);
int ll_get_fid_by_name(struct inode *parent, const char *name,
diff --git a/drivers/staging/lustre/lustre/llite/namei.c b/drivers/staging/lustre/lustre/llite/namei.c
index 9ac7f09..b41f189 100644
--- a/drivers/staging/lustre/lustre/llite/namei.c
+++ b/drivers/staging/lustre/lustre/llite/namei.c
@@ -1195,7 +1195,10 @@ static int ll_rename(struct inode *src, struct dentry *src_dchild,
.getattr = ll_getattr,
.permission = ll_inode_permission,
.listxattr = ll_listxattr,
- .get_acl = ll_get_acl,
+#ifdef CONFIG_FS_POSIX_ACL
+ .get_acl = ll_get_acl,
+ .set_acl = ll_set_acl,
+#endif
};
const struct inode_operations ll_special_inode_operations = {
@@ -1203,5 +1206,8 @@ static int ll_rename(struct inode *src, struct dentry *src_dchild,
.getattr = ll_getattr,
.permission = ll_inode_permission,
.listxattr = ll_listxattr,
- .get_acl = ll_get_acl,
+#ifdef CONFIG_FS_POSIX_ACL
+ .get_acl = ll_get_acl,
+ .set_acl = ll_set_acl,
+#endif
};
--
1.8.3.1
On Mon, May 14 2018, James Simmons wrote:
> From: Dmitry Eremin <[email protected]>
>
> Linux kernel v3.14 adds set_acl method to inode operations.
> This patch adds support to Lustre for proper acl management.
>
> Signed-off-by: Dmitry Eremin <[email protected]>
> Signed-off-by: John L. Hammond <[email protected]>
> Signed-off-by: James Simmons <[email protected]>
> Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-9183
> Reviewed-on: https://review.whamcloud.com/25965
> Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-10541
> Reviewed-on: https://review.whamcloud.com/31588
> Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-10926
> Reviewed-on: https://review.whamcloud.com/32045
> Reviewed-by: Bob Glossman <[email protected]>
> Reviewed-by: James Simmons <[email protected]>
> Reviewed-by: Andreas Dilger <[email protected]>
> Reviewed-by: Dmitry Eremin <[email protected]>
> Reviewed-by: Oleg Drokin <[email protected]>
> Signed-off-by: James Simmons <[email protected]>
> ---
> Changelog:
>
> v1) Initial patch ported to staging tree
> v2) Fixed up goto handling and avoid BUG() when calling
> forget_cached_acl()with invalid type as pointed out by Dan Carpenter
>
> drivers/staging/lustre/lustre/llite/file.c | 62 ++++++++++++++++++++++
> .../staging/lustre/lustre/llite/llite_internal.h | 4 ++
> drivers/staging/lustre/lustre/llite/namei.c | 10 +++-
> 3 files changed, 74 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c
> index 0026fde..64a5698 100644
> --- a/drivers/staging/lustre/lustre/llite/file.c
> +++ b/drivers/staging/lustre/lustre/llite/file.c
> @@ -3030,6 +3030,7 @@ static int ll_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> return rc;
> }
>
> +#ifdef CONFIG_FS_POSIX_ACL
Using #ifdef in .c files is generally discouraged.
The "standard" approach here is:
- put the acl code in a separate file (acl.c)
- optionally include it via the Make file
lustre-$(CONFIG_FS_POSIX_ACL) += acl.o
- in the header where ll_get_acl and ll_set_acl are declared have
#ifdef CONFIG_FS_POSIX_ACL
declare the functions
#else
#define ll_get_acl NULL
#define ll_set_acl NULL
#endif
Now as this is staging and that is (presumably) an upstream patch
lightly improved it is probably fine to include the patch as-is,
but in that case we will probably want to fix it up later.
Thanks,
NeilBrown
> struct posix_acl *ll_get_acl(struct inode *inode, int type)
> {
> struct ll_inode_info *lli = ll_i2info(inode);
> @@ -3043,6 +3044,64 @@ struct posix_acl *ll_get_acl(struct inode *inode, int type)
> return acl;
> }
>
> +int ll_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> +{
> + struct ll_sb_info *sbi = ll_i2sbi(inode);
> + struct ptlrpc_request *req = NULL;
> + const char *name = NULL;
> + size_t value_size = 0;
> + char *value = NULL;
> + int rc;
> +
> + switch (type) {
> + case ACL_TYPE_ACCESS:
> + name = XATTR_NAME_POSIX_ACL_ACCESS;
> + if (acl)
> + rc = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> + break;
> +
> + case ACL_TYPE_DEFAULT:
> + name = XATTR_NAME_POSIX_ACL_DEFAULT;
> + if (!S_ISDIR(inode->i_mode))
> + rc = acl ? -EACCES : 0;
> + break;
> +
> + default:
> + rc = -EINVAL;
> + break;
> + }
> + if (rc)
> + return rc;
> +
> + if (acl) {
> + value_size = posix_acl_xattr_size(acl->a_count);
> + value = kmalloc(value_size, GFP_NOFS);
> + if (!value) {
> + rc = -ENOMEM;
> + goto out;
> + }
> +
> + rc = posix_acl_to_xattr(&init_user_ns, acl, value, value_size);
> + if (rc < 0)
> + goto out_value;
> + }
> +
> + rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
> + value ? OBD_MD_FLXATTR : OBD_MD_FLXATTRRM,
> + name, value, value_size, 0, 0, 0, &req);
> +
> + ptlrpc_req_finished(req);
> +out_value:
> + kfree(value);
> +out:
> + if (rc)
> + forget_cached_acl(inode, type);
> + else
> + set_cached_acl(inode, type, acl);
> + return rc;
> +}
> +#endif /* CONFIG_FS_POSIX_ACL */
> +
> int ll_inode_permission(struct inode *inode, int mask)
> {
> struct ll_sb_info *sbi;
> @@ -3164,7 +3223,10 @@ int ll_inode_permission(struct inode *inode, int mask)
> .permission = ll_inode_permission,
> .listxattr = ll_listxattr,
> .fiemap = ll_fiemap,
> +#ifdef CONFIG_FS_POSIX_ACL
> .get_acl = ll_get_acl,
> + .set_acl = ll_set_acl,
> +#endif
> };
>
> /* dynamic ioctl number support routines */
> diff --git a/drivers/staging/lustre/lustre/llite/llite_internal.h b/drivers/staging/lustre/lustre/llite/llite_internal.h
> index 6504850..2280327 100644
> --- a/drivers/staging/lustre/lustre/llite/llite_internal.h
> +++ b/drivers/staging/lustre/lustre/llite/llite_internal.h
> @@ -754,7 +754,11 @@ enum ldlm_mode ll_take_md_lock(struct inode *inode, __u64 bits,
> int ll_md_real_close(struct inode *inode, fmode_t fmode);
> int ll_getattr(const struct path *path, struct kstat *stat,
> u32 request_mask, unsigned int flags);
> +#ifdef CONFIG_FS_POSIX_ACL
> struct posix_acl *ll_get_acl(struct inode *inode, int type);
> +int ll_set_acl(struct inode *inode, struct posix_acl *acl, int type);
> +#endif /* CONFIG_FS_POSIX_ACL */
> +
> int ll_migrate(struct inode *parent, struct file *file, int mdtidx,
> const char *name, int namelen);
> int ll_get_fid_by_name(struct inode *parent, const char *name,
> diff --git a/drivers/staging/lustre/lustre/llite/namei.c b/drivers/staging/lustre/lustre/llite/namei.c
> index 9ac7f09..b41f189 100644
> --- a/drivers/staging/lustre/lustre/llite/namei.c
> +++ b/drivers/staging/lustre/lustre/llite/namei.c
> @@ -1195,7 +1195,10 @@ static int ll_rename(struct inode *src, struct dentry *src_dchild,
> .getattr = ll_getattr,
> .permission = ll_inode_permission,
> .listxattr = ll_listxattr,
> - .get_acl = ll_get_acl,
> +#ifdef CONFIG_FS_POSIX_ACL
> + .get_acl = ll_get_acl,
> + .set_acl = ll_set_acl,
> +#endif
> };
>
> const struct inode_operations ll_special_inode_operations = {
> @@ -1203,5 +1206,8 @@ static int ll_rename(struct inode *src, struct dentry *src_dchild,
> .getattr = ll_getattr,
> .permission = ll_inode_permission,
> .listxattr = ll_listxattr,
> - .get_acl = ll_get_acl,
> +#ifdef CONFIG_FS_POSIX_ACL
> + .get_acl = ll_get_acl,
> + .set_acl = ll_set_acl,
> +#endif
> };
> --
> 1.8.3.1
On Tue, May 15, 2018 at 01:53:02PM +1000, NeilBrown wrote:
> On Mon, May 14 2018, James Simmons wrote:
>
> > From: Dmitry Eremin <[email protected]>
> >
> > Linux kernel v3.14 adds set_acl method to inode operations.
> > This patch adds support to Lustre for proper acl management.
> >
> > Signed-off-by: Dmitry Eremin <[email protected]>
> > Signed-off-by: John L. Hammond <[email protected]>
> > Signed-off-by: James Simmons <[email protected]>
> > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-9183
> > Reviewed-on: https://review.whamcloud.com/25965
> > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-10541
> > Reviewed-on: https://review.whamcloud.com/31588
> > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-10926
> > Reviewed-on: https://review.whamcloud.com/32045
> > Reviewed-by: Bob Glossman <[email protected]>
> > Reviewed-by: James Simmons <[email protected]>
> > Reviewed-by: Andreas Dilger <[email protected]>
> > Reviewed-by: Dmitry Eremin <[email protected]>
> > Reviewed-by: Oleg Drokin <[email protected]>
> > Signed-off-by: James Simmons <[email protected]>
> > ---
> > Changelog:
> >
> > v1) Initial patch ported to staging tree
> > v2) Fixed up goto handling and avoid BUG() when calling
> > forget_cached_acl()with invalid type as pointed out by Dan Carpenter
> >
> > drivers/staging/lustre/lustre/llite/file.c | 62 ++++++++++++++++++++++
> > .../staging/lustre/lustre/llite/llite_internal.h | 4 ++
> > drivers/staging/lustre/lustre/llite/namei.c | 10 +++-
> > 3 files changed, 74 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c
> > index 0026fde..64a5698 100644
> > --- a/drivers/staging/lustre/lustre/llite/file.c
> > +++ b/drivers/staging/lustre/lustre/llite/file.c
> > @@ -3030,6 +3030,7 @@ static int ll_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> > return rc;
> > }
> >
> > +#ifdef CONFIG_FS_POSIX_ACL
>
> Using #ifdef in .c files is generally discouraged.
> The "standard" approach here is:
> - put the acl code in a separate file (acl.c)
> - optionally include it via the Make file
> lustre-$(CONFIG_FS_POSIX_ACL) += acl.o
>
> - in the header where ll_get_acl and ll_set_acl are declared have
> #ifdef CONFIG_FS_POSIX_ACL
> declare the functions
> #else
> #define ll_get_acl NULL
> #define ll_set_acl NULL
> #endif
>
> Now as this is staging and that is (presumably) an upstream patch
> lightly improved it is probably fine to include the patch as-is,
> but in that case we will probably want to fix it up later.
Let's get it right the first time if at all possible please.
I'll drop this series from my queue and wait for the next version of it.
thanks,
greg k-h
On Mon, May 14, 2018 at 10:16:59PM -0400, James Simmons wrote:
> +#ifdef CONFIG_FS_POSIX_ACL
> struct posix_acl *ll_get_acl(struct inode *inode, int type)
> {
> struct ll_inode_info *lli = ll_i2info(inode);
> @@ -3043,6 +3044,64 @@ struct posix_acl *ll_get_acl(struct inode *inode, int type)
> return acl;
> }
>
> +int ll_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> +{
> + struct ll_sb_info *sbi = ll_i2sbi(inode);
> + struct ptlrpc_request *req = NULL;
> + const char *name = NULL;
> + size_t value_size = 0;
> + char *value = NULL;
> + int rc;
"rc" needs to be initialized to zero. It's disapppointing that GCC
doesn't catch this.
> +
> + switch (type) {
> + case ACL_TYPE_ACCESS:
> + name = XATTR_NAME_POSIX_ACL_ACCESS;
> + if (acl)
> + rc = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> + break;
> +
> + case ACL_TYPE_DEFAULT:
> + name = XATTR_NAME_POSIX_ACL_DEFAULT;
> + if (!S_ISDIR(inode->i_mode))
> + rc = acl ? -EACCES : 0;
> + break;
> +
> + default:
> + rc = -EINVAL;
> + break;
> + }
> + if (rc)
> + return rc;
Otherwise rc can be uninitialized here.
regards,
dan carpenter
> On Mon, May 14, 2018 at 10:16:59PM -0400, James Simmons wrote:
> > +#ifdef CONFIG_FS_POSIX_ACL
> > struct posix_acl *ll_get_acl(struct inode *inode, int type)
> > {
> > struct ll_inode_info *lli = ll_i2info(inode);
> > @@ -3043,6 +3044,64 @@ struct posix_acl *ll_get_acl(struct inode *inode, int type)
> > return acl;
> > }
> >
> > +int ll_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> > +{
> > + struct ll_sb_info *sbi = ll_i2sbi(inode);
> > + struct ptlrpc_request *req = NULL;
> > + const char *name = NULL;
> > + size_t value_size = 0;
> > + char *value = NULL;
> > + int rc;
>
> "rc" needs to be initialized to zero. It's disapppointing that GCC
> doesn't catch this.
Thanks Dan. Will fix.
> > +
> > + switch (type) {
> > + case ACL_TYPE_ACCESS:
> > + name = XATTR_NAME_POSIX_ACL_ACCESS;
> > + if (acl)
> > + rc = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> > + break;
> > +
> > + case ACL_TYPE_DEFAULT:
> > + name = XATTR_NAME_POSIX_ACL_DEFAULT;
> > + if (!S_ISDIR(inode->i_mode))
> > + rc = acl ? -EACCES : 0;
> > + break;
> > +
> > + default:
> > + rc = -EINVAL;
> > + break;
> > + }
> > + if (rc)
> > + return rc;
>
> Otherwise rc can be uninitialized here.
>
> regards,
> dan carpenter
>
>
> On Mon, May 14 2018, James Simmons wrote:
>
> > From: Dmitry Eremin <[email protected]>
> >
> > Linux kernel v3.14 adds set_acl method to inode operations.
> > This patch adds support to Lustre for proper acl management.
> >
> > Signed-off-by: Dmitry Eremin <[email protected]>
> > Signed-off-by: John L. Hammond <[email protected]>
> > Signed-off-by: James Simmons <[email protected]>
> > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-9183
> > Reviewed-on: https://review.whamcloud.com/25965
> > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-10541
> > Reviewed-on: https://review.whamcloud.com/31588
> > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-10926
> > Reviewed-on: https://review.whamcloud.com/32045
> > Reviewed-by: Bob Glossman <[email protected]>
> > Reviewed-by: James Simmons <[email protected]>
> > Reviewed-by: Andreas Dilger <[email protected]>
> > Reviewed-by: Dmitry Eremin <[email protected]>
> > Reviewed-by: Oleg Drokin <[email protected]>
> > Signed-off-by: James Simmons <[email protected]>
> > ---
> > Changelog:
> >
> > v1) Initial patch ported to staging tree
> > v2) Fixed up goto handling and avoid BUG() when calling
> > forget_cached_acl()with invalid type as pointed out by Dan Carpenter
> >
> > drivers/staging/lustre/lustre/llite/file.c | 62 ++++++++++++++++++++++
> > .../staging/lustre/lustre/llite/llite_internal.h | 4 ++
> > drivers/staging/lustre/lustre/llite/namei.c | 10 +++-
> > 3 files changed, 74 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c
> > index 0026fde..64a5698 100644
> > --- a/drivers/staging/lustre/lustre/llite/file.c
> > +++ b/drivers/staging/lustre/lustre/llite/file.c
> > @@ -3030,6 +3030,7 @@ static int ll_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> > return rc;
> > }
> >
> > +#ifdef CONFIG_FS_POSIX_ACL
>
> Using #ifdef in .c files is generally discouraged.
> The "standard" approach here is:
> - put the acl code in a separate file (acl.c)
> - optionally include it via the Make file
> lustre-$(CONFIG_FS_POSIX_ACL) += acl.o
>
> - in the header where ll_get_acl and ll_set_acl are declared have
> #ifdef CONFIG_FS_POSIX_ACL
> declare the functions
> #else
> #define ll_get_acl NULL
> #define ll_set_acl NULL
> #endif
>
> Now as this is staging and that is (presumably) an upstream patch
> lightly improved it is probably fine to include the patch as-is,
> but in that case we will probably want to fix it up later.
So you want Lustre to be like everyone else :-)
Okay I will fix up and send a new patch series.
> Thanks,
> NeilBrown
>
> > struct posix_acl *ll_get_acl(struct inode *inode, int type)
> > {
> > struct ll_inode_info *lli = ll_i2info(inode);
> > @@ -3043,6 +3044,64 @@ struct posix_acl *ll_get_acl(struct inode *inode, int type)
> > return acl;
> > }
> >
> > +int ll_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> > +{
> > + struct ll_sb_info *sbi = ll_i2sbi(inode);
> > + struct ptlrpc_request *req = NULL;
> > + const char *name = NULL;
> > + size_t value_size = 0;
> > + char *value = NULL;
> > + int rc;
> > +
> > + switch (type) {
> > + case ACL_TYPE_ACCESS:
> > + name = XATTR_NAME_POSIX_ACL_ACCESS;
> > + if (acl)
> > + rc = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> > + break;
> > +
> > + case ACL_TYPE_DEFAULT:
> > + name = XATTR_NAME_POSIX_ACL_DEFAULT;
> > + if (!S_ISDIR(inode->i_mode))
> > + rc = acl ? -EACCES : 0;
> > + break;
> > +
> > + default:
> > + rc = -EINVAL;
> > + break;
> > + }
> > + if (rc)
> > + return rc;
> > +
> > + if (acl) {
> > + value_size = posix_acl_xattr_size(acl->a_count);
> > + value = kmalloc(value_size, GFP_NOFS);
> > + if (!value) {
> > + rc = -ENOMEM;
> > + goto out;
> > + }
> > +
> > + rc = posix_acl_to_xattr(&init_user_ns, acl, value, value_size);
> > + if (rc < 0)
> > + goto out_value;
> > + }
> > +
> > + rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
> > + value ? OBD_MD_FLXATTR : OBD_MD_FLXATTRRM,
> > + name, value, value_size, 0, 0, 0, &req);
> > +
> > + ptlrpc_req_finished(req);
> > +out_value:
> > + kfree(value);
> > +out:
> > + if (rc)
> > + forget_cached_acl(inode, type);
> > + else
> > + set_cached_acl(inode, type, acl);
> > + return rc;
> > +}
> > +#endif /* CONFIG_FS_POSIX_ACL */
> > +
> > int ll_inode_permission(struct inode *inode, int mask)
> > {
> > struct ll_sb_info *sbi;
> > @@ -3164,7 +3223,10 @@ int ll_inode_permission(struct inode *inode, int mask)
> > .permission = ll_inode_permission,
> > .listxattr = ll_listxattr,
> > .fiemap = ll_fiemap,
> > +#ifdef CONFIG_FS_POSIX_ACL
> > .get_acl = ll_get_acl,
> > + .set_acl = ll_set_acl,
> > +#endif
> > };
> >
> > /* dynamic ioctl number support routines */
> > diff --git a/drivers/staging/lustre/lustre/llite/llite_internal.h b/drivers/staging/lustre/lustre/llite/llite_internal.h
> > index 6504850..2280327 100644
> > --- a/drivers/staging/lustre/lustre/llite/llite_internal.h
> > +++ b/drivers/staging/lustre/lustre/llite/llite_internal.h
> > @@ -754,7 +754,11 @@ enum ldlm_mode ll_take_md_lock(struct inode *inode, __u64 bits,
> > int ll_md_real_close(struct inode *inode, fmode_t fmode);
> > int ll_getattr(const struct path *path, struct kstat *stat,
> > u32 request_mask, unsigned int flags);
> > +#ifdef CONFIG_FS_POSIX_ACL
> > struct posix_acl *ll_get_acl(struct inode *inode, int type);
> > +int ll_set_acl(struct inode *inode, struct posix_acl *acl, int type);
> > +#endif /* CONFIG_FS_POSIX_ACL */
> > +
> > int ll_migrate(struct inode *parent, struct file *file, int mdtidx,
> > const char *name, int namelen);
> > int ll_get_fid_by_name(struct inode *parent, const char *name,
> > diff --git a/drivers/staging/lustre/lustre/llite/namei.c b/drivers/staging/lustre/lustre/llite/namei.c
> > index 9ac7f09..b41f189 100644
> > --- a/drivers/staging/lustre/lustre/llite/namei.c
> > +++ b/drivers/staging/lustre/lustre/llite/namei.c
> > @@ -1195,7 +1195,10 @@ static int ll_rename(struct inode *src, struct dentry *src_dchild,
> > .getattr = ll_getattr,
> > .permission = ll_inode_permission,
> > .listxattr = ll_listxattr,
> > - .get_acl = ll_get_acl,
> > +#ifdef CONFIG_FS_POSIX_ACL
> > + .get_acl = ll_get_acl,
> > + .set_acl = ll_set_acl,
> > +#endif
> > };
> >
> > const struct inode_operations ll_special_inode_operations = {
> > @@ -1203,5 +1206,8 @@ static int ll_rename(struct inode *src, struct dentry *src_dchild,
> > .getattr = ll_getattr,
> > .permission = ll_inode_permission,
> > .listxattr = ll_listxattr,
> > - .get_acl = ll_get_acl,
> > +#ifdef CONFIG_FS_POSIX_ACL
> > + .get_acl = ll_get_acl,
> > + .set_acl = ll_set_acl,
> > +#endif
> > };
> > --
> > 1.8.3.1
>