Bug 511170

Created attachment 353922 [details]
Network Trace file

I'm attaching a network trace that shows the reproduced problem.  Packets 65, 70, and 73 contain the READDIRPLUS packets that are used to populate the readdir cache.  The client indeed lists all 50 entries:

$ ls /mnt/readdir
f.1   f.13  f.17  f.20  f.24  f.28  f.31  f.35  f.39  f.42  f.46  f.5   f.8
f.10  f.14  f.18  f.21  f.25  f.29  f.32  f.36  f.4   f.43  f.47  f.50  f.9
f.11  f.15  f.19  f.22  f.26  f.3   f.33  f.37  f.40  f.44  f.48  f.6
f.12  f.16  f.2   f.23  f.27  f.30  f.34  f.38  f.41  f.45  f.49  f.7

As noted before, the instrumented client artificially invalidates the second page of the cache, forcing a subsequent 'ls' to issue an over-the-wire READDIRPLUS request for cookie '22' in packet 77.  Notice that the server replies with 21 entries (one short than in packet 70).  This is legal though.  The client should then issue a follow up READDIRPLUS request with cookie '43'.  Instead, the client uses the cached contents for the next page, missing entry f.47 altogether.

labiaga@royalflush$ ls /mnt/readdir
f.1   f.13  f.17  f.20  f.24  f.28  f.31  f.35  f.39  f.42  f.46  f.50  f.9
f.10  f.14  f.18  f.21  f.25  f.29  f.32  f.36  f.4   f.43  f.48  f.6
f.11  f.15  f.19  f.22  f.26  f.3   f.33  f.37  f.40  f.44  f.49  f.7
f.12  f.16  f.2   f.23  f.27  f.30  f.34  f.38  f.41  f.45  f.5   f.8

Created attachment 356164 [details]
patchset -- backport 2 fixes to code that calls invalidate_inode_pages2

Initial stab at a patchset to fix this. In addition to the patch that Ricardo pointed out, this backports an earlier patch that makes the code deal correctly with invalidate_inode_pages2 failures.

I suspect that that change is not really needed to fix this issue, but it seems reasonable and it makes the other patch apply cleanly.

Ricardo,
  Any chance you could pass along your fault injection patch so we can use it to verify the fix here? I'd also appreciate your opinion on the possible patchset in comment #3.

I'm also trying to determine why the fix for bug 364351 doesn't help this. It seems like we should be getting an iput on the dir since you're doing multiple ls's with the reproducer in comment #2.

(In reply to comment #4)
> Ricardo,
>   Any chance you could pass along your fault injection patch so we can use it
> to verify the fix here? 

The client side is pretty simple.  The diffs are from a pNFS git (2.6.31) tree but hopefully make sense in a vanilla 2.6.31 context.  The first nfs_readdir_filler() change simply backs out the fix and brings it to the equivalent RHEL5.3 behavior.  The nfs_do_filldir() changes invalidate the second page, so that a subsequent readdir request will be forced across the wire.

$ git diff dir.c
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 89f98e9..a765c99 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -208,10 +208,8 @@ int nfs_readdir_filler(nfs_readdir_descriptor_t *desc, stru
         * Note: assumes we have exclusive access to this mapping either
         *       through inode->i_mutex or some other mechanism.
         */
-       if (invalidate_inode_pages2_range(inode->i_mapping, page->index + 1, -1)
-               /* Should never happen */
-               nfs_zap_mapping(inode, inode->i_mapping);
-       }
+       if (page->index == 0)
+               invalidate_inode_pages2_range(inode->i_mapping, PAGE_CACHE_SIZE,
        unlock_page(page);
        return 0;
  error:
@@ -404,6 +402,10 @@ int nfs_do_filldir(nfs_readdir_descriptor_t *desc, void *di
                   filldir_t filldir)
 {
        struct file     *file = desc->file;
+/* XXXRLHACK */
+       struct inode    *inode = file->f_path.dentry->d_inode;
+/* XXXRLHACK */
+
        struct nfs_entry *entry = desc->entry;
        struct dentry   *dentry = NULL;
        u64             fileid;
@@ -439,7 +441,13 @@ int nfs_do_filldir(nfs_readdir_descriptor_t *desc, void *di
                *desc->dir_cookie = entry->cookie;
                if (dir_decode(desc) != 0) {
                        desc->page_index ++;
+/* XXXRL HACK to test */
+if (desc->page_index == 1) {
+       printk("Invalidating page 1\n");
+       invalidate_inode_pages2_range(inode->i_mapping, 1, 1);
+}
                        break;
+
                }
                if (loop_count++ > 200) {
                        loop_count = 0;

Unfortunately, the server side instrumentation was based on ONTAP 8 cluster-mode code.  It has not yet been released, and may not be straight forward for you to install inhouse.  I see two options:  

1) Instrument the Linux NFS server to do the equivalent behavior.  
2) Send me a binary (perhaps a virtual machine image) with the fix and the above instrumentation, and I can test it internally.  

#2 is definitely more expedient.

> I'd also appreciate your opinion on the possible
> patchset in comment #3.  

Seems reasonable.

P.S. I've already tested the recommended fix and it indeed addressed the problem.

Thanks for the info Ricardo. I'll see about getting a test kernel rolled up soon with your fault injection code. Do you have a RHEL5 host set up for it already? If so, what kernel variant is it using?

I also understand why the patch for bug 364351 doesn't help. We're not deallocating the dentry in this case, so dentry_iput doesn't get called.

We'd prefer to have a way to independently verify this, but it doesn't appear to be trivial to do a similar fault injection patch for knfsd. I'll look over the code a bit more before I give up on it, but it's probably not going to be worth it.

(In reply to comment #8)
> Do you have a RHEL5 host set up for it
> already? If so, what kernel variant is it using?

RHEL5.3: 2.6.18-128.el5 #1 SMP Wed Dec 17 11:42:39 EST 2008 i686 i686 i386 GNU/Linux

Created attachment 357200 [details]
patchset -- two patches + fault injection patch

Hi Ricardo,
Sorry for the delay. I built some test kernels with the attached patchset and put the i686 and srpm here:

http://people.redhat.com/jlayton/bz511170/

They have the patchset in comment #3 + the fault injection patch that you posted in comment #6. Can you test this kernel and verify that it fixes the problem?

Let me know if you need a different arch or kernel variant.

Thanks Jeff.  I verified that kernel-2.6.18-162.el5.bz511170.1.i686.rpm indeed addresses the problem when mounting the instrumented ONTAP filer.

I'll go ahead and have our QA group test on one of the RHEL5.3 machines that was failing in the "test production" and verify that it indeed addresses the problem in "real life".

Thanks Ricardo. Let me know what your QA group finds.

Ricardo, any feedback from your QA folks?

I hope to hear from them soon.

I should know more in about two weeks when the person who owns the test infrastructure returns to the office to test the patch.  As I mentioned before, I verified in my "fault injection" environment, but would be good to verify the fix in the environment that triggered the initial problem.

Thanks Ricardo. No huge hurry from our standpoint. The patch deadline for the next RHEL5 release is still quite a ways out.

Jeff, can you provide a 64-bit patch for test in our QA environment?  Thanks.

I assume you mean an x86_64 kernel package? If so, then yes but it'll take a day or so to rebuild. The build system has already cleaned out my last one.

Yes, x86_64.  Thanks.

Our QA team was unable to reproduce the problem after applying this patch.  Looks good to us, thanks for addressing this problem so rapidly!

Can we get a patch for RHEL5.3 as well, while the patch is going to be available in RHEL5.4?

(In reply to comment #22)
> Can we get a patch for RHEL5.3 as well, while the patch is going to be
> available in RHEL5.4?  

Sorry Bikash, missed your request...

The same patch will apply to 5.3 kernels with a little fuzz. YMMV with that patch, of course...

Thanks Jeff.
I think the question was more along the lines of an official patch.  Can you
integrate this fix into your patch stream for 5.3 and 5.4?

It would be better for that sort of request to bubble up from our support folks. Andrius, can you help drive that?

Jeff - there is no official path for NetApp through RH Support since they do not have TAM coverage. Let me bring in jwest, maybe he can approve this for errata streams...

This may be a good idea to have in 5.3. While the number of customers we have using 5.3 extended update support is relatively small, most of that number are customers who would be interested in having this patch.  I'll approve this from the support side for 5.3 and 5.4.  This still needs additional approvals however and isn't a guarantee. 

Thanks
Jeremy West

This request was evaluated by Red Hat Product Management for inclusion in a Red
Hat Enterprise Linux maintenance release.  Product Management has requested
further review of this request by Red Hat Engineering, for potential
inclusion in a Red Hat Enterprise Linux Update release for currently deployed
products.  This request is not yet committed for inclusion in an Update
release.

in kernel-2.6.18-168.el5
You can download this test kernel from http://people.redhat.com/dzickus/el5

Please do NOT transition this bugzilla state to VERIFIED until our QE team
has sent specific instructions indicating when to do so.  However feel free
to provide a comment indicating that this fix has been verified.

We are unable to reproduce the problem with the 2.6.18-168.el5 kernel on RHEL5.3.  So the fix is verified as far as we're concerned.

Ricardo - thanks for testing this! This will be closed when it is delivered in RHEL 5.5. Thanks again!

*** Bug 570061 has been marked as a duplicate of this bug. ***

An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHSA-2010-0178.html