On Mon, Feb 13, 2012 at 12:48:06PM -0500, Christoph Hellwig wrote:
> On Thu, Feb 09, 2012 at 05:09:20PM +1100, Dave Chinner wrote:
> > if (create) {
> > - lockmode = XFS_ILOCK_EXCL;
> > + /*
> > + * For direct IO, we lock in shared mode so that write
> > + * operations that don't require allocation can occur
> > + * concurrently. The ilock has to be dropped over the allocation
> > + * transaction reservation, so the only thing the ilock is
> > + * providing here is modification exclusion. i.e. there is no
> > + * need to hold the lock exclusive.
> > + *
> > + * For buffered IO, if we need to do delayed allocation then
> > + * hold the ilock exclusive so that the lookup and delalloc
> > + * reservation is atomic.
> > + */
> > + if (direct)
> > + lockmode = XFS_ILOCK_SHARED;
> > + else
> > + lockmode = XFS_ILOCK_EXCL;
> > xfs_ilock(ip, lockmode);
> > } else {
> > lockmode = xfs_ilock_map_shared(ip);
>
> We'll actually need to use xfs_ilock_map_shared for the the direct
> create case too, to make sure we have the exclusive lock when we first
> read the extent list in.
Good point.
> Also xfs_qm_dqattach_locked really wants the inode locked exclusively,
> which your current code doesn't handle.
I didn't consider quotas. Looking at the code, it seems that it
wants an exclusive lock purely for ensuring there are no races
attaching the dquots to the inode. The xfs_dqget() code can actually
drop and regain the ilock, so I can't see that it is for any other
specific purpose.
I think that we could probably attach the dquot after dropping the
shared lock but before starting the transaction via a call to
xfs_qm_dqattach() which handles the locking internally. It does mean
an extra lock traversal for the quota case, but it still allows the
initial mapping to be done with a shared lock....
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
