* [PATCH 1/5] xfs: avoid taking the ilock unnessecarily in xfs_qm_dqattach
2012-03-26 21:14 [PATCH 0/5] reduce exclusive ilock hold times Christoph Hellwig
@ 2012-03-26 21:14 ` Christoph Hellwig
2012-03-26 22:13 ` Dave Chinner
2012-03-26 21:14 ` [PATCH 2/5] xfs: reduce ilock hold times in xfs_file_aio_write_checks Christoph Hellwig
` (4 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2012-03-26 21:14 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-optimize-dqattach-locking --]
[-- Type: text/plain, Size: 1846 bytes --]
Check if we actually need to attach a dquot before taking the ilock in
xfs_qm_dqattach. This avoid superflous lock roundtrips for the common cases
of quota support compiled in but not activated on a filesystem and an
inode that already has the dquots attached.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_qm.c | 26 +++++++++++++++++++++-----
1 file changed, 21 insertions(+), 5 deletions(-)
Index: xfs/fs/xfs/xfs_qm.c
===================================================================
--- xfs.orig/fs/xfs/xfs_qm.c 2012-03-26 21:11:12.000000000 +0200
+++ xfs/fs/xfs/xfs_qm.c 2012-03-26 21:13:13.342148303 +0200
@@ -476,6 +476,23 @@ done:
xfs_dqunlock(udq);
}
+static inline bool
+xfs_qm_need_dqattach(
+ struct xfs_inode *ip)
+{
+ struct xfs_mount *mp = ip->i_mount;
+
+ if (!XFS_IS_QUOTA_RUNNING(mp))
+ return false;
+ if (!XFS_IS_QUOTA_ON(mp))
+ return false;
+ if (!XFS_NOT_DQATTACHED(mp, ip))
+ return false;
+ if (ip->i_ino == mp->m_sb.sb_uquotino ||
+ ip->i_ino == mp->m_sb.sb_gquotino)
+ return false;
+ return true;
+}
/*
* Given a locked inode, attach dquot(s) to it, taking U/G/P-QUOTAON
@@ -493,11 +510,7 @@ xfs_qm_dqattach_locked(
uint nquotas = 0;
int error = 0;
- if (!XFS_IS_QUOTA_RUNNING(mp) ||
- !XFS_IS_QUOTA_ON(mp) ||
- !XFS_NOT_DQATTACHED(mp, ip) ||
- ip->i_ino == mp->m_sb.sb_uquotino ||
- ip->i_ino == mp->m_sb.sb_gquotino)
+ if (!xfs_qm_need_dqattach(ip))
return 0;
ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
@@ -568,6 +581,9 @@ xfs_qm_dqattach(
{
int error;
+ if (!xfs_qm_need_dqattach(ip))
+ return 0;
+
xfs_ilock(ip, XFS_ILOCK_EXCL);
error = xfs_qm_dqattach_locked(ip, flags);
xfs_iunlock(ip, XFS_ILOCK_EXCL);
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/5] xfs: avoid taking the ilock unnessecarily in xfs_qm_dqattach
2012-03-26 21:14 ` [PATCH 1/5] xfs: avoid taking the ilock unnessecarily in xfs_qm_dqattach Christoph Hellwig
@ 2012-03-26 22:13 ` Dave Chinner
0 siblings, 0 replies; 14+ messages in thread
From: Dave Chinner @ 2012-03-26 22:13 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Mar 26, 2012 at 05:14:22PM -0400, Christoph Hellwig wrote:
> Check if we actually need to attach a dquot before taking the ilock in
> xfs_qm_dqattach. This avoid superflous lock roundtrips for the common cases
> of quota support compiled in but not activated on a filesystem and an
> inode that already has the dquots attached.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Looks good. One minor thing:
> +static inline bool
> +xfs_qm_need_dqattach(
> + struct xfs_inode *ip)
> +{
> + struct xfs_mount *mp = ip->i_mount;
> +
> + if (!XFS_IS_QUOTA_RUNNING(mp))
> + return false;
> + if (!XFS_IS_QUOTA_ON(mp))
> + return false;
> + if (!XFS_NOT_DQATTACHED(mp, ip))
> + return false;
> + if (ip->i_ino == mp->m_sb.sb_uquotino ||
> + ip->i_ino == mp->m_sb.sb_gquotino)
> + return false;
> + return true;
> +}
That's probably a little large for an inline function. Let the
compiler decide whether to inline it or not. In most cases, it will
inline it because it is a static function....
Otherwise,
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/5] xfs: reduce ilock hold times in xfs_file_aio_write_checks
2012-03-26 21:14 [PATCH 0/5] reduce exclusive ilock hold times Christoph Hellwig
2012-03-26 21:14 ` [PATCH 1/5] xfs: avoid taking the ilock unnessecarily in xfs_qm_dqattach Christoph Hellwig
@ 2012-03-26 21:14 ` Christoph Hellwig
2012-03-26 22:15 ` Dave Chinner
2012-03-26 21:14 ` [PATCH 3/5] xfs: reduce ilock hold times in xfs_setattr_size Christoph Hellwig
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2012-03-26 21:14 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-reduce-ilock-in-write-preparation --]
[-- Type: text/plain, Size: 2297 bytes --]
We do not need the ilock for generic_write_checks and the i_size read,
which are protected by i_mutex and/or iolock, so reduce the ilock
critical section to just the call to xfs_zero_eof.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_file.c | 23 +++++++++--------------
1 file changed, 9 insertions(+), 14 deletions(-)
Index: xfs/fs/xfs/xfs_file.c
===================================================================
--- xfs.orig/fs/xfs/xfs_file.c 2012-03-26 21:22:01.495953714 +0200
+++ xfs/fs/xfs/xfs_file.c 2012-03-26 21:24:07.955268625 +0200
@@ -593,35 +593,31 @@ xfs_file_aio_write_checks(
struct xfs_inode *ip = XFS_I(inode);
int error = 0;
- xfs_rw_ilock(ip, XFS_ILOCK_EXCL);
restart:
error = generic_write_checks(file, pos, count, S_ISBLK(inode->i_mode));
- if (error) {
- xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
+ if (error)
return error;
- }
/*
* 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 zeroing is needed and we are currently holding the
- * iolock shared, we need to update it to exclusive which involves
- * dropping all locks and relocking to maintain correct locking order.
- * If we do this, restart the function to ensure all checks and values
- * are still valid.
+ * iolock shared, we need to update it to exclusive which implies
+ * having to redo all checks before.
*/
if (*pos > i_size_read(inode)) {
if (*iolock == XFS_IOLOCK_SHARED) {
- xfs_rw_iunlock(ip, XFS_ILOCK_EXCL | *iolock);
+ xfs_rw_iunlock(ip, *iolock);
*iolock = XFS_IOLOCK_EXCL;
- xfs_rw_ilock(ip, XFS_ILOCK_EXCL | *iolock);
+ xfs_rw_ilock(ip, *iolock);
goto restart;
}
+ xfs_rw_ilock(ip, XFS_ILOCK_EXCL);
error = -xfs_zero_eof(ip, *pos, i_size_read(inode));
+ xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
+ if (error)
+ return error;
}
- xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
- if (error)
- return error;
/*
* Updating the timestamps will grab the ilock again from
@@ -638,7 +634,6 @@ restart:
* people from modifying setuid and setgid binaries.
*/
return file_remove_suid(file);
-
}
/*
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/5] xfs: reduce ilock hold times in xfs_file_aio_write_checks
2012-03-26 21:14 ` [PATCH 2/5] xfs: reduce ilock hold times in xfs_file_aio_write_checks Christoph Hellwig
@ 2012-03-26 22:15 ` Dave Chinner
0 siblings, 0 replies; 14+ messages in thread
From: Dave Chinner @ 2012-03-26 22:15 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Mar 26, 2012 at 05:14:23PM -0400, Christoph Hellwig wrote:
> We do not need the ilock for generic_write_checks and the i_size read,
> which are protected by i_mutex and/or iolock, so reduce the ilock
> critical section to just the call to xfs_zero_eof.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
That answers one of the open questions I had about my patch -
whether we needed to hold the ilock at all over
generic_write_checks(). I wasn't 100% sure, but this patch confirms
my thinking on it.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/5] xfs: reduce ilock hold times in xfs_setattr_size
2012-03-26 21:14 [PATCH 0/5] reduce exclusive ilock hold times Christoph Hellwig
2012-03-26 21:14 ` [PATCH 1/5] xfs: avoid taking the ilock unnessecarily in xfs_qm_dqattach Christoph Hellwig
2012-03-26 21:14 ` [PATCH 2/5] xfs: reduce ilock hold times in xfs_file_aio_write_checks Christoph Hellwig
@ 2012-03-26 21:14 ` Christoph Hellwig
2012-03-26 22:17 ` Dave Chinner
2012-03-26 21:14 ` [PATCH 4/5] xfs: push the ilock into xfs_zero_eof Christoph Hellwig
` (2 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2012-03-26 21:14 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-reduce-ilock-in-truncate --]
[-- Type: text/plain, Size: 2132 bytes --]
We do not need the ilock for most checks done in the beginning of
xfs_setattr_size. Replace the long critical section before starting the
transaction with a smaller one around xfs_zero_eof and an optional one
inside xfs_qm_dqattach that isn't entered unless using quotas. While
this isn't a big optimization for xfs_setattr_size itself it will allow
pushing the ilock into xfs_zero_eof itself later.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_iops.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
Index: xfs/fs/xfs/xfs_iops.c
===================================================================
--- xfs.orig/fs/xfs/xfs_iops.c 2012-03-26 15:17:47.088854526 +0200
+++ xfs/fs/xfs/xfs_iops.c 2012-03-26 15:17:57.265521382 +0200
@@ -700,7 +700,7 @@ xfs_setattr_size(
xfs_off_t oldsize, newsize;
struct xfs_trans *tp;
int error;
- uint lock_flags;
+ uint lock_flags = 0;
uint commit_flags = 0;
trace_xfs_setattr(ip);
@@ -720,10 +720,10 @@ xfs_setattr_size(
ATTR_MTIME_SET|ATTR_KILL_SUID|ATTR_KILL_SGID|
ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
- lock_flags = XFS_ILOCK_EXCL;
- if (!(flags & XFS_ATTR_NOLOCK))
+ if (!(flags & XFS_ATTR_NOLOCK)) {
lock_flags |= XFS_IOLOCK_EXCL;
- xfs_ilock(ip, lock_flags);
+ xfs_ilock(ip, lock_flags);
+ }
oldsize = inode->i_size;
newsize = iattr->ia_size;
@@ -746,7 +746,7 @@ xfs_setattr_size(
/*
* Make sure that the dquots are attached to the inode.
*/
- error = xfs_qm_dqattach_locked(ip, 0);
+ error = xfs_qm_dqattach(ip, 0);
if (error)
goto out_unlock;
@@ -764,12 +764,12 @@ xfs_setattr_size(
* before the inode is joined to the transaction to modify
* i_size.
*/
+ xfs_ilock(ip, XFS_ILOCK_EXCL);
error = xfs_zero_eof(ip, newsize, oldsize);
+ xfs_iunlock(ip, XFS_ILOCK_EXCL);
if (error)
goto out_unlock;
}
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
- lock_flags &= ~XFS_ILOCK_EXCL;
/*
* We are going to log the inode size change in this transaction so
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 3/5] xfs: reduce ilock hold times in xfs_setattr_size
2012-03-26 21:14 ` [PATCH 3/5] xfs: reduce ilock hold times in xfs_setattr_size Christoph Hellwig
@ 2012-03-26 22:17 ` Dave Chinner
0 siblings, 0 replies; 14+ messages in thread
From: Dave Chinner @ 2012-03-26 22:17 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Mar 26, 2012 at 05:14:24PM -0400, Christoph Hellwig wrote:
> We do not need the ilock for most checks done in the beginning of
> xfs_setattr_size. Replace the long critical section before starting the
> transaction with a smaller one around xfs_zero_eof and an optional one
> inside xfs_qm_dqattach that isn't entered unless using quotas. While
> this isn't a big optimization for xfs_setattr_size itself it will allow
> pushing the ilock into xfs_zero_eof itself later.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Looks sane to me.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/5] xfs: push the ilock into xfs_zero_eof
2012-03-26 21:14 [PATCH 0/5] reduce exclusive ilock hold times Christoph Hellwig
` (2 preceding siblings ...)
2012-03-26 21:14 ` [PATCH 3/5] xfs: reduce ilock hold times in xfs_setattr_size Christoph Hellwig
@ 2012-03-26 21:14 ` Christoph Hellwig
2012-03-27 1:15 ` Dave Chinner
2012-03-26 21:14 ` [PATCH 5/5] xfs: use shared ilock mode for direct IO writes by default Christoph Hellwig
2012-03-27 6:08 ` [PATCH 0/5] reduce exclusive ilock hold times Dave Chinner
5 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2012-03-26 21:14 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-push-ilock-into-xfs_zero_eof --]
[-- Type: text/plain, Size: 8972 bytes --]
Instead of calling xfs_zero_eof with the ilock held only take it internally
for the minimall required critical section around xfs_bmapi_read. This
also requires changing the calling convention for xfs_zero_last_block
slightly. The actual zeroing operation is still serialized by the iolock,
which must be taken exclusively over the call to xfs_zero_eof.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_file.c | 161 ++++++++++++++++++++----------------------------------
fs/xfs/xfs_iops.c | 2
2 files changed, 62 insertions(+), 101 deletions(-)
Index: xfs/fs/xfs/xfs_file.c
===================================================================
--- xfs.orig/fs/xfs/xfs_file.c 2012-03-26 21:24:07.955268625 +0200
+++ xfs/fs/xfs/xfs_file.c 2012-03-26 21:24:14.588566021 +0200
@@ -396,114 +396,96 @@ xfs_file_splice_write(
}
/*
- * This routine is called to handle zeroing any space in the last
- * block of the file that is beyond the EOF. We do this since the
- * size is being increased without writing anything to that block
- * and we don't want anyone to read the garbage on the disk.
+ * This routine is called to handle zeroing any space in the last block of the
+ * file that is beyond the EOF. We do this since the size is being increased
+ * without writing anything to that block and we don't want to read the
+ * garbage on the disk.
*/
STATIC int /* error (positive) */
xfs_zero_last_block(
- xfs_inode_t *ip,
- xfs_fsize_t offset,
- xfs_fsize_t isize)
+ struct xfs_inode *ip,
+ xfs_fsize_t offset,
+ xfs_fsize_t isize)
{
- xfs_fileoff_t last_fsb;
- xfs_mount_t *mp = ip->i_mount;
- int nimaps;
- int zero_offset;
- int zero_len;
- int error = 0;
- xfs_bmbt_irec_t imap;
-
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
-
- zero_offset = XFS_B_FSB_OFFSET(mp, isize);
- if (zero_offset == 0) {
- /*
- * There are no extra bytes in the last block on disk to
- * zero, so return.
- */
- return 0;
- }
+ struct xfs_mount *mp = ip->i_mount;
+ xfs_fileoff_t last_fsb = XFS_B_TO_FSBT(mp, isize);
+ int zero_offset = XFS_B_FSB_OFFSET(mp, isize);
+ int zero_len;
+ int nimaps = 1;
+ int error = 0;
+ struct xfs_bmbt_irec imap;
- last_fsb = XFS_B_TO_FSBT(mp, isize);
- nimaps = 1;
+ xfs_ilock(ip, XFS_ILOCK_EXCL);
error = xfs_bmapi_read(ip, last_fsb, 1, &imap, &nimaps, 0);
+ xfs_iunlock(ip, XFS_ILOCK_EXCL);
if (error)
return error;
+
ASSERT(nimaps > 0);
+
/*
* If the block underlying isize is just a hole, then there
* is nothing to zero.
*/
- if (imap.br_startblock == HOLESTARTBLOCK) {
+ if (imap.br_startblock == HOLESTARTBLOCK)
return 0;
- }
- /*
- * Zero the part of the last block beyond the EOF, and write it
- * out sync. We need to drop the ilock while we do this so we
- * don't deadlock when the buffer cache calls back to us.
- */
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
zero_len = mp->m_sb.sb_blocksize - zero_offset;
if (isize + zero_len > offset)
zero_len = offset - isize;
- error = xfs_iozero(ip, isize, zero_len);
-
- xfs_ilock(ip, XFS_ILOCK_EXCL);
- ASSERT(error >= 0);
- return error;
+ return xfs_iozero(ip, isize, zero_len);
}
/*
- * Zero any on disk space between the current EOF and the new,
- * larger EOF. This handles the normal case of zeroing the remainder
- * of the last block in the file and the unusual case of zeroing blocks
- * out beyond the size of the file. This second case only happens
- * with fixed size extents and when the system crashes before the inode
- * size was updated but after blocks were allocated. If fill is set,
- * then any holes in the range are filled and zeroed. If not, the holes
- * are left alone as holes.
+ * Zero any on disk space between the current EOF and the new, larger EOF.
+ *
+ * This handles the normal case of zeroing the remainder of the last block in
+ * the file and the unusual case of zeroing blocks out beyond the size of the
+ * file. This second case only happens with fixed size extents and when the
+ * system crashes before the inode size was updated but after blocks were
+ * allocated.
+ *
+ * Expects the iolock to be held exclusive, and will take the ilock internally.
*/
-
int /* error (positive) */
xfs_zero_eof(
- xfs_inode_t *ip,
- xfs_off_t offset, /* starting I/O offset */
- xfs_fsize_t isize) /* current inode size */
+ struct xfs_inode *ip,
+ xfs_off_t offset, /* starting I/O offset */
+ xfs_fsize_t isize) /* current inode size */
{
- xfs_mount_t *mp = ip->i_mount;
- xfs_fileoff_t start_zero_fsb;
- xfs_fileoff_t end_zero_fsb;
- xfs_fileoff_t zero_count_fsb;
- xfs_fileoff_t last_fsb;
- xfs_fileoff_t zero_off;
- xfs_fsize_t zero_len;
- int nimaps;
- int error = 0;
- xfs_bmbt_irec_t imap;
+ struct xfs_mount *mp = ip->i_mount;
+ xfs_fileoff_t start_zero_fsb;
+ xfs_fileoff_t end_zero_fsb;
+ xfs_fileoff_t zero_count_fsb;
+ xfs_fileoff_t last_fsb;
+ xfs_fileoff_t zero_off;
+ xfs_fsize_t zero_len;
+ int nimaps;
+ int error = 0;
+ struct xfs_bmbt_irec imap;
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_IOLOCK_EXCL));
+ ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
ASSERT(offset > isize);
/*
* First handle zeroing the block on which isize resides.
+ *
* We only zero a part of that block so it is handled specially.
*/
- error = xfs_zero_last_block(ip, offset, isize);
- if (error) {
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_IOLOCK_EXCL));
- return error;
+ if (XFS_B_FSB_OFFSET(mp, isize) != 0) {
+ error = xfs_zero_last_block(ip, offset, isize);
+ if (error)
+ return error;
}
/*
- * Calculate the range between the new size and the old
- * where blocks needing to be zeroed may exist. To get the
- * block where the last byte in the file currently resides,
- * we need to subtract one from the size and truncate back
- * to a block boundary. We subtract 1 in case the size is
- * exactly on a block boundary.
+ * Calculate the range between the new size and the old where blocks
+ * needing to be zeroed may exist.
+ *
+ * To get the block where the last byte in the file currently resides,
+ * we need to subtract one from the size and truncate back to a block
+ * boundary. We subtract 1 in case the size is exactly on a block
+ * boundary.
*/
last_fsb = isize ? XFS_B_TO_FSBT(mp, isize - 1) : (xfs_fileoff_t)-1;
start_zero_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)isize);
@@ -521,23 +503,18 @@ xfs_zero_eof(
while (start_zero_fsb <= end_zero_fsb) {
nimaps = 1;
zero_count_fsb = end_zero_fsb - start_zero_fsb + 1;
+
+ xfs_ilock(ip, XFS_ILOCK_EXCL);
error = xfs_bmapi_read(ip, start_zero_fsb, zero_count_fsb,
&imap, &nimaps, 0);
- if (error) {
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_IOLOCK_EXCL));
+ xfs_iunlock(ip, XFS_ILOCK_EXCL);
+ if (error)
return error;
- }
+
ASSERT(nimaps > 0);
if (imap.br_state == XFS_EXT_UNWRITTEN ||
imap.br_startblock == HOLESTARTBLOCK) {
- /*
- * This loop handles initializing pages that were
- * partially initialized by the code below this
- * loop. It basically zeroes the part of the page
- * that sits on a hole and sets the page as P_HOLE
- * and calls remapf if it is a mapped file.
- */
start_zero_fsb = imap.br_startoff + imap.br_blockcount;
ASSERT(start_zero_fsb <= (end_zero_fsb + 1));
continue;
@@ -545,11 +522,7 @@ xfs_zero_eof(
/*
* There are blocks we need to zero.
- * Drop the inode lock while we're doing the I/O.
- * We'll still have the iolock to protect us.
*/
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
-
zero_off = XFS_FSB_TO_B(mp, start_zero_fsb);
zero_len = XFS_FSB_TO_B(mp, imap.br_blockcount);
@@ -557,22 +530,14 @@ xfs_zero_eof(
zero_len = offset - zero_off;
error = xfs_iozero(ip, zero_off, zero_len);
- if (error) {
- goto out_lock;
- }
+ if (error)
+ return error;
start_zero_fsb = imap.br_startoff + imap.br_blockcount;
ASSERT(start_zero_fsb <= (end_zero_fsb + 1));
-
- xfs_ilock(ip, XFS_ILOCK_EXCL);
}
return 0;
-
-out_lock:
- xfs_ilock(ip, XFS_ILOCK_EXCL);
- ASSERT(error >= 0);
- return error;
}
/*
@@ -612,9 +577,7 @@ restart:
xfs_rw_ilock(ip, *iolock);
goto restart;
}
- xfs_rw_ilock(ip, XFS_ILOCK_EXCL);
error = -xfs_zero_eof(ip, *pos, i_size_read(inode));
- xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
if (error)
return error;
}
Index: xfs/fs/xfs/xfs_iops.c
===================================================================
--- xfs.orig/fs/xfs/xfs_iops.c 2012-03-26 21:24:13.791903671 +0200
+++ xfs/fs/xfs/xfs_iops.c 2012-03-26 21:24:14.588566021 +0200
@@ -764,9 +764,7 @@ xfs_setattr_size(
* before the inode is joined to the transaction to modify
* i_size.
*/
- xfs_ilock(ip, XFS_ILOCK_EXCL);
error = xfs_zero_eof(ip, newsize, oldsize);
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
if (error)
goto out_unlock;
}
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 4/5] xfs: push the ilock into xfs_zero_eof
2012-03-26 21:14 ` [PATCH 4/5] xfs: push the ilock into xfs_zero_eof Christoph Hellwig
@ 2012-03-27 1:15 ` Dave Chinner
2012-03-27 5:48 ` Christoph Hellwig
0 siblings, 1 reply; 14+ messages in thread
From: Dave Chinner @ 2012-03-27 1:15 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Mar 26, 2012 at 05:14:25PM -0400, Christoph Hellwig wrote:
> Instead of calling xfs_zero_eof with the ilock held only take it internally
> for the minimall required critical section around xfs_bmapi_read. This
> also requires changing the calling convention for xfs_zero_last_block
> slightly. The actual zeroing operation is still serialized by the iolock,
> which must be taken exclusively over the call to xfs_zero_eof.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
.....
> + struct xfs_mount *mp = ip->i_mount;
> + xfs_fileoff_t last_fsb = XFS_B_TO_FSBT(mp, isize);
> + int zero_offset = XFS_B_FSB_OFFSET(mp, isize);
> + int zero_len;
> + int nimaps = 1;
> + int error = 0;
> + struct xfs_bmbt_irec imap;
>
> - last_fsb = XFS_B_TO_FSBT(mp, isize);
> - nimaps = 1;
> + xfs_ilock(ip, XFS_ILOCK_EXCL);
> error = xfs_bmapi_read(ip, last_fsb, 1, &imap, &nimaps, 0);
> + xfs_iunlock(ip, XFS_ILOCK_EXCL);
Does that even need to be an exclusive lock? a shared lock is all
that is needed to do a lookup, and this is just a lookup...
....
> @@ -521,23 +503,18 @@ xfs_zero_eof(
> while (start_zero_fsb <= end_zero_fsb) {
> nimaps = 1;
> zero_count_fsb = end_zero_fsb - start_zero_fsb + 1;
> +
> + xfs_ilock(ip, XFS_ILOCK_EXCL);
> error = xfs_bmapi_read(ip, start_zero_fsb, zero_count_fsb,
> &imap, &nimaps, 0);
> - if (error) {
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_IOLOCK_EXCL));
> + xfs_iunlock(ip, XFS_ILOCK_EXCL);
Same question - it is a read lookup so why do we need exclusive
locking here?
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 4/5] xfs: push the ilock into xfs_zero_eof
2012-03-27 1:15 ` Dave Chinner
@ 2012-03-27 5:48 ` Christoph Hellwig
2012-03-27 6:11 ` Dave Chinner
0 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2012-03-27 5:48 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
On Tue, Mar 27, 2012 at 12:15:40PM +1100, Dave Chinner wrote:
> > + xfs_ilock(ip, XFS_ILOCK_EXCL);
> > error = xfs_bmapi_read(ip, last_fsb, 1, &imap, &nimaps, 0);
> > + xfs_iunlock(ip, XFS_ILOCK_EXCL);
>
> Does that even need to be an exclusive lock? a shared lock is all
> that is needed to do a lookup, and this is just a lookup...
It has to be a xfs_ilock_map_shared - but given that we hold the iolock
exclusive anyway I didn't bother to optimize this further.
Same for the second xfs_bmapi_read call.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/5] xfs: push the ilock into xfs_zero_eof
2012-03-27 5:48 ` Christoph Hellwig
@ 2012-03-27 6:11 ` Dave Chinner
0 siblings, 0 replies; 14+ messages in thread
From: Dave Chinner @ 2012-03-27 6:11 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Tue, Mar 27, 2012 at 01:48:27AM -0400, Christoph Hellwig wrote:
> On Tue, Mar 27, 2012 at 12:15:40PM +1100, Dave Chinner wrote:
> > > + xfs_ilock(ip, XFS_ILOCK_EXCL);
> > > error = xfs_bmapi_read(ip, last_fsb, 1, &imap, &nimaps, 0);
> > > + xfs_iunlock(ip, XFS_ILOCK_EXCL);
> >
> > Does that even need to be an exclusive lock? a shared lock is all
> > that is needed to do a lookup, and this is just a lookup...
>
> It has to be a xfs_ilock_map_shared - but given that we hold the iolock
> exclusive anyway I didn't bother to optimize this further.
Ok, makes sense.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 5/5] xfs: use shared ilock mode for direct IO writes by default
2012-03-26 21:14 [PATCH 0/5] reduce exclusive ilock hold times Christoph Hellwig
` (3 preceding siblings ...)
2012-03-26 21:14 ` [PATCH 4/5] xfs: push the ilock into xfs_zero_eof Christoph Hellwig
@ 2012-03-26 21:14 ` Christoph Hellwig
2012-03-27 1:39 ` Dave Chinner
2012-03-27 6:08 ` [PATCH 0/5] reduce exclusive ilock hold times Dave Chinner
5 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2012-03-26 21:14 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-optimize-direct-io-locking --]
[-- Type: text/plain, Size: 5443 bytes --]
From: Dave Chinner <dchinner@redhat.com>
For the direct IO write path, we only really need the ilock to be taken in
exclusive mode during IO submission if we need to do extent allocation
instaled of all the time.
Change the block mapping code to take the ilock in shared mode for the
initial block mapping, and only retake it exclusively when we actually
have to perform extent allocations. We were already dropping the ilock
for the transaction allocation, so this doesn't introduce new race windows.
Based on an earlier patch from Dave Chinner.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_aops.c | 21 +++++++++++++++++----
fs/xfs/xfs_iomap.c | 45 +++++++++++++++++++--------------------------
2 files changed, 36 insertions(+), 30 deletions(-)
Index: xfs/fs/xfs/xfs_aops.c
===================================================================
--- xfs.orig/fs/xfs/xfs_aops.c 2012-03-26 21:06:52.184213212 +0200
+++ xfs/fs/xfs/xfs_aops.c 2012-03-26 21:16:40.744358040 +0200
@@ -1146,7 +1146,14 @@ __xfs_get_blocks(
if (!create && direct && offset >= i_size_read(inode))
return 0;
- if (create) {
+ /*
+ * Direct I/O is usually done on preallocated files, so try getting
+ * a block mapping without an exclusive lock first. For buffer
+ * writes we already have the exclusive iolock anyway, so avoiding
+ * a lock roundtrip here by taking the ilock exclusive from the
+ * beginning is a useful micro optimization.
+ */
+ if (create && !direct) {
lockmode = XFS_ILOCK_EXCL;
xfs_ilock(ip, lockmode);
} else {
@@ -1169,22 +1176,28 @@ __xfs_get_blocks(
(imap.br_startblock == HOLESTARTBLOCK ||
imap.br_startblock == DELAYSTARTBLOCK))) {
if (direct) {
+ xfs_iunlock(ip, lockmode);
+
error = xfs_iomap_write_direct(ip, offset, size,
&imap, nimaps);
+ if (error)
+ return -error;
} else {
error = xfs_iomap_write_delay(ip, offset, size, &imap);
+ if (error)
+ goto out_unlock;
+
+ xfs_iunlock(ip, lockmode);
}
- if (error)
- goto out_unlock;
trace_xfs_get_blocks_alloc(ip, offset, size, 0, &imap);
} else if (nimaps) {
trace_xfs_get_blocks_found(ip, offset, size, 0, &imap);
+ xfs_iunlock(ip, lockmode);
} else {
trace_xfs_get_blocks_notfound(ip, offset, size);
goto out_unlock;
}
- xfs_iunlock(ip, lockmode);
if (imap.br_startblock != HOLESTARTBLOCK &&
imap.br_startblock != DELAYSTARTBLOCK) {
Index: xfs/fs/xfs/xfs_iomap.c
===================================================================
--- xfs.orig/fs/xfs/xfs_iomap.c 2012-03-26 21:06:52.200879787 +0200
+++ xfs/fs/xfs/xfs_iomap.c 2012-03-26 21:21:34.406100475 +0200
@@ -142,11 +142,7 @@ xfs_iomap_write_direct(
int committed;
int error;
- /*
- * Make sure that the dquots are there. This doesn't hold
- * the ilock across a disk read.
- */
- error = xfs_qm_dqattach_locked(ip, 0);
+ error = xfs_qm_dqattach(ip, 0);
if (error)
return XFS_ERROR(error);
@@ -158,7 +154,7 @@ xfs_iomap_write_direct(
if ((offset + count) > XFS_ISIZE(ip)) {
error = xfs_iomap_eof_align_last_fsb(mp, ip, extsz, &last_fsb);
if (error)
- goto error_out;
+ return XFS_ERROR(error);
} else {
if (nmaps && (imap->br_startblock == HOLESTARTBLOCK))
last_fsb = MIN(last_fsb, (xfs_fileoff_t)
@@ -190,7 +186,6 @@ xfs_iomap_write_direct(
/*
* Allocate and setup the transaction
*/
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
tp = xfs_trans_alloc(mp, XFS_TRANS_DIOSTRAT);
error = xfs_trans_reserve(tp, resblks,
XFS_WRITE_LOG_RES(mp), resrtextents,
@@ -199,15 +194,16 @@ xfs_iomap_write_direct(
/*
* Check for running out of space, note: need lock to return
*/
- if (error)
+ if (error) {
xfs_trans_cancel(tp, 0);
+ return XFS_ERROR(error);
+ }
+
xfs_ilock(ip, XFS_ILOCK_EXCL);
- if (error)
- goto error_out;
error = xfs_trans_reserve_quota_nblks(tp, ip, qblocks, 0, quota_flag);
if (error)
- goto error1;
+ goto out_trans_cancel;
xfs_trans_ijoin(tp, ip, 0);
@@ -224,42 +220,39 @@ xfs_iomap_write_direct(
error = xfs_bmapi_write(tp, ip, offset_fsb, count_fsb, bmapi_flag,
&firstfsb, 0, imap, &nimaps, &free_list);
if (error)
- goto error0;
+ goto out_bmap_cancel;
/*
* Complete the transaction
*/
error = xfs_bmap_finish(&tp, &free_list, &committed);
if (error)
- goto error0;
+ goto out_bmap_cancel;
error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
if (error)
- goto error_out;
+ goto out_unlock;
/*
* Copy any maps to caller's array and return any error.
*/
if (nimaps == 0) {
- error = ENOSPC;
- goto error_out;
+ error = XFS_ERROR(ENOSPC);
+ goto out_unlock;
}
- if (!(imap->br_startblock || XFS_IS_REALTIME_INODE(ip))) {
+ if (!(imap->br_startblock || XFS_IS_REALTIME_INODE(ip)))
error = xfs_alert_fsblock_zero(ip, imap);
- goto error_out;
- }
- return 0;
+out_unlock:
+ xfs_iunlock(ip, XFS_ILOCK_EXCL);
+ return error;
-error0: /* Cancel bmap, unlock inode, unreserve quota blocks, cancel trans */
+out_bmap_cancel:
xfs_bmap_cancel(&free_list);
xfs_trans_unreserve_quota_nblks(tp, ip, qblocks, 0, quota_flag);
-
-error1: /* Just cancel transaction */
+out_trans_cancel:
xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
-
-error_out:
- return XFS_ERROR(error);
+ goto out_unlock;
}
/*
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 5/5] xfs: use shared ilock mode for direct IO writes by default
2012-03-26 21:14 ` [PATCH 5/5] xfs: use shared ilock mode for direct IO writes by default Christoph Hellwig
@ 2012-03-27 1:39 ` Dave Chinner
0 siblings, 0 replies; 14+ messages in thread
From: Dave Chinner @ 2012-03-27 1:39 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Mar 26, 2012 at 05:14:26PM -0400, Christoph Hellwig wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> For the direct IO write path, we only really need the ilock to be taken in
> exclusive mode during IO submission if we need to do extent allocation
> instaled of all the time.
>
> Change the block mapping code to take the ilock in shared mode for the
> initial block mapping, and only retake it exclusively when we actually
> have to perform extent allocations. We were already dropping the ilock
> for the transaction allocation, so this doesn't introduce new race windows.
>
> Based on an earlier patch from Dave Chinner.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> ---
> fs/xfs/xfs_aops.c | 21 +++++++++++++++++----
> fs/xfs/xfs_iomap.c | 45 +++++++++++++++++++--------------------------
> 2 files changed, 36 insertions(+), 30 deletions(-)
>
> Index: xfs/fs/xfs/xfs_aops.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_aops.c 2012-03-26 21:06:52.184213212 +0200
> +++ xfs/fs/xfs/xfs_aops.c 2012-03-26 21:16:40.744358040 +0200
> @@ -1146,7 +1146,14 @@ __xfs_get_blocks(
> if (!create && direct && offset >= i_size_read(inode))
> return 0;
>
> - if (create) {
> + /*
> + * Direct I/O is usually done on preallocated files, so try getting
> + * a block mapping without an exclusive lock first. For buffer
buffered
.....
> @@ -1169,22 +1176,28 @@ __xfs_get_blocks(
> (imap.br_startblock == HOLESTARTBLOCK ||
> imap.br_startblock == DELAYSTARTBLOCK))) {
> if (direct) {
> + xfs_iunlock(ip, lockmode);
> +
> error = xfs_iomap_write_direct(ip, offset, size,
> &imap, nimaps);
> + if (error)
> + return -error;
> } else {
> error = xfs_iomap_write_delay(ip, offset, size, &imap);
> + if (error)
> + goto out_unlock;
> +
> + xfs_iunlock(ip, lockmode);
I'm not sure that I like the implicit different lock semantics here.
One function drops the lock first, the other requires the lock to be
held. Perhaps a comment is in order, such that direct IO requires
transactions for allocation here and hence will drop the lock to do
so and this avoids a needless lock round-trip?
Actually, when would we *ever* take the direct IO path when we have
imap.br_startblock == DELAYSTARTBLOCK? Buffered write regions are
supposed to have been flushed before the DIO write gets here.
Perhaps there should be an ASSERT(imap.br_startblock ==
HOLESTARTBLOCK) in the direct IO path.
Otherwise it looks OK.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/5] reduce exclusive ilock hold times
2012-03-26 21:14 [PATCH 0/5] reduce exclusive ilock hold times Christoph Hellwig
` (4 preceding siblings ...)
2012-03-26 21:14 ` [PATCH 5/5] xfs: use shared ilock mode for direct IO writes by default Christoph Hellwig
@ 2012-03-27 6:08 ` Dave Chinner
5 siblings, 0 replies; 14+ messages in thread
From: Dave Chinner @ 2012-03-27 6:08 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Mar 26, 2012 at 05:14:21PM -0400, Christoph Hellwig wrote:
> This series tries to reduce the amount we hold the ilock exclusively,
> especially during direct I/O writes where they currently hurt us.
>
> Dave showed that his earlier version which is less aggressive than this
> one can already provide magnitudes of better throughput and iops for
> parallel direct I/O workloads, and this one should be even better.
Shows the same results for the recent sysbench testing as my patch,
but that is CPU bound and so there is little scope for improvement.
It is showing about 4.9GB/s as the maximum write rate with 16k IOs.
I get similar results from the rrtest code that was previously used
to demonstrate this problem - it's showing about about 1.2 million
4k IOPS, which is about 4.8GB/s as well. I think that must be the
limit of what the ramdisk code can handle on my setup.
So the performance side of this works just fine.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 14+ messages in thread