On ftp.whamcloud.com (uploading now):
uploads/LU-6227/
Dump, log, vmlinux and ko files:
150209_full_debug.tar.gz
Test script and test:
test.sh
diotest1
Transfer died... Trying again: 150209_full_debug2.tar.gz
I've also made it available for non-Intel folks on the Cray FTP.
ftp.cray.com
anonynmous
outbound/xyratex/lu-6227
Files:
150209_full_debug.tar.gz
diotest1
test.sh
Patrick, the test was added with patch - test_241 sanity.sh dio vs bio. And we saw regression for it without the fix. Yeap, the issue looks like LU-3192, and requires further analyze.
It looks like the client is not invalidating the buffered page from cache before submitting the dio page. There are a couple of solutions to that. One us to actually drop the buffered page, the other is to disallow merging these into a single RPC.
My understanding is that http://review.whamcloud.com/#/c/10930 took the second route - Disallowing merging - but it appears that it's not working any more. (It fixed the problem originally, at least in 2.5 for Cray. I'm not sure we tested it on master at the time.)
Base on the assert message
<0>LustreError: 7700:0:(osc_request.c:1219:osc_brw_prep_request()) ASSERTION( i == 0 || pg->off > pg_prev->off ) failed: i 3 p_c 10 pg ffffea00017a5208 [pri 0 ind 2771] off 16384 prev_pg ffffea00017a51d0 [pri 0 ind 2256] off 16384
The type of both pages is direct io, and they has the same offset. So this is not LU-3192, when we have mix with dio and buffered page.
I have found the commit which introduce this regression.
commit e6f592fc57b048223e2676408232b6662aad712d
Author: Prakash Surya <surya1@llnl.gov>
Date: Wed Oct 2 17:16:51 2013 -0700
LU-1669 vvp: Use lockless __generic_file_aio_write
Testing multi-threaded single shard file write performance has shown
the inode mutex to be a limiting factor when using the
generic_file_aio_write function. To work around this bottle neck, this
change replaces the locked version of that call with the lock less
version, specifically, __generic_file_aio_write.
Hmmm. Unfortunately, that makes some sense... The purpose of that patch was to allow multiple threads on a single client to write to one file at the same time, so there are some possible concurrency issues.
I wonder if this issue is only in the context of direct IO... Obviously this crash is with direct IO pages, but I wonder about the underlying problem.
I think the parllel io is still guarded with some sort of range lock, so should we better understand why did that not help?
I looked at this the other day, and the range lock is taken in ll_file_io_generic, which is not used in the direct IO path.
The two conflicting pages in this instance are created by calls in to ll_direct_IO_26, then the assert fires while pages are being written out after, somehow in the process of an ll_file_read call.
The ll_file_read call uses the range lock, but two conflicting pages are already present at that time. The direct IO path does not use the range lock. Should it?
I actually don't see how the patch called out by Alex Boyko above changes the behavior of the direct IO path in a way that would have prevented this issue - It only takes the inode mutex on the read path. What about the write path? (I'm not arguing the patch isn't source of the problem, just saying I can't see how. It seems there's something, perhaps several somethings, I don't understand here. 
)
Looking closer...
The particular crash is two reads competing.
00000008:00040000:3.0:1423457026.801250:0:7700:0:(osc_request.c:1219:osc_brw_prep_request()) ASSERTION( i == 0 || pg->off > pg_prev->off ) failed: i 3 p_c 10 pg ffffea00017a5208 [pri 0 ind 2771] off 16384 prev_pg ffffea00017a51d0 [pri 0 ind 2256] off 16384
00000080:00000001:2.0:1423457026.799792:0:1562:0:(file.c:1285:ll_file_aio_read()) Process entered
00000020:00001000:2.0:1423457026.799792:0:1562:0:(cl_object.c:849:cl_env_peek()) 2@ffff88013b8cebb8
00000080:00000001:2.0:1423457026.799793:0:1562:0:(file.c:1145:ll_file_io_generic()) Process entered
00000080:00200000:2.0:1423457026.799793:0:1562:0:(file.c:1148:ll_file_io_generic()) file: infile, type: 0 ppos: 16384, count: 8192
00000020:00008000:2.0:1423457026.800026:0:1562:0:(cl_page.c:250:cl_page_find()) 4@[0x20000a040:0x316:0x0] ffffea00017a51d0 0 2
00000008:00000040:2.0:1423457026.800036:0:1562:0:(osc_cache.c:2272:osc_prep_async_page()) oap ffff8800bf9d7550 page ffffea00017a51d0 obj off 16384
00000080:00000001:3.0:1423457026.800382:0:1915:0:(file.c:1285:ll_file_aio_read()) Process entered
00000020:00001000:3.0:1423457026.800383:0:1915:0:(cl_object.c:849:cl_env_peek()) 2@ffff88005c8eb9c0
00000080:00000001:3.0:1423457026.800384:0:1915:0:(file.c:1145:ll_file_io_generic()) Process entered
00000080:00200000:3.0:1423457026.800384:0:1915:0:(file.c:1148:ll_file_io_generic()) file: infile, type: 0 ppos: 16384, count: 8192
00000020:00008000:3.0:1423457026.800593:0:1915:0:(cl_page.c:250:cl_page_find()) 4@[0x20000a040:0x316:0x0] ffffea00017a5208 0 2
00000008:00000040:3.0:1423457026.800603:0:1915:0:(osc_cache.c:2272:osc_prep_async_page()) oap ffff8800bf31e750 page ffffea00017a5208 obj off 16384
Reads do not take the range lock, but for direct IO, this can mean they try to read the same page at the same time.
This doesn't happen for non direct IO reads - Presumably this happens for direct IO because some of the other locking machinery is disabled/not used.
I'll submit a patch to use the range lock for direct IO reads... But if someone else feels this concurrency should be handled at another layer, please let me know where and how.
Patrick Farrell (paf@cray.com) uploaded a new patch: http://review.whamcloud.com/14385
Subject: LU-6227 vvp: Use range lock for direct IO reads
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 462a5a1cac1e20c07fcc1eab5715dc370b4f71d4
The patch submitted above passes testing with Cray's reproducer for this bug.
The bug was introduced specifically in this part of the change highlighted by Alexander:
http://review.whamcloud.com/#/c/6672/12/lustre/llite/rw26.c
The comment there does not mention that DIO reads must also be kept separate from one another, so it appears safe to remove the concurrency control, as the range locking covers the cases described in that comment.
Hi Patrick,
I didn't read all the comments above but the patch. Isn't it a better idea to take inode mutex in direct IO path?
Jinshan -
I think not, for the same reason we don't take it (any more) in the non-direct IO path. It allows parallelism between multiple threads doing direct IO. Why do you think we should take the mutex?
Also, if the old comment removed by LU-1669 was right, we also need to protect it from the buffered IO path, and since that path doesn't take the mutex any more, that means the range lock:
/* 0. Need locking between buffered and direct access. and race with
- size changing by concurrent truncates and writes.
- 1. Need inode mutex to operate transient pages.
*/
if (rw == READ)
mutex_lock(&inode->i_mutex);
Oleg Drokin (oleg.drokin@intel.com) merged in patch http://review.whamcloud.com/14385/
Subject: LU-6227 vvp: Use range lock for direct IO reads
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: 5b464b45746153889e3dead4e2254d3ebda77f8d
Landed for 2.8
ORNL just ran into this for our 2.7 clients. Looks like a back port is needed.
James, the affects version is 2.7.0. So, of course it's needed there. The patch shouldn't need any particular porting, though.