[LU-3240] The link count is not updated after the mkdir

• Status: Resolved
• Project: Lustre
• Affects Version/s: Lustre 2.2.0, Lustre 2.3.0, Lustre 2.1.5
• Fix Version/s: Lustre 2.5.0, Lustre 2.4.2
• Type: Bug
• Priority: Blocker
• Reporter: Shuichi Ihara (Inactive)
• Assignee: Nathaniel Clark
• Resolution: Fixed
• Labels: mn1, patch

Environment

RHEL6.3

• Severity: 3

Description

We sometimes saw incorrect link counts after the mkdir or remove directories.
The link counts seem not to be updated after create/remove directory.

We created a simple reproducer for this problem below.

#!/bin/sh -x 

TESTDIR=/lustre/linkcount_test
[ -d "$TESTDIR" ] && rm -rf $TESTDIR

mkdir $TESTDIR
cd $TESTDIR
ls
mkdir a b
ls
ls -adl $TESTDIR
rmdir b
ls -ald

# ./test.sh
+ TESTDIR=/lustre/linkcount_test
+ '[' -d /lustre/linkcount_test ']'
+ rm -rf /lustre/linkcount_test
+ mkdir /lustre/linkcount_test
+ cd /lustre/linkcount_test
+ ls
+ mkdir a b
+ ls
a  b
+ ls -adl /lustre/linkcount_test
drwxr-xr-x 2 root root 4096 Apr 30 02:52 /lustre/linkcount_test
          ^^^
          This should be 4 after two directories created.
+ rmdir b
+ ls -ald
drwxr-xr-x 3 root root 4096 Apr 30 02:52 .
          ^^^
          Updated after an directory is removed.

This problem happens on b2_1, b2_2 and b2_3, but not happens if both server and clients are running with current master.
However, master (server) - b2_1(client), we see same problem.

Issue Links

