* [PATCH 1/8] xfs: ensure sync write errors are returned
2011-01-10 23:37 [PATCH 0/8] xfs: prevent corruption due to overlapping AIO DIO V4 Dave Chinner
@ 2011-01-10 23:37 ` Dave Chinner
2011-01-10 23:37 ` [PATCH 2/8] xfs: factor common post-write isize handling code Dave Chinner
` (5 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Dave Chinner @ 2011-01-10 23:37 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
xfs_file_aio_write() only returns the error from synchronous
flushing of the data and inode if error == 0. At the point where
error is being checked, it is guaranteed to be > 0. Therefore any
errors returned by the data or fsync flush will never be returned.
Fix the checks so we overwrite the current error once and only if an
error really occurred.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Alex Elder <aelder@sgi.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/linux-2.6/xfs_file.c | 49 ++++++++++++++++++++----------------------
1 files changed, 23 insertions(+), 26 deletions(-)
diff --git a/fs/xfs/linux-2.6/xfs_file.c b/fs/xfs/linux-2.6/xfs_file.c
index ba8ad42..10b7fb4 100644
--- a/fs/xfs/linux-2.6/xfs_file.c
+++ b/fs/xfs/linux-2.6/xfs_file.c
@@ -574,7 +574,7 @@ xfs_file_aio_write(
struct inode *inode = mapping->host;
struct xfs_inode *ip = XFS_I(inode);
struct xfs_mount *mp = ip->i_mount;
- ssize_t ret = 0, error = 0;
+ ssize_t ret = 0;
int ioflags = 0;
xfs_fsize_t isize, new_size;
int iolock;
@@ -590,9 +590,9 @@ xfs_file_aio_write(
if (file->f_mode & FMODE_NOCMTIME)
ioflags |= IO_INVIS;
- error = generic_segment_checks(iovp, &nr_segs, &ocount, VERIFY_READ);
- if (error)
- return error;
+ ret = generic_segment_checks(iovp, &nr_segs, &ocount, VERIFY_READ);
+ if (ret)
+ return ret;
count = ocount;
if (count == 0)
@@ -616,9 +616,9 @@ relock:
xfs_ilock(ip, XFS_ILOCK_EXCL|iolock);
start:
- error = -generic_write_checks(file, &pos, &count,
+ ret = generic_write_checks(file, &pos, &count,
S_ISBLK(inode->i_mode));
- if (error) {
+ if (ret) {
xfs_iunlock(ip, XFS_ILOCK_EXCL|iolock);
goto out_unlock_mutex;
}
@@ -660,8 +660,8 @@ start:
*/
if (pos > ip->i_size) {
- error = xfs_zero_eof(ip, pos, ip->i_size);
- if (error) {
+ ret = -xfs_zero_eof(ip, pos, ip->i_size);
+ if (ret) {
xfs_iunlock(ip, XFS_ILOCK_EXCL);
goto out_unlock_internal;
}
@@ -674,8 +674,8 @@ start:
* by root. This keeps people from modifying setuid and
* setgid binaries.
*/
- error = -file_remove_suid(file);
- if (unlikely(error))
+ ret = file_remove_suid(file);
+ if (unlikely(ret))
goto out_unlock_internal;
/* We can write back this queue in page reclaim */
@@ -684,10 +684,10 @@ start:
if ((ioflags & IO_ISDIRECT)) {
if (mapping->nrpages) {
WARN_ON(need_i_mutex == 0);
- error = xfs_flushinval_pages(ip,
+ ret = -xfs_flushinval_pages(ip,
(pos & PAGE_CACHE_MASK),
-1, FI_REMAPF_LOCKED);
- if (error)
+ if (ret)
goto out_unlock_internal;
}
@@ -720,24 +720,22 @@ start:
}
} else {
int enospc = 0;
- ssize_t ret2 = 0;
write_retry:
trace_xfs_file_buffered_write(ip, count, iocb->ki_pos, ioflags);
- ret2 = generic_file_buffered_write(iocb, iovp, nr_segs,
+ ret = generic_file_buffered_write(iocb, iovp, nr_segs,
pos, &iocb->ki_pos, count, ret);
/*
* if we just got an ENOSPC, flush the inode now we
* aren't holding any page locks and retry *once*
*/
- if (ret2 == -ENOSPC && !enospc) {
- error = xfs_flush_pages(ip, 0, -1, 0, FI_NONE);
- if (error)
+ if (ret == -ENOSPC && !enospc) {
+ ret = xfs_flush_pages(ip, 0, -1, 0, FI_NONE);
+ if (ret)
goto out_unlock_internal;
enospc = 1;
goto write_retry;
}
- ret = ret2;
}
current->backing_dev_info = NULL;
@@ -753,7 +751,6 @@ write_retry:
xfs_iunlock(ip, XFS_ILOCK_EXCL);
}
- error = -ret;
if (ret <= 0)
goto out_unlock_internal;
@@ -762,23 +759,23 @@ write_retry:
/* Handle various SYNC-type writes */
if ((file->f_flags & O_DSYNC) || IS_SYNC(inode)) {
loff_t end = pos + ret - 1;
- int error2;
+ int error, error2;
xfs_iunlock(ip, iolock);
if (need_i_mutex)
mutex_unlock(&inode->i_mutex);
- error2 = filemap_write_and_wait_range(mapping, pos, end);
- if (!error)
- error = error2;
+ error = filemap_write_and_wait_range(mapping, pos, end);
if (need_i_mutex)
mutex_lock(&inode->i_mutex);
xfs_ilock(ip, iolock);
error2 = -xfs_file_fsync(file,
(file->f_flags & __O_SYNC) ? 0 : 1);
- if (!error)
- error = error2;
+ if (error)
+ ret = error;
+ else if (error2)
+ ret = error2;
}
out_unlock_internal:
@@ -800,7 +797,7 @@ write_retry:
out_unlock_mutex:
if (need_i_mutex)
mutex_unlock(&inode->i_mutex);
- return -error;
+ return ret;
}
STATIC int
--
1.7.2.3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 2/8] xfs: factor common post-write isize handling code
2011-01-10 23:37 [PATCH 0/8] xfs: prevent corruption due to overlapping AIO DIO V4 Dave Chinner
2011-01-10 23:37 ` [PATCH 1/8] xfs: ensure sync write errors are returned Dave Chinner
@ 2011-01-10 23:37 ` Dave Chinner
2011-01-10 23:37 ` [PATCH 4/8] xfs: introduce xfs_rw_lock() helpers for locking the inode Dave Chinner
` (4 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Dave Chinner @ 2011-01-10 23:37 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Alex Elder <aelder@sgi.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/linux-2.6/xfs_file.c | 54 ++++++++++++++++++++++--------------------
1 files changed, 28 insertions(+), 26 deletions(-)
diff --git a/fs/xfs/linux-2.6/xfs_file.c b/fs/xfs/linux-2.6/xfs_file.c
index 10b7fb4..b3915bf 100644
--- a/fs/xfs/linux-2.6/xfs_file.c
+++ b/fs/xfs/linux-2.6/xfs_file.c
@@ -321,6 +321,30 @@ xfs_file_splice_read(
return ret;
}
+STATIC void
+xfs_aio_write_isize_update(
+ struct inode *inode,
+ loff_t *ppos,
+ ssize_t bytes_written)
+{
+ struct xfs_inode *ip = XFS_I(inode);
+ xfs_fsize_t isize = i_size_read(inode);
+
+ if (bytes_written > 0)
+ XFS_STATS_ADD(xs_write_bytes, bytes_written);
+
+ if (unlikely(bytes_written < 0 && bytes_written != -EFAULT &&
+ *ppos > isize))
+ *ppos = isize;
+
+ if (*ppos > ip->i_size) {
+ xfs_ilock(ip, XFS_ILOCK_EXCL);
+ if (*ppos > ip->i_size)
+ ip->i_size = *ppos;
+ xfs_iunlock(ip, XFS_ILOCK_EXCL);
+ }
+}
+
STATIC ssize_t
xfs_file_splice_write(
struct pipe_inode_info *pipe,
@@ -331,7 +355,7 @@ xfs_file_splice_write(
{
struct inode *inode = outfilp->f_mapping->host;
struct xfs_inode *ip = XFS_I(inode);
- xfs_fsize_t isize, new_size;
+ xfs_fsize_t new_size;
int ioflags = 0;
ssize_t ret;
@@ -355,19 +379,8 @@ xfs_file_splice_write(
trace_xfs_file_splice_write(ip, count, *ppos, ioflags);
ret = generic_file_splice_write(pipe, outfilp, ppos, count, flags);
- if (ret > 0)
- XFS_STATS_ADD(xs_write_bytes, ret);
-
- isize = i_size_read(inode);
- if (unlikely(ret < 0 && ret != -EFAULT && *ppos > isize))
- *ppos = isize;
- if (*ppos > ip->i_size) {
- xfs_ilock(ip, XFS_ILOCK_EXCL);
- if (*ppos > ip->i_size)
- ip->i_size = *ppos;
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
- }
+ xfs_aio_write_isize_update(inode, ppos, ret);
if (ip->i_new_size) {
xfs_ilock(ip, XFS_ILOCK_EXCL);
@@ -576,7 +589,7 @@ xfs_file_aio_write(
struct xfs_mount *mp = ip->i_mount;
ssize_t ret = 0;
int ioflags = 0;
- xfs_fsize_t isize, new_size;
+ xfs_fsize_t new_size;
int iolock;
size_t ocount = 0, count;
int need_i_mutex;
@@ -740,22 +753,11 @@ write_retry:
current->backing_dev_info = NULL;
- isize = i_size_read(inode);
- if (unlikely(ret < 0 && ret != -EFAULT && iocb->ki_pos > isize))
- iocb->ki_pos = isize;
-
- if (iocb->ki_pos > ip->i_size) {
- xfs_ilock(ip, XFS_ILOCK_EXCL);
- if (iocb->ki_pos > ip->i_size)
- ip->i_size = iocb->ki_pos;
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
- }
+ xfs_aio_write_isize_update(inode, &iocb->ki_pos, ret);
if (ret <= 0)
goto out_unlock_internal;
- XFS_STATS_ADD(xs_write_bytes, ret);
-
/* Handle various SYNC-type writes */
if ((file->f_flags & O_DSYNC) || IS_SYNC(inode)) {
loff_t end = pos + ret - 1;
--
1.7.2.3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 4/8] xfs: introduce xfs_rw_lock() helpers for locking the inode
2011-01-10 23:37 [PATCH 0/8] xfs: prevent corruption due to overlapping AIO DIO V4 Dave Chinner
2011-01-10 23:37 ` [PATCH 1/8] xfs: ensure sync write errors are returned Dave Chinner
2011-01-10 23:37 ` [PATCH 2/8] xfs: factor common post-write isize handling code Dave Chinner
@ 2011-01-10 23:37 ` Dave Chinner
2011-01-11 17:36 ` Christoph Hellwig
2011-01-10 23:37 ` [PATCH 5/8] xfs: split direct IO write path from xfs_file_aio_write Dave Chinner
` (3 subsequent siblings)
6 siblings, 1 reply; 13+ messages in thread
From: Dave Chinner @ 2011-01-10 23:37 UTC (permalink / raw)
To: xfs
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.
Note that this changes some of the exclusive locking serialisation in
that serialisation will occur against the i_mutex instead of the
XFS_IOLOCK_EXCL. This does not change any behaviour, and it is
arguably more efficient to use the mutex for such serialisation than
the rw_sem.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
fs/xfs/linux-2.6/xfs_file.c | 136 ++++++++++++++++++++++++++-----------------
1 files changed, 82 insertions(+), 54 deletions(-)
diff --git a/fs/xfs/linux-2.6/xfs_file.c b/fs/xfs/linux-2.6/xfs_file.c
index c47d7dc0..941d7c2 100644
--- a/fs/xfs/linux-2.6/xfs_file.c
+++ b/fs/xfs/linux-2.6/xfs_file.c
@@ -41,6 +41,40 @@
static const struct vm_operations_struct xfs_file_vm_ops;
/*
+ * Locking primitives for read and write IO paths to ensure we consistently use
+ * and order the inode->i_mutex, ip->i_lock and ip->i_iolock.
+ */
+static inline void
+xfs_rw_ilock(
+ struct xfs_inode *ip,
+ int type)
+{
+ if (type & XFS_IOLOCK_EXCL)
+ mutex_lock(&VFS_I(ip)->i_mutex);
+ xfs_ilock(ip, type);
+}
+
+static inline void
+xfs_rw_iunlock(
+ struct xfs_inode *ip,
+ int type)
+{
+ xfs_iunlock(ip, type);
+ if (type & XFS_IOLOCK_EXCL)
+ mutex_unlock(&VFS_I(ip)->i_mutex);
+}
+
+static inline void
+xfs_rw_ilock_demote(
+ struct xfs_inode *ip,
+ int type)
+{
+ xfs_ilock_demote(ip, type);
+ if (type & XFS_IOLOCK_EXCL)
+ mutex_unlock(&VFS_I(ip)->i_mutex);
+}
+
+/*
* xfs_iozero
*
* xfs_iozero clears the specified range of buffer supplied,
@@ -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);
+
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);
@@ -285,7 +318,7 @@ xfs_file_aio_read(
if (ret > 0)
XFS_STATS_ADD(xs_read_bytes, ret);
- xfs_iunlock(ip, XFS_IOLOCK_SHARED);
+ xfs_rw_iunlock(ip, XFS_IOLOCK_SHARED);
return ret;
}
@@ -309,7 +342,7 @@ xfs_file_splice_read(
if (XFS_FORCED_SHUTDOWN(ip->i_mount))
return -EIO;
- xfs_ilock(ip, XFS_IOLOCK_SHARED);
+ xfs_rw_ilock(ip, XFS_IOLOCK_SHARED);
trace_xfs_file_splice_read(ip, count, *ppos, ioflags);
@@ -317,7 +350,7 @@ xfs_file_splice_read(
if (ret > 0)
XFS_STATS_ADD(xs_read_bytes, ret);
- xfs_iunlock(ip, XFS_IOLOCK_SHARED);
+ xfs_rw_iunlock(ip, XFS_IOLOCK_SHARED);
return ret;
}
@@ -338,10 +371,10 @@ xfs_aio_write_isize_update(
*ppos = isize;
if (*ppos > ip->i_size) {
- xfs_ilock(ip, XFS_ILOCK_EXCL);
+ xfs_rw_ilock(ip, XFS_ILOCK_EXCL);
if (*ppos > ip->i_size)
ip->i_size = *ppos;
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
+ xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
}
}
@@ -356,14 +389,22 @@ xfs_aio_write_newsize_update(
struct xfs_inode *ip)
{
if (ip->i_new_size) {
- xfs_ilock(ip, XFS_ILOCK_EXCL);
+ xfs_rw_ilock(ip, XFS_ILOCK_EXCL);
ip->i_new_size = 0;
if (ip->i_d.di_size > ip->i_size)
ip->i_d.di_size = ip->i_size;
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
+ xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
}
}
+/*
+ * xfs_file_splice_write() does not use xfs_rw_ilock() because
+ * generic_file_splice_write() takes the i_mutex itself. This, in theory,
+ * couuld cause lock inversions between the aio_write path and the splice path
+ * if someone is doing concurrent splice(2) based writes and write(2) based
+ * writes to the same inode. The only real way to fix this is to re-implement
+ * the generic code here with correct locking orders.
+ */
STATIC ssize_t
xfs_file_splice_write(
struct pipe_inode_info *pipe,
@@ -386,14 +427,13 @@ xfs_file_splice_write(
if (XFS_FORCED_SHUTDOWN(ip->i_mount))
return -EIO;
- xfs_ilock(ip, XFS_IOLOCK_EXCL);
+ xfs_ilock(ip, XFS_ILOCK_EXCL|XFS_IOLOCK_EXCL);
new_size = *ppos + count;
- xfs_ilock(ip, XFS_ILOCK_EXCL);
if (new_size > ip->i_size)
ip->i_new_size = new_size;
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
+ xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
trace_xfs_file_splice_write(ip, count, *ppos, ioflags);
@@ -604,7 +644,6 @@ xfs_file_aio_write(
xfs_fsize_t new_size;
int iolock;
size_t ocount = 0, count;
- int need_i_mutex;
XFS_STATS_INC(xs_write_calls);
@@ -631,21 +670,17 @@ 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);
-
start:
+ xfs_rw_ilock(ip, XFS_ILOCK_EXCL|iolock);
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 +689,20 @@ 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);
}
- if (!need_i_mutex && (mapping->nrpages || pos > ip->i_size)) {
- xfs_iunlock(ip, XFS_ILOCK_EXCL|iolock);
+ /*
+ * 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.
+ */
+ if (iolock != XFS_IOLOCK_EXCL &&
+ (mapping->nrpages || pos > ip->i_size)) {
+ xfs_rw_iunlock(ip, XFS_ILOCK_EXCL|iolock);
iolock = XFS_IOLOCK_EXCL;
- need_i_mutex = 1;
- mutex_lock(&inode->i_mutex);
- xfs_ilock(ip, XFS_ILOCK_EXCL|iolock);
goto start;
}
}
@@ -687,11 +726,11 @@ start:
if (pos > ip->i_size) {
ret = -xfs_zero_eof(ip, pos, ip->i_size);
if (ret) {
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
+ xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
goto out_unlock_internal;
}
}
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
+ xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
/*
* If we're writing the file then make sure to clear the
@@ -708,7 +747,7 @@ start:
if ((ioflags & IO_ISDIRECT)) {
if (mapping->nrpages) {
- WARN_ON(need_i_mutex == 0);
+ WARN_ON(iolock != XFS_IOLOCK_EXCL);
ret = -xfs_flushinval_pages(ip,
(pos & PAGE_CACHE_MASK),
-1, FI_REMAPF_LOCKED);
@@ -716,13 +755,10 @@ start:
goto out_unlock_internal;
}
- if (need_i_mutex) {
+ if (iolock == XFS_IOLOCK_EXCL) {
/* demote the lock now the cached pages are gone */
- xfs_ilock_demote(ip, XFS_IOLOCK_EXCL);
- mutex_unlock(&inode->i_mutex);
-
+ xfs_rw_ilock_demote(ip, XFS_IOLOCK_EXCL);
iolock = XFS_IOLOCK_SHARED;
- need_i_mutex = 0;
}
trace_xfs_file_direct_write(ip, count, iocb->ki_pos, ioflags);
@@ -740,7 +776,7 @@ start:
count -= ret;
ioflags &= ~IO_ISDIRECT;
- xfs_iunlock(ip, iolock);
+ xfs_rw_iunlock(ip, iolock);
goto relock;
}
} else {
@@ -775,14 +811,9 @@ write_retry:
loff_t end = pos + ret - 1;
int error, error2;
- xfs_iunlock(ip, iolock);
- if (need_i_mutex)
- mutex_unlock(&inode->i_mutex);
-
+ xfs_rw_iunlock(ip, iolock);
error = filemap_write_and_wait_range(mapping, pos, end);
- if (need_i_mutex)
- mutex_lock(&inode->i_mutex);
- xfs_ilock(ip, iolock);
+ xfs_rw_ilock(ip, iolock);
error2 = -xfs_file_fsync(file,
(file->f_flags & __O_SYNC) ? 0 : 1);
@@ -794,10 +825,7 @@ write_retry:
out_unlock_internal:
xfs_aio_write_newsize_update(ip);
- xfs_iunlock(ip, iolock);
- out_unlock_mutex:
- if (need_i_mutex)
- mutex_unlock(&inode->i_mutex);
+ xfs_rw_iunlock(ip, iolock);
return ret;
}
--
1.7.2.3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 4/8] xfs: introduce xfs_rw_lock() helpers for locking the inode
2011-01-10 23:37 ` [PATCH 4/8] xfs: introduce xfs_rw_lock() helpers for locking the inode Dave Chinner
@ 2011-01-11 17:36 ` Christoph Hellwig
2011-01-11 21:02 ` Dave Chinner
0 siblings, 1 reply; 13+ messages in thread
From: Christoph Hellwig @ 2011-01-11 17:36 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On Tue, Jan 11, 2011 at 10:37:44AM +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.
>
> Note that this changes some of the exclusive locking serialisation in
> that serialisation will occur against the i_mutex instead of the
> XFS_IOLOCK_EXCL. This does not change any behaviour, and it is
> arguably more efficient to use the mutex for such serialisation than
> the rw_sem.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> fs/xfs/linux-2.6/xfs_file.c | 136 ++++++++++++++++++++++++++-----------------
> 1 files changed, 82 insertions(+), 54 deletions(-)
>
> diff --git a/fs/xfs/linux-2.6/xfs_file.c b/fs/xfs/linux-2.6/xfs_file.c
> index c47d7dc0..941d7c2 100644
> --- a/fs/xfs/linux-2.6/xfs_file.c
> +++ b/fs/xfs/linux-2.6/xfs_file.c
> @@ -41,6 +41,40 @@
> static const struct vm_operations_struct xfs_file_vm_ops;
>
> /*
> + * Locking primitives for read and write IO paths to ensure we consistently use
> + * and order the inode->i_mutex, ip->i_lock and ip->i_iolock.
> + */
> +static inline void
> +xfs_rw_ilock(
> + struct xfs_inode *ip,
> + int type)
> +{
> + if (type & XFS_IOLOCK_EXCL)
> + mutex_lock(&VFS_I(ip)->i_mutex);
> + xfs_ilock(ip, type);
> +}
> +
> +static inline void
> +xfs_rw_iunlock(
> + struct xfs_inode *ip,
> + int type)
> +{
> + xfs_iunlock(ip, type);
> + if (type & XFS_IOLOCK_EXCL)
> + mutex_unlock(&VFS_I(ip)->i_mutex);
> +}
> +
> +static inline void
> +xfs_rw_ilock_demote(
> + struct xfs_inode *ip,
> + int type)
> +{
> + xfs_ilock_demote(ip, type);
> + if (type & XFS_IOLOCK_EXCL)
> + mutex_unlock(&VFS_I(ip)->i_mutex);
> +}
> +
> +/*
> * xfs_iozero
> *
> * xfs_iozero clears the specified range of buffer supplied,
> @@ -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);
> +
> 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);
>
> @@ -285,7 +318,7 @@ xfs_file_aio_read(
> if (ret > 0)
> XFS_STATS_ADD(xs_read_bytes, ret);
>
> - xfs_iunlock(ip, XFS_IOLOCK_SHARED);
> + xfs_rw_iunlock(ip, XFS_IOLOCK_SHARED);
> return ret;
> }
>
> @@ -309,7 +342,7 @@ xfs_file_splice_read(
> if (XFS_FORCED_SHUTDOWN(ip->i_mount))
> return -EIO;
>
> - xfs_ilock(ip, XFS_IOLOCK_SHARED);
> + xfs_rw_ilock(ip, XFS_IOLOCK_SHARED);
>
> trace_xfs_file_splice_read(ip, count, *ppos, ioflags);
>
> @@ -317,7 +350,7 @@ xfs_file_splice_read(
> if (ret > 0)
> XFS_STATS_ADD(xs_read_bytes, ret);
>
> - xfs_iunlock(ip, XFS_IOLOCK_SHARED);
> + xfs_rw_iunlock(ip, XFS_IOLOCK_SHARED);
> return ret;
> }
>
> @@ -338,10 +371,10 @@ xfs_aio_write_isize_update(
> *ppos = isize;
>
> if (*ppos > ip->i_size) {
> - xfs_ilock(ip, XFS_ILOCK_EXCL);
> + xfs_rw_ilock(ip, XFS_ILOCK_EXCL);
> if (*ppos > ip->i_size)
> ip->i_size = *ppos;
> - xfs_iunlock(ip, XFS_ILOCK_EXCL);
> + xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
> }
> }
>
> @@ -356,14 +389,22 @@ xfs_aio_write_newsize_update(
> struct xfs_inode *ip)
> {
> if (ip->i_new_size) {
> - xfs_ilock(ip, XFS_ILOCK_EXCL);
> + xfs_rw_ilock(ip, XFS_ILOCK_EXCL);
> ip->i_new_size = 0;
> if (ip->i_d.di_size > ip->i_size)
> ip->i_d.di_size = ip->i_size;
> - xfs_iunlock(ip, XFS_ILOCK_EXCL);
> + xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
> }
> }
>
> +/*
> + * xfs_file_splice_write() does not use xfs_rw_ilock() because
> + * generic_file_splice_write() takes the i_mutex itself. This, in theory,
> + * couuld cause lock inversions between the aio_write path and the splice path
> + * if someone is doing concurrent splice(2) based writes and write(2) based
> + * writes to the same inode. The only real way to fix this is to re-implement
> + * the generic code here with correct locking orders.
> + */
> STATIC ssize_t
> xfs_file_splice_write(
> struct pipe_inode_info *pipe,
> @@ -386,14 +427,13 @@ xfs_file_splice_write(
> if (XFS_FORCED_SHUTDOWN(ip->i_mount))
> return -EIO;
>
> - xfs_ilock(ip, XFS_IOLOCK_EXCL);
> + xfs_ilock(ip, XFS_ILOCK_EXCL|XFS_IOLOCK_EXCL);
>
> new_size = *ppos + count;
>
> - xfs_ilock(ip, XFS_ILOCK_EXCL);
> if (new_size > ip->i_size)
> ip->i_new_size = new_size;
> - xfs_iunlock(ip, XFS_ILOCK_EXCL);
> + xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
While using the xfs_rw_iunlock here actually is correct I think it's
highly confusing, given that we didn't use it to take the ilock. What
about just leaving all the code in xfs_file_splice_write as-is and not
touching it at all?
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 4/8] xfs: introduce xfs_rw_lock() helpers for locking the inode
2011-01-11 17:36 ` Christoph Hellwig
@ 2011-01-11 21:02 ` Dave Chinner
2011-01-11 21:03 ` Christoph Hellwig
2011-01-11 21:36 ` Alex Elder
0 siblings, 2 replies; 13+ messages in thread
From: Dave Chinner @ 2011-01-11 21:02 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Tue, Jan 11, 2011 at 12:36:27PM -0500, Christoph Hellwig wrote:
> On Tue, Jan 11, 2011 at 10:37:44AM +1100, Dave Chinner wrote:
> > +/*
> > + * xfs_file_splice_write() does not use xfs_rw_ilock() because
> > + * generic_file_splice_write() takes the i_mutex itself. This, in theory,
> > + * couuld cause lock inversions between the aio_write path and the splice path
> > + * if someone is doing concurrent splice(2) based writes and write(2) based
> > + * writes to the same inode. The only real way to fix this is to re-implement
> > + * the generic code here with correct locking orders.
> > + */
> > STATIC ssize_t
> > xfs_file_splice_write(
> > struct pipe_inode_info *pipe,
> > @@ -386,14 +427,13 @@ xfs_file_splice_write(
> > if (XFS_FORCED_SHUTDOWN(ip->i_mount))
> > return -EIO;
> >
> > - xfs_ilock(ip, XFS_IOLOCK_EXCL);
> > + xfs_ilock(ip, XFS_ILOCK_EXCL|XFS_IOLOCK_EXCL);
> >
> > new_size = *ppos + count;
> >
> > - xfs_ilock(ip, XFS_ILOCK_EXCL);
> > if (new_size > ip->i_size)
> > ip->i_new_size = new_size;
> > - xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > + xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
>
> While using the xfs_rw_iunlock here actually is correct I think it's
Argh! I thought I reverted all the changes to
xfs_file_splice_write().
> highly confusing, given that we didn't use it to take the ilock.
It definitely held the ilock around that size update before this
series. ;)
> What
> about just leaving all the code in xfs_file_splice_write as-is and not
> touching it at all?
Updated version below.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
xfs: introduce xfs_rw_lock() helpers for locking the inode
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.
Note that this changes some of the exclusive locking serialisation in
that serialisation will occur against the i_mutex instead of the
XFS_IOLOCK_EXCL. This does not change any behaviour, and it is
arguably more efficient to use the mutex for such serialisation than
the rw_sem.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
fs/xfs/linux-2.6/xfs_file.c | 131 ++++++++++++++++++++++++++-----------------
1 files changed, 80 insertions(+), 51 deletions(-)
diff --git a/fs/xfs/linux-2.6/xfs_file.c b/fs/xfs/linux-2.6/xfs_file.c
index c47d7dc0..b5e13fb 100644
--- a/fs/xfs/linux-2.6/xfs_file.c
+++ b/fs/xfs/linux-2.6/xfs_file.c
@@ -41,6 +41,40 @@
static const struct vm_operations_struct xfs_file_vm_ops;
/*
+ * Locking primitives for read and write IO paths to ensure we consistently use
+ * and order the inode->i_mutex, ip->i_lock and ip->i_iolock.
+ */
+static inline void
+xfs_rw_ilock(
+ struct xfs_inode *ip,
+ int type)
+{
+ if (type & XFS_IOLOCK_EXCL)
+ mutex_lock(&VFS_I(ip)->i_mutex);
+ xfs_ilock(ip, type);
+}
+
+static inline void
+xfs_rw_iunlock(
+ struct xfs_inode *ip,
+ int type)
+{
+ xfs_iunlock(ip, type);
+ if (type & XFS_IOLOCK_EXCL)
+ mutex_unlock(&VFS_I(ip)->i_mutex);
+}
+
+static inline void
+xfs_rw_ilock_demote(
+ struct xfs_inode *ip,
+ int type)
+{
+ xfs_ilock_demote(ip, type);
+ if (type & XFS_IOLOCK_EXCL)
+ mutex_unlock(&VFS_I(ip)->i_mutex);
+}
+
+/*
* xfs_iozero
*
* xfs_iozero clears the specified range of buffer supplied,
@@ -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);
+
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);
@@ -285,7 +318,7 @@ xfs_file_aio_read(
if (ret > 0)
XFS_STATS_ADD(xs_read_bytes, ret);
- xfs_iunlock(ip, XFS_IOLOCK_SHARED);
+ xfs_rw_iunlock(ip, XFS_IOLOCK_SHARED);
return ret;
}
@@ -309,7 +342,7 @@ xfs_file_splice_read(
if (XFS_FORCED_SHUTDOWN(ip->i_mount))
return -EIO;
- xfs_ilock(ip, XFS_IOLOCK_SHARED);
+ xfs_rw_ilock(ip, XFS_IOLOCK_SHARED);
trace_xfs_file_splice_read(ip, count, *ppos, ioflags);
@@ -317,7 +350,7 @@ xfs_file_splice_read(
if (ret > 0)
XFS_STATS_ADD(xs_read_bytes, ret);
- xfs_iunlock(ip, XFS_IOLOCK_SHARED);
+ xfs_rw_iunlock(ip, XFS_IOLOCK_SHARED);
return ret;
}
@@ -338,10 +371,10 @@ xfs_aio_write_isize_update(
*ppos = isize;
if (*ppos > ip->i_size) {
- xfs_ilock(ip, XFS_ILOCK_EXCL);
+ xfs_rw_ilock(ip, XFS_ILOCK_EXCL);
if (*ppos > ip->i_size)
ip->i_size = *ppos;
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
+ xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
}
}
@@ -356,14 +389,22 @@ xfs_aio_write_newsize_update(
struct xfs_inode *ip)
{
if (ip->i_new_size) {
- xfs_ilock(ip, XFS_ILOCK_EXCL);
+ xfs_rw_ilock(ip, XFS_ILOCK_EXCL);
ip->i_new_size = 0;
if (ip->i_d.di_size > ip->i_size)
ip->i_d.di_size = ip->i_size;
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
+ xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
}
}
+/*
+ * xfs_file_splice_write() does not use xfs_rw_ilock() because
+ * generic_file_splice_write() takes the i_mutex itself. This, in theory,
+ * couuld cause lock inversions between the aio_write path and the splice path
+ * if someone is doing concurrent splice(2) based writes and write(2) based
+ * writes to the same inode. The only real way to fix this is to re-implement
+ * the generic code here with correct locking orders.
+ */
STATIC ssize_t
xfs_file_splice_write(
struct pipe_inode_info *pipe,
@@ -604,7 +645,6 @@ xfs_file_aio_write(
xfs_fsize_t new_size;
int iolock;
size_t ocount = 0, count;
- int need_i_mutex;
XFS_STATS_INC(xs_write_calls);
@@ -631,21 +671,17 @@ 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);
-
start:
+ xfs_rw_ilock(ip, XFS_ILOCK_EXCL|iolock);
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 +690,20 @@ 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);
}
- if (!need_i_mutex && (mapping->nrpages || pos > ip->i_size)) {
- xfs_iunlock(ip, XFS_ILOCK_EXCL|iolock);
+ /*
+ * 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.
+ */
+ if (iolock != XFS_IOLOCK_EXCL &&
+ (mapping->nrpages || pos > ip->i_size)) {
+ xfs_rw_iunlock(ip, XFS_ILOCK_EXCL|iolock);
iolock = XFS_IOLOCK_EXCL;
- need_i_mutex = 1;
- mutex_lock(&inode->i_mutex);
- xfs_ilock(ip, XFS_ILOCK_EXCL|iolock);
goto start;
}
}
@@ -687,11 +727,11 @@ start:
if (pos > ip->i_size) {
ret = -xfs_zero_eof(ip, pos, ip->i_size);
if (ret) {
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
+ xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
goto out_unlock_internal;
}
}
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
+ xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
/*
* If we're writing the file then make sure to clear the
@@ -708,7 +748,7 @@ start:
if ((ioflags & IO_ISDIRECT)) {
if (mapping->nrpages) {
- WARN_ON(need_i_mutex == 0);
+ WARN_ON(iolock != XFS_IOLOCK_EXCL);
ret = -xfs_flushinval_pages(ip,
(pos & PAGE_CACHE_MASK),
-1, FI_REMAPF_LOCKED);
@@ -716,13 +756,10 @@ start:
goto out_unlock_internal;
}
- if (need_i_mutex) {
+ if (iolock == XFS_IOLOCK_EXCL) {
/* demote the lock now the cached pages are gone */
- xfs_ilock_demote(ip, XFS_IOLOCK_EXCL);
- mutex_unlock(&inode->i_mutex);
-
+ xfs_rw_ilock_demote(ip, XFS_IOLOCK_EXCL);
iolock = XFS_IOLOCK_SHARED;
- need_i_mutex = 0;
}
trace_xfs_file_direct_write(ip, count, iocb->ki_pos, ioflags);
@@ -740,7 +777,7 @@ start:
count -= ret;
ioflags &= ~IO_ISDIRECT;
- xfs_iunlock(ip, iolock);
+ xfs_rw_iunlock(ip, iolock);
goto relock;
}
} else {
@@ -775,14 +812,9 @@ write_retry:
loff_t end = pos + ret - 1;
int error, error2;
- xfs_iunlock(ip, iolock);
- if (need_i_mutex)
- mutex_unlock(&inode->i_mutex);
-
+ xfs_rw_iunlock(ip, iolock);
error = filemap_write_and_wait_range(mapping, pos, end);
- if (need_i_mutex)
- mutex_lock(&inode->i_mutex);
- xfs_ilock(ip, iolock);
+ xfs_rw_ilock(ip, iolock);
error2 = -xfs_file_fsync(file,
(file->f_flags & __O_SYNC) ? 0 : 1);
@@ -794,10 +826,7 @@ write_retry:
out_unlock_internal:
xfs_aio_write_newsize_update(ip);
- xfs_iunlock(ip, iolock);
- out_unlock_mutex:
- if (need_i_mutex)
- mutex_unlock(&inode->i_mutex);
+ xfs_rw_iunlock(ip, iolock);
return ret;
}
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 4/8] xfs: introduce xfs_rw_lock() helpers for locking the inode
2011-01-11 21:02 ` Dave Chinner
@ 2011-01-11 21:03 ` Christoph Hellwig
2011-01-11 21:36 ` Alex Elder
1 sibling, 0 replies; 13+ messages in thread
From: Christoph Hellwig @ 2011-01-11 21:03 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/8] xfs: introduce xfs_rw_lock() helpers for locking the inode
2011-01-11 21:02 ` Dave Chinner
2011-01-11 21:03 ` Christoph Hellwig
@ 2011-01-11 21:36 ` Alex Elder
1 sibling, 0 replies; 13+ messages in thread
From: Alex Elder @ 2011-01-11 21:36 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
On Wed, 2011-01-12 at 08:02 +1100, Dave Chinner wrote:
> On Tue, Jan 11, 2011 at 12:36:27PM -0500, Christoph Hellwig wrote:
> > On Tue, Jan 11, 2011 at 10:37:44AM +1100, Dave Chinner wrote:
> > > +/*
> > > + * xfs_file_splice_write() does not use xfs_rw_ilock() because
> > > + * generic_file_splice_write() takes the i_mutex itself. This, in theory,
. . .
> > > + xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
> >
> > While using the xfs_rw_iunlock here actually is correct I think it's
>
> Argh! I thought I reverted all the changes to
> xfs_file_splice_write().
>
> > highly confusing, given that we didn't use it to take the ilock.
>
> It definitely held the ilock around that size update before this
> series. ;)
>
> > What
> > about just leaving all the code in xfs_file_splice_write as-is and not
> > touching it at all?
>
> Updated version below.
Your explanation before and this update addressed all
my substantive issues. I have one other thing (which
is essentially a style issue), which I'll still mention
below, but I think your next patch may make it moot
anyway...
In any case:
Reviewed-by: Alex Elder <aelder@sgi.com>
. . .
> @@ -654,16 +690,20 @@ 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);
> }
>
> - if (!need_i_mutex && (mapping->nrpages || pos > ip->i_size)) {
> - xfs_iunlock(ip, XFS_ILOCK_EXCL|iolock);
> + /*
> + * 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.
> + */
> + if (iolock != XFS_IOLOCK_EXCL &&
I would prefer: if (iolock == XFS_IOLOCK_SHARED)
> + (mapping->nrpages || pos > ip->i_size)) {
> + xfs_rw_iunlock(ip, XFS_ILOCK_EXCL|iolock);
and (maybe) this could be XFS_ILOCK_EXCL|XFS_IOLOCK_SHARED);
> iolock = XFS_IOLOCK_EXCL;
> - need_i_mutex = 1;
> - mutex_lock(&inode->i_mutex);
> - xfs_ilock(ip, XFS_ILOCK_EXCL|iolock);
> goto start;
> }
> }
. . .
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 5/8] xfs: split direct IO write path from xfs_file_aio_write
2011-01-10 23:37 [PATCH 0/8] xfs: prevent corruption due to overlapping AIO DIO V4 Dave Chinner
` (2 preceding siblings ...)
2011-01-10 23:37 ` [PATCH 4/8] xfs: introduce xfs_rw_lock() helpers for locking the inode Dave Chinner
@ 2011-01-10 23:37 ` Dave Chinner
2011-01-11 21:44 ` Alex Elder
2011-01-10 23:37 ` [PATCH 6/8] xfs: split buffered " Dave Chinner
` (2 subsequent siblings)
6 siblings, 1 reply; 13+ messages in thread
From: Dave Chinner @ 2011-01-10 23:37 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
The current xfs_file_aio_write code is a mess of locking shenanigans
to handle the different locking requirements of buffered and direct
IO. Start to clean this up by disentangling the direct IO path from
the mess.
This also removes the failed direct IO fallback path to buffered IO.
XFS handles all direct IO cases without needing to fall back to
buffered IO, so we can safely remove this unused path. This greatly
simplifies the logic and locking needed in the write path.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/linux-2.6/xfs_file.c | 179 ++++++++++++++++++++++++++++---------------
1 files changed, 116 insertions(+), 63 deletions(-)
diff --git a/fs/xfs/linux-2.6/xfs_file.c b/fs/xfs/linux-2.6/xfs_file.c
index 941d7c2..07ab0b6 100644
--- a/fs/xfs/linux-2.6/xfs_file.c
+++ b/fs/xfs/linux-2.6/xfs_file.c
@@ -627,6 +627,116 @@ out_lock:
return error;
}
+/*
+ * xfs_file_dio_aio_write - handle direct IO writes
+ *
+ * Lock the inode appropriately to prepare for and issue a direct IO write.
+ * By spearating it from the buffered write path we remove all the tricky to
+ * follow locking changes and looping.
+ *
+ * Returns with locks held indicated by @iolock and errors indicated by
+ * negative return values.
+ */
+STATIC ssize_t
+xfs_file_dio_aio_write(
+ struct kiocb *iocb,
+ const struct iovec *iovp,
+ unsigned long nr_segs,
+ loff_t pos,
+ size_t ocount,
+ int *iolock)
+{
+ struct file *file = iocb->ki_filp;
+ struct address_space *mapping = file->f_mapping;
+ struct inode *inode = mapping->host;
+ struct xfs_inode *ip = XFS_I(inode);
+ struct xfs_mount *mp = ip->i_mount;
+ ssize_t ret = 0;
+ xfs_fsize_t new_size;
+ size_t count = ocount;
+ struct xfs_buftarg *target = XFS_IS_REALTIME_INODE(ip) ?
+ mp->m_rtdev_targp : mp->m_ddev_targp;
+
+ *iolock = 0;
+ if ((pos & target->bt_smask) || (count & target->bt_smask))
+ return -XFS_ERROR(EINVAL);
+
+ /*
+ * 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.
+ */
+ if (mapping->nrpages || pos > ip->i_size)
+ *iolock = XFS_IOLOCK_EXCL;
+ else
+ *iolock = XFS_IOLOCK_SHARED;
+ xfs_rw_ilock(ip, XFS_ILOCK_EXCL | *iolock);
+
+ ret = generic_write_checks(file, &pos, &count,
+ S_ISBLK(inode->i_mode));
+ if (ret) {
+ xfs_rw_iunlock(ip, XFS_ILOCK_EXCL | *iolock);
+ *iolock = 0;
+ return ret;
+ }
+
+ new_size = pos + count;
+ if (new_size > ip->i_size)
+ ip->i_new_size = new_size;
+
+ if (likely(!(file->f_mode & FMODE_NOCMTIME)))
+ file_update_time(file);
+
+ /*
+ * If the offset is beyond the size of the file, we have a couple of
+ * things to do. First, if there is already space allocated we need to
+ * either create holes or zero the disk or ...
+ *
+ * If there is a page where the previous size lands, we need to zero it
+ * out up to the new size.
+ */
+ if (pos > ip->i_size) {
+ ret = -xfs_zero_eof(ip, pos, ip->i_size);
+ if (ret) {
+ xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
+ return ret;
+ }
+ }
+ xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
+
+ /*
+ * If we're writing the file then make sure to clear the setuid and
+ * setgid bits if the process is not being run by root. This keeps
+ * people from modifying setuid and setgid binaries.
+ */
+ ret = file_remove_suid(file);
+ if (unlikely(ret))
+ return ret;
+
+ if (mapping->nrpages) {
+ WARN_ON(*iolock != XFS_IOLOCK_EXCL);
+ ret = -xfs_flushinval_pages(ip, (pos & PAGE_CACHE_MASK), -1,
+ FI_REMAPF_LOCKED);
+ if (ret)
+ return ret;
+ }
+
+ if (*iolock == XFS_IOLOCK_EXCL) {
+ /* demote the lock now the cached pages are gone */
+ xfs_rw_ilock_demote(ip, XFS_IOLOCK_EXCL);
+ *iolock = XFS_IOLOCK_SHARED;
+ }
+
+ trace_xfs_file_direct_write(ip, count, iocb->ki_pos, 0);
+ ret = generic_file_direct_write(iocb, iovp,
+ &nr_segs, pos, &iocb->ki_pos, count, ocount);
+
+ /* No fallback to buffered IO on errors for XFS. */
+ ASSERT(ret < 0 || ret == count);
+ return ret;
+}
+
STATIC ssize_t
xfs_file_aio_write(
struct kiocb *iocb,
@@ -669,12 +779,12 @@ xfs_file_aio_write(
relock:
if (ioflags & IO_ISDIRECT) {
- iolock = XFS_IOLOCK_SHARED;
- } else {
- iolock = XFS_IOLOCK_EXCL;
+ ret = xfs_file_dio_aio_write(iocb, iovp, nr_segs, pos,
+ ocount, &iolock);
+ goto done_io;
}
+ iolock = XFS_IOLOCK_EXCL;
-start:
xfs_rw_ilock(ip, XFS_ILOCK_EXCL|iolock);
ret = generic_write_checks(file, &pos, &count,
S_ISBLK(inode->i_mode));
@@ -683,30 +793,6 @@ start:
return ret;
}
- if (ioflags & IO_ISDIRECT) {
- xfs_buftarg_t *target =
- XFS_IS_REALTIME_INODE(ip) ?
- mp->m_rtdev_targp : mp->m_ddev_targp;
-
- if ((pos & target->bt_smask) || (count & target->bt_smask)) {
- xfs_rw_iunlock(ip, XFS_ILOCK_EXCL|iolock);
- return XFS_ERROR(-EINVAL);
- }
-
- /*
- * 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.
- */
- if (iolock != XFS_IOLOCK_EXCL &&
- (mapping->nrpages || pos > ip->i_size)) {
- xfs_rw_iunlock(ip, XFS_ILOCK_EXCL|iolock);
- iolock = XFS_IOLOCK_EXCL;
- goto start;
- }
- }
-
new_size = pos + count;
if (new_size > ip->i_size)
ip->i_new_size = new_size;
@@ -745,41 +831,7 @@ start:
/* We can write back this queue in page reclaim */
current->backing_dev_info = mapping->backing_dev_info;
- if ((ioflags & IO_ISDIRECT)) {
- if (mapping->nrpages) {
- WARN_ON(iolock != XFS_IOLOCK_EXCL);
- ret = -xfs_flushinval_pages(ip,
- (pos & PAGE_CACHE_MASK),
- -1, FI_REMAPF_LOCKED);
- if (ret)
- goto out_unlock_internal;
- }
-
- if (iolock == XFS_IOLOCK_EXCL) {
- /* demote the lock now the cached pages are gone */
- xfs_rw_ilock_demote(ip, XFS_IOLOCK_EXCL);
- iolock = XFS_IOLOCK_SHARED;
- }
-
- trace_xfs_file_direct_write(ip, count, iocb->ki_pos, ioflags);
- ret = generic_file_direct_write(iocb, iovp,
- &nr_segs, pos, &iocb->ki_pos, count, ocount);
-
- /*
- * direct-io write to a hole: fall through to buffered I/O
- * for completing the rest of the request.
- */
- if (ret >= 0 && ret != count) {
- XFS_STATS_ADD(xs_write_bytes, ret);
-
- pos += ret;
- count -= ret;
-
- ioflags &= ~IO_ISDIRECT;
- xfs_rw_iunlock(ip, iolock);
- goto relock;
- }
- } else {
+ if (!(ioflags & IO_ISDIRECT)) {
int enospc = 0;
write_retry:
@@ -801,6 +853,7 @@ write_retry:
current->backing_dev_info = NULL;
+done_io:
xfs_aio_write_isize_update(inode, &iocb->ki_pos, ret);
if (ret <= 0)
--
1.7.2.3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 5/8] xfs: split direct IO write path from xfs_file_aio_write
2011-01-10 23:37 ` [PATCH 5/8] xfs: split direct IO write path from xfs_file_aio_write Dave Chinner
@ 2011-01-11 21:44 ` Alex Elder
0 siblings, 0 replies; 13+ messages in thread
From: Alex Elder @ 2011-01-11 21:44 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On Tue, 2011-01-11 at 10:37 +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> The current xfs_file_aio_write code is a mess of locking shenanigans
> to handle the different locking requirements of buffered and direct
> IO. Start to clean this up by disentangling the direct IO path from
> the mess.
>
> This also removes the failed direct IO fallback path to buffered IO.
> XFS handles all direct IO cases without needing to fall back to
> buffered IO, so we can safely remove this unused path. This greatly
> simplifies the logic and locking needed in the write path.
I reviewed this before, and your statement here
now explains the important change you made about
the dead code for falling back to buffered I/O.
So:
Reviewed-by: Alex Elder <aelder@sgi.com>
I think Christoph and I have signed off on all eight
of the patches in this series now.
-Alex
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 6/8] xfs: split buffered IO write path from xfs_file_aio_write
2011-01-10 23:37 [PATCH 0/8] xfs: prevent corruption due to overlapping AIO DIO V4 Dave Chinner
` (3 preceding siblings ...)
2011-01-10 23:37 ` [PATCH 5/8] xfs: split direct IO write path from xfs_file_aio_write Dave Chinner
@ 2011-01-10 23:37 ` Dave Chinner
2011-01-10 23:37 ` [PATCH 7/8] xfs: factor common write setup code Dave Chinner
2011-01-10 23:37 ` [PATCH 8/8] xfs: serialise unaligned direct IOs Dave Chinner
6 siblings, 0 replies; 13+ messages in thread
From: Dave Chinner @ 2011-01-10 23:37 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
Complete the split of the different write IO paths by splitting the
buffered IO write path out of xfs_file_aio_write(). This makes the
different mechanisms of the write patchs easier to follow.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Alex Elder <aelder@sgi.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/linux-2.6/xfs_file.c | 146 ++++++++++++++++++++-----------------------
1 files changed, 69 insertions(+), 77 deletions(-)
diff --git a/fs/xfs/linux-2.6/xfs_file.c b/fs/xfs/linux-2.6/xfs_file.c
index 07ab0b6..ccd2959 100644
--- a/fs/xfs/linux-2.6/xfs_file.c
+++ b/fs/xfs/linux-2.6/xfs_file.c
@@ -738,58 +738,31 @@ xfs_file_dio_aio_write(
}
STATIC ssize_t
-xfs_file_aio_write(
+xfs_file_buffered_aio_write(
struct kiocb *iocb,
const struct iovec *iovp,
unsigned long nr_segs,
- loff_t pos)
+ loff_t pos,
+ size_t ocount,
+ int *iolock)
{
struct file *file = iocb->ki_filp;
struct address_space *mapping = file->f_mapping;
struct inode *inode = mapping->host;
struct xfs_inode *ip = XFS_I(inode);
- struct xfs_mount *mp = ip->i_mount;
- ssize_t ret = 0;
- int ioflags = 0;
+ ssize_t ret;
+ int enospc = 0;
xfs_fsize_t new_size;
- int iolock;
- size_t ocount = 0, count;
-
- XFS_STATS_INC(xs_write_calls);
-
- BUG_ON(iocb->ki_pos != pos);
-
- if (unlikely(file->f_flags & O_DIRECT))
- ioflags |= IO_ISDIRECT;
- if (file->f_mode & FMODE_NOCMTIME)
- ioflags |= IO_INVIS;
-
- ret = generic_segment_checks(iovp, &nr_segs, &ocount, VERIFY_READ);
- if (ret)
- return ret;
-
- count = ocount;
- if (count == 0)
- return 0;
-
- xfs_wait_for_freeze(mp, SB_FREEZE_WRITE);
-
- if (XFS_FORCED_SHUTDOWN(mp))
- return -EIO;
+ size_t count = ocount;
-relock:
- if (ioflags & IO_ISDIRECT) {
- ret = xfs_file_dio_aio_write(iocb, iovp, nr_segs, pos,
- ocount, &iolock);
- goto done_io;
- }
- iolock = XFS_IOLOCK_EXCL;
+ *iolock = XFS_IOLOCK_EXCL;
+ xfs_rw_ilock(ip, XFS_ILOCK_EXCL | *iolock);
- xfs_rw_ilock(ip, XFS_ILOCK_EXCL|iolock);
ret = generic_write_checks(file, &pos, &count,
S_ISBLK(inode->i_mode));
if (ret) {
- xfs_rw_iunlock(ip, XFS_ILOCK_EXCL|iolock);
+ xfs_rw_iunlock(ip, XFS_ILOCK_EXCL | *iolock);
+ *iolock = 0;
return ret;
}
@@ -797,67 +770,86 @@ relock:
if (new_size > ip->i_size)
ip->i_new_size = new_size;
- if (likely(!(ioflags & IO_INVIS)))
+ if (likely(!(file->f_mode & FMODE_NOCMTIME)))
file_update_time(file);
- /*
- * If the offset is beyond the size of the file, we have a couple
- * of things to do. First, if there is already space allocated
- * we need to either create holes or zero the disk or ...
- *
- * If there is a page where the previous size lands, we need
- * to zero it out up to the new size.
- */
-
if (pos > ip->i_size) {
ret = -xfs_zero_eof(ip, pos, ip->i_size);
if (ret) {
xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
- goto out_unlock_internal;
+ return ret;
}
}
xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
- /*
- * If we're writing the file then make sure to clear the
- * setuid and setgid bits if the process is not being run
- * by root. This keeps people from modifying setuid and
- * setgid binaries.
- */
ret = file_remove_suid(file);
if (unlikely(ret))
- goto out_unlock_internal;
+ return ret;
/* We can write back this queue in page reclaim */
current->backing_dev_info = mapping->backing_dev_info;
- if (!(ioflags & IO_ISDIRECT)) {
- int enospc = 0;
-
write_retry:
- trace_xfs_file_buffered_write(ip, count, iocb->ki_pos, ioflags);
- ret = generic_file_buffered_write(iocb, iovp, nr_segs,
- pos, &iocb->ki_pos, count, ret);
- /*
- * if we just got an ENOSPC, flush the inode now we
- * aren't holding any page locks and retry *once*
- */
- if (ret == -ENOSPC && !enospc) {
- ret = xfs_flush_pages(ip, 0, -1, 0, FI_NONE);
- if (ret)
- goto out_unlock_internal;
- enospc = 1;
- goto write_retry;
- }
+ trace_xfs_file_buffered_write(ip, count, iocb->ki_pos, 0);
+ ret = generic_file_buffered_write(iocb, iovp, nr_segs,
+ pos, &iocb->ki_pos, count, ret);
+ /*
+ * if we just got an ENOSPC, flush the inode now we aren't holding any
+ * page locks and retry *once*
+ */
+ if (ret == -ENOSPC && !enospc) {
+ ret = -xfs_flush_pages(ip, 0, -1, 0, FI_NONE);
+ if (ret)
+ return ret;
+ enospc = 1;
+ goto write_retry;
}
-
current->backing_dev_info = NULL;
+ return ret;
+}
+
+STATIC ssize_t
+xfs_file_aio_write(
+ struct kiocb *iocb,
+ const struct iovec *iovp,
+ unsigned long nr_segs,
+ loff_t pos)
+{
+ struct file *file = iocb->ki_filp;
+ struct address_space *mapping = file->f_mapping;
+ struct inode *inode = mapping->host;
+ struct xfs_inode *ip = XFS_I(inode);
+ ssize_t ret;
+ int iolock;
+ size_t ocount = 0;
+
+ XFS_STATS_INC(xs_write_calls);
+
+ BUG_ON(iocb->ki_pos != pos);
+
+ ret = generic_segment_checks(iovp, &nr_segs, &ocount, VERIFY_READ);
+ if (ret)
+ return ret;
+
+ if (ocount == 0)
+ return 0;
+
+ xfs_wait_for_freeze(ip->i_mount, SB_FREEZE_WRITE);
+
+ if (XFS_FORCED_SHUTDOWN(ip->i_mount))
+ return -EIO;
+
+ if (unlikely(file->f_flags & O_DIRECT))
+ ret = xfs_file_dio_aio_write(iocb, iovp, nr_segs, pos,
+ ocount, &iolock);
+ else
+ ret = xfs_file_buffered_aio_write(iocb, iovp, nr_segs, pos,
+ ocount, &iolock);
-done_io:
xfs_aio_write_isize_update(inode, &iocb->ki_pos, ret);
if (ret <= 0)
- goto out_unlock_internal;
+ goto out_unlock;
/* Handle various SYNC-type writes */
if ((file->f_flags & O_DSYNC) || IS_SYNC(inode)) {
@@ -876,7 +868,7 @@ done_io:
ret = error2;
}
- out_unlock_internal:
+out_unlock:
xfs_aio_write_newsize_update(ip);
xfs_rw_iunlock(ip, iolock);
return ret;
--
1.7.2.3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 7/8] xfs: factor common write setup code
2011-01-10 23:37 [PATCH 0/8] xfs: prevent corruption due to overlapping AIO DIO V4 Dave Chinner
` (4 preceding siblings ...)
2011-01-10 23:37 ` [PATCH 6/8] xfs: split buffered " Dave Chinner
@ 2011-01-10 23:37 ` Dave Chinner
2011-01-10 23:37 ` [PATCH 8/8] xfs: serialise unaligned direct IOs Dave Chinner
6 siblings, 0 replies; 13+ messages in thread
From: Dave Chinner @ 2011-01-10 23:37 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
The buffered IO and direct IO write paths share a common set of
checks and limiting code prior to issuing the write. Factor that
into a common helper function.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Alex Elder <aelder@sgi.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/linux-2.6/xfs_file.c | 123 +++++++++++++++++++-----------------------
1 files changed, 56 insertions(+), 67 deletions(-)
diff --git a/fs/xfs/linux-2.6/xfs_file.c b/fs/xfs/linux-2.6/xfs_file.c
index ccd2959..41f3ceb 100644
--- a/fs/xfs/linux-2.6/xfs_file.c
+++ b/fs/xfs/linux-2.6/xfs_file.c
@@ -628,6 +628,58 @@ out_lock:
}
/*
+ * Common pre-write limit and setup checks.
+ *
+ * Returns with iolock held according to @iolock.
+ */
+STATIC ssize_t
+xfs_file_aio_write_checks(
+ struct file *file,
+ loff_t *pos,
+ size_t *count,
+ int *iolock)
+{
+ struct inode *inode = file->f_mapping->host;
+ struct xfs_inode *ip = XFS_I(inode);
+ xfs_fsize_t new_size;
+ int error = 0;
+
+ error = generic_write_checks(file, pos, count, S_ISBLK(inode->i_mode));
+ if (error) {
+ xfs_rw_iunlock(ip, XFS_ILOCK_EXCL | *iolock);
+ *iolock = 0;
+ return error;
+ }
+
+ new_size = *pos + *count;
+ if (new_size > ip->i_size)
+ ip->i_new_size = new_size;
+
+ if (likely(!(file->f_mode & FMODE_NOCMTIME)))
+ file_update_time(file);
+
+ /*
+ * If the offset is beyond the size of the file, we need to zero any
+ * blocks that fall between the existing EOF and the start of this
+ * write.
+ */
+ if (*pos > ip->i_size)
+ error = -xfs_zero_eof(ip, *pos, ip->i_size);
+
+ xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
+ if (error)
+ return error;
+
+ /*
+ * If we're writing the file then make sure to clear the setuid and
+ * setgid bits if the process is not being run by root. This keeps
+ * people from modifying setuid and setgid binaries.
+ */
+ return file_remove_suid(file);
+
+}
+
+/*
* xfs_file_dio_aio_write - handle direct IO writes
*
* Lock the inode appropriately to prepare for and issue a direct IO write.
@@ -652,7 +704,6 @@ xfs_file_dio_aio_write(
struct xfs_inode *ip = XFS_I(inode);
struct xfs_mount *mp = ip->i_mount;
ssize_t ret = 0;
- xfs_fsize_t new_size;
size_t count = ocount;
struct xfs_buftarg *target = XFS_IS_REALTIME_INODE(ip) ?
mp->m_rtdev_targp : mp->m_ddev_targp;
@@ -673,45 +724,8 @@ xfs_file_dio_aio_write(
*iolock = XFS_IOLOCK_SHARED;
xfs_rw_ilock(ip, XFS_ILOCK_EXCL | *iolock);
- ret = generic_write_checks(file, &pos, &count,
- S_ISBLK(inode->i_mode));
- if (ret) {
- xfs_rw_iunlock(ip, XFS_ILOCK_EXCL | *iolock);
- *iolock = 0;
- return ret;
- }
-
- new_size = pos + count;
- if (new_size > ip->i_size)
- ip->i_new_size = new_size;
-
- if (likely(!(file->f_mode & FMODE_NOCMTIME)))
- file_update_time(file);
-
- /*
- * If the offset is beyond the size of the file, we have a couple of
- * things to do. First, if there is already space allocated we need to
- * either create holes or zero the disk or ...
- *
- * If there is a page where the previous size lands, we need to zero it
- * out up to the new size.
- */
- if (pos > ip->i_size) {
- ret = -xfs_zero_eof(ip, pos, ip->i_size);
- if (ret) {
- xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
- return ret;
- }
- }
- xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
-
- /*
- * If we're writing the file then make sure to clear the setuid and
- * setgid bits if the process is not being run by root. This keeps
- * people from modifying setuid and setgid binaries.
- */
- ret = file_remove_suid(file);
- if (unlikely(ret))
+ ret = xfs_file_aio_write_checks(file, &pos, &count, iolock);
+ if (ret)
return ret;
if (mapping->nrpages) {
@@ -752,38 +766,13 @@ xfs_file_buffered_aio_write(
struct xfs_inode *ip = XFS_I(inode);
ssize_t ret;
int enospc = 0;
- xfs_fsize_t new_size;
size_t count = ocount;
*iolock = XFS_IOLOCK_EXCL;
xfs_rw_ilock(ip, XFS_ILOCK_EXCL | *iolock);
- ret = generic_write_checks(file, &pos, &count,
- S_ISBLK(inode->i_mode));
- if (ret) {
- xfs_rw_iunlock(ip, XFS_ILOCK_EXCL | *iolock);
- *iolock = 0;
- return ret;
- }
-
- new_size = pos + count;
- if (new_size > ip->i_size)
- ip->i_new_size = new_size;
-
- if (likely(!(file->f_mode & FMODE_NOCMTIME)))
- file_update_time(file);
-
- if (pos > ip->i_size) {
- ret = -xfs_zero_eof(ip, pos, ip->i_size);
- if (ret) {
- xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
- return ret;
- }
- }
- xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
-
- ret = file_remove_suid(file);
- if (unlikely(ret))
+ ret = xfs_file_aio_write_checks(file, &pos, &count, iolock);
+ if (ret)
return ret;
/* We can write back this queue in page reclaim */
--
1.7.2.3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 8/8] xfs: serialise unaligned direct IOs
2011-01-10 23:37 [PATCH 0/8] xfs: prevent corruption due to overlapping AIO DIO V4 Dave Chinner
` (5 preceding siblings ...)
2011-01-10 23:37 ` [PATCH 7/8] xfs: factor common write setup code Dave Chinner
@ 2011-01-10 23:37 ` Dave Chinner
6 siblings, 0 replies; 13+ messages in thread
From: Dave Chinner @ 2011-01-10 23:37 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
When two concurrent unaligned, non-overlapping direct IOs are issued
to the same block, the direct Io layer will race to zero the block.
The result is that one of the concurrent IOs will overwrite data
written by the other IO with zeros. This is demonstrated by the
xfsqa test 240.
To avoid this problem, serialise all unaligned direct IOs to an
inode with a big hammer. We need a big hammer approach as we need to
serialise AIO as well, so we can't just block writes on locks.
Hence, the big hammer is calling xfs_ioend_wait() while holding out
other unaligned direct IOs from starting.
We don't bother trying to serialised aligned vs unaligned IOs as
they are overlapping IO and the result of concurrent overlapping IOs
is undefined - the result of either IO is a valid result so we let
them race. Hence we only penalise unaligned IO, which already has a
major overhead compared to aligned IO so this isn't a major problem.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Alex Elder <aelder@sgi.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/linux-2.6/xfs_file.c | 38 ++++++++++++++++++++++++++++----------
1 files changed, 28 insertions(+), 10 deletions(-)
diff --git a/fs/xfs/linux-2.6/xfs_file.c b/fs/xfs/linux-2.6/xfs_file.c
index 41f3ceb..bdb35c4b 100644
--- a/fs/xfs/linux-2.6/xfs_file.c
+++ b/fs/xfs/linux-2.6/xfs_file.c
@@ -683,9 +683,24 @@ xfs_file_aio_write_checks(
* xfs_file_dio_aio_write - handle direct IO writes
*
* Lock the inode appropriately to prepare for and issue a direct IO write.
- * By spearating it from the buffered write path we remove all the tricky to
+ * By separating it from the buffered write path we remove all the tricky to
* follow locking changes and looping.
*
+ * 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.
+ *
+ * In most cases the direct IO writes will be done holding IOLOCK_SHARED
+ * allowing them to be done in parallel with reads and other direct IO writes.
+ * However, if the IO is not aligned to filesystem blocks, the direct IO layer
+ * needs to do sub-block zeroing and that requires serialisation against other
+ * direct IOs to the same block. In this case we need to serialise the
+ * submission of the unaligned IOs so that we don't get racing block zeroing in
+ * the dio layer. To avoid the problem with aio, we also need to wait for
+ * outstanding IOs to complete so that unwritten extent conversion is completed
+ * before we try to map the overlapping block. This is currently implemented by
+ * hitting it with a big hammer (i.e. xfs_ioend_wait()).
+ *
* Returns with locks held indicated by @iolock and errors indicated by
* negative return values.
*/
@@ -705,6 +720,7 @@ xfs_file_dio_aio_write(
struct xfs_mount *mp = ip->i_mount;
ssize_t ret = 0;
size_t count = ocount;
+ int unaligned_io = 0;
struct xfs_buftarg *target = XFS_IS_REALTIME_INODE(ip) ?
mp->m_rtdev_targp : mp->m_ddev_targp;
@@ -712,13 +728,10 @@ xfs_file_dio_aio_write(
if ((pos & target->bt_smask) || (count & target->bt_smask))
return -XFS_ERROR(EINVAL);
- /*
- * 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.
- */
- if (mapping->nrpages || pos > ip->i_size)
+ if ((pos & mp->m_blockmask) || ((pos + count) & mp->m_blockmask))
+ unaligned_io = 1;
+
+ if (unaligned_io || mapping->nrpages || pos > ip->i_size)
*iolock = XFS_IOLOCK_EXCL;
else
*iolock = XFS_IOLOCK_SHARED;
@@ -736,8 +749,13 @@ xfs_file_dio_aio_write(
return ret;
}
- if (*iolock == XFS_IOLOCK_EXCL) {
- /* demote the lock now the cached pages are gone */
+ /*
+ * If we are doing unaligned IO, wait for all other IO to drain,
+ * otherwise demote the lock if we had to flush cached pages
+ */
+ if (unaligned_io)
+ xfs_ioend_wait(ip);
+ else if (*iolock == XFS_IOLOCK_EXCL) {
xfs_rw_ilock_demote(ip, XFS_IOLOCK_EXCL);
*iolock = XFS_IOLOCK_SHARED;
}
--
1.7.2.3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 13+ messages in thread