xfs
[Top] [All Lists]

Re: [PATCH 08/10] xfs: move the inode locking outside xfs_fsync()

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 08/10] xfs: move the inode locking outside xfs_fsync()
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 4 Feb 2010 10:08:33 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20100203112917.GB19996@xxxxxxxxxxxxx>
References: <1265153104-29680-1-git-send-email-david@xxxxxxxxxxxxx> <1265153104-29680-9-git-send-email-david@xxxxxxxxxxxxx> <20100203112917.GB19996@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
On Wed, Feb 03, 2010 at 06:29:17AM -0500, Christoph Hellwig wrote:
> On Wed, Feb 03, 2010 at 10:25:02AM +1100, Dave Chinner wrote:
> > We have a need for a delayed write inode flush operation
> > to be made atomically with an fsync to avoid physically
> > writing inodes but still keeping inode buffer information
> > up to date for bulkstat.
> > 
> > Move the inode locking outside xfs_fsync() to allow this to
> > be done.
> 
> What's the point of the lock_flags argument?  It should always
> be IOLOCK_SHARED, so instead of passing it in as an argument
> I'd rather add an assert to enforce it.

Fair enough. Updated patch below.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

xfs: move the inode locking outside xfs_fsync() V2

We have a need for a delayed write inode flush operation
to be made atomically with an fsync to avoid physically
writing inodes but still keeping inode buffer information
up to date for bulkstat.

Move the inode locking outside xfs_fsync() to allow this to
be done.

Version 2
- kill the lock_flags argument and simply assert XFS_IOLOCK_SHARED
  in xfs_fsync().

Signed-off-by: Dave Chinner <david@xxxxxxxxxxxxx>
---
 fs/xfs/linux-2.6/xfs_file.c |    6 +++++-
 fs/xfs/linux-2.6/xfs_lrw.c  |    3 ++-
 fs/xfs/xfs_vnodeops.c       |   29 +++++++++++++----------------
 3 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_file.c b/fs/xfs/linux-2.6/xfs_file.c
index e4caeb2..94d9d6d 100644
--- a/fs/xfs/linux-2.6/xfs_file.c
+++ b/fs/xfs/linux-2.6/xfs_file.c
@@ -177,9 +177,13 @@ xfs_file_fsync(
        int                     datasync)
 {
        struct xfs_inode        *ip = XFS_I(dentry->d_inode);
+       int                     error;
 
        xfs_iflags_clear(ip, XFS_ITRUNCATED);
-       return -xfs_fsync(ip);
+       xfs_ilock(ip, XFS_ILOCK_SHARED);
+       error = -xfs_fsync(ip);
+       xfs_iunlock(ip, XFS_ILOCK_SHARED);
+       return error;
 }
 
 STATIC int
diff --git a/fs/xfs/linux-2.6/xfs_lrw.c b/fs/xfs/linux-2.6/xfs_lrw.c
index c80fa00..d7f1a71 100644
--- a/fs/xfs/linux-2.6/xfs_lrw.c
+++ b/fs/xfs/linux-2.6/xfs_lrw.c
@@ -754,11 +754,12 @@ write_retry:
                        error = error2;
                if (need_i_mutex)
                        mutex_lock(&inode->i_mutex);
-               xfs_ilock(xip, iolock);
+               xfs_ilock(xip, iolock | XFS_ILOCK_SHARED);
 
                error2 = xfs_fsync(xip);
                if (!error)
                        error = error2;
+               xfs_iunlock(xip, XFS_ILOCK_SHARED);
        }
 
  out_unlock_internal:
diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
index 43241e2..b5689a4 100644
--- a/fs/xfs/xfs_vnodeops.c
+++ b/fs/xfs/xfs_vnodeops.c
@@ -590,6 +590,17 @@ xfs_readlink(
  * the I/O lock while flushing the data, and the inode lock while flushing the
  * inode.  The inode lock CANNOT be held while flushing the data, so acquire
  * after we're done with that.
+ *
+ * We always need to make sure that the required inode state is safe on disk.
+ * The inode might be clean but we still might need to force the log because of
+ * committed transactions that haven't hit the disk yet.  Likewise, there could
+ * be unflushed non-transactional changes to the inode core that have to go to
+ * disk and this requires us to issue a synchronous transaction to capture
+ * these changes correctly.
+ *
+ * This code relies on the assumption that if the update_* fields of the inode
+ * are clear and the inode is unpinned then it is clean and no action is
+ * required.
  */
 int
 xfs_fsync(
@@ -600,24 +611,11 @@ xfs_fsync(
        int             log_flushed = 0;
 
        xfs_itrace_entry(ip);
+       ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED));
 
        if (XFS_FORCED_SHUTDOWN(ip->i_mount))
                return XFS_ERROR(EIO);
 
-       /*
-        * We always need to make sure that the required inode state is safe on
-        * disk.  The inode might be clean but we still might need to force the
-        * log because of committed transactions that haven't hit the disk yet.
-        * Likewise, there could be unflushed non-transactional changes to the
-        * inode core that have to go to disk and this requires us to issue
-        * a synchronous transaction to capture these changes correctly.
-        *
-        * This code relies on the assumption that if the update_* fields
-        * of the inode are clear and the inode is unpinned then it is clean
-        * and no action is required.
-        */
-       xfs_ilock(ip, XFS_ILOCK_SHARED);
-
        if (!ip->i_update_core) {
                /*
                 * Timestamps/size haven't changed since last inode flush or
@@ -627,7 +625,6 @@ xfs_fsync(
                 * disk yet, the inode will be still be pinned.  If it is,
                 * force the log.
                 */
-               xfs_iunlock(ip, XFS_ILOCK_SHARED);
                if (xfs_ipincount(ip)) {
                        error = _xfs_log_force(ip->i_mount, XFS_LOG_SYNC,
                                               &log_flushed);
@@ -662,7 +659,7 @@ xfs_fsync(
                xfs_trans_set_sync(tp);
                error = _xfs_trans_commit(tp, 0, &log_flushed);
 
-               xfs_iunlock(ip, XFS_ILOCK_EXCL);
+               xfs_ilock_demote(ip, XFS_ILOCK_EXCL);
        }
 
        if (ip->i_mount->m_flags & XFS_MOUNT_BARRIER) {

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