From 8a71fd5061bd073e055e6cbba1d238305e6827bb Mon Sep 17 00:00:00 2001 From: Hongchao Zhang Date: Sun, 28 Jan 2018 05:21:35 +0800 Subject: [PATCH] LU-5152 quota: enforce block quota for chgrp When an unprivileged user calls chgrp to change the group of one of his files, the block quota limit of that new group should be checked to ensure it not exceeds the limit. The side effect of this patch could be, 1.The performance of chgrp from non-privileged user will be very slow, no matter if quota is enabled. Since we assume that chgrp issued from non-privileged user is very rare, the performance impact possibly is acceptable. 2.If MDT crash while performing chgrp, inconsistency (group ownership among MDT and OST objects) will be created. It should be acceptable. This patch has fixed the bug while calculating the disk space of some file for ldiskfs and zfs, the block unit is always 512. Change-Id: I4b781e94493fe63c8cbd5700dc68293b2504c2ac Signed-off-by: Hongchao Zhang Reviewed-on: https://review.whamcloud.com/30146 Reviewed-by: Fan Yong Reviewed-by: Niu Yawei Tested-by: Jenkins Tested-by: Maloo Reviewed-by: Oleg Drokin --- lustre/include/uapi/linux/lustre/lustre_idl.h | 1 + lustre/mdd/mdd_object.c | 59 +++++++++++++++++++-------- lustre/osd-ldiskfs/osd_handler.c | 33 +++++++++------ lustre/osd-zfs/osd_object.c | 20 +++++---- lustre/osp/osp_object.c | 33 ++++++++++++++- lustre/quota/qsd_handler.c | 7 ++++ lustre/target/out_lib.c | 4 ++ lustre/tests/sanity-quota.sh | 41 +++++++++++++++++++ 8 files changed, 161 insertions(+), 37 deletions(-) diff --git a/lustre/include/uapi/linux/lustre/lustre_idl.h b/lustre/include/uapi/linux/lustre/lustre_idl.h index 3a65228..a158f70 100644 --- a/lustre/include/uapi/linux/lustre/lustre_idl.h +++ b/lustre/include/uapi/linux/lustre/lustre_idl.h @@ -1674,6 +1674,7 @@ enum { * 2. If these flags needs to be stored into inode, they will be * stored in LMA. see LMAI_XXXX */ LUSTRE_ORPHAN_FL = 0x00002000, + LUSTRE_SET_SYNC_FL = 0x00040000, /* Synchronous setattr on OSTs */ LUSTRE_LMA_FL_MASKS = LUSTRE_ORPHAN_FL, }; diff --git a/lustre/mdd/mdd_object.c b/lustre/mdd/mdd_object.c index 2bc73a4..b2a6d5c 100644 --- a/lustre/mdd/mdd_object.c +++ b/lustre/mdd/mdd_object.c @@ -879,10 +879,11 @@ int mdd_attr_set(const struct lu_env *env, struct md_object *obj, { struct mdd_object *mdd_obj = md2mdd_obj(obj); struct mdd_device *mdd = mdo2mdd(obj); - struct thandle *handle; + struct thandle *handle = NULL; struct lu_attr *la_copy = &mdd_env_info(env)->mti_la_for_fix; struct lu_attr *attr = MDD_ENV_VAR(env, cattr); const struct lu_attr *la = &ma->ma_attr; + struct lu_ucred *uc; int rc; ENTRY; @@ -908,17 +909,38 @@ int mdd_attr_set(const struct lu_env *env, struct md_object *obj, RETURN(0); } - handle = mdd_trans_create(env, mdd); - if (IS_ERR(handle)) - RETURN(PTR_ERR(handle)); + /* If an unprivileged user changes group of some file, + * the setattr operation will be processed synchronously to + * honor the quota limit of the corresponding group. see LU-5152 */ + uc = lu_ucred_check(env); + if (S_ISREG(attr->la_mode) && la->la_valid & LA_GID && + la->la_gid != attr->la_gid && uc != NULL && uc->uc_fsuid != 0) { + la_copy->la_valid |= LA_FLAGS; + la_copy->la_flags |= LUSTRE_SET_SYNC_FL; + + /* Flush the possible existing sync requests to OSTs to + * keep the order of sync for the current setattr operation + * will be sent directly to OSTs. see LU-5152 */ + rc = dt_sync(env, mdd->mdd_child); + if (rc) + GOTO(out, rc); + } + + handle = mdd_trans_create(env, mdd); + if (IS_ERR(handle)) { + rc = PTR_ERR(handle); + handle = NULL; + + GOTO(out, rc); + } rc = mdd_declare_attr_set(env, mdd, mdd_obj, la_copy, handle); - if (rc) - GOTO(stop, rc); + if (rc) + GOTO(out, rc); - rc = mdd_trans_start(env, mdd, handle); - if (rc) - GOTO(stop, rc); + rc = mdd_trans_start(env, mdd, handle); + if (rc) + GOTO(out, rc); if (mdd->mdd_sync_permission && permission_needs_sync(attr, la)) handle->th_sync = 1; @@ -928,20 +950,25 @@ int mdd_attr_set(const struct lu_env *env, struct md_object *obj, la->la_mtime, la->la_ctime); mdd_write_lock(env, mdd_obj, MOR_TGT_CHILD); - if (la_copy->la_valid & LA_FLAGS) - rc = mdd_attr_set_internal(env, mdd_obj, la_copy, handle, 1); - else if (la_copy->la_valid) /* setattr */ + if (la_copy->la_valid) { rc = mdd_attr_set_internal(env, mdd_obj, la_copy, handle, 1); + + if (rc == -EDQUOT && la_copy->la_flags & LUSTRE_SET_SYNC_FL) { + /* rollback to the original gid */ + la_copy->la_flags &= ~LUSTRE_SET_SYNC_FL; + la_copy->la_gid = attr->la_gid; + mdd_attr_set_internal(env, mdd_obj, la_copy, handle, 1); + } + } mdd_write_unlock(env, mdd_obj); +out: if (rc == 0) rc = mdd_attr_set_changelog(env, obj, handle, la_copy->la_valid); - GOTO(stop, rc); - -stop: - rc = mdd_trans_stop(env, mdd, rc, handle); + if (handle != NULL) + rc = mdd_trans_stop(env, mdd, rc, handle); return rc; } diff --git a/lustre/osd-ldiskfs/osd_handler.c b/lustre/osd-ldiskfs/osd_handler.c index 118bec0..ad77d4d6 100644 --- a/lustre/osd-ldiskfs/osd_handler.c +++ b/lustre/osd-ldiskfs/osd_handler.c @@ -2479,7 +2479,7 @@ static int osd_declare_attr_qid(const struct lu_env *env, struct osd_object *obj, struct osd_thandle *oh, long long bspace, qid_t old_id, qid_t new_id, bool enforce, - unsigned type) + unsigned type, bool ignore_edquot) { int rc; struct osd_thread_info *info = osd_oti_get(env); @@ -2494,7 +2494,7 @@ static int osd_declare_attr_qid(const struct lu_env *env, qi->lqi_space = 1; /* Reserve credits for the new id */ rc = osd_declare_qid(env, oh, qi, NULL, enforce, NULL); - if (rc == -EDQUOT || rc == -EINPROGRESS) + if (ignore_edquot && (rc == -EDQUOT || rc == -EINPROGRESS)) rc = 0; if (rc) RETURN(rc); @@ -2503,7 +2503,7 @@ static int osd_declare_attr_qid(const struct lu_env *env, qi->lqi_id.qid_uid = old_id; qi->lqi_space = -1; rc = osd_declare_qid(env, oh, qi, obj, enforce, NULL); - if (rc == -EDQUOT || rc == -EINPROGRESS) + if (ignore_edquot && (rc == -EDQUOT || rc == -EINPROGRESS)) rc = 0; if (rc) RETURN(rc); @@ -2519,7 +2519,7 @@ static int osd_declare_attr_qid(const struct lu_env *env, * to save credit reservation. */ rc = osd_declare_qid(env, oh, qi, obj, enforce, NULL); - if (rc == -EDQUOT || rc == -EINPROGRESS) + if (ignore_edquot && (rc == -EDQUOT || rc == -EINPROGRESS)) rc = 0; if (rc) RETURN(rc); @@ -2528,7 +2528,7 @@ static int osd_declare_attr_qid(const struct lu_env *env, qi->lqi_id.qid_uid = old_id; qi->lqi_space = -bspace; rc = osd_declare_qid(env, oh, qi, obj, enforce, NULL); - if (rc == -EDQUOT || rc == -EINPROGRESS) + if (ignore_edquot && (rc == -EDQUOT || rc == -EINPROGRESS)) rc = 0; RETURN(rc); @@ -2566,22 +2566,30 @@ static int osd_declare_attr_set(const struct lu_env *env, if (attr == NULL || obj->oo_inode == NULL) RETURN(rc); - bspace = obj->oo_inode->i_blocks; - bspace <<= obj->oo_inode->i_sb->s_blocksize_bits; + bspace = obj->oo_inode->i_blocks << 9; bspace = toqb(bspace); /* Changing ownership is always preformed by super user, it should not - * fail with EDQUOT. + * fail with EDQUOT unless required explicitly. * * We still need to call the osd_declare_qid() to calculate the journal * credits for updating quota accounting files and to trigger quota * space adjustment once the operation is completed.*/ if (attr->la_valid & LA_UID || attr->la_valid & LA_GID) { + bool ignore_edquot = !(attr->la_flags & LUSTRE_SET_SYNC_FL); + + if (!ignore_edquot) + CDEBUG(D_QUOTA, "%s: enforce quota on UID %u, GID %u" + "(the quota space is %lld)\n", + obj->oo_inode->i_sb->s_id, attr->la_uid, + attr->la_gid, bspace); + /* USERQUOTA */ uid = i_uid_read(obj->oo_inode); enforce = (attr->la_valid & LA_UID) && (attr->la_uid != uid); rc = osd_declare_attr_qid(env, obj, oh, bspace, uid, - attr->la_uid, enforce, USRQUOTA); + attr->la_uid, enforce, USRQUOTA, + true); if (rc) RETURN(rc); @@ -2589,7 +2597,8 @@ static int osd_declare_attr_set(const struct lu_env *env, enforce = (attr->la_valid & LA_GID) && (attr->la_gid != gid); rc = osd_declare_attr_qid(env, obj, oh, bspace, i_gid_read(obj->oo_inode), - attr->la_gid, enforce, GRPQUOTA); + attr->la_gid, enforce, GRPQUOTA, + ignore_edquot); if (rc) RETURN(rc); @@ -2601,7 +2610,7 @@ static int osd_declare_attr_set(const struct lu_env *env, (attr->la_projid != projid); rc = osd_declare_attr_qid(env, obj, oh, bspace, (qid_t)projid, (qid_t)attr->la_projid, - enforce, PRJQUOTA); + enforce, PRJQUOTA, true); if (rc) RETURN(rc); } @@ -5618,7 +5627,7 @@ static int osd_index_declare_ea_insert(const struct lu_env *env, i_projid_read(inode) != 0) rc = osd_declare_attr_qid(env, osd_dt_obj(dt), oh, 0, i_projid_read(inode), - 0, false, PRJQUOTA); + 0, false, PRJQUOTA, true); #endif } diff --git a/lustre/osd-zfs/osd_object.c b/lustre/osd-zfs/osd_object.c index 853ef27..c835e92 100644 --- a/lustre/osd-zfs/osd_object.c +++ b/lustre/osd-zfs/osd_object.c @@ -1037,7 +1037,7 @@ static inline int qsd_transfer(const struct lu_env *env, struct qsd_instance *qsd, struct lquota_trans *trans, int qtype, __u64 orig_id, __u64 new_id, __u64 bspace, - struct lquota_id_info *qi) + struct lquota_id_info *qi, bool ignore_edquot) { int rc; @@ -1054,7 +1054,7 @@ static inline int qsd_transfer(const struct lu_env *env, qi->lqi_id.qid_uid = new_id; qi->lqi_space = 1; rc = qsd_op_begin(env, qsd, trans, qi, NULL); - if (rc == -EDQUOT || rc == -EINPROGRESS) + if (ignore_edquot && (rc == -EDQUOT || rc == -EINPROGRESS)) rc = 0; if (rc) return rc; @@ -1076,7 +1076,7 @@ static inline int qsd_transfer(const struct lu_env *env, qi->lqi_id.qid_uid = new_id; qi->lqi_space = bspace; rc = qsd_op_begin(env, qsd, trans, qi, NULL); - if (rc == -EDQUOT || rc == -EINPROGRESS) + if (ignore_edquot && (rc == -EDQUOT || rc == -EINPROGRESS)) rc = 0; if (rc) return rc; @@ -1146,7 +1146,11 @@ static int osd_declare_attr_set(const struct lu_env *env, if (attr && (attr->la_valid & (LA_UID | LA_GID | LA_PROJID))) { sa_object_size(obj->oo_sa_hdl, &blksize, &bspace); - bspace = toqb(bspace * blksize); + bspace = toqb(bspace * 512); + + CDEBUG(D_QUOTA, "%s: enforce quota on UID %u, GID %u," + "the quota space is %lld (%u)\n", osd->od_svname, + attr->la_uid, attr->la_gid, bspace, blksize); } if (attr && attr->la_valid & LA_UID) { @@ -1155,7 +1159,7 @@ static int osd_declare_attr_set(const struct lu_env *env, rc = qsd_transfer(env, osd->od_quota_slave, &oh->ot_quota_trans, USRQUOTA, obj->oo_attr.la_uid, attr->la_uid, - bspace, &info->oti_qi); + bspace, &info->oti_qi, true); if (rc) GOTO(out, rc); } @@ -1166,7 +1170,9 @@ static int osd_declare_attr_set(const struct lu_env *env, rc = qsd_transfer(env, osd->od_quota_slave, &oh->ot_quota_trans, GRPQUOTA, obj->oo_attr.la_gid, attr->la_gid, - bspace, &info->oti_qi); + bspace, &info->oti_qi, + !(attr->la_flags & + LUSTRE_SET_SYNC_FL)); if (rc) GOTO(out, rc); } @@ -1197,7 +1203,7 @@ static int osd_declare_attr_set(const struct lu_env *env, &oh->ot_quota_trans, PRJQUOTA, obj->oo_attr.la_projid, attr->la_projid, bspace, - &info->oti_qi); + &info->oti_qi, true); if (rc) GOTO(out, rc); } diff --git a/lustre/osp/osp_object.c b/lustre/osp/osp_object.c index 0604892..4e316c4 100644 --- a/lustre/osp/osp_object.c +++ b/lustre/osp/osp_object.c @@ -709,8 +709,37 @@ static int osp_attr_set(const struct lu_env *env, struct dt_object *dt, RETURN(0); if (!is_only_remote_trans(th)) { - rc = osp_sync_add(env, o, MDS_SETATTR64_REC, th, attr); - /* XXX: send new uid/gid to OST ASAP? */ + if (attr->la_flags & LUSTRE_SET_SYNC_FL) { + struct ptlrpc_request *req = NULL; + struct osp_update_request *update = NULL; + struct osp_device *osp = lu2osp_dev(dt->do_lu.lo_dev); + + update = osp_update_request_create(&osp->opd_dt_dev); + if (IS_ERR(update)) + RETURN(PTR_ERR(update)); + + rc = osp_update_rpc_pack(env, attr_set, update, + OUT_ASTTR_SET, + lu_object_fid(&dt->do_lu), + attr); + if (rc != 0) { + CERROR("%s: update error "DFID": rc = %d\n", + osp->opd_obd->obd_name, + PFID(lu_object_fid(&dt->do_lu)), rc); + + osp_update_request_destroy(env, update); + RETURN(rc); + } + + rc = osp_remote_sync(env, osp, update, &req); + if (req != NULL) + ptlrpc_req_finished(req); + + osp_update_request_destroy(env, update); + } else { + rc = osp_sync_add(env, o, MDS_SETATTR64_REC, th, attr); + /* XXX: send new uid/gid to OST ASAP? */ + } } else { struct lu_attr *la; diff --git a/lustre/quota/qsd_handler.c b/lustre/quota/qsd_handler.c index b3c43a0..eb0bddb 100644 --- a/lustre/quota/qsd_handler.c +++ b/lustre/quota/qsd_handler.c @@ -652,6 +652,13 @@ static bool qsd_acquire(const struct lu_env *env, struct lquota_entry *lqe, * rc < 0, something bad happened */ break; + /* if we have gotten some quota and stil wait more quota, + * it's better to give QMT some time to reclaim from clients */ + if (count > 0) { + set_current_state(TASK_INTERRUPTIBLE); + schedule_timeout(cfs_time_seconds(1)); + } + /* need to acquire more quota space from master */ rc = qsd_acquire_remote(env, lqe); } diff --git a/lustre/target/out_lib.c b/lustre/target/out_lib.c index 14bc9bd..1f55422 100644 --- a/lustre/target/out_lib.c +++ b/lustre/target/out_lib.c @@ -657,6 +657,10 @@ int out_attr_set_add_exec(const struct lu_env *env, struct dt_object *dt_obj, if (rc != 0) return rc; + if (attr->la_valid & LA_FLAGS && + attr->la_flags & LUSTRE_SET_SYNC_FL) + th->th_sync |= 1; + arg = tx_add_exec(ta, out_tx_attr_set_exec, out_tx_attr_set_undo, file, line); if (IS_ERR(arg)) diff --git a/lustre/tests/sanity-quota.sh b/lustre/tests/sanity-quota.sh index 524ceae..431c5d2 100755 --- a/lustre/tests/sanity-quota.sh +++ b/lustre/tests/sanity-quota.sh @@ -2983,6 +2983,47 @@ test_54() { } run_test 54 "basic lfs project interface test" +test_55() { + setup_quota_test || error "setup quota failed with $?" + + set_ost_qtype $QTYPE || error "enable ost quota failed" + quota_init + + #add second group to TSTUSR + usermod -G $TSTUSR,$TSTUSR2 $TSTUSR + + #prepare test file + $RUNAS dd if=/dev/zero of=$DIR/$tdir/$tfile bs=1024 count=100000 || + error "failed to dd" + + cancel_lru_locks osc + sync; sync_all_data || true + + $LFS setquota -g $TSTUSR2 -b 0 -B 50M $DIR || + error "failed to setquota on group $TSTUSR2" + + $LFS quota -v -g $TSTUSR2 $DIR + + runas -u $TSTUSR -g $TSTUSR2 chgrp $TSTUSR2 $DIR/$tdir/$tfile && + error "chgrp should failed with -EDQUOT" + + USED=$(getquota -g $TSTUSR2 global curspace) + echo "$USED" + + $LFS setquota -g $TSTUSR2 -b 0 -B 300M $DIR || + error "failed to setquota on group $TSTUSR2" + + $LFS quota -v -g $TSTUSR2 $DIR + + runas -u $TSTUSR -g $TSTUSR2 chgrp $TSTUSR2 $DIR/$tdir/$tfile || + error "chgrp should succeed" + + $LFS quota -v -g $TSTUSR2 $DIR + + cleanup_quota_test +} +run_test 55 "Chgrp should be affected by group quota" + quota_fini() { do_nodes $(comma_list $(nodes_list)) "lctl set_param debug=-quota" -- 1.8.3.1