xfs
[Top] [All Lists]

Re: concurrent direct IO write in xfs

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: concurrent direct IO write in xfs
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 14 Feb 2012 10:07:31 +1100
Cc: Zheng Da <zhengda1936@xxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20120213174806.GA7630@xxxxxxxxxxxxx>
References: <CAFLer83FBZG9ZCrT2jUZBcTC2a2tx_CDmykyPF4cTP0dbHGw7Q@xxxxxxxxxxxxxx> <20120116232549.GC6922@dastard> <CAFLer81XkMTh_gxd95pzxCEs1yGRsTrZijX3c7ewgRzeA7DCSQ@xxxxxxxxxxxxxx> <20120123051155.GI15102@dastard> <CAFLer82QxfgXEx7ofzOHOK2YKiA+ab+_Aizd10SWHvnC-mVUHg@xxxxxxxxxxxxxx> <CAFLer81GWSCCCMppU=2dE+5KKqD-hYVKAA0hz9n-CBbxAs_xfw@xxxxxxxxxxxxxx> <20120124035431.GD6922@dastard> <20120209060920.GF7479@dastard> <20120213174806.GA7630@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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

<Prev in Thread] Current Thread [Next in Thread>