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

[Li Dongyang]

Hi, Joel
On Wednesday 14 April 2010 07:54:35 Joel Becker wrote:
> 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.
> 
Thanks for the great explanation and analysis, but I only see we down write the
OCFS2_I(inode)->ip_alloc_sem in ->write_begin() and we are taking
inode->i_alloc_sem in o_f_a_w() when we try to do a direct write, not the ip_alloc_sem.
Am I missing something?

Br,
Li Dongyang
> Joel
>