Re: [PATCH 4/8] xfs: introduce xfs_rw_lock() helpers for locking the inode

[Dave Chinner <david@fromorbit.com>]

On Tue, Jan 04, 2011 at 07:54:41PM -0600, Alex Elder wrote:
> On Tue, 2011-01-04 at 15:48 +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > We need to obtain the i_mutex, i_iolock and i_ilock during the read
> > and write paths. Add a set of wrapper functions to neatly
> > encapsulate the lock ordering and shared/exclusive semantics to make
> > the locking easier to follow and get right.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> 
> I like this change, but I think you missed a lock call.
> I also notice there are some locking differences, and
> I don't really question them but I wonder if you can
> offer a little more explanation.
> 
> > ---
> >  fs/xfs/linux-2.6/xfs_file.c |  123 ++++++++++++++++++++++++-------------------
> >  1 files changed, 68 insertions(+), 55 deletions(-)
> > 
> > diff --git a/fs/xfs/linux-2.6/xfs_file.c b/fs/xfs/linux-2.6/xfs_file.c
> > index 33a688c..0d6111e 100644
> > --- a/fs/xfs/linux-2.6/xfs_file.c
> > +++ b/fs/xfs/linux-2.6/xfs_file.c
> 
> . . .
> 
> > @@ -262,22 +296,21 @@ xfs_file_aio_read(
> >  	if (XFS_FORCED_SHUTDOWN(mp))
> >  		return -EIO;
> >  
> > -	if (unlikely(ioflags & IO_ISDIRECT))
> > -		mutex_lock(&inode->i_mutex);
> > -	xfs_ilock(ip, XFS_IOLOCK_SHARED);
> > -
> >  	if (unlikely(ioflags & IO_ISDIRECT)) {
> > +		xfs_rw_ilock(ip, XFS_IOLOCK_EXCL);
> > +
> 
> Previously only XFS_IOLOCK_SHARED was used here.
> I understand that using the IOLOCK_EXCL now gets
> the desired mutex_lock() call.  Is the previous
> code in error here though?

No, it isn't wrong.  However, the new code takes advantage of the
demote process to drop back to IOLOCK_SHARED before the i_mutex is
dropped so there's no externally visible change in behaviour.

This means that the locking heirarchy is much easier to maintain and
understand as all operations use the same wrappers.  i.e. there is
no real need for a i_mutex + IOLOCK_SHARED state because i_mutex
means exclusive access and is the outer lock. Hence the
IOLOCK_SHARED vs IOLOCK_EXCL is pretty much irrelevant so I picked
the simpler one to implement and understand.

> Can you anticipate
> any different behavior because of this lock change?

None that I can think of, because we are supposed to hold the
i_mutex over page cache flushes and it is the outermost lock.

> Does this specific change justify separating it
> into a small patch just before this one?

IMO, not really, but I can if you want. It'll just churn the patch
series, though...

> >  		if (inode->i_mapping->nrpages) {
> >  			ret = -xfs_flushinval_pages(ip,
> >  					(iocb->ki_pos & PAGE_CACHE_MASK),
> >  					-1, FI_REMAPF_LOCKED);
> > +			if (ret) {
> > +				xfs_rw_iunlock(ip, XFS_IOLOCK_EXCL);
> > +				return ret;
> > +			}
> >  		}
> > -		mutex_unlock(&inode->i_mutex);
> > -		if (ret) {
> > -			xfs_iunlock(ip, XFS_IOLOCK_SHARED);
> > -			return ret;
> > -		}
> > -	}
> > +		xfs_rw_ilock_demote(ip, XFS_IOLOCK_EXCL);
> > +	} else
> > +		xfs_rw_ilock(ip, XFS_IOLOCK_SHARED);
> >  
> >  	trace_xfs_file_read(ip, size, iocb->ki_pos, ioflags);
> >  
> 
> . . .
> 
> > @@ -386,14 +419,13 @@ xfs_file_splice_write(
> >  	if (XFS_FORCED_SHUTDOWN(ip->i_mount))
> >  		return -EIO;
> >  
> > -	xfs_ilock(ip, XFS_IOLOCK_EXCL);
> > +	xfs_rw_ilock(ip, XFS_ILOCK_EXCL|XFS_IOLOCK_EXCL);
> 
> Similar sentiments here.  We will now be acquiring i_mutex
> here where previously we did not.  Is that OK?

It should be because it will have exactly the same serialisation
effect as the current XFS_IOLOCK_EXCL has - exclusive access across
the write.

> > @@ -631,21 +662,16 @@ xfs_file_aio_write(
> >  relock:
> >  	if (ioflags & IO_ISDIRECT) {
> >  		iolock = XFS_IOLOCK_SHARED;
> > -		need_i_mutex = 0;
> >  	} else {
> >  		iolock = XFS_IOLOCK_EXCL;
> > -		need_i_mutex = 1;
> > -		mutex_lock(&inode->i_mutex);
> >  	}
> >  
> > -	xfs_ilock(ip, XFS_ILOCK_EXCL|iolock);
> > -
> 
> Maybe I'm missing something, but I think you want to
> insert this here:
> 	xfs_rw_ilock(ip, XFS_ILOCK_EXCL|iolock);
> ...because (for starters) if generic_write_checks()
> returns an error below you're going to be calling
> the unlock routine.

Yes, good catch. I didn't notice that because I didn't test the
series patch by patch - just the end result. Will fix.

> 
> >  start:
> >  	ret = generic_write_checks(file, &pos, &count,
> >  					S_ISBLK(inode->i_mode));
> >  	if (ret) {
> > -		xfs_iunlock(ip, XFS_ILOCK_EXCL|iolock);
> > -		goto out_unlock_mutex;
> > +		xfs_rw_iunlock(ip, XFS_ILOCK_EXCL|iolock);
> > +		return ret;
> >  	}
> >  
> >  	if (ioflags & IO_ISDIRECT) {
> > @@ -654,16 +680,14 @@ start:
> >  				mp->m_rtdev_targp : mp->m_ddev_targp;
> >  
> >  		if ((pos & target->bt_smask) || (count & target->bt_smask)) {
> > -			xfs_iunlock(ip, XFS_ILOCK_EXCL|iolock);
> > +			xfs_rw_iunlock(ip, XFS_ILOCK_EXCL|iolock);
> >  			return XFS_ERROR(-EINVAL);
> >  		}
> >  
> 
> One can get a little lost in this code.  I don't know if
> this comment is exactly right, but something like it might
> be helpful (while you're in here).
> 
> 		/*
> 		 * For direct I/O, if there are cached pages or
> 		 * we're extending the file, we need IOLOCK_EXCL
> 		 * until we're sure the bytes at the new EOF have
> 		 * been zeroed and/or the cached pages are flushed
> 		 * out.  Upgrade the I/O lock and start again.
> 		 */

Probably a good idea. I'll add something there.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs