Whamcloud - gitweb
LU-15880 quota: fix issues in reserving quota 25/47425/4
authorHongchao Zhang <hongchao@whamcloud.com>
Fri, 24 Jun 2022 14:06:08 +0000 (22:06 +0800)
committerOleg Drokin <green@whamcloud.com>
Mon, 18 Jul 2022 20:24:52 +0000 (20:24 +0000)
Calling "chgrp" with unprivileged user will reserve quota space
before changing the GID of the file, and the reserved quota space
will be freed after its transaction is committed. there are some
issues in the current implementation,
1, the reserved quota isn't freed in case of error in "mdd_attr_set"
   and "tgt_cb_last_committed".
2, during freeing the reserved quota, the quota space to free is
   set as the same parameter as reserving the quota, which could
   be wrong, for instance, the reserving quota space will be 0 if
   the corresponding quota ID isn't enforces, but the call will
   return without error.

Like the "qsd_op_begin/qsd_op_end",The patch also adds reference to
the lquota_entry gotten during reserving quota and release it during
freeing the reserved quota to prevent potential issue.

Signed-off-by: Hongchao Zhang <hongchao@whamcloud.com>
Change-Id: I098cde7d5e89fe8b9eaab0ae4bc285a4ac6c2281
Reviewed-on: https://review.whamcloud.com/47425
Tested-by: jenkins <devops@whamcloud.com>
Tested-by: Maloo <maloo@whamcloud.com>
Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
Reviewed-by: Bobi Jam <bobijam@hotmail.com>
Reviewed-by: Sergey Cheremencev <sergey.cheremencev@hpe.com>
Reviewed-by: Oleg Drokin <green@whamcloud.com>
lustre/include/dt_object.h
lustre/include/lustre_quota.h
lustre/mdd/mdd_object.c
lustre/osd-ldiskfs/osd_handler.c
lustre/osd-zfs/osd_handler.c
lustre/quota/qsd_handler.c
lustre/target/tgt_lastrcvd.c

index fb28777..cb386cd 100644 (file)
@@ -49,7 +49,7 @@
  * super-class definitions.
  */
 #include <lu_object.h>
-
+#include <lustre_quota.h>
 #include <libcfs/libcfs.h>
 
 struct seq_file;
@@ -316,29 +316,22 @@ struct dt_device_operations {
                                 struct dt_device *dev);
 
        /**
-        * The unit of \a count is byte for block or inodes for metadata.
-        *
-        * If \a count > 0, reserve quota in advance of an operation that
-        * changes the quota assignment, such as chgrp() or rename() into
+        * If \a qi->lqi_space > 0, reserve quota in advance of an operation
+        * that changes the quota assignment, such as chgrp() or rename() into
         * a directory with a different group ID.
         *
-        * If \a count < 0, free the reserved quota previously.
+        * If \a qi->lqi_space < 0, free the reserved quota previously.
         *
         * \param[in] env       execution environment for this thread
         * \param[in] dev       the bottom OSD device to reserve quota
-        * \param[in] type      quota type (LQUOTA_RES_DT or LQUOTA_RES_MD)
-        * \param[in] uid       quota uid
-        * \param[in] gid       quota gid
-        * \param[in] count     space (bytes or inodes) to reserve or free
-        * \param[in] md        true for inode, false for block
+        * \param[in] qi        quota id & space required to reserve
         *
         * \retval 0            on success
         * \retval negative     negated errno on error
         */
        int   (*dt_reserve_or_free_quota)(const struct lu_env *env,
                                          struct dt_device *dev,
-                                         enum quota_type type, __u64 uid,
-                                         __u64 gid, __s64 count, bool md);
+                                         struct lquota_id_info *qi);
 };
 
 struct dt_index_features {
@@ -1985,12 +1978,6 @@ static inline struct dt_object *dt_object_child(struct dt_object *o)
                            struct dt_object, do_lu);
 }
 
-struct dt_quota_reserve_rec {
-       enum quota_type  qrr_type;
-       union lquota_id  qrr_id;
-       __u64            qrr_count;
-};
-
 /**
  * This is the general purpose transaction handle.
  * 1. Transaction Life Cycle
@@ -2018,7 +2005,7 @@ struct thandle {
        struct thandle  *th_top;
 
        /* reserved quota for this handle */
-       struct dt_quota_reserve_rec th_reserved_quota;
+       struct lquota_id_info   th_reserved_quota;
 
        /** the last operation result in this transaction.
         * this value is used in recovery */
@@ -2937,14 +2924,12 @@ static inline int dt_commit_async(const struct lu_env *env,
 
 static inline int dt_reserve_or_free_quota(const struct lu_env *env,
                                           struct dt_device *dev,
-                                          enum quota_type type, __u64 uid,
-                                          __u64 gid, int count, bool is_md)
+                                          struct lquota_id_info *qi)
 {
        LASSERT(dev);
        LASSERT(dev->dd_ops);
        LASSERT(dev->dd_ops->dt_reserve_or_free_quota);
-       return dev->dd_ops->dt_reserve_or_free_quota(env, dev, type, uid, gid,
-                                                    count, is_md);
+       return dev->dd_ops->dt_reserve_or_free_quota(env, dev, qi);
 }
 
 static inline int dt_lookup(const struct lu_env *env,
index 6b98f4c..c1d5828 100644 (file)
@@ -252,29 +252,5 @@ struct lquota_trans {
  * on slave */
 int lquotactl_slv(const struct lu_env *, struct dt_device *,
                  struct obd_quotactl *);
-
-static inline int quota_reserve_or_free(const struct lu_env *env,
-                                       struct qsd_instance *qsd,
-                                       struct lquota_id_info *qi,
-                                       enum quota_type type, __u64 uid,
-                                       __u64 gid, __s64 count, bool is_md)
-{
-       qi->lqi_type = type;
-       if (count > 0)
-               qi->lqi_space = toqb(count);
-       else
-               qi->lqi_space = -toqb(-count);
-
-       if (is_md)
-               qi->lqi_is_blk = false;
-       else
-               qi->lqi_is_blk = true;
-
-       qi->lqi_id.qid_uid = uid;
-       qi->lqi_id.qid_gid = gid;
-
-       return qsd_reserve_or_free_quota(env, qsd, qi);
-}
-
 /** @} quota */
 #endif /* _LUSTRE_QUOTA_H */
index 3e4d7ed..7de1017 100644 (file)
@@ -1217,9 +1217,9 @@ int mdd_attr_set(const struct lu_env *env, struct md_object *obj,
        struct lu_attr *attr = MDD_ENV_VAR(env, cattr);
        const struct lu_attr *la = &ma->ma_attr;
        struct lu_ucred  *uc;
+       struct lquota_id_info qi;
        bool quota_reserved = false;
        bool chrgrp_by_unprivileged_user = false;
-       __s64 quota_size = 0;
        int rc;
        ENTRY;
 
@@ -1250,31 +1250,34 @@ int mdd_attr_set(const struct lu_env *env, struct md_object *obj,
         * the setattr operation will be processed synchronously to
         * honor the quota limit of the corresponding group. see LU-5152 */
        uc = lu_ucred_check(env);
+       memset(&qi, 0, sizeof(qi));
        if (S_ISREG(attr->la_mode) && la->la_valid & LA_GID &&
            la->la_gid != attr->la_gid && uc != NULL && uc->uc_fsuid != 0) {
                CDEBUG(D_QUOTA, "%s: reserve quota for changing group: gid=%u size=%llu\n",
                       mdd2obd_dev(mdd)->obd_name, la->la_gid, la->la_size);
 
                if (la->la_valid & LA_BLOCKS)
-                       quota_size = la->la_blocks << 9;
+                       qi.lqi_space = la->la_blocks << 9;
                else if (la->la_valid & LA_SIZE)
-                       quota_size = la->la_size;
+                       qi.lqi_space = la->la_size;
                /* use local attr gotten above */
                else if (attr->la_valid & LA_BLOCKS)
-                       quota_size = attr->la_blocks << 9;
+                       qi.lqi_space = attr->la_blocks << 9;
                else if (attr->la_valid & LA_SIZE)
-                       quota_size = attr->la_size;
+                       qi.lqi_space = attr->la_size;
 
-               if (quota_size > 0) {
+               if (qi.lqi_space > 0) {
+                       qi.lqi_id.qid_gid = la->la_gid;
+                       qi.lqi_type = GRPQUOTA;
+                       qi.lqi_space = toqb(qi.lqi_space);
+                       qi.lqi_is_blk = true;
                        rc = dt_reserve_or_free_quota(env, mdd->mdd_bottom,
-                                                     GRPQUOTA, attr->la_uid,
-                                                     la->la_gid, quota_size,
-                                                     false);
+                                                     &qi);
 
                        if (rc) {
                                CDEBUG(D_QUOTA, "%s: failed to reserve quota for gid %d size %llu\n",
                                       mdd2obd_dev(mdd)->obd_name,
-                                      la->la_gid, quota_size);
+                                      la->la_gid, qi.lqi_space);
 
                                GOTO(out, rc);
                        }
@@ -1363,14 +1366,14 @@ out:
 
                sub_th = thandle_get_sub_by_dt(env, handle, mdd->mdd_bottom);
                if (unlikely(IS_ERR(sub_th))) {
-                       dt_reserve_or_free_quota(env, mdd->mdd_bottom, GRPQUOTA,
-                                                attr->la_uid, la->la_gid,
-                                                -quota_size, false);
+                       qi.lqi_space *= -1;
+                       dt_reserve_or_free_quota(env, mdd->mdd_bottom, &qi);
                } else {
-                       sub_th->th_reserved_quota.qrr_type = GRPQUOTA;
-                       sub_th->th_reserved_quota.qrr_id.qid_gid = la->la_gid;
-                       sub_th->th_reserved_quota.qrr_count = quota_size;
+                       sub_th->th_reserved_quota = qi;
                }
+       } else if (quota_reserved) {
+               qi.lqi_space *= -1;
+               dt_reserve_or_free_quota(env, mdd->mdd_bottom, &qi);
        }
 
        if (handle != NULL)
index 8d45211..ab7facf 100644 (file)
@@ -2541,23 +2541,20 @@ const int osd_dto_credits_noquota[DTO_NR] = {
 /* reserve or free quota for some operation */
 static int osd_reserve_or_free_quota(const struct lu_env *env,
                                     struct dt_device *dev,
-                                    enum quota_type type, __u64 uid,
-                                    __u64 gid, __s64 count, bool is_md)
+                                    struct lquota_id_info *qi)
 {
-       int rc;
        struct osd_device       *osd = osd_dt_dev(dev);
-       struct osd_thread_info  *info = osd_oti_get(env);
-       struct lquota_id_info   *qi = &info->oti_qi;
        struct qsd_instance     *qsd = NULL;
+       int rc;
 
        ENTRY;
 
-       if (is_md)
-               qsd = osd->od_quota_slave_md;
-       else
+       if (qi->lqi_is_blk)
                qsd = osd->od_quota_slave_dt;
+       else
+               qsd = osd->od_quota_slave_md;
 
-       rc = quota_reserve_or_free(env, qsd, qi, type, uid, gid, count, is_md);
+       rc = qsd_reserve_or_free_quota(env, qsd, qi);
        RETURN(rc);
 }
 
index ecc043f..7d1b579 100644 (file)
@@ -701,21 +701,20 @@ static int osd_ro(const struct lu_env *env, struct dt_device *d)
 /* reserve or free quota for some operation */
 static int osd_reserve_or_free_quota(const struct lu_env *env,
                                     struct dt_device *dev,
-                                    enum quota_type type, __u64 uid,
-                                    __u64 gid, __s64 count, bool is_md)
+                                    struct lquota_id_info *qi)
 {
-       int rc;
        struct osd_device       *osd = osd_dt_dev(dev);
-       struct osd_thread_info  *info = osd_oti_get(env);
-       struct lquota_id_info   *qi = &info->oti_qi;
        struct qsd_instance     *qsd = NULL;
+       int rc;
 
-       if (is_md)
-               qsd = osd->od_quota_slave_md;
-       else
+       ENTRY;
+
+       if (qi->lqi_is_blk)
                qsd = osd->od_quota_slave_dt;
+       else
+               qsd = osd->od_quota_slave_md;
 
-       rc = quota_reserve_or_free(env, qsd, qi, type, uid, gid, count, is_md);
+       rc = qsd_reserve_or_free_quota(env, qsd, qi);
        RETURN(rc);
 }
 
index 1c8e43a..0c34e6c 100644 (file)
@@ -1064,8 +1064,16 @@ static void qsd_op_end0(const struct lu_env *env, struct qsd_qtype_info *qqi,
                qsd_refresh_usage(env, lqe);
 
        lqe_write_lock(lqe);
-       if (qid->lqi_space > 0)
-               lqe->lqe_pending_write -= qid->lqi_space;
+       if (qid->lqi_space > 0) {
+               if (lqe->lqe_pending_write < qid->lqi_space) {
+                       LQUOTA_ERROR(lqe,
+                                    "More pending write quota to reduce (pending %llu, space %lld)\n",
+                                    lqe->lqe_pending_write, qid->lqi_space);
+                       lqe->lqe_pending_write = 0;
+               } else {
+                       lqe->lqe_pending_write -= qid->lqi_space;
+               }
+       }
        if (env != NULL)
                adjust = qsd_adjust_needed(lqe);
        else
@@ -1290,10 +1298,9 @@ int qsd_reserve_or_free_quota(const struct lu_env *env,
                              struct qsd_instance *qsd,
                              struct lquota_id_info *qi)
 {
-       struct lquota_entry *lqe;
        struct qsd_qtype_info  *qqi;
-       int rc = 0;
        bool is_free = qi->lqi_space < 0;
+       int rc = 0;
 
        ENTRY;
 
@@ -1327,31 +1334,21 @@ int qsd_reserve_or_free_quota(const struct lu_env *env,
         * or - the user/group is root
         * or - quota accounting isn't enabled
         */
-       if (!qsd_type_enabled(qsd, qi->lqi_type) || qi->lqi_id.qid_uid == 0 ||
-           (qsd->qsd_type_array[qi->lqi_type])->qqi_acct_failed)
+       if (!is_free &&
+           (!qsd_type_enabled(qsd, qi->lqi_type) || qi->lqi_id.qid_uid == 0 ||
+             (qsd->qsd_type_array[qi->lqi_type])->qqi_acct_failed))
                RETURN(0);
 
        if (is_free) {
-               /* look up lquota entry associated with qid */
-               lqe = lqe_locate(env, qqi->qqi_site, &qi->lqi_id);
-               if (IS_ERR(lqe))
-                       RETURN(PTR_ERR(lqe));
-               if (!lqe->lqe_enforced) {
-                       lqe_putref(lqe);
-                       RETURN(0);
-               }
-
-               qi->lqi_qentry = lqe;
-
-               /* lqe will be put in qsd_op_end0 */
                qsd_op_end0(env, qsd->qsd_type_array[qi->lqi_type], qi);
-               qi->lqi_qentry = NULL;
        } else {
-               /* manage quota enforcement for this ID */
-               rc = qsd_op_begin0(env, qsd->qsd_type_array[qi->lqi_type], qi,
-                                  qi->lqi_space, NULL);
+               long long qspace = qi->lqi_space;
 
-               if (qi->lqi_qentry != NULL) {
+               /* the acquired quota will add to lqi_space in qsd_op_begin0 */
+               qi->lqi_space = 0;
+               rc = qsd_op_begin0(env, qsd->qsd_type_array[qi->lqi_type], qi,
+                                  qspace, NULL);
+               if (rc && qi->lqi_qentry) {
                        lqe_putref(qi->lqi_qentry);
                        qi->lqi_qentry = NULL;
                }
index b0f200c..387223c 100644 (file)
@@ -872,28 +872,15 @@ static void tgt_cb_last_committed(struct lu_env *env, struct thandle *th,
        LASSERT(ccb->llcc_tgt != NULL);
        LASSERT(ccb->llcc_exp->exp_obd == ccb->llcc_tgt->lut_obd);
 
-       if (th->th_reserved_quota.qrr_count > 0) {
-               struct lu_env            temp_env;
-               int rc;
-
+       if (th->th_reserved_quota.lqi_space > 0) {
                CDEBUG(D_QUOTA, "free quota %llu %llu\n",
-                      th->th_reserved_quota.qrr_id.qid_gid,
-                      th->th_reserved_quota.qrr_count);
-
-               rc = lu_env_init(&temp_env, LCT_DT_THREAD);
-               if (rc) {
-                       CERROR("%s: can't initialize environment: rc = %d\n",
-                              ccb->llcc_tgt->lut_obd->obd_name, rc);
-                       goto out;
-               }
+                      th->th_reserved_quota.lqi_id.qid_gid,
+                      th->th_reserved_quota.lqi_space);
 
-               dt_reserve_or_free_quota(&temp_env, th->th_dev,
-                                        th->th_reserved_quota.qrr_type,
-                                        th->th_reserved_quota.qrr_id.qid_uid,
-                                        th->th_reserved_quota.qrr_id.qid_gid,
-                                        -th->th_reserved_quota.qrr_count,
-                                        false);
-               lu_env_fini(&temp_env);
+               /* env can be NULL for freeing reserved quota */
+               th->th_reserved_quota.lqi_space *= -1;
+               dt_reserve_or_free_quota(NULL, th->th_dev,
+                                        &th->th_reserved_quota);
        }
 
        /* error hit, don't update last committed to provide chance to