• is related to: LU-3849 Client cache directory information is...
• is related to: LU-3385 missing permission lock check in ll_h...
• is related to: LU-4050 NFS reexport issue
• is related to: LU-4405 stat degradation (interoperability bu...
• is related to: LU-4095 Eliminate extra rpc when revalidating...

Comments

[Shuichi Ihara (Inactive)]

After analysis, we found which commit started not caused problem on current master.
commit 75ae281dac43534f65df0113a4bf5ccaf5aedca9 is related to this issue.

Especially, following codes changes in lustre/llite/name.c affects to this problem. If MDS_INODELOCK_PERM is removed from the codes, we saw same problem.

@@ -284,7 +284,7 @@ int ll_md_blocking_ast(struct ldlm_lock *lock, struct ldlm_lock_desc *desc,
 
 		if (inode->i_sb->s_root &&
 		    inode != inode->i_sb->s_root->d_inode &&
-		    (bits & MDS_INODELOCK_LOOKUP))
+		    (bits & (MDS_INODELOCK_LOOKUP | MDS_INODELOCK_PERM)))
 			ll_invalidate_aliases(inode);
                 iput(inode);
                 break;

Here is test results.

current master
# ./test.sh 
+ TESTDIR=/lustre/linkcount_test
+ '[' -d /lustre/linkcount_test ']'
+ rm -rf /lustre/linkcount_test
+ mkdir /lustre/linkcount_test
+ cd /lustre/linkcount_test
+ ls
+ mkdir a b
+ ls
a  b
+ ls -adl /lustre/linkcount_test
drwxr-xr-x 4 root root 4096 Apr 30 07:37 /lustre/linkcount_test
          ^^^
          the link count is updated correctly.
+ rmdir b
+ ls -ald
drwxr-xr-x 3 root root 4096 Apr 30 07:37 .

remove "MDS_INODELOCK_PERM" above codes and revert previous code on the client.
# ./test.sh
+ TESTDIR=/lustre/linkcount_test
+ '[' -d /lustre/linkcount_test ']'
+ rm -rf /lustre/linkcount_test
+ mkdir /lustre/linkcount_test
+ cd /lustre/linkcount_test
+ ls
+ mkdir a b
+ ls
a  b
+ ls -adl /lustre/linkcount_test
drwxr-xr-x 2 root root 4096 Apr 30 08:24 /lustre/linkcount_test
          ^^^
          not updated 
+ rmdir b
+ ls -ald
drwxr-xr-x 3 root root 4096 Apr 30 08:24 .

We still don't know, why this is related to not cause this problem and in any case, what is root cause of this problem...

[Peter Jones]

Nathaniel

Could you please look into this issue?

Thanks

Peter

[Andreas Dilger]

I did a quick test on my 2.1.3 system and could reproduce this. Fortunately, the link count is actually correct on another node and on disk, it is just a cache consistency issue on the client. Note that the link count on the client is "read only", so this could not lead to inconsistency on the server, but might cause programs like "find" or "ls -R" to miss scanning newly-created subdirectories.

[Shuichi Ihara (Inactive)]

Andreas,
yes, the link count is correct if we checked it on another client. And when the cache is clear (lctl set_param ldlm.namespaces.mdc.lru_size=clear) or umount/mount the Lustre, the link count is updated correctly.

And, "find" command detects incorrect link count, it showed the following warning messages. We saw this issue at the production system often. This was original problem we found, then started analysis and making a producer..

find: WARNING: Hard link count is wrong for `./test' (saw only
st_nlink=5 but we already saw 3 subdirectories): this may be a bug in
your file system driver. Automatically tur
ning on find's -noleaf option. Earlier results may have failed to
include directories that should have been searched.

[Shuichi Ihara (Inactive)]

Hi Nathaniel, Andreas,
Any ideas what this problem is? This quite happens at the production system..

[Andreas Dilger]

I think the root of the problem here is that creating a subdirectory should be revoking the MDS_INODELOCK_UPDATE bit (though allowing the MDS_INODELOCK_LOOKUP bit to remain on the client in a separate lock), and this needs to be re-fetched when doing a "stat" on the inode. This appears to be the case:

    vfs_getattr()
        ll_getattr_it()
            ll_inode_revalidate_it(ibits=(MDS_INODELOCK_UPDATE | MDS_INODELOCK_LOOKUP))

but this isn't getting the correct link count from the MDS. Just updating the link count on the creating client is not sufficient, since the same problem could exist on other clients also creating directories concurrently.

[Manisha salve (Inactive)]

By checking MDS_INODELOCK_UPDATE bit in ll_md_blocking_ast() will solve the issue.

                if (inode->i_sb->s_root &&
                    inode != inode->i_sb->s_root->d_inode &&
                    (bits & (MDS_INODELOCK_LOOKUP | MDS_INODELOCK_UPDATE)))
                        ll_invalidate_aliases(inode);

The ll_invalidate_aliases() will make the dentry invalid. Now as the part of revalidation , it will get the inode from MDS and will have correct nlink count.
I tried with this fix on mounting two clients creating directories inside same parent directory simultaneously. And it shows correct nlink count from both the clients.

[Andreas Dilger]

In addition to the question of whether this change will cause the inode to be invalidated, there is the separate question of whether it will remain cached on the client when the inode is not being modified... Can you please improve the test case to check whether repeated "stat" calls on either mountpoint do not result in extra lock enqueues on the MDS.

[Manisha salve (Inactive)]

Hi Andreas,
I have performed some tests as you suggested. From the test results my observation is:
With the suggested fix , inode gets invalidated on mkdir. Then stat on the directory will revalidate the inode by fetching it from MDS and will update the client's local cache. On the repetitive stat do not result in extra lock enqueues and uses the local cache inode only.

Following test results will verify the above claims.
(Client1 = C1, Client2 = C2)
Initially ldlm_stats is clean:
C1:
[root@pun-lsfs21-node07 lustre-release]# cat /proc/fs/lustre/mdt/lustre-MDT0000/exports/0@lo/ldlm_stats
snapshot_time 1368185120.170741 secs.usecs

C2:
[root@pun-lsfs21-node07 lustre-release]# cat /proc/fs/lustre/mdt/lustre-MDT0000/exports/192.168.100.183@tcp/ldlm_stats
snapshot_time 1368185109.744530 secs.usecs

• Run stat on C1 which will bring inode to its local cache.

C1: stat /mnt/lustre (One lock request from C1 gets en-queued on mds)
[root@pun-lsfs21-node07 lustre-release]# cat /proc/fs/lustre/mdt/lustre-MDT0000/exports/0@lo/ldlm_stats
snapshot_time 1368185270.884595 secs.usecs
ldlm_enqueue 1 samples [reqs]

• mkdir on C2 which will make lock cancel on client C1 and will invalidate the inode on client C2.

C2: mkdir /mnt/lustre/a (One lock request from C2 gets en-queued on mds and Lock cancel request from C1)
[root@pun-lsfs21-node07 lustre-release]# cat /proc/fs/lustre/mdt/lustre-MDT0000/exports/192.168.100.183@tcp/ldlm_stats
snapshot_time 1368185411.223637 secs.usecs
ldlm_enqueue 1 samples [reqs]

[root@pun-lsfs21-node07 lustre-release]# cat /proc/fs/lustre/mdt/lustre-MDT0000/exports/0@lo/ldlm_stats
snapshot_time 1368185406.840111 secs.usecs
ldlm_enqueue 1 samples [reqs]
ldlm_cancel 1 samples [reqs]
ldlm_bl_callback 1 samples [reqs]

Again stat on C2 will revalidate the inode and will get updated inode contents from MDS:
[root@pun-lsfs21-node07 lustre-release]# cat /proc/fs/lustre/mdt/lustre-MDT0000/exports/192.168.100.183@tcp/ldlm_stats
snapshot_time 1368187928.914755 secs.usecs
ldlm_enqueue 2 samples [reqs]

[Alexey Lyashkov]

                if (inode->i_sb->s_root &&
                    inode != inode->i_sb->s_root->d_inode &&
                    (bits & (MDS_INODELOCK_LOOKUP | MDS_INODELOCK_UPDATE)))
                        ll_invalidate_aliases(inode);

using a update bit to invalidate aliases is wrong. because it's bit should be protect an attributes, not a name - but you tried to remove whole information from a dcache, in that case new lookup will triggered. it's solve that particular issue, but too bad at all as need to send an additional rpc in case we need just lookup name without attributes (or need minimal attributes subset to process open an security checks).

that bug may reproduced easy

tmp/test> mkdir newdir; ls; find . -print
newdir
.
find: WARNING: Hard link count is wrong for `.' (saw only st_nlink=2 but we already saw 0 subdirectories): this may be a bug in your file system driver. Automatically turning on find's -noleaf option. Earlier results may have failed to include directories that should have been searched.
./newdir
tmp/test>

Xyratex plan to start work on that issue tomorrow.

[John Hammond]

Working on another issue I noticed that ll_have_me_lock() cannot clear MDS_INODELOCK_PERM from *bits since MDS_INODELOCK_MAXSHIFT is 4, while MDS_INODELOCK_PERM is 0x10:

int ll_have_md_lock(struct inode *inode, __u64 *bits,  ldlm_mode_t l_req_mode)
{
        struct lustre_handle lockh;
        ldlm_policy_data_t policy;
        ldlm_mode_t mode = (l_req_mode == LCK_MINMODE) ?
                                (LCK_CR|LCK_CW|LCK_PR|LCK_PW) : l_req_mode;
        struct lu_fid *fid;
        __u64 flags;
        int i;
        ENTRY;

        if (!inode)
               RETURN(0);

        fid = &ll_i2info(inode)->lli_fid;
        CDEBUG(D_INFO, "trying to match res "DFID" mode %s\n", PFID(fid),
               ldlm_lockname[mode]);

        flags = LDLM_FL_BLOCK_GRANTED | LDLM_FL_CBPENDING | LDLM_FL_TEST_LOCK;
        for (i = 0; i < MDS_INODELOCK_MAXSHIFT && *bits != 0; i++) {
                policy.l_inodebits.bits = *bits & (1 << i);
                if (policy.l_inodebits.bits == 0)
                        continue;
...

[Alexey Lyashkov]

Based in Panda (Andrew Perepechko <[e-mail]>) initial investigation problem is

• client have both open and lookup|update lock
• creating file drops a lookup|update lock
• readdir open a file via ll_local_open()
• readdir get a UPDATE lock to protect directory page cache, but forget to update a inode attributes as someone forget about update lock protects attributes except needs to open and security checks.
• mdt lost a part of intent handler (exist in 1.8 code) so don't send a inode body over network with update lock.
  -.....

BUG hit.

so.. drop openlock will caused full revalidate inode and ll_intent_open call so attributes will updates correctly.
or (and) ll_readdir need correct invalidate function to update inode body after lock grant, and need invested mdt part. 1.8 have full intent with lock replace and sending inode body similar to getattr - but 2.x lost that part.

ps. readdir forget update a inode attributes from a 1.4 (i don't have early version on quick access).

[Alexey Lyashkov]

in discussion with green, we found - readdir take a LCK_CR mode lock. it mode should be don't matched in revalidate (in theory), if we will able remove skipping revalidate we will have valid attributes in stat() syscall.

[Alexey Lyashkov]

bash-3.2$ git diff
diff --git a/lustre/llite/dir.c b/lustre/llite/dir.c
index 24ba16d..77ee138 100644
--- a/lustre/llite/dir.c
+++ b/lustre/llite/dir.c
@@ -368,7 +368,7 @@ struct page *ll_get_dir_page(struct file *filp, struct inode *dir, __u64 hash,
         struct ll_inode_info *lli = ll_i2info(dir);
         int hash64 = ll_i2sbi(dir)->ll_flags & LL_SBI_64BIT_HASH;
 
-        mode = LCK_PR;
+        mode = LCK_CR;
         rc = md_lock_match(ll_i2sbi(dir)->ll_md_exp, LDLM_FL_BLOCK_GRANTED,
                            ll_inode2fid(dir), LDLM_IBITS, &policy, mode, &lockh);
         if (!rc) {
diff --git a/lustre/mdc/mdc_locks.c b/lustre/mdc/mdc_locks.c
index 84389c8..21dbecd 100644
--- a/lustre/mdc/mdc_locks.c
+++ b/lustre/mdc/mdc_locks.c
@@ -923,15 +923,19 @@ int mdc_revalidate_lock(struct obd_export *exp, struct lookup_intent *it,
         struct lustre_handle lockh;
         ldlm_policy_data_t policy;
         ldlm_mode_t mode;
+       uint64_t lock_mode = LCK_PR|LCK_PW;
         ENTRY;
 
         fid_build_reg_res_name(fid, &res_id);
         policy.l_inodebits.bits = (it->it_op == IT_GETATTR) ?
                                   MDS_INODELOCK_UPDATE : MDS_INODELOCK_LOOKUP;
 
+       /* don't match agains open lock, that is have no guaranted to atributes
+        * to be valid */
+       lock_mode |= it->it_op == IT_GETATTR ? 0 : LCK_CW | LCK_CR;
         mode = ldlm_lock_match(exp->exp_obd->obd_namespace,
                                LDLM_FL_BLOCK_GRANTED, &res_id, LDLM_IBITS,
-                               &policy, LCK_CR|LCK_CW|LCK_PR|LCK_PW, &lockh, 0);
+                               &policy, lock_mode, &lockh, 0);
         if (mode) {
                 it->d.lustre.it_lock_handle = lockh.cookie;
                 it->d.lustre.it_lock_mode = mode;

that patch restore ~1.8.6 behavior (request LCK_CR instead of LCK_PR in readdir) and fix issue when attributes request matched against openlock.

my results after it

[root@rhel6-64 tests]# 
[root@rhel6-64 tmp]# sh bug1.sh 
a  b
drwxr-xr-x 4 root root 4096 May 17 09:53 /mnt/lustre/linkcount_test
drwxr-xr-x 3 root root 4096 May 17 09:53 .

Manisha,

may you test that patch and verify it?

[Alexey Lyashkov]

first part of bug introduced in cmd3 branch.

Index: dir.c
===================================================================
RCS file: /lustre/lustre-core/llite/dir.c,v
retrieving revision 1.54.2.7.10.6.2.1.4.4.2.38
retrieving revision 1.54.2.7.10.6.2.1.4.4.2.39
diff -u -r1.54.2.7.10.6.2.1.4.4.2.38 -r1.54.2.7.10.6.2.1.4.4.2.39
--- dir.c       13 Oct 2006 12:32:11 -0000      1.54.2.7.10.6.2.1.4.4.2.38
+++ dir.c       20 Oct 2006 11:27:35 -0000      1.54.2.7.10.6.2.1.4.4.2.39
@@ -278,14 +278,20 @@
         ldlm_policy_data_t policy = {.l_inodebits = {MDS_INODELOCK_UPDATE} };
         struct address_space *mapping = dir->i_mapping;
         struct lustre_handle lockh;
-        struct page *page;
         struct lu_dirpage *dp;
+        struct page *page;
+        ldlm_mode_t mode;
         int rc;
         __u32 start;
         __u32 end;
 
+#ifdef CONFIG_PDIROPS
+        mode = LCK_PR;
+#else
+        mode = LCK_CR;
+#endif
         rc = md_lock_match(ll_i2sbi(dir)->ll_md_exp, LDLM_FL_BLOCK_GRANTED,
-                           ll_inode2fid(dir), LDLM_IBITS, &policy, LCK_CR, &lockh);
+                           ll_inode2fid(dir), LDLM_IBITS, &policy, mode, &lockh);
         if (!rc) {
                 struct lookup_intent it = { .it_op = IT_READDIR };
                 struct ptlrpc_request *request;
@@ -296,7 +302,7 @@
                         return ERR_PTR(-ENOMEM);
 
                 rc = md_enqueue(ll_i2sbi(dir)->ll_md_exp, LDLM_IBITS, &it,
-                                LCK_CR, op_data, &lockh, NULL, 0,
+                                mode, op_data, &lockh, NULL, 0,
                                 ldlm_completion_ast, ll_md_blocking_ast, dir,
                                 0);

and put in HEAD with b_post_cmd3 merge.

[Manisha salve (Inactive)]

Verified the patch , works fine.

[Andreas Dilger]

The patch in LU-3385 http://review.whamcloud.com/6438 fixes the problem with MDS_INODELOCK_MAXSHIFT in 2.4, though I don't think this is relevant for 2.1. This bug was introduced in 2.2 via commit cc283673 (http://review.whamcloud.com/1170), but wasn't causing any problems until 2.4, because the higher MDS_INODELOCK bits weren't used on the clients.

[Andreas Dilger]

Shadow, can you please submit that patch to Gerrit?

[Alexander Boyko]

I have submitted patch http://review.whamcloud.com/6460.

[Patrick Farrell (Inactive)]

We're monitoring this one from the Cray side... I see the patch failed one of the sanityn tests, specifically test_13.

That's "test directory page revocation", and at least seems relevant to this patch. Thoughts/updates?

[Alexey Lyashkov]

Correct, that is related to that patch - unlink modification don't flush an page cache. That is may be solution for a lock ping-pong with unlinking a file in parallel to the create, but test failed.
I will prepare a new patch with correctly update an attributes after take a lock from MDT.

[Shuichi Ihara (Inactive)]

Any updates on this, new patches?

[Alexey Lyashkov]

I pushed refreshed version few days ago.

[Patrick Valentin (Inactive)]

I tested set 4 of patch http://review.whamcloud.com/6460 on a lustre 2.4.0 client, and it also fixes the issue described in LU-3849.

[Dmitry Eremin (Inactive)]

I tested Patch Set 5 of http://review.whamcloud.com/6460 with master, it partially solve the LU-4050.

[Cory Spitz]

Should 'Fix Version/s' == 2.5.0?

[Peter Jones]

Cory yes that would be more clear

[Jodi Levi (Inactive)]

Eliminating the extra roc when revalidating remote objects is being handled in LU-4095.

[Sebastien Buisson (Inactive)]

I am sorry, but in addition to patch http://review.whamcloud.com/6460 that was landed on master, I can also see patch http://review.whamcloud.com/7910 merged at the same time.
Where this patch is coming from? Is this a mandatory part of the solution to this bug? Or is this just a bonus patch?

TIA,
Sebastien.

[Patrick Farrell (Inactive)]

Sebastien,

As I understand it, that second patch is not required. Oleg commented (I can't find where right now) that it was unnecessary to send the parent FID when doing that operation, as the child FID is sufficient. I don't believe it fixes any specific problem, it's just cleanup/improvement.

To summarize:
http://review.whamcloud.com/6460 is, by itself, the full fix for LU-3240. 7910 is as you said - a bonus patch that came out of the work for 6460.

[Bob Glossman (Inactive)]

backport to b2_4
http://review.whamcloud.com/8317

[Jian Yu]

The patch was cherry-picked to Lustre b2_4 branch.

[Alexey Lyashkov]

please reopen a ticket due introducing an regressions with open with last Oleg changes
and removing one huge and important part from original patch.

ps. it's strange - but i don't able to reopen ticket.