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

[Joel Becker]

On Wed, Apr 14, 2010 at 01:58:20PM +0800, Li Dongyang wrote:
> On Wednesday 14 April 2010 07:54:35 Joel Becker wrote:
> > 	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?

	You're right, we use i_alloc_sem in the direct case and
ip_alloc_sem in the buffered case.  It is, however, for the same reason.
i_alloc_sem is about competing with the VFS (eg, vs vfs_truncate()).
ip_alloc_sem is about competing with ourselves (ocfs2_truncate(),
ocfs2_readpage(), etc).
	While I should be saying i_alloc_sem above for the direct I/O
case, the rest of the analysis is still correct.  We need to be holding
i_alloc_sem if we're going to be issuing direct I/Os, and we are not
holding it in the fallback to buffered case.

Joel

-- 

"Depend on the rabbit's foot if you will, but remember, it didn't
 help the rabbit."
	- R. E. Shay

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