[LU-6378] Quota performance issue for 2.7 Created: 18/Mar/15  Updated: 12/May/16  Resolved: 29/Apr/15

Status: Resolved
Project: Lustre
Component/s: None
Affects Version/s: Lustre 2.7.0, Lustre 2.8.0
Fix Version/s: Lustre 2.8.0

Type: Bug Priority: Major
Reporter: Di Wang Assignee: nasf (Inactive)
Resolution: Fixed Votes: 0
Labels: None

Issue Links:
Related
is related to LU-6381 replace global dq_state_lock/dq_list_... Open
Severity: 3
Rank (Obsolete): 9223372036854775807

 Description   

In lustre 2.7, inode I_NEW state is cleared in the later phase, which cause the following owner/group changing (in osd_attr_init0()) will be performed in I_NEW state, which means the dquot_initialize() won't be skipped for these operations,

2.7.0

/**
 * Helper function for osd_object_create()
 *
 * \retval 0, on success
 */
static int __osd_object_create(struct osd_thread_info *info,
                               struct osd_object *obj, struct lu_attr *attr,
                               struct dt_allocation_hint *hint,
                               struct dt_object_format *dof,
                               struct thandle *th)
{
        int     result;
        __u32   umask;

        /* we drop umask so that permissions we pass are not affected */
        umask = current->fs->umask;
        current->fs->umask = 0;

        result = osd_create_type_f(dof->dof_type)(info, obj, attr, hint, dof,
                                                  th);
        if (result == 0) {
                osd_attr_init(info, obj, attr, dof);
                osd_object_init0(obj);
        }

        if (obj->oo_inode != NULL) {
                LASSERT(obj->oo_inode->i_state & I_NEW);

                unlock_new_inode(obj->oo_inode);  --->It will unlock new inode here, so it will cause the above osd_attr_init to call quota_initialization (with I_NEW).  But in 2.6.0, we used to unlock the new inode in ldiskfs_new_inode.
        }

        /* restore previous umask value */
        current->fs->umask = umask;

        return result;
}

It is caused by this patch
http://review.whamcloud.com/13187



 Comments   
Comment by Niu Yawei (Inactive) [ 18/Mar/15 ]

Actually, the quota-avoid-dqget-calls.patch shouldn't rely on I_NEW to skip the dqget()/dqput() calls, instead, it should check the i_dquot pointer directly.

Comment by nasf (Inactive) [ 18/Mar/15 ]

Here is the patch:
http://review.whamcloud.com/#/c/14102/

Comment by Andreas Dilger [ 18/Mar/15 ]

I think the correct fix is as Niu writes. The patch that was landed upstream didn't check I_NEW, but instead directly checked if the inode already had the quota initialized. The lustre/kernel_patches/patches/quota-avoid-dqget-calls patches should be updated to use the same code as was committed to the upstream kernel.

commit 1ea06bec78a128adc995ca32bd906a6c9bb9cf91
Author: Niu Yawei <yawei.niu@gmail.com>
Date:   Wed Jun 4 12:20:30 2014 +0800

    quota: avoid unnecessary dqget()/dqput() calls
    
    Avoid unnecessary dqget()/dqput() calls in __dquot_initialize(),
    that will introduce global lock contention otherwise.
    
    Signed-off-by: Lai Siyao <lai.siyao@intel.com>
    Signed-off-by: Niu Yawei <yawei.niu@intel.com>
    Signed-off-by: Jan Kara <jack@suse.cz>

diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index 7f30bdc..2517719 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -1402,7 +1402,7 @@ static int dquot_active(const struct inode *inode)
  */
 static void __dquot_initialize(struct inode *inode, int type)
 {
-       int cnt;
+       int cnt, init_needed = 0;
        struct dquot *got[MAXQUOTAS];
        struct super_block *sb = inode->i_sb;
        qsize_t rsv;
@@ -1418,6 +1418,15 @@ static void __dquot_initialize(struct inode *inode, int type)
                got[cnt] = NULL;
                if (type != -1 && cnt != type)
                        continue;
+               /*
+                * The i_dquot should have been initialized in most cases,
+                * we check it without locking here to avoid unnecessary
+                * dqget()/dqput() calls.
+                */
+               if (inode->i_dquot[cnt])
+                       continue;
+               init_needed = 1;
+
                switch (cnt) {
                case USRQUOTA:
                        qid = make_kqid_uid(inode->i_uid);
@@ -1429,6 +1438,10 @@ static void __dquot_initialize(struct inode *inode, int type)
                got[cnt] = dqget(sb, qid);
        }
 
+       /* All required i_dquot has been initialized */
+       if (!init_needed)
+               return;
+
        down_write(&sb_dqopt(sb)->dqptr_sem);
        if (IS_NOQUOTA(inode))
                goto out_err;
Comment by nasf (Inactive) [ 19/Mar/15 ]

Since it has been committed to upstream, why still patch the kernel? As for the locked inode returned by ldiskfs_create_inode(), we can unlock it after the LDISKFS_STATE_LUSTRE_NOSCRUB set. It is unnecessary to hold the lock for osd_attr_init().

Anyway, I do not mind the way of fixing quota patch.

Comment by Andreas Dilger [ 19/Mar/15 ]

It is only in the upstream kernel after 3.16 or so, but we are applying the older patch to RHEL 6 and SLES 11 that checks I_NEW.

If the current patch doesn't cause problems with LFSCK, then I don't mind to land it also, because holding a lock for less time is always better (assuming it is still correct). I'd still like to fix the quota patch, because without that the performance will still be bad for non-root users.

Comment by nasf (Inactive) [ 19/Mar/15 ]

I have refreshed the patch http://review.whamcloud.com/#/c/14102/ with this LU ticket.

Niu, would you please to port your quota patch to master branch? Thanks!

Comment by Gerrit Updater [ 23/Mar/15 ]

Niu Yawei (yawei.niu@intel.com) uploaded a new patch: http://review.whamcloud.com/14135
Subject: LU-6378 kernel: simplify quota-avoid-dqget-call.patch
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 6fbe23b01f38847211334153330642eaf54b949a

Comment by Niu Yawei (Inactive) [ 23/Mar/15 ]

Niu, would you please to port your quota patch to master branch? Thanks!

Patch updated: http://review.whamcloud.com/14135

Comment by Gerrit Updater [ 26/Mar/15 ]

Oleg Drokin (oleg.drokin@intel.com) merged in patch http://review.whamcloud.com/14135/
Subject: LU-6378 kernel: simplify quota-avoid-dqget-call.patch
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: 8b83f21acfd00c0dd8aeed529c07fbc1ecca5e22

Comment by nasf (Inactive) [ 20/Apr/15 ]

Oleg, would you please to consider the patch http://review.whamcloud.com/#/c/14102/, then we can close this ticket. Thanks!

Comment by Gerrit Updater [ 28/Apr/15 ]

Oleg Drokin (oleg.drokin@intel.com) merged in patch http://review.whamcloud.com/14102/
Subject: LU-6378 osd-ldiskfs: unlock inode before attr_init
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: cef4a7fdb1183deaf2cbb34849a3f9f7b9037da1

Comment by nasf (Inactive) [ 29/Apr/15 ]

The patches have been landed to master.

Generated at Sat Feb 10 01:59:41 UTC 2024 using Jira 9.4.14#940014-sha1:734e6822bbf0d45eff9af51f82432957f73aa32c.