[Fwd: [PATCH] Fix race in xfs_write() between direct and buffered I/O with DMAPI]

[Christoph Hellwig]

On Tue, Dec 09, 2008 at 04:10:21PM +1100, Lachlan McIlroy wrote:
> Thanks for looking at this Christoph.
> 
> I'm not sure what you mean by duplicating the checks.  I assume you
> mean this check:
> 
> 		if (!need_i_mutex && (mapping->nrpages || pos > xip->i_size)) {
> 			xfs_iunlock(xip, XFS_ILOCK_EXCL|iolock);
> 			iolock = XFS_IOLOCK_EXCL;
> 			need_i_mutex = 1;
> 			mutex_lock(&inode->i_mutex);
> 			xfs_ilock(xip, XFS_ILOCK_EXCL|iolock);
> 			goto start;
> 		}

Yes.

> This check is there because the default case for direct I/O is to
> acquire the iolock in shared mode.  If we have work to do which
> requires the iolock to be held exclusive then drop the lock and get
> it again.  Since we dropped the lock then restart.
> 
> In the dmapi post-write event it doesn't matter if we have the
> iolock shared or exclusive - it will be dropped regardless so I
> don't see how checking the state of the iolock will allow us to
> avoid a restart.

All very true, but it doesn't matter :)  When you do the goto start
after the dmapi post event you will run through the above check anyway,
and take the lock exclusive even if you don't need it.  By doing
the check right after the dmapi even you only run through the sequence
of checks leading to the above one guaranteed once, instead of
potentially twice (in addition to the initial once or twice before the
dmapi event).  Alternatively you could also have a flag that sais don't
bother with taking the lock exclusive that gets set after the dmapi
nospace even code.