[Ocfs2-devel] [PATCH] ocfs2: avoid direct write if we fall back to buffered

[Joel Becker]

On Mon, Apr 12, 2010 at 01:16:43PM +0800, Tao Ma wrote:
> Dong Yang Li wrote:
> > I still get a bug with this check and without my patch:
> yes, the check doesn't work actually in this case.
> > 
> > 
> > [16179.955148] (13400,1):ocfs2_truncate_file:465 ERROR: bug expression: le64_to_cpu(fe->i_size) != i_size_read(inode)
> > [16179.955157] (13400,1):ocfs2_truncate_file:465 ERROR: Inode 254789, inode i_size = 811008 != di i_size = 809011, i_flags = 0x1
> > the call trace is the same.
> > 
> > 
> > the problem is this check in ocfs2_direct_IO_get_blocks just check if we are going beyond the blocks right now,
> > so if a direct write won't play with new blocks but extending the i_size still get a pass, like the error above said, di->i_size is 809011, using 198 blocks and the direct write end up with i_size 811008, just same 198 blocks.
> yeah, you are right.

	I think Sunil and I have found the real culprit.
	If a file is opened for O_DIRECT, and there are no holes,
refcounts or anything, we are doing direct I/O.  ocfs2_file_aio_write()
(o_f_a_w() from now on) locks things down like so:  lock(i_mutex),
down_read(ip_alloc_sem), PR(rw_lock).  We have ip_alloc_sem preventing
size changes on the local node and rw_lock preventing size changes on
other nodes.  We call generic_file_direct_write() ourselves.
	If a file is not opened with O_DIRECT, we are doing regular
buffered writes.  o_f_a_w() locks like so: lock(i_mutex),
EX(rw_lock).  It is protecting against other nodes, but it does not
touch ip_alloc_sem.  Why?  Because we call __generic_file_aio_write(),
which will call ->write_begin().  ip_alloc_sem will be taken inside
->write_begin().  That's where we protect against other local processes.  
	You may already see where I'm going with this.  If we are open
with O_DIRECT, but we have to fall back to buffered, we will do this
locking:  lock(i_mutex), down_read(ip_alloc_sem), PR(rw_lock),
NL(rw_lock), up_read(ip_alloc_sem), EX(rw_lock).  That is, we start with
the direct I/O locking, then back off and do the buffered locking.  But
when we get into __g_f_a_w(), it will try the direct I/O again.  If the
leading portion of the I/O is capable of direct I/O, it will go into
direct mode *without ever taking ip_alloc_sem*.  Once it gets to the
portion of the I/O that cannot be done direct, it will fall back to
buffered for the rest of the I/O and will call ->write_begin() as
expected.
	So this I/O that extends i_size to the end of the allocation
will proceed as a direct I/O but will not have ip_alloc_sem.  Thus
truncate (and any other allocation change) can race on the local
machine.
	I think some form of Dong Yang's patch is going to be necessary.

Joel

-- 

Life's Little Instruction Book #306

	"Take a nap on Sunday afternoons."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127