* [PATCH 0/3] try to avoid locked inodes during log force
@ 2011-09-18 20:47 Christoph Hellwig
2011-09-18 20:47 ` [PATCH 1/3] xfs: unlock the inode before log force in xfs_fsync Christoph Hellwig
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Christoph Hellwig @ 2011-09-18 20:47 UTC (permalink / raw)
To: xfs
This series fixes a few places to unlock the inode before doing
synchronous log forces to avoid potential overly long log hold
times.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/3] xfs: unlock the inode before log force in xfs_fsync 2011-09-18 20:47 [PATCH 0/3] try to avoid locked inodes during log force Christoph Hellwig @ 2011-09-18 20:47 ` Christoph Hellwig 2011-09-18 22:59 ` Dave Chinner 2011-09-29 16:59 ` [PATCH 1/3] " Alex Elder 2011-09-18 20:47 ` [PATCH 2/3] xfs: unlock the inode before log force in xfs_fs_nfs_commit_metadata Christoph Hellwig 2011-09-18 20:47 ` [PATCH 3/3] xfs: unlock the inode before log force in xfs_change_file_space Christoph Hellwig 2 siblings, 2 replies; 11+ messages in thread From: Christoph Hellwig @ 2011-09-18 20:47 UTC (permalink / raw) To: xfs [-- Attachment #1: xfs-optimize-fsync-2 --] [-- Type: text/plain, Size: 3561 bytes --] Only read the LSN we need to push to with the ilock held, and then release it before we do the log force to improve concurrency. This also removes the only direct caller of _xfs_trans_commit, thus allowing it to be merged into the plain xfs_trans_commit again. Signed-off-by: Christoph Hellwig <hch@lst.de> Index: xfs/fs/xfs/xfs_file.c =================================================================== --- xfs.orig/fs/xfs/xfs_file.c 2011-09-07 10:59:55.742961175 +0200 +++ xfs/fs/xfs/xfs_file.c 2011-09-07 11:08:12.830462974 +0200 @@ -137,6 +137,7 @@ xfs_file_fsync( struct xfs_trans *tp; int error = 0; int log_flushed = 0; + xfs_lsn_t lsn = 0; trace_xfs_file_fsync(ip); @@ -214,8 +215,10 @@ xfs_file_fsync( */ xfs_trans_ijoin(tp, ip); xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); - xfs_trans_set_sync(tp); - error = _xfs_trans_commit(tp, 0, &log_flushed); + error = xfs_trans_commit(tp, 0); + + ASSERT(xfs_ipincount(ip)); + lsn = ip->i_itemp->ili_last_lsn; xfs_iunlock(ip, XFS_ILOCK_EXCL); } else { @@ -227,14 +230,14 @@ xfs_file_fsync( * disk yet, the inode will be still be pinned. If it is, * force the log. */ - if (xfs_ipincount(ip)) { - error = _xfs_log_force_lsn(mp, - ip->i_itemp->ili_last_lsn, - XFS_LOG_SYNC, &log_flushed); - } + if (xfs_ipincount(ip)) + lsn = ip->i_itemp->ili_last_lsn; xfs_iunlock(ip, XFS_ILOCK_SHARED); } + if (!error && lsn) + error = _xfs_log_force_lsn(mp, lsn, XFS_LOG_SYNC, &log_flushed); + /* * If we only have a single device, and the log force about was * a no-op we might have to flush the data device cache here. Index: xfs/fs/xfs/xfs_trans.c =================================================================== --- xfs.orig/fs/xfs/xfs_trans.c 2011-09-07 10:59:55.702959720 +0200 +++ xfs/fs/xfs/xfs_trans.c 2011-09-07 11:06:20.686461455 +0200 @@ -1790,9 +1790,7 @@ xfs_trans_commit_cil( } /* - * xfs_trans_commit - * - * Commit the given transaction to the log a/synchronously. + * Commit the given transaction to the log. * * XFS disk error handling mechanism is not based on a typical * transaction abort mechanism. Logically after the filesystem @@ -1804,10 +1802,9 @@ xfs_trans_commit_cil( * Do not reference the transaction structure after this call. */ int -_xfs_trans_commit( +xfs_trans_commit( struct xfs_trans *tp, - uint flags, - int *log_flushed) + uint flags) { struct xfs_mount *mp = tp->t_mountp; xfs_lsn_t commit_lsn = -1; @@ -1866,7 +1863,7 @@ _xfs_trans_commit( if (sync) { if (!error) { error = _xfs_log_force_lsn(mp, commit_lsn, - XFS_LOG_SYNC, log_flushed); + XFS_LOG_SYNC, NULL); } XFS_STATS_INC(xs_trans_sync); } else { Index: xfs/fs/xfs/xfs_trans.h =================================================================== --- xfs.orig/fs/xfs/xfs_trans.h 2011-09-07 10:59:55.722959398 +0200 +++ xfs/fs/xfs/xfs_trans.h 2011-09-07 11:05:11.766461375 +0200 @@ -487,10 +487,7 @@ void xfs_trans_log_efd_extent(xfs_trans struct xfs_efd_log_item *, xfs_fsblock_t, xfs_extlen_t); -int _xfs_trans_commit(xfs_trans_t *, - uint flags, - int *); -#define xfs_trans_commit(tp, flags) _xfs_trans_commit(tp, flags, NULL) +int xfs_trans_commit(xfs_trans_t *, uint flags); void xfs_trans_cancel(xfs_trans_t *, int); int xfs_trans_ail_init(struct xfs_mount *); void xfs_trans_ail_destroy(struct xfs_mount *); _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] xfs: unlock the inode before log force in xfs_fsync 2011-09-18 20:47 ` [PATCH 1/3] xfs: unlock the inode before log force in xfs_fsync Christoph Hellwig @ 2011-09-18 22:59 ` Dave Chinner 2011-09-19 14:55 ` [PATCH 1/3 v2] " Christoph Hellwig 2011-09-29 16:59 ` [PATCH 1/3] " Alex Elder 1 sibling, 1 reply; 11+ messages in thread From: Dave Chinner @ 2011-09-18 22:59 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs On Sun, Sep 18, 2011 at 04:47:49PM -0400, Christoph Hellwig wrote: > Only read the LSN we need to push to with the ilock held, and then release > it before we do the log force to improve concurrency. > > This also removes the only direct caller of _xfs_trans_commit, thus > allowing it to be merged into the plain xfs_trans_commit again. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > Index: xfs/fs/xfs/xfs_file.c > =================================================================== > --- xfs.orig/fs/xfs/xfs_file.c 2011-09-07 10:59:55.742961175 +0200 > +++ xfs/fs/xfs/xfs_file.c 2011-09-07 11:08:12.830462974 +0200 > @@ -137,6 +137,7 @@ xfs_file_fsync( > struct xfs_trans *tp; > int error = 0; > int log_flushed = 0; > + xfs_lsn_t lsn = 0; > > trace_xfs_file_fsync(ip); > > @@ -214,8 +215,10 @@ xfs_file_fsync( > */ > xfs_trans_ijoin(tp, ip); > xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); > - xfs_trans_set_sync(tp); > - error = _xfs_trans_commit(tp, 0, &log_flushed); > + error = xfs_trans_commit(tp, 0); > + > + ASSERT(xfs_ipincount(ip)); That's a racy assert. If the trans commit causes the CIL to be pushed, that could complete and unpin the inode before we start executing here again and check the pin count. 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] 11+ messages in thread
* [PATCH 1/3 v2] xfs: unlock the inode before log force in xfs_fsync 2011-09-18 22:59 ` Dave Chinner @ 2011-09-19 14:55 ` Christoph Hellwig 0 siblings, 0 replies; 11+ messages in thread From: Christoph Hellwig @ 2011-09-19 14:55 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs Only read the LSN we need to push to with the ilock held, and then release it before we do the log force to improve concurrency. This also removes the only direct caller of _xfs_trans_commit, thus allowing it to be merged into the plain xfs_trans_commit again. Signed-off-by: Christoph Hellwig <hch@lst.de> Index: xfs/fs/xfs/xfs_file.c =================================================================== --- xfs.orig/fs/xfs/xfs_file.c 2011-09-18 16:34:03.995780628 -0400 +++ xfs/fs/xfs/xfs_file.c 2011-09-19 10:47:07.679732387 -0400 @@ -137,6 +137,7 @@ xfs_file_fsync( struct xfs_trans *tp; int error = 0; int log_flushed = 0; + xfs_lsn_t lsn = 0; trace_xfs_file_fsync(ip); @@ -214,9 +215,9 @@ xfs_file_fsync( */ xfs_trans_ijoin(tp, ip); xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); - xfs_trans_set_sync(tp); - error = _xfs_trans_commit(tp, 0, &log_flushed); + error = xfs_trans_commit(tp, 0); + lsn = ip->i_itemp->ili_last_lsn; xfs_iunlock(ip, XFS_ILOCK_EXCL); } else { /* @@ -227,14 +228,14 @@ xfs_file_fsync( * disk yet, the inode will be still be pinned. If it is, * force the log. */ - if (xfs_ipincount(ip)) { - error = _xfs_log_force_lsn(mp, - ip->i_itemp->ili_last_lsn, - XFS_LOG_SYNC, &log_flushed); - } + if (xfs_ipincount(ip)) + lsn = ip->i_itemp->ili_last_lsn; xfs_iunlock(ip, XFS_ILOCK_SHARED); } + if (!error && lsn) + error = _xfs_log_force_lsn(mp, lsn, XFS_LOG_SYNC, &log_flushed); + /* * If we only have a single device, and the log force about was * a no-op we might have to flush the data device cache here. Index: xfs/fs/xfs/xfs_trans.c =================================================================== --- xfs.orig/fs/xfs/xfs_trans.c 2011-09-18 16:31:44.255777336 -0400 +++ xfs/fs/xfs/xfs_trans.c 2011-09-19 10:46:47.181231805 -0400 @@ -1790,9 +1790,7 @@ xfs_trans_commit_cil( } /* - * xfs_trans_commit - * - * Commit the given transaction to the log a/synchronously. + * Commit the given transaction to the log. * * XFS disk error handling mechanism is not based on a typical * transaction abort mechanism. Logically after the filesystem @@ -1804,10 +1802,9 @@ xfs_trans_commit_cil( * Do not reference the transaction structure after this call. */ int -_xfs_trans_commit( +xfs_trans_commit( struct xfs_trans *tp, - uint flags, - int *log_flushed) + uint flags) { struct xfs_mount *mp = tp->t_mountp; xfs_lsn_t commit_lsn = -1; @@ -1866,7 +1863,7 @@ _xfs_trans_commit( if (sync) { if (!error) { error = _xfs_log_force_lsn(mp, commit_lsn, - XFS_LOG_SYNC, log_flushed); + XFS_LOG_SYNC, NULL); } XFS_STATS_INC(xs_trans_sync); } else { Index: xfs/fs/xfs/xfs_trans.h =================================================================== --- xfs.orig/fs/xfs/xfs_trans.h 2011-09-18 16:31:44.271777154 -0400 +++ xfs/fs/xfs/xfs_trans.h 2011-09-19 10:46:47.081232193 -0400 @@ -487,10 +487,7 @@ void xfs_trans_log_efd_extent(xfs_trans struct xfs_efd_log_item *, xfs_fsblock_t, xfs_extlen_t); -int _xfs_trans_commit(xfs_trans_t *, - uint flags, - int *); -#define xfs_trans_commit(tp, flags) _xfs_trans_commit(tp, flags, NULL) +int xfs_trans_commit(xfs_trans_t *, uint flags); void xfs_trans_cancel(xfs_trans_t *, int); int xfs_trans_ail_init(struct xfs_mount *); void xfs_trans_ail_destroy(struct xfs_mount *); _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] xfs: unlock the inode before log force in xfs_fsync 2011-09-18 20:47 ` [PATCH 1/3] xfs: unlock the inode before log force in xfs_fsync Christoph Hellwig 2011-09-18 22:59 ` Dave Chinner @ 2011-09-29 16:59 ` Alex Elder 1 sibling, 0 replies; 11+ messages in thread From: Alex Elder @ 2011-09-29 16:59 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs On Sun, 2011-09-18 at 16:47 -0400, Christoph Hellwig wrote: > Only read the LSN we need to push to with the ilock held, and then release > it before we do the log force to improve concurrency. > > This also removes the only direct caller of _xfs_trans_commit, thus > allowing it to be merged into the plain xfs_trans_commit again. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Looks good. The idea here is that once the transaction is committed (which also sets the LSN to use), everything we need to know about any changes are in the log so there's no need to prevent others from moving on from that point--allowing the inode to (transactionally) change to a new state--while the log gets forced out. Right? Reviewed-by: Alex Elder <aelder@sgi.com> _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] xfs: unlock the inode before log force in xfs_fs_nfs_commit_metadata 2011-09-18 20:47 [PATCH 0/3] try to avoid locked inodes during log force Christoph Hellwig 2011-09-18 20:47 ` [PATCH 1/3] xfs: unlock the inode before log force in xfs_fsync Christoph Hellwig @ 2011-09-18 20:47 ` Christoph Hellwig 2011-09-18 23:00 ` Dave Chinner 2011-09-29 16:59 ` Alex Elder 2011-09-18 20:47 ` [PATCH 3/3] xfs: unlock the inode before log force in xfs_change_file_space Christoph Hellwig 2 siblings, 2 replies; 11+ messages in thread From: Christoph Hellwig @ 2011-09-18 20:47 UTC (permalink / raw) To: xfs [-- Attachment #1: xfs-optimize-commit_metadata --] [-- Type: text/plain, Size: 1119 bytes --] Only read the LSN we need to push to with the ilock held, and then release it before we do the log force to improve concurrency. Signed-off-by: Christoph Hellwig <hch@lst.de> Index: xfs/fs/xfs/xfs_export.c =================================================================== --- xfs.orig/fs/xfs/xfs_export.c 2011-09-07 11:08:48.670961391 +0200 +++ xfs/fs/xfs/xfs_export.c 2011-09-07 11:08:56.515960258 +0200 @@ -229,16 +229,16 @@ xfs_fs_nfs_commit_metadata( { struct xfs_inode *ip = XFS_I(inode); struct xfs_mount *mp = ip->i_mount; - int error = 0; + xfs_lsn_t lsn = 0; xfs_ilock(ip, XFS_ILOCK_SHARED); - if (xfs_ipincount(ip)) { - error = _xfs_log_force_lsn(mp, ip->i_itemp->ili_last_lsn, - XFS_LOG_SYNC, NULL); - } + if (xfs_ipincount(ip)) + lsn = ip->i_itemp->ili_last_lsn; xfs_iunlock(ip, XFS_ILOCK_SHARED); - return error; + if (!lsn) + return 0; + return _xfs_log_force_lsn(mp, lsn, XFS_LOG_SYNC, NULL); } const struct export_operations xfs_export_operations = { _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] xfs: unlock the inode before log force in xfs_fs_nfs_commit_metadata 2011-09-18 20:47 ` [PATCH 2/3] xfs: unlock the inode before log force in xfs_fs_nfs_commit_metadata Christoph Hellwig @ 2011-09-18 23:00 ` Dave Chinner 2011-09-29 16:59 ` Alex Elder 1 sibling, 0 replies; 11+ messages in thread From: Dave Chinner @ 2011-09-18 23:00 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs On Sun, Sep 18, 2011 at 04:47:50PM -0400, Christoph Hellwig wrote: > Only read the LSN we need to push to with the ilock held, and then release > it before we do the log force to improve concurrency. > > Signed-off-by: Christoph Hellwig <hch@lst.de> 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] 11+ messages in thread
* Re: [PATCH 2/3] xfs: unlock the inode before log force in xfs_fs_nfs_commit_metadata 2011-09-18 20:47 ` [PATCH 2/3] xfs: unlock the inode before log force in xfs_fs_nfs_commit_metadata Christoph Hellwig 2011-09-18 23:00 ` Dave Chinner @ 2011-09-29 16:59 ` Alex Elder 1 sibling, 0 replies; 11+ messages in thread From: Alex Elder @ 2011-09-29 16:59 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs On Sun, 2011-09-18 at 16:47 -0400, Christoph Hellwig wrote: > Only read the LSN we need to push to with the ilock held, and then release > it before we do the log force to improve concurrency. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Looks good. Reviewed-by: Alex Elder <aelder@sgi.com> _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] xfs: unlock the inode before log force in xfs_change_file_space 2011-09-18 20:47 [PATCH 0/3] try to avoid locked inodes during log force Christoph Hellwig 2011-09-18 20:47 ` [PATCH 1/3] xfs: unlock the inode before log force in xfs_fsync Christoph Hellwig 2011-09-18 20:47 ` [PATCH 2/3] xfs: unlock the inode before log force in xfs_fs_nfs_commit_metadata Christoph Hellwig @ 2011-09-18 20:47 ` Christoph Hellwig 2011-09-18 23:00 ` Dave Chinner 2011-09-29 16:59 ` Alex Elder 2 siblings, 2 replies; 11+ messages in thread From: Christoph Hellwig @ 2011-09-18 20:47 UTC (permalink / raw) To: xfs [-- Attachment #1: xfs-optimize-osync-prealloc --] [-- Type: text/plain, Size: 1024 bytes --] Let the transaction commit unlock the inode before it potentially causes a synchronous log force. Signed-off-by: Christoph Hellwig <hch@lst.de> Index: xfs/fs/xfs/xfs_vnodeops.c =================================================================== --- xfs.orig/fs/xfs/xfs_vnodeops.c 2011-09-18 11:41:41.000000000 -0400 +++ xfs/fs/xfs/xfs_vnodeops.c 2011-09-18 11:43:05.951778942 -0400 @@ -2342,8 +2342,7 @@ xfs_change_file_space( } xfs_ilock(ip, XFS_ILOCK_EXCL); - - xfs_trans_ijoin(tp, ip); + xfs_trans_ijoin_ref(tp, ip, XFS_ILOCK_EXCL); if ((attr_flags & XFS_ATTR_DMI) == 0) { ip->i_d.di_mode &= ~S_ISUID; @@ -2368,10 +2367,5 @@ xfs_change_file_space( xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); if (attr_flags & XFS_ATTR_SYNC) xfs_trans_set_sync(tp); - - error = xfs_trans_commit(tp, 0); - - xfs_iunlock(ip, XFS_ILOCK_EXCL); - - return error; + return xfs_trans_commit(tp, 0); } _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] xfs: unlock the inode before log force in xfs_change_file_space 2011-09-18 20:47 ` [PATCH 3/3] xfs: unlock the inode before log force in xfs_change_file_space Christoph Hellwig @ 2011-09-18 23:00 ` Dave Chinner 2011-09-29 16:59 ` Alex Elder 1 sibling, 0 replies; 11+ messages in thread From: Dave Chinner @ 2011-09-18 23:00 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs On Sun, Sep 18, 2011 at 04:47:51PM -0400, Christoph Hellwig wrote: > Let the transaction commit unlock the inode before it potentially causes > a synchronous log force. > > Signed-off-by: Christoph Hellwig <hch@lst.de> 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] 11+ messages in thread
* Re: [PATCH 3/3] xfs: unlock the inode before log force in xfs_change_file_space 2011-09-18 20:47 ` [PATCH 3/3] xfs: unlock the inode before log force in xfs_change_file_space Christoph Hellwig 2011-09-18 23:00 ` Dave Chinner @ 2011-09-29 16:59 ` Alex Elder 1 sibling, 0 replies; 11+ messages in thread From: Alex Elder @ 2011-09-29 16:59 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs On Sun, 2011-09-18 at 16:47 -0400, Christoph Hellwig wrote: > Let the transaction commit unlock the inode before it potentially causes > a synchronous log force. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Looks good. Reviewed-by: Alex Elder <aelder@sgi.com> _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2011-09-29 16:59 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-09-18 20:47 [PATCH 0/3] try to avoid locked inodes during log force Christoph Hellwig 2011-09-18 20:47 ` [PATCH 1/3] xfs: unlock the inode before log force in xfs_fsync Christoph Hellwig 2011-09-18 22:59 ` Dave Chinner 2011-09-19 14:55 ` [PATCH 1/3 v2] " Christoph Hellwig 2011-09-29 16:59 ` [PATCH 1/3] " Alex Elder 2011-09-18 20:47 ` [PATCH 2/3] xfs: unlock the inode before log force in xfs_fs_nfs_commit_metadata Christoph Hellwig 2011-09-18 23:00 ` Dave Chinner 2011-09-29 16:59 ` Alex Elder 2011-09-18 20:47 ` [PATCH 3/3] xfs: unlock the inode before log force in xfs_change_file_space Christoph Hellwig 2011-09-18 23:00 ` Dave Chinner 2011-09-29 16:59 ` Alex Elder
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox