xfs: remove the xfssyncd mess
v2 cover xfs: remove the xfssyncd mess
Version 2 of the patchset I described here:
http://oss.sgi.com/archives/xfs/2012-06/msg00064.html
This version has run through xfstests completely once, so it's less likely to let smoke out....
Version 2:
- fix writeback_inodes_sb_if_idle call in xfs_create()
- refreshed patch 13 before sending.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 1/13 xfs: xfs_syncd_stop must die
From: Dave Chinner <[email protected]>
xfs_syncd_start and xfs_syncd_stop tie a bunch of unrelated functionailty together that actually have different start and stop requirements. Kill these functions and open code the start/stop methods for each of the background functions.
Subsequent patches will move the start/stop functions around to the correct places to avoid races and shutdown issues.
--- fs/xfs/xfs_super.c | 25 ++++++++++++++++++------- fs/xfs/xfs_sync.c | 30 ++++-------------------------- fs/xfs/xfs_sync.h | 6 ++++-- 3 files changed, 26 insertions(+), 35 deletions(-)
fs/xfs/xfs_super.c +18 -7
@@ -920,7 +920,11 @@ xfs_fs_put_super(xfs_filestream_unmount (mp);xfs_unmountfs (mp); -xfs_syncd_stop (mp); + +cancel_delayed_work_sync (&mp->m_sync_work); +cancel_delayed_work_sync (&mp->m_reclaim_work); +cancel_work_sync (&mp->m_flush_work); +xfs_freesb (mp);xfs_icsb_destroy_counters (mp);xfs_destroy_mount_workqueues (mp);
@@ -1290,9 +1294,11 @@ xfs_fs_fill_super( sb->s_time_gran =1 ;set_posix_acl_flag (sb); - error =xfs_syncd_init (mp); -if (error) - goto out_filestream_unmount; +INIT_WORK (&mp->m_flush_work, xfs_flush_worker); +INIT_DELAYED_WORK (&mp->m_sync_work, xfs_sync_worker); +INIT_DELAYED_WORK (&mp->m_reclaim_work, xfs_reclaim_worker); + +xfs_syncd_queue_sync (mp); error =xfs_mountfs (mp);if (error)
@@ -1315,8 +1321,10 @@ xfs_fs_fill_super(return 0 ; out_syncd_stop: -xfs_syncd_stop (mp); - out_filestream_unmount: +cancel_delayed_work_sync (&mp->m_sync_work); +cancel_delayed_work_sync (&mp->m_reclaim_work); +cancel_work_sync (&mp->m_flush_work); +xfs_filestream_unmount (mp); out_free_sb:xfs_freesb (mp);
@@ -1335,7 +1343,10 @@ out_destroy_workqueues: out_unmount:xfs_filestream_unmount (mp);xfs_unmountfs (mp); -xfs_syncd_stop (mp); + +cancel_delayed_work_sync (&mp->m_sync_work); +cancel_delayed_work_sync (&mp->m_reclaim_work); +cancel_work_sync (&mp->m_flush_work); goto out_free_sb; }
fs/xfs/xfs_sync.c +4 -26
@@ -370,7 +370,7 @@ xfs_quiesce_attr(xfs_buf_unlock (mp->m_sb_bp); } -static void +void xfs_syncd_queue_sync (struct xfs_mount *mp) {
@@ -383,7 +383,7 @@ xfs_syncd_queue_sync(* disk quotas. We might need to cover the log to indicate that the * filesystem is idle and not frozen. */ -STATIC void +void xfs_sync_worker (struct work_struct *work) {
@@ -445,7 +445,7 @@ xfs_syncd_queue_reclaim(* goes low. It scans as quickly as possible avoiding locked inodes or those * already being flushed, and once done schedules a future pass. */ -STATIC void +void xfs_reclaim_worker (struct work_struct *work) {
@@ -478,7 +478,7 @@ xfs_flush_inodes(flush_work_sync (&mp->m_flush_work); } -STATIC void +void xfs_flush_worker (struct work_struct *work) {
@@ -489,28 +489,6 @@ xfs_flush_worker(xfs_sync_data (mp,SYNC_TRYLOCK |SYNC_WAIT ); } -int -xfs_syncd_init ( -struct xfs_mount *mp) -{ -INIT_WORK (&mp->m_flush_work, xfs_flush_worker); -INIT_DELAYED_WORK (&mp->m_sync_work, xfs_sync_worker); -INIT_DELAYED_WORK (&mp->m_reclaim_work, xfs_reclaim_worker); - -xfs_syncd_queue_sync (mp); - -return 0 ; -} - -void -xfs_syncd_stop ( -struct xfs_mount *mp) -{ -cancel_delayed_work_sync (&mp->m_sync_work); -cancel_delayed_work_sync (&mp->m_reclaim_work); -cancel_work_sync (&mp->m_flush_work); -} - void __xfs_inode_set_reclaim_tag( struct xfs_perag *pag,
fs/xfs/xfs_sync.h +4 -2
@@ -26,8 +26,10 @@ struct xfs_perag;extern struct workqueue_struct *xfs_syncd_wq;/* sync workqueue */ -int xfs_syncd_init (struct xfs_mount *mp); -void xfs_syncd_stop (struct xfs_mount *mp); +void xfs_syncd_queue_sync (struct xfs_mount *mp); +void xfs_sync_worker (struct work_struct *work); +void xfs_flush_worker (struct work_struct *work); +void xfs_reclaim_worker (struct work_struct *work);int xfs_quiesce_data (struct xfs_mount *mp);void xfs_quiesce_attr (struct xfs_mount *mp);
Signature · 6 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 01/13] xfs: xfs_syncd_stop must die
Quoted 11 lines · Dave Chinner
From: Dave Chinner <[email protected]>
xfs_syncd_startandxfs_syncd_stoptie a bunch of unrelated functionailty together that actually have different start and stop requirements. Kill these functions and open code the start/stop methods for each of the background functions.Subsequent patches will move the start/stop functions around to the correct places to avoid races and shutdown issues.
Signed-off-by Dave Chinner <[email protected]>
Looks good,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 01/13] xfs: xfs_syncd_stop must die
In reply to Dave ChinnerQuoted 11 lines · Dave Chinner
From: Dave Chinner<[email protected]>
xfs_syncd_startandxfs_syncd_stoptie a bunch of unrelated functionailty together that actually have different start and stop requirements. Kill these functions and open code the start/stop methods for each of the background functions.Subsequent patches will move the start/stop functions around to the correct places to avoid races and shutdown issues.
Signed-off-by Dave Chinner<[email protected]>
Looks good.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 2/13 xfs: rename the xfs_syncd workqueue
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
There is nothing "sync" realted to this work queue any more. It is a general purpose per-filesystem work queue. Rename it appropriately, and remove the "syncd" naming from various functions.
--- fs/xfs/xfs_mount.c | 1 + fs/xfs/xfs_mount.h | 2 ++ fs/xfs/xfs_super.c | 14 +++++++------- fs/xfs/xfs_sync.c | 19 +++++++++---------- fs/xfs/xfs_sync.h | 4 +--- 5 files changed, 20 insertions(+), 20 deletions(-)
fs/xfs/xfs_mount.c +1 -0
@@ -43,6 +43,7 @@#include "xfs_utils.h" #include "xfs_trace.h" +struct workqueue_struct *xfs_mount_wq;#ifdef HAVE_PERCPU_SB STATIC void xfs_icsb_balance_counter(xfs_mount_t *, xfs_sb_field_t,
fs/xfs/xfs_mount.h +2 -0
@@ -60,6 +60,8 @@ struct xfs_nameops;struct xfs_ail ;struct xfs_quotainfo ; +extern struct workqueue_struct *xfs_mount_wq; +#ifdef HAVE_PERCPU_SB /*
fs/xfs/xfs_super.c +7 -7
@@ -1298,7 +1298,7 @@ xfs_fs_fill_super(INIT_DELAYED_WORK (&mp->m_sync_work, xfs_sync_worker);INIT_DELAYED_WORK (&mp->m_reclaim_work, xfs_reclaim_worker); -xfs_syncd_queue_sync (mp); +xfs_sync_work_queue (mp); error =xfs_mountfs (mp);if (error)
@@ -1542,8 +1542,8 @@ xfs_init_workqueues(void)* competing for ressources. Use the default large max_active value * so that even lots of filesystems can perform these task in parallel. */ - xfs_syncd_wq =alloc_workqueue ("xfssyncd" ,WQ_NON_REENTRANT ,0 ); -if (!xfs_syncd_wq) + xfs_mount_wq =alloc_workqueue ("xfsmount" ,WQ_NON_REENTRANT ,0 ); +if (!xfs_mount_wq)return -ENOMEM ;/*
@@ -1554,12 +1554,12 @@ xfs_init_workqueues(void)*/ xfs_alloc_wq =alloc_workqueue ("xfsalloc" ,WQ_MEM_RECLAIM ,0 );if (!xfs_alloc_wq) - goto out_destroy_syncd; + goto out_destroy_mount;return 0 ; -out_destroy_syncd: -destroy_workqueue (xfs_syncd_wq); +out_destroy_mount: +destroy_workqueue (xfs_mount_wq);return -ENOMEM ; }
@@ -1567,7 +1567,7 @@ STATIC voidxfs_destroy_workqueues (void) {destroy_workqueue (xfs_alloc_wq); -destroy_workqueue (xfs_syncd_wq); +destroy_workqueue (xfs_mount_wq); }STATIC int__init
fs/xfs/xfs_sync.c +9 -10
@@ -39,7 +39,6 @@#include <linux/kthread.h> #include <linux/freezer.h> -struct workqueue_struct *xfs_syncd_wq;/* sync workqueue */ /* * The inode lookup is done in batches to keep the amount of lock traffic and
@@ -371,10 +370,10 @@ xfs_quiesce_attr( }void -xfs_syncd_queue_sync ( +xfs_sync_work_queue (struct xfs_mount *mp) { -queue_delayed_work (xfs_syncd_wq, &mp->m_sync_work, +queue_delayed_work (xfs_mount_wq, &mp->m_sync_work,msecs_to_jiffies (xfs_syncd_centisecs *10 )); }
@@ -415,7 +414,7 @@ xfs_sync_worker( }/* queue us up again */ -xfs_syncd_queue_sync (mp); +xfs_sync_work_queue (mp); }/*
@@ -426,13 +425,13 @@ xfs_sync_worker(* aggressive. */ static void -xfs_syncd_queue_reclaim ( +xfs_reclaim_queue_work (struct xfs_mount *mp) {rcu_read_lock ();if (radix_tree_tagged (&mp->m_perag_tree,XFS_ICI_RECLAIM_TAG )) { -queue_delayed_work (xfs_syncd_wq, &mp->m_reclaim_work, +queue_delayed_work (xfs_mount_wq, &mp->m_reclaim_work,msecs_to_jiffies (xfs_syncd_centisecs /6 *10 )); }rcu_read_unlock ();
@@ -453,7 +452,7 @@ xfs_reclaim_worker( struct xfs_mount, m_reclaim_work);xfs_reclaim_inodes (mp,SYNC_TRYLOCK ); -xfs_syncd_queue_reclaim (mp); +xfs_reclaim_queue_work (mp); }/*
@@ -474,7 +473,7 @@ xfs_flush_inodes( {struct xfs_mount *mp = ip->i_mount; -queue_work (xfs_syncd_wq, &mp->m_flush_work); +queue_work (xfs_mount_wq, &mp->m_flush_work);flush_work_sync (&mp->m_flush_work); }
@@ -507,7 +506,7 @@ __xfs_inode_set_reclaim_tag(spin_unlock (&ip->i_mount->m_perag_lock);/* schedule periodic background inode reclaim */ -xfs_syncd_queue_reclaim (ip->i_mount); +xfs_reclaim_queue_work (ip->i_mount);trace_xfs_perag_set_reclaim (ip->i_mount, pag->pag_agno,-1 , _RET_IP_);
@@ -923,7 +922,7 @@ xfs_reclaim_inodes_nr( int nr_to_scan) {/* kick background reclaimer and push the AIL */ -xfs_syncd_queue_reclaim (mp); +xfs_reclaim_queue_work (mp);xfs_ail_push_all (mp->m_ail);xfs_reclaim_inodes_ag (mp,SYNC_TRYLOCK |SYNC_WAIT , &nr_to_scan);
fs/xfs/xfs_sync.h +1 -3
@@ -24,9 +24,7 @@ struct xfs_perag;#define SYNC_WAIT 0x0001/* wait for i/o to complete */ #define SYNC_TRYLOCK 0x0002/* only try to lock inodes */ -extern struct workqueue_struct *xfs_syncd_wq;/* sync workqueue */ - -void xfs_syncd_queue_sync (struct xfs_mount *mp); +void xfs_sync_work_queue (struct xfs_mount *mp);void xfs_sync_worker (struct work_struct *work);void xfs_flush_worker (struct work_struct *work);void xfs_reclaim_worker (struct work_struct *work);
Signature · 6 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 02/13] xfs: rename the xfs_syncd workqueue
Quoted 7 lines · Dave Chinner
From: Dave Chinner <[email protected]>
There is nothing "sync" realted to this work queue any more. It is a general purpose per-filesystem work queue. Rename it appropriately, and remove the "syncd" naming from various functions.
Signed-off-by Dave Chinner <[email protected]>
With the current leight-weight workqueues I don't see why we'd even want a shared one for the different callers. Let's move the patch until after killing xfs_flush_worker & co, and then add a workqueue for background and the log sync individually.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 02/13] xfs: rename the xfs_syncd workqueue
Quoted 7 lines · Christoph Hellwig
Quoted 6 lines · Dave Chinner
From: Dave Chinner <[email protected]>
There is nothing "sync" realted to this work queue any more. It is a general purpose per-filesystem work queue. Rename it appropriately, and remove the "syncd" naming from various functions.
Quoted 6 lines
Quoted 1 line
Signed-off-by Dave Chinner <[email protected]>With the current leight-weight workqueues I don't see why we'd even want a shared one for the different callers. Let's move the patch until after killing
xfs_flush_worker& co, and then add a workqueue for background and the log sync individually.
OK. It starts to make workqueues like slab caches - all the cool kids have got one. :)
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 3/13 xfs: rationalise xfs_mount_wq users
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
Instead of starting and stopping background work on the xfs_mount_wq all at the same time, separate them to where they really are needed to start and stop.
The xfs_sync_worker, only needs to be started after all the mount processing has completed successfully, while it needs to be stopped before the log is unmounted.
The xfs_reclaim_worker is started on demand, and can be stopped before the unmount process does it's own inode reclaim pass.
The xfs_flush_inodes work is run on demand, and so we really only need to ensure that it has stopped running before we start processing an unmount, freeze or remount,ro.
--- fs/xfs/xfs_mount.c | 6 ++++-- fs/xfs/xfs_super.c | 32 +++++++++++++------------------- fs/xfs/xfs_sync.c | 21 +++++---------------- 3 files changed, 22 insertions(+), 37 deletions(-)
fs/xfs/xfs_mount.c +4 -2
@@ -1484,9 +1484,11 @@ xfs_unmountfs(/* * And reclaim all inodes. At this point there should be no dirty -* inode, and none should be pinned or locked, but use synchronous -* reclaim just to be sure. +* inodes and none should be pinned or locked, but use synchronous +* reclaim just to be sure. We can stop background inode reclaim +* here as well if it is still running. */ +cancel_delayed_work_sync (&mp->m_reclaim_work);xfs_reclaim_inodes (mp,SYNC_WAIT );xfs_qm_unmount (mp);
fs/xfs/xfs_super.c +13 -19
@@ -918,13 +918,11 @@ xfs_fs_put_super( {struct xfs_mount *mp =XFS_M (sb); -xfs_filestream_unmount (mp); -xfs_unmountfs (mp); -cancel_delayed_work_sync (&mp->m_sync_work); -cancel_delayed_work_sync (&mp->m_reclaim_work);cancel_work_sync (&mp->m_flush_work); +xfs_filestream_unmount (mp); +xfs_unmountfs (mp);xfs_freesb (mp);xfs_icsb_destroy_counters (mp);xfs_destroy_mount_workqueues (mp);
@@ -1231,6 +1229,9 @@ xfs_fs_fill_super(spin_lock_init (&mp->m_sb_lock);mutex_init (&mp->m_growlock);atomic_set (&mp->m_active_trans,0 ); +INIT_WORK (&mp->m_flush_work, xfs_flush_worker); +INIT_DELAYED_WORK (&mp->m_sync_work, xfs_sync_worker); +INIT_DELAYED_WORK (&mp->m_reclaim_work, xfs_reclaim_worker); mp->m_super = sb; sb->s_fs_info = mp;
@@ -1294,15 +1295,9 @@ xfs_fs_fill_super( sb->s_time_gran =1 ;set_posix_acl_flag (sb); -INIT_WORK (&mp->m_flush_work, xfs_flush_worker); -INIT_DELAYED_WORK (&mp->m_sync_work, xfs_sync_worker); -INIT_DELAYED_WORK (&mp->m_reclaim_work, xfs_reclaim_worker); - -xfs_sync_work_queue (mp); - error =xfs_mountfs (mp);if (error) - goto out_syncd_stop; + goto out_filestream_unmount; root =igrab (VFS_I (mp->m_rootip));if (!root) {
@@ -1319,12 +1314,15 @@ xfs_fs_fill_super( goto out_unmount; } +/* +* The filesystem is successfully mounted, so we can start background +* sync work now. +*/ +xfs_sync_work_queue (mp); +return 0 ; - out_syncd_stop: -cancel_delayed_work_sync (&mp->m_sync_work); -cancel_delayed_work_sync (&mp->m_reclaim_work); -cancel_work_sync (&mp->m_flush_work); + out_filestream_unmount:xfs_filestream_unmount (mp); out_free_sb:xfs_freesb (mp);
@@ -1343,10 +1341,6 @@ out_destroy_workqueues: out_unmount:xfs_filestream_unmount (mp);xfs_unmountfs (mp); - -cancel_delayed_work_sync (&mp->m_sync_work); -cancel_delayed_work_sync (&mp->m_reclaim_work); -cancel_work_sync (&mp->m_flush_work); goto out_free_sb; }
fs/xfs/xfs_sync.c +5 -16
@@ -378,9 +378,9 @@ xfs_sync_work_queue( }/* -* Every sync period we need to unpin all items, reclaim inodes and sync -* disk quotas. We might need to cover the log to indicate that the -* filesystem is idle and not frozen. +* Every sync period we need to unpin all items in the AIL and push them to +* disk. If there is nothing dirty, then we might need to cover the log to +* indicate that the filesystem is idle and not frozen. */ void xfs_sync_worker(
@@ -390,17 +390,7 @@ xfs_sync_worker( struct xfs_mount, m_sync_work);int error; -/* -* We shouldn't write/force the log if we are in the mount/unmount -* process or on a read only filesystem. The workqueue still needs to be -* active in both cases, however, because it is used for inode reclaim -* during these times. Use the MS_ACTIVE flag to avoid doing anything -* during mount. Doing work during unmount is avoided by calling -* cancel_delayed_work_sync on this work queue before tearing down -* the ail and the log in xfs_log_unmount. -*/ -if (!(mp->m_super->s_flags &MS_ACTIVE ) && - !(mp->m_flags &XFS_MOUNT_RDONLY )) { +if (!(mp->m_flags &XFS_MOUNT_RDONLY )) {/* dgc: errors ignored here */ if (mp->m_super->s_writers.frozen ==SB_UNFROZEN &&xfs_log_need_covered (mp))
@@ -408,8 +398,7 @@ xfs_sync_worker(else xfs_log_force (mp,0 ); -/* start pushing all the metadata that is currently -* dirty */ +/* start pushing all the metadata that is currently dirty */ xfs_ail_push_all (mp->m_ail); }
Signature · 6 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 03/13] xfs: rationalise xfs_mount_wq users
Quoted 16 lines · Dave Chinner
fs/xfs/xfs_sync.c +1
-11-/* -* We shouldn't write/force the log if we are in the mount/unmount -* process or on a read only filesystem. The workqueue still needs to be -* active in both cases, however, because it is used for inode reclaim -* during these times. Use the MS_ACTIVE flag to avoid doing anything -* during mount. Doing work during unmount is avoided by calling -* cancel_delayed_work_sync on this work queue before tearing down -* the ail and the log in xfs_log_unmount. -*/ -if (!(mp->m_super->s_flags&MS_ACTIVE )&& - !(mp->m_flags&XFS_MOUNT_RDONLY )) { +if (!(mp->m_flags&XFS_MOUNT_RDONLY )) {/* dgc: errors ignored here */ if (mp->m_super->s_writers.frozen ==SB_UNFROZEN &&xfs_log_need_covered (mp))
fs/xfs/xfs_sync.c xfs_sync_worker( · line 404
Quoted 9 lines
fs/xfs/xfs_sync.c +1
-2@@ -408,8 +398,7 @@ xfs_sync_worker(else xfs_log_force (mp,0 ); -/* start pushing all the metadata that is currently -* dirty */ +/* start pushing all the metadata that is currently dirty */ xfs_ail_push_all (mp->m_ail); }
It appears that the removal of the MS_ACTIVE flag is causing the "atomic_read(&bp->b_hold)> 0," ASSERT.
--Mark.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 03/13] xfs: rationalise xfs_mount_wq users
Quoted 20 lines · Mark Tinguely
Quoted 19 lines · Dave Chinner
fs/xfs/xfs_sync.c +1
-11-/* -* We shouldn't write/force the log if we are in the mount/unmount -* process or on a read only filesystem. The workqueue still needs to be -* active in both cases, however, because it is used for inode reclaim -* during these times. Use the MS_ACTIVE flag to avoid doing anything -* during mount. Doing work during unmount is avoided by calling -* cancel_delayed_work_sync on this work queue before tearing down -* the ail and the log in xfs_log_unmount. -*/ -if (!(mp->m_super->s_flags&MS_ACTIVE )&& - !(mp->m_flags&XFS_MOUNT_RDONLY )) { +if (!(mp->m_flags&XFS_MOUNT_RDONLY )) {/* dgc: errors ignored here */ if (mp->m_super->s_writers.frozen ==SB_UNFROZEN &&xfs_log_need_covered (mp))@@ -408,8 +398,7 @@ xfs_sync_worker(else xfs_log_force (mp,0 );
Quoted 9 lines
fs/xfs/xfs_sync.c xfs_sync_worker( · line 404
Quoted 6 lines
fs/xfs/xfs_sync.c +1
-2-/* start pushing all the metadata that is currently -* dirty */ +/* start pushing all the metadata that is currently dirty */ xfs_ail_push_all (mp->m_ail); }It appears that the removal of the
MS_ACTIVEflag is causing the "atomic_read(&bp->b_hold)> 0," ASSERT.
I must be being slow today - I don't see why that would cause any problems. The worker is not started at the end of the mount process after everything is set up (i.e. just before MS_ACTIVE is removed), and the worker is stopped before anything is torn down. That should effectively replicate what the MS_ACTIVE flag is providing in the old code.
Can you explain in more detail what lead you to this conclusion?
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 03/13] xfs: rationalise xfs_mount_wq users
Quoted 38 lines · Dave Chinner
Quoted 20 lines · Mark Tinguely
Quoted 19 lines · Dave Chinner
fs/xfs/xfs_sync.c +1
-11-/* -* We shouldn't write/force the log if we are in the mount/unmount -* process or on a read only filesystem. The workqueue still needs to be -* active in both cases, however, because it is used for inode reclaim -* during these times. Use the MS_ACTIVE flag to avoid doing anything -* during mount. Doing work during unmount is avoided by calling -* cancel_delayed_work_sync on this work queue before tearing down -* the ail and the log in xfs_log_unmount. -*/ -if (!(mp->m_super->s_flags&MS_ACTIVE )&& - !(mp->m_flags&XFS_MOUNT_RDONLY )) { +if (!(mp->m_flags&XFS_MOUNT_RDONLY )) {/* dgc: errors ignored here */ if (mp->m_super->s_writers.frozen ==SB_UNFROZEN &&xfs_log_need_covered (mp))@@ -408,8 +398,7 @@ xfs_sync_worker(else xfs_log_force (mp,0 );Quoted 9 lines
fs/xfs/xfs_sync.c xfs_sync_worker( · line 404
Quoted 6 lines
fs/xfs/xfs_sync.c +1
-2-/* start pushing all the metadata that is currently -* dirty */ +/* start pushing all the metadata that is currently dirty */ xfs_ail_push_all (mp->m_ail); }It appears that the removal of the
MS_ACTIVEflag is causing the
"atomic_read(&bp->b_hold)> 0," ASSERT.I must be being slow today - I don't see why that would cause any problems. The worker is not started at the end of the mount process after everything is set up (i.e. just before
MS_ACTIVEis removed), and the worker is stopped before anything is torn down. That should effectively replicate what theMS_ACTIVEflag is providing in the old code.
Quoted 5 lines
Can you explain in more detail what lead you to this conclusion?
Cheers,
Dave.
You are correct, it does not make sense, but with the
!(mp->m_super->s_flags & MS_ACTIVE)
test removed, test 107 causes the above assert on different machines/architectures. Place the test in, the assert does not happen.
I will see if I can get it to dump on the x86_32 machine.
--Mark.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 03/13] xfs: rationalise xfs_mount_wq users
Quoted 52 lines · Mark Tinguely
Quoted 39 lines · Dave Chinner
Quoted 21 lines · Mark Tinguely
Quoted 20 lines · Dave Chinner
- /*
- * We shouldn't write/force the log if we are in the mount/unmount
- * process or on a read only filesystem. The workqueue still needs to be
- * active in both cases, however, because it is used for inode reclaim
- * during these times. Use theMS_ACTIVEflag to avoid doing anything
- * during mount. Doing work during unmount is avoided by calling
- *cancel_delayed_work_syncon this work queue before tearing down
- * the ail and the log inxfs_log_unmount.
- */
- if (!(mp->m_super->s_flags& MS_ACTIVE)&&
- !(mp->m_flags&XFS_MOUNT_RDONLY)) {
+ if (!(mp->m_flags&XFS_MOUNT_RDONLY)) {
/* dgc: errors ignored here */
if (mp->m_super->s_writers.frozen== SB_UNFROZEN&&xfs_log_need_covered(mp))(quoted diff) +0
-0@@ -408,8 +398,7 @@ xfs_sync_worker(else
xfs_log_force(mp, 0);Quoted 9 lines
Quoted 6 lines
- /* start pushing all the metadata that is currently
- * dirty */
+ /* start pushing all the metadata that is currently dirty */xfs_ail_push_all(mp->m_ail);
}It appears that the removal of the
MS_ACTIVEflag is causing the "atomic_read(&bp->b_hold)> 0," ASSERT.I must be being slow today - I don't see why that would cause any problems. The worker is not started at the end of the mount process after everything is set up (i.e. just before
MS_ACTIVEis removed), and the worker is stopped before anything is torn down. That should effectively replicate what theMS_ACTIVEflag is providing in the old code.Quoted 5 lines
Can you explain in more detail what lead you to this conclusion?
Cheers,
Dave.
You are correct, it does not make sense, but with the !(mp->m_super->s_flags &
MS_ACTIVE)
test removed, test 107 causes the above assert on different machines/architectures. Place the test in, the assert does not happen.
Quoted 3 lines
I will see if I can get it to dump on the
x86_32machine.--Mark.
Make that xfstest 179. The ASSERT happens right away. I have a dump from x86_32 machine. I will take a quick look at it.
--Mark.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 03/13] xfs: rationalise xfs_mount_wq users
In reply to Mark TinguelyQuoted 43 lines · Mark Tinguely
Quoted 42 lines · Dave Chinner
Quoted 20 lines · Mark Tinguely
Quoted 19 lines · Dave Chinner
fs/xfs/xfs_sync.c +1
-11-/* -* We shouldn't write/force the log if we are in the mount/unmount -* process or on a read only filesystem. The workqueue still needs to be -* active in both cases, however, because it is used for inode reclaim -* during these times. Use the MS_ACTIVE flag to avoid doing anything -* during mount. Doing work during unmount is avoided by calling -* cancel_delayed_work_sync on this work queue before tearing down -* the ail and the log in xfs_log_unmount. -*/ -if (!(mp->m_super->s_flags&MS_ACTIVE )&& - !(mp->m_flags&XFS_MOUNT_RDONLY )) { +if (!(mp->m_flags&XFS_MOUNT_RDONLY )) {/* dgc: errors ignored here */ if (mp->m_super->s_writers.frozen ==SB_UNFROZEN &&xfs_log_need_covered (mp))@@ -408,8 +398,7 @@ xfs_sync_worker(else xfs_log_force (mp,0 );Quoted 9 lines
fs/xfs/xfs_sync.c xfs_sync_worker( · line 404
Quoted 6 lines
fs/xfs/xfs_sync.c +1
-2-/* start pushing all the metadata that is currently -* dirty */ +/* start pushing all the metadata that is currently dirty */ xfs_ail_push_all (mp->m_ail); }It appears that the removal of the
MS_ACTIVEflag is causing the
"atomic_read(&bp->b_hold)> 0," ASSERT.I must be being slow today - I don't see why that would cause any problems. The worker is not started at the end of the mount process after everything is set up (i.e. just before
MS_ACTIVEis removed), and the worker is stopped before anything is torn down. That should effectively replicate what theMS_ACTIVEflag is providing in the old code.Can you explain in more detail what lead you to this conclusion?
Cheers,
Quoted 7 lines
Quoted 1 line
Dave.
You are correct, it does not make sense, but with the
!(mp->m_super->s_flags &MS_ACTIVE)
test removed, test 107 causes the above assert on different machines/architectures. Place the test in, the assert does not happen.
test 107 is not in the auto group. That means it is generally unreliable as a regression test, so I don't run it. That said, I don't see anything unusual in that test that would cause problems...
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 03/13] xfs: rationalise xfs_mount_wq users
Quoted 52 lines · Dave Chinner
Quoted 43 lines · Mark Tinguely
Quoted 42 lines · Dave Chinner
Quoted 20 lines · Mark Tinguely
Quoted 19 lines · Dave Chinner
fs/xfs/xfs_sync.c +1
-11-/* -* We shouldn't write/force the log if we are in the mount/unmount -* process or on a read only filesystem. The workqueue still needs to be -* active in both cases, however, because it is used for inode reclaim -* during these times. Use the MS_ACTIVE flag to avoid doing anything -* during mount. Doing work during unmount is avoided by calling -* cancel_delayed_work_sync on this work queue before tearing down -* the ail and the log in xfs_log_unmount. -*/ -if (!(mp->m_super->s_flags&MS_ACTIVE )&& - !(mp->m_flags&XFS_MOUNT_RDONLY )) { +if (!(mp->m_flags&XFS_MOUNT_RDONLY )) {/* dgc: errors ignored here */ if (mp->m_super->s_writers.frozen ==SB_UNFROZEN &&xfs_log_need_covered (mp))@@ -408,8 +398,7 @@ xfs_sync_worker(else xfs_log_force (mp,0 );Quoted 9 lines
fs/xfs/xfs_sync.c xfs_sync_worker( · line 404
Quoted 6 lines
fs/xfs/xfs_sync.c +1
-2-/* start pushing all the metadata that is currently -* dirty */ +/* start pushing all the metadata that is currently dirty */ xfs_ail_push_all (mp->m_ail); }It appears that the removal of the
MS_ACTIVEflag is causing the
"atomic_read(&bp->b_hold)> 0," ASSERT.I must be being slow today - I don't see why that would cause any problems. The worker is not started at the end of the mount process after everything is set up (i.e. just before
MS_ACTIVEis removed), and the worker is stopped before anything is torn down. That should effectively replicate what theMS_ACTIVEflag is providing in the old code.Can you explain in more detail what lead you to this conclusion?
Cheers,
Quoted 7 lines
Quoted 1 line
Dave.
You are correct, it does not make sense, but with the
!(mp->m_super->s_flags&MS_ACTIVE)
test removed, test 107 causes the above assert on different machines/architectures. Place the test in, the assert does not happen.
Quoted 7 lines
test 107 is not in the auto group. That means it is generally unreliable as a regression test, so I don't run it. That said, I don't see anything unusual in that test that would cause problems...
Cheers,
Dave.
I misspoke, it is xfs test 179. I hit it doing a "check -g auto".
My test boxes had CONFIG_XFS_DEBUG=y which may be a factor. The test ran fine on a box without the debug enabled and assert as soon as I added it back.
The buffer with zero b_hold count is the freelist buffer (XAGF) for AG0. The buffer is marked STALE, it has already gone through the release code, so there is no transaction pointer nor log item pointer. The xlog_cil_committed() is being called with the XFS_LI_ABORTED flag.
The X86_32 machine is now asserting with:
XFS: Assertion failed: fs_is_ok, file: /xfs/fs/xfs/xfs_alloc.c, line:
1503
The X86_64 machines are still asserting on the zero b_hold.
Adding back the MS_ACTIVE or (it appears) not compiling with the CONFIG_XFS_DEBUG option seems to make the problem go away too. Timing? Does not explain the removal of the XFS_DEBUG.
Sorry if this is a wild goose chase.
--Mark T.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 03/13] xfs: rationalise xfs_mount_wq users
Quoted 5 lines · Mark Tinguely
I misspoke, it is xfs test 179. I hit it doing a "check -g auto".
My test boxes had
CONFIG_XFS_DEBUG=ywhich may be a factor. The test ran fine on a box without the debug enabled and assert as soon as I added it back.
It is - the assert doesn't exist on a non-DEBUG kernel, so it won't fail. ;)
Quoted 5 lines
The buffer with zero
b_holdcount is the freelist buffer (XAGF) for AG0. The buffer is marked STALE, it has already gone through the release code, so there is no transaction pointer nor log item pointer. Thexlog_cil_committed()is being called with theXFS_LI_ABORTEDflag.
It looks like another case of this problem:
http://oss.sgi.com/archives/xfs/2012-09/msg00021.html
Which appears to be another case of the failure that Eric reported he was seeing on test 137:
http://oss.sgi.com/archives/xfs/2012-08/msg00019.html
http://oss.sgi.com/archives/xfs/2012-09/msg00017.html
i.e. what appears to be a double free of a buffer during a forced shutdown. The b_hold assertion that is being hit here is just prior to doing the second free of a buffer. Given that Eric's case was bisected down to the delwri queuing changes, I'd say this is the same cause of this issue. i.e. unrelated to the patch set that was posted.
What I can't work out is what reference is going missing, and I can't reproduce it to be able to debug it....
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 03/13] xfs: rationalise xfs_mount_wq users
In reply to Dave ChinnerQuoted 13 lines · Dave Chinner
From: Dave Chinner<[email protected]>
Instead of starting and stopping background work on the
xfs_mount_wqall at the same time, separate them to where they really are needed to start and stop.The
xfs_sync_worker, only needs to be started after all the mount processing has completed successfully, while it needs to be stopped before the log is unmounted.The
xfs_reclaim_workeris started on demand, and can be stopped before the unmount process does it's own inode reclaim pass.
Quoted 6 lines
The
xfs_flush_inodeswork is run on demand, and so we really only need to ensure that it has stopped running before we start processing an unmount, freeze or remount,ro.Signed-off-by Dave Chinner<[email protected]>---
The patch itself looks good. and the "atomic_read(&bp->b_hold)> 0" assert appears specific to my machines, so forgive the noise.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 4/13 xfs: don't run the sync work if the filesyste is read-only
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
If the filesytem is mounted or remounted read-only, stop the sync worker that tries to flush or cover the log if the filesystem is dirty. It's read-only, so it isn't dirty. Restart it on a remount,rw as necessary. This avoids the need for RO checks in the work.
Similarly, stop the sync work when the filesystem is frozen, and start it again when the filesysetm is thawed. This avoids the need for special freeze checks in the work.
--- fs/xfs/xfs_super.c | 2 ++ fs/xfs/xfs_sync.c | 26 ++++++++++++++------------ 2 files changed, 16 insertions(+), 12 deletions(-)
fs/xfs/xfs_super.c +2 -0
@@ -1106,6 +1106,7 @@ xfs_fs_remount(* value if it is non-zero, otherwise go with the default. */ xfs_restore_resvblks (mp); +xfs_sync_queue_work (mp); }/* rw -> ro */
@@ -1151,6 +1152,7 @@ xfs_fs_unfreeze(struct xfs_mount *mp =XFS_M (sb);xfs_restore_resvblks (mp); +xfs_sync_queue_work (mp);return 0 ; }
fs/xfs/xfs_sync.c +14 -12
@@ -322,6 +322,9 @@ xfs_quiesce_data(* Second stage of a quiesce. The data is already synced, now we have to take * care of the metadata. New transactions are already blocked, so we need to * wait for any remaining transactions to drain out before proceeding. +* +* Note: this stops background sync work - the callers must ensure it is started +* again when appropriate. */ void xfs_quiesce_attr(
@@ -340,6 +343,9 @@ xfs_quiesce_attr(/* flush all pending changes from the AIL */ xfs_ail_push_all_sync (mp->m_ail); +/* stop background sync work */ +cancel_delayed_work_sync (&mp->m_sync_work); +/* * Just warn here till VFS can correctly support * read-only remount without racing.
@@ -380,7 +386,7 @@ xfs_sync_work_queue(/* * Every sync period we need to unpin all items in the AIL and push them to * disk. If there is nothing dirty, then we might need to cover the log to -* indicate that the filesystem is idle and not frozen. +* indicate that the filesystem is idle. */ void xfs_sync_worker(
@@ -388,19 +394,15 @@ xfs_sync_worker( {struct xfs_mount *mp =container_of (to_delayed_work (work), struct xfs_mount, m_sync_work); -int error; -if (!(mp->m_flags &XFS_MOUNT_RDONLY )) { -/* dgc: errors ignored here */ -if (mp->m_super->s_writers.frozen ==SB_UNFROZEN && -xfs_log_need_covered (mp)) - error =xfs_fs_log_dummy (mp); -else -xfs_log_force (mp,0 ); +/* dgc: errors ignored - not fatal and nowhere to report them */ +if (xfs_log_need_covered (mp)) +xfs_fs_log_dummy (mp); +else +xfs_log_force (mp,0 ); -/* start pushing all the metadata that is currently dirty */ -xfs_ail_push_all (mp->m_ail); - } +/* start pushing all the metadata that is currently dirty */ +xfs_ail_push_all (mp->m_ail);/* queue us up again */ xfs_sync_work_queue (mp);
Signature · 6 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 04/13] xfs: don't run the sync work if the filesyste is read-only
Quoted 7 lines · Dave Chinner
From: Dave Chinner<[email protected]>
If the filesytem is mounted or remounted read-only, stop the sync worker that tries to flush or cover the log if the filesystem is dirty. It's read-only, so it isn't dirty. Restart it on a remount,rw as necessary. This avoids the need for RO checks in the work.
Quoted 5 lines
Similarly, stop the sync work when the filesystem is frozen, and start it again when the filesysetm is thawed. This avoids the need for special freeze checks in the work.
Signed-off-by Dave Chinner<[email protected]>
Looks good.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 5/13 xfs: sync work is now only periodic log work
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
The only thing the periodic sync work does now is flush the AIL and idle the log. These are really functions of the log code, so move the work to xfs_log.c and rename it appropriately.
The only wart that this leaves behind is the xfssyncd_centisecs sysctl, otherwise the xfssyncd is dead. Clean up any comments that related to xfssyncd to reflect it's passing.
--- fs/xfs/xfs_log.c | 41 ++++++++++++++++++++++++++++++++++++++++- fs/xfs/xfs_log.h | 3 +++ fs/xfs/xfs_log_priv.h | 1 + fs/xfs/xfs_mount.h | 1 - fs/xfs/xfs_super.c | 16 ++++------------ fs/xfs/xfs_sync.c | 38 +++----------------------------------- fs/xfs/xfs_sync.h | 2 -- 7 files changed, 51 insertions(+), 51 deletions(-)
fs/xfs/xfs_log.c +40 -1
@@ -34,6 +34,7 @@#include "xfs_dinode.h" #include "xfs_inode.h" #include "xfs_trace.h" +#include "xfs_fsops.h" kmem_zone_t *xfs_log_ticket_zone;
@@ -698,6 +699,8 @@ xfs_log_mount_finish(xfs_mount_t *mp)ASSERT (mp->m_flags &XFS_MOUNT_RDONLY ); } +xfs_log_work_queue (mp); +return error; }
@@ -858,7 +861,7 @@ xfs_log_unmount_write(xfs_mount_t *mp)void xfs_log_unmount (xfs_mount_t *mp) { -cancel_delayed_work_sync (&mp->m_sync_work); +cancel_delayed_work_sync (&mp->m_log->l_work);xfs_trans_ail_destroy (mp);xlog_dealloc_log (mp->m_log); }
@@ -1161,6 +1164,40 @@ done: }/* xlog_get_iclog_buffer_size */ +void +xfs_log_work_queue ( +struct xfs_mount *mp) +{ +queue_delayed_work (xfs_mount_wq, &mp->m_log->l_work, +msecs_to_jiffies (xfs_syncd_centisecs *10 )); +} + +/* +* Every sync period we need to unpin all items in the AIL and push them to +* disk. If there is nothing dirty, then we might need to cover the log to +* indicate that the filesystem is idle. +*/ +void +xfs_log_worker ( +struct work_struct *work) +{ +struct xlog *log =container_of (to_delayed_work (work), + struct xlog, l_work); +struct xfs_mount *mp = log->l_mp; + +/* dgc: errors ignored - not fatal and nowhere to report them */ +if (xfs_log_need_covered (mp)) +xfs_fs_log_dummy (mp); +else +xfs_log_force (mp,0 ); + +/* start pushing all the metadata that is currently dirty */ +xfs_ail_push_all (mp->m_ail); + +/* queue us up again */ +xfs_log_work_queue (mp); +} +/* * This routine initializes some of the log structure for a given mount point. * Its primary purpose is to fill in enough, so recovery can occur. However,
@@ -1195,6 +1232,7 @@ xlog_alloc_log( log->l_logBBsize = num_bblks; log->l_covered_state =XLOG_STATE_COVER_IDLE ; log->l_flags |=XLOG_ACTIVE_RECOVERY ; +INIT_DELAYED_WORK (&log->l_work, xfs_log_worker); log->l_prev_block =-1 ;/* log->l_tail_lsn = 0x100000000LL; cycle = 1; current block = 0 */
@@ -3700,3 +3738,4 @@ xlog_iclogs_empty( }while (iclog != log->l_iclog);return 1 ; } +
fs/xfs/xfs_log.h +3 -0
@@ -181,5 +181,8 @@ int xfs_log_commit_cil(struct xfs_mount *mp, struct xfs_trans *tp, xfs_lsn_t *commit_lsn, int flags);bool xfs_log_item_in_current_chkpt (struct xfs_log_item *lip); +void xfs_log_work_queue (struct xfs_mount *mp); +void xfs_log_worker (struct work_struct *work); +#endif #endif /* __XFS_LOG_H__ */
fs/xfs/xfs_log_priv.h +1 -0
@@ -495,6 +495,7 @@ struct xlog {struct xfs_buf *l_xbuf;/* extra buffer for log * wrapping */ struct xfs_buftarg *l_targ;/* buftarg of log */ +struct delayed_work l_work;/* background flush work */ uint l_flags;uint l_quotaoffs_flag;/* XFS_DQ_*, for QUOTAOFFs */ struct list_head *l_buf_cancel_table;
fs/xfs/xfs_mount.h +0 -1
@@ -199,7 +199,6 @@ typedef struct xfs_mount {struct mutex m_icsb_mutex;/* balancer sync lock */ #endif struct xfs_mru_cache *m_filestream;/* per-mount filestream data */ -struct delayed_work m_sync_work;/* background sync work */ struct delayed_work m_reclaim_work;/* background inode reclaim */ struct work_struct m_flush_work;/* background inode flush */ __int64_t m_update_flags;/* sb flags we need to update
fs/xfs/xfs_super.c +4 -12
@@ -918,7 +918,6 @@ xfs_fs_put_super( {struct xfs_mount *mp =XFS_M (sb); -cancel_delayed_work_sync (&mp->m_sync_work);cancel_work_sync (&mp->m_flush_work);xfs_filestream_unmount (mp);
@@ -952,10 +951,10 @@ xfs_fs_sync_fs(if (laptop_mode) {/* * The disk must be active because we're syncing. -* We schedule xfssyncd now (now that the disk is +* We schedule log work now (now that the disk is * active) instead of later (when it might not be). */ -flush_delayed_work_sync (&mp->m_sync_work); +flush_delayed_work_sync (&mp->m_log->l_work); }return 0 ;
@@ -1106,7 +1105,7 @@ xfs_fs_remount(* value if it is non-zero, otherwise go with the default. */ xfs_restore_resvblks (mp); -xfs_sync_queue_work (mp); +xfs_log_work_queue (mp); }/* rw -> ro */
@@ -1152,7 +1151,7 @@ xfs_fs_unfreeze(struct xfs_mount *mp =XFS_M (sb);xfs_restore_resvblks (mp); -xfs_sync_queue_work (mp); +xfs_log_work_queue (mp);return 0 ; }
@@ -1232,7 +1231,6 @@ xfs_fs_fill_super(mutex_init (&mp->m_growlock);atomic_set (&mp->m_active_trans,0 );INIT_WORK (&mp->m_flush_work, xfs_flush_worker); -INIT_DELAYED_WORK (&mp->m_sync_work, xfs_sync_worker);INIT_DELAYED_WORK (&mp->m_reclaim_work, xfs_reclaim_worker); mp->m_super = sb;
@@ -1316,12 +1314,6 @@ xfs_fs_fill_super( goto out_unmount; } -/* -* The filesystem is successfully mounted, so we can start background -* sync work now. -*/ -xfs_sync_work_queue (mp); -return 0 ; out_filestream_unmount:
fs/xfs/xfs_sync.c +3 -35
@@ -19,6 +19,7 @@#include "xfs_fs.h" #include "xfs_types.h" #include "xfs_log.h" +#include "xfs_log_priv.h" #include "xfs_inum.h" #include "xfs_trans.h" #include "xfs_trans_priv.h"
@@ -343,8 +344,8 @@ xfs_quiesce_attr(/* flush all pending changes from the AIL */ xfs_ail_push_all_sync (mp->m_ail); -/* stop background sync work */ -cancel_delayed_work_sync (&mp->m_sync_work); +/* stop background log work */ +cancel_delayed_work_sync (&mp->m_log->l_work);/* * Just warn here till VFS can correctly support
@@ -375,39 +376,6 @@ xfs_quiesce_attr(xfs_buf_unlock (mp->m_sb_bp); } -void -xfs_sync_work_queue ( -struct xfs_mount *mp) -{ -queue_delayed_work (xfs_mount_wq, &mp->m_sync_work, -msecs_to_jiffies (xfs_syncd_centisecs *10 )); -} - -/* -* Every sync period we need to unpin all items in the AIL and push them to -* disk. If there is nothing dirty, then we might need to cover the log to -* indicate that the filesystem is idle. -*/ -void -xfs_sync_worker ( -struct work_struct *work) -{ -struct xfs_mount *mp =container_of (to_delayed_work (work), - struct xfs_mount, m_sync_work); - -/* dgc: errors ignored - not fatal and nowhere to report them */ -if (xfs_log_need_covered (mp)) -xfs_fs_log_dummy (mp); -else -xfs_log_force (mp,0 ); - -/* start pushing all the metadata that is currently dirty */ -xfs_ail_push_all (mp->m_ail); - -/* queue us up again */ -xfs_sync_work_queue (mp); -} -/* * Queue a new inode reclaim pass if there are reclaimable inodes and there * isn't a reclaim pass already in progress. By default it runs every 5s based
fs/xfs/xfs_sync.h +0 -2
@@ -24,8 +24,6 @@ struct xfs_perag;#define SYNC_WAIT 0x0001/* wait for i/o to complete */ #define SYNC_TRYLOCK 0x0002/* only try to lock inodes */ -void xfs_sync_work_queue (struct xfs_mount *mp); -void xfs_sync_worker (struct work_struct *work);void xfs_flush_worker (struct work_struct *work);void xfs_reclaim_worker (struct work_struct *work);
Signature · 6 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 05/13] xfs: sync work is now only periodic log work
Quoted 9 lines · Dave Chinner
From: Dave Chinner <[email protected]>
The only thing the periodic sync work does now is flush the AIL and idle the log. These are really functions of the log code, so move the work to
xfs_log.cand rename it appropriately.The only wart that this leaves behind is the
xfssyncd_centisecssysctl, otherwise the xfssyncd is dead. Clean up any comments that related to xfssyncd to reflect it's passing.
FYI: A while ago I looked into folding the work here into xfsaild. While soing the periodic ail push there aka just waking up with a timeout which we did more often anyway at that point was easy, getting the log force / dummy log never really worked out.
I think at least the former probably should be dropped here as well.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 05/13] xfs: sync work is now only periodic log work
Quoted 11 lines · Christoph Hellwig
Quoted 9 lines · Dave Chinner
From: Dave Chinner <[email protected]>
The only thing the periodic sync work does now is flush the AIL and idle the log. These are really functions of the log code, so move the work to
xfs_log.cand rename it appropriately.The only wart that this leaves behind is the
xfssyncd_centisecssysctl, otherwise the xfssyncd is dead. Clean up any comments that related to xfssyncd to reflect it's passing.
Quoted 6 lines
FYI: A while ago I looked into folding the work here into xfsaild. While soing the periodic ail push there aka just waking up with a timeout which we did more often anyway at that point was easy, getting the log force / dummy log never really worked out.
I think at least the former probably should be dropped here as well.
Perhaps. We've always had a periodic log force as a get out gaol free card for issues with pinned objects, so I'm not really inclined to remove it/change that behaviour in this series.
IOWs, while I do agree that the log covering and AIL pushing code should be more tightly integrated so that we can get the filesystem to an idle state much faster than we do now, I don't think this patchset is the place to start doing bits of that work.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 05/13] xfs: sync work is now only periodic log work
In reply to Dave ChinnerQuoted 14 lines · Dave Chinner
+ * Every sync period we need to unpin all items in the AIL and push them to
+ * disk. If there is nothing dirty, then we might need to cover the log to
+ * indicate that the filesystem is idle.
+ */fs/xfs/xfs_log.c +10
-0+void +xfs_log_worker ( +struct work_struct *work) +{ +struct xlog *log =container_of (to_delayed_work (work), + struct xlog, l_work); +struct xfs_mount *mp = log->l_mp; + +/* dgc: errors ignored - not fatal and nowhere to report them */ +if (xfs_log_need_covered (mp))
fs/xfs/xfs_log.c done: · line 1199
Quoted 10 lines
fs/xfs/xfs_log.c +10
-0+xfs_fs_log_dummy (mp); + else + xfs_log_force(mp, 0); + +/* start pushing all the metadata that is currently dirty */ +xfs_ail_push_all (mp->m_ail); + +/* queue us up again */ +xfs_log_work_queue (mp); +}
Having log space for the xfs_fs_log_dummy() is the weak point in the remaining log hang problems.
I agree with Dave and the above should remain in this series and log issue handled in another series.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 05/13] xfs: sync work is now only periodic log work
In reply to Dave ChinnerQuoted 12 lines · Dave Chinner
From: Dave Chinner<[email protected]>
The only thing the periodic sync work does now is flush the AIL and idle the log. These are really functions of the log code, so move the work to
xfs_log.cand rename it appropriately.The only wart that this leaves behind is the
xfssyncd_centisecssysctl, otherwise the xfssyncd is dead. Clean up any comments that related to xfssyncd to reflect it's passing.Signed-off-by Dave Chinner<[email protected]>---
...
Quoted 14 lines
fs/xfs/xfs_log.c +1
-0@@ -34,6 +34,7 @@#include "xfs_dinode.h" #include "xfs_inode.h" #include "xfs_trace.h" +#include "xfs_fsops.h" kmem_zone_t *xfs_log_ticket_zone;
fs/xfs/xfs_log.c xfs_log_mount_finish(xfs_mount_t *mp) · line 701
Quoted 3 lines
fs/xfs/xfs_log.c +0
-0@@ -698,6 +699,8 @@ xfs_log_mount_finish(xfs_mount_t *mp)ASSERT (mp->m_flags&XFS_MOUNT_RDONLY ); }
Looking at this closer, shouldn't there be a check for recovery failure. If it did, the mount will stop:
if (!error)
fs/xfs/xfs_log.c xfs_log_mount_finish(xfs_mount_t *mp) · line 705
Quoted 4 lines
fs/xfs/xfs_log.c +2
-0+xfs_log_work_queue (mp); +return error; }
--Mark.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 05/13] xfs: sync work is now only periodic log work
Quoted 37 lines · Mark Tinguely
Quoted 12 lines · Dave Chinner
From: Dave Chinner<[email protected]>
The only thing the periodic sync work does now is flush the AIL and idle the log. These are really functions of the log code, so move the work to
xfs_log.cand rename it appropriately.The only wart that this leaves behind is the
xfssyncd_centisecssysctl, otherwise the xfssyncd is dead. Clean up any comments that related to xfssyncd to reflect it's passing.Signed-off-by Dave Chinner<[email protected]>---
...
Quoted 14 lines
fs/xfs/xfs_log.c +1
-0@@ -34,6 +34,7 @@#include "xfs_dinode.h" #include "xfs_inode.h" #include "xfs_trace.h" +#include "xfs_fsops.h" kmem_zone_t *xfs_log_ticket_zone;fs/xfs/xfs_log.c xfs_log_mount_finish(xfs_mount_t *mp) · line 701
Quoted 3 lines
fs/xfs/xfs_log.c +0
-0@@ -698,6 +699,8 @@ xfs_log_mount_finish(xfs_mount_t *mp)ASSERT (mp->m_flags&XFS_MOUNT_RDONLY ); }Looking at this closer, shouldn't there be a check for recovery failure. If it did, the mount will stop:
Quoted 5 lines
if (!error)fs/xfs/xfs_log.c xfs_log_mount_finish(xfs_mount_t *mp) · line 704
Quoted 3 lines
fs/xfs/xfs_log.c +2
-0+xfs_log_work_queue (mp); +return error;
Probably should, but the error handling for a xfs_log_mount_finish() failure calls xfs_log_unmount() and that shuts down the worker correctly even if this failure occurs.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 6/13 xfs: Bring some sanity to log unmounting
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
When unmounting the filesystem, there are lots of operations that need to be done in a specific order, and they are spread across across a couple of functions. We have to drain the AIL before we write the unmount record, and we have to shut down the background log work before we do either of them.
But this is all split haphazardly across xfs_unmountfs() and xfs_log_unmount(). Move all the AIL flushing and log manipulations to xfs_log_unmount() so that the responisbilities of each function is clear and the operations they perform obvious.
--- fs/xfs/xfs_log.c | 29 ++++++++++++++++++++++++++--- fs/xfs/xfs_mount.c | 24 ------------------------ 2 files changed, 26 insertions(+), 27 deletions(-)
fs/xfs/xfs_log.c +26 -3
@@ -853,15 +853,38 @@ xfs_log_unmount_write(xfs_mount_t *mp) }/* xfs_log_unmount_write */ /* -* Deallocate log structures for unmount/relocation. +* Shut down and release the AIL and Log. * -* We need to stop the aild from running before we destroy -* and deallocate the log as the aild references the log. +* During unmount, we need to ensure we flush all the dirty metadata objects +* from the AIL so that the log is empty before we write the unmount record to +* the log. +* +* To do this, we first need to shut down the background log work so it is not +* trying to cover the log as we clean up. We then need to unpin all objects in +* the log so we can then flush them out. Once they have completed their IO and +* run the callbacks removing themselves from the AIL, we can write the unmount +* record, tear down the AIL and finally free the log. */ void xfs_log_unmount (xfs_mount_t *mp) {cancel_delayed_work_sync (&mp->m_log->l_work); +xfs_log_force (mp,XFS_LOG_SYNC ); + +/* +* The superblock buffer is uncached and while xfs_ail_push_all_sync() +* will push it, xfs_wait_buftarg() will not wait for it. Further, +* xfs_buf_iowait() cannot be used because it was pushed with the +* XBF_ASYNC flag set, so we need to use a lock/unlock pair to wait for +* the IO to complete. +*/ +xfs_ail_push_all_sync (mp->m_ail); +xfs_wait_buftarg (mp->m_ddev_targp); +xfs_buf_lock (mp->m_sb_bp); +xfs_buf_unlock (mp->m_sb_bp); + +xfs_log_unmount_write (mp); +xfs_trans_ail_destroy (mp);xlog_dealloc_log (mp->m_log); }
fs/xfs/xfs_mount.c +0 -24
@@ -1494,13 +1494,6 @@ xfs_unmountfs(xfs_qm_unmount (mp);/* -* Flush out the log synchronously so that we know for sure -* that nothing is pinned. This is important because bflush() -* will skip pinned buffers. -*/ -xfs_log_force (mp,XFS_LOG_SYNC ); - -/* * Unreserve any blocks we have so that when we unmount we don't account * the reserved free space as used. This is really only necessary for * lazy superblock counting because it trusts the incore superblock
@@ -1525,23 +1518,6 @@ xfs_unmountfs(xfs_warn (mp,"Unable to update superblock counters. " "Freespace may not be correct on next mount." ); -/* -* At this point we might have modified the superblock again and thus -* added an item to the AIL, thus flush it again. -*/ -xfs_ail_push_all_sync (mp->m_ail); -xfs_wait_buftarg (mp->m_ddev_targp); - -/* -* The superblock buffer is uncached and xfsaild_push() will lock and -* set the XBF_ASYNC flag on the buffer. We cannot do xfs_buf_iowait() -* here but a lock on the superblock buffer will block until iodone() -* has completed. -*/ -xfs_buf_lock (mp->m_sb_bp); -xfs_buf_unlock (mp->m_sb_bp); - -xfs_log_unmount_write (mp);xfs_log_unmount (mp);xfs_uuid_unmount (mp);
Signature · 6 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 06/13] xfs: Bring some sanity to log unmounting
Looks good,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 06/13] xfs: Bring some sanity to log unmounting
In reply to Dave ChinnerQuoted 8 lines · Dave Chinner
From: Dave Chinner<[email protected]>
When unmounting the filesystem, there are lots of operations that need to be done in a specific order, and they are spread across across a couple of functions. We have to drain the AIL before we write the unmount record, and we have to shut down the background log work before we do either of them.
Quoted 7 lines
But this is all split haphazardly across
xfs_unmountfs()andxfs_log_unmount(). Move all the AIL flushing and log manipulations toxfs_log_unmount()so that the responisbilities of each function is clear and the operations they perform obvious.Signed-off-by Dave Chinner<[email protected]>---
Looks good.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 7/13 xfs: xfs_sync_data is redundant.
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
We don't do any data writeback from XFS any more - the VFS is completely responsible for that, including for freeze. We can replace the remaining caller with the VFS level function that achieves the same thing, but without conflicting with current writeback work - writeback_inodes_sb_if_idle().
This means we can remove the flush_work and xfs_flush_inodes() - the VFS functionality completely replaces the internal flush queue for doing this writeback work in a separate context to avoid stack overruns..
--- fs/xfs/xfs_iomap.c | 3 +- fs/xfs/xfs_mount.h | 1 - fs/xfs/xfs_super.c | 3 -- fs/xfs/xfs_sync.c | 78 ------------------------------------------------- fs/xfs/xfs_sync.h | 3 -- fs/xfs/xfs_vnodeops.c | 3 +- 6 files changed, 4 insertions(+), 87 deletions(-)
fs/xfs/xfs_iomap.c +2 -1
@@ -446,7 +446,8 @@ retry:if (error ==ENOSPC ) {xfs_iunlock (ip,XFS_ILOCK_EXCL ); -xfs_flush_inodes (ip); +writeback_inodes_sb_if_idle (VFS_I (ip)->i_sb, +WB_REASON_FS_FREE_SPACE );xfs_ilock (ip,XFS_ILOCK_EXCL ); }
fs/xfs/xfs_mount.h +0 -1
@@ -200,7 +200,6 @@ typedef struct xfs_mount {#endif struct xfs_mru_cache *m_filestream;/* per-mount filestream data */ struct delayed_work m_reclaim_work;/* background inode reclaim */ -struct work_struct m_flush_work;/* background inode flush */ __int64_t m_update_flags;/* sb flags we need to update on the next remount,rw */ struct shrinker m_inode_shrink;/* inode reclaim shrinker */
fs/xfs/xfs_super.c +0 -3
@@ -918,8 +918,6 @@ xfs_fs_put_super( {struct xfs_mount *mp =XFS_M (sb); -cancel_work_sync (&mp->m_flush_work); -xfs_filestream_unmount (mp);xfs_unmountfs (mp);xfs_freesb (mp);
@@ -1230,7 +1228,6 @@ xfs_fs_fill_super(spin_lock_init (&mp->m_sb_lock);mutex_init (&mp->m_growlock);atomic_set (&mp->m_active_trans,0 ); -INIT_WORK (&mp->m_flush_work, xfs_flush_worker);INIT_DELAYED_WORK (&mp->m_reclaim_work, xfs_reclaim_worker); mp->m_super = sb;
fs/xfs/xfs_sync.c +0 -78
90 lines not on this page · raw message
fs/xfs/xfs_sync.h +0 -3
@@ -24,14 +24,11 @@ struct xfs_perag;#define SYNC_WAIT 0x0001/* wait for i/o to complete */ #define SYNC_TRYLOCK 0x0002/* only try to lock inodes */ -void xfs_flush_worker (struct work_struct *work);void xfs_reclaim_worker (struct work_struct *work);int xfs_quiesce_data (struct xfs_mount *mp);void xfs_quiesce_attr (struct xfs_mount *mp); -void xfs_flush_inodes (struct xfs_inode *ip); -int xfs_reclaim_inodes (struct xfs_mount *mp,int mode);int xfs_reclaim_inodes_count (struct xfs_mount *mp);void xfs_reclaim_inodes_nr (struct xfs_mount *mp,int nr_to_scan);
fs/xfs/xfs_vnodeops.c +2 -1
@@ -777,7 +777,8 @@ xfs_create( XFS_TRANS_PERM_LOG_RES, log_count);if (error ==ENOSPC ) {/* flush outstanding delalloc blocks and retry */ -xfs_flush_inodes (dp); +writeback_inodes_sb_if_idle (VFS_I (dp)->i_sb, +WB_REASON_FS_FREE_SPACE ); error =xfs_trans_reserve (tp, resblks, log_res,0 ,XFS_TRANS_PERM_LOG_RES , log_count); }
Signature · 6 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 07/13] xfs: xfs_sync_data is redundant.
Quoted 8 lines · Dave Chinner
From: Dave Chinner <[email protected]>
We don't do any data writeback from XFS any more - the VFS is completely responsible for that, including for freeze. We can replace the remaining caller with the VFS level function that achieves the same thing, but without conflicting with current writeback work -
writeback_inodes_sb_if_idle().
Quoted 4 lines
This means we can remove the
flush_workandxfs_flush_inodes()- the VFS functionality completely replaces the internal flush queue for doing this writeback work in a separate context to avoid stack overruns..
Are the lock ordering issues with writeback_inodes_sb_if_idle sorted out by now? IIRC it still needs to be switch to a trylock.
fs/xfs/xfs_iomap.c retry: · line 450
Quoted 3 lines
fs/xfs/xfs_iomap.c +2
-1-xfs_flush_inodes (ip); +writeback_inodes_sb_if_idle (VFS_I (ip)->i_sb, +WB_REASON_FS_FREE_SPACE );
I'd prefer to still keep a wrapper for an ugly call like this if we can.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 07/13] xfs: xfs_sync_data is redundant.
Quoted 9 lines · Christoph Hellwig
Quoted 8 lines · Dave Chinner
From: Dave Chinner <[email protected]>
We don't do any data writeback from XFS any more - the VFS is completely responsible for that, including for freeze. We can replace the remaining caller with the VFS level function that achieves the same thing, but without conflicting with current writeback work -
writeback_inodes_sb_if_idle().
Quoted 7 lines
Quoted 4 lines
This means we can remove the
flush_workandxfs_flush_inodes()- the VFS functionality completely replaces the internal flush queue for doing this writeback work in a separate context to avoid stack overruns..Are the lock ordering issues with
writeback_inodes_sb_if_idlesorted out by now? IIRC it still needs to be switch to a trylock.
It never gets called from unmount or freeze context, so I can't see how it would deadlock in these use cases. It's only when we call writeback_inodes_sb_if_idle() from a context that already holds s_umount that the locking it has matters - that's where btrfs and ext4 have been getting into trouble with this.
Quoted 5 lines
fs/xfs/xfs_iomap.c retry: · line 450
Quoted 3 lines
fs/xfs/xfs_iomap.c +2
-1-xfs_flush_inodes (ip); +writeback_inodes_sb_if_idle (VFS_I (ip)->i_sb, +WB_REASON_FS_FREE_SPACE );I'd prefer to still keep a wrapper for an ugly call like this if we can.
OK, I'll add an inline function to do this.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 07/13] xfs: xfs_sync_data is redundant.
In reply to Dave ChinnerQuoted 8 lines · Dave Chinner
From: Dave Chinner<[email protected]>
We don't do any data writeback from XFS any more - the VFS is completely responsible for that, including for freeze. We can replace the remaining caller with the VFS level function that achieves the same thing, but without conflicting with current writeback work -
writeback_inodes_sb_if_idle().
Quoted 7 lines
This means we can remove the
flush_workandxfs_flush_inodes()- the VFS functionality completely replaces the internal flush queue for doing this writeback work in a separate context to avoid stack overruns..Signed-off-by Dave Chinner<[email protected]>---
I get a XFS hang on xfstest 205 - couple different machines:
# cat /proc/413/stack [<ffffffff810fa889>] sleep_on_page+0x9/0x10 [<ffffffff810fa874>] __lock_page+0x64/0x70 [<ffffffff81104f58>] write_cache_pages+0x368/0x510 [<ffffffff8110514c>] generic_writepages+0x4c/0x70 [<ffffffffa046d084>] xfs_vm_writepages+0x54/0x70 [xfs] [<ffffffff8110518b>] do_writepages+0x1b/0x40 [<ffffffff8117ad85>] __writeback_single_inode+0x45/0x160 [<ffffffff8117c0c7>] writeback_sb_inodes+0x2a7/0x490 [<ffffffff8117c539>] wb_writeback+0x119/0x2b0 [<ffffffff8117c7a4>] wb_do_writeback+0xd4/0x230 [<ffffffff8117c9db>] bdi_writeback_thread+0xdb/0x230 [<ffffffff810650be>] kthread+0x9e/0xb0 [<ffffffff81432dc4>] kernel_thread_helper+0x4/0x10 [<ffffffffffffffff>] 0xffffffffffffffff
# cat /proc/12489/stack (dd command) [<ffffffff8117b415>] writeback_inodes_sb_nr+0x85/0xb0 [<ffffffff8117b77c>] writeback_inodes_sb+0x5c/0x80 [<ffffffff8117b7e2>] writeback_inodes_sb_if_idle+0x42/0x60 [<ffffffffa047b54e>] xfs_iomap_write_delay+0x28e/0x320 [xfs] [<ffffffffa046c738>] __xfs_get_blocks+0x2b8/0x500 [xfs] [<ffffffffa046c9ac>] xfs_get_blocks+0xc/0x10 [xfs] [<ffffffff811863df>] __block_write_begin+0x2af/0x5c0 [<ffffffffa046cfa1>] xfs_vm_write_begin+0x61/0xd0 [xfs] [<ffffffff810f9c02>] generic_perform_write+0xc2/0x1e0 [<ffffffff810f9d80>] generic_file_buffered_write+0x60/0xa0 [<ffffffffa047454d>] xfs_file_buffered_aio_write+0x11d/0x1b0 [xfs] [<ffffffffa04746f0>] xfs_file_aio_write+0x110/0x170 [xfs] [<ffffffff811530e1>] do_sync_write+0xa1/0xf0 [<ffffffff811536eb>] vfs_write+0xcb/0x130 [<ffffffff81153840>] sys_write+0x50/0x90 [<ffffffff81431d39>] system_call_fastpath+0x16/0x1b [<ffffffffffffffff>] 0xffffffffffffffff
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 07/13] xfs: xfs_sync_data is redundant.
Quoted 19 lines · Mark Tinguely
Quoted 8 lines · Dave Chinner
From: Dave Chinner<[email protected]>
We don't do any data writeback from XFS any more - the VFS is completely responsible for that, including for freeze. We can replace the remaining caller with the VFS level function that achieves the same thing, but without conflicting with current writeback work -
writeback_inodes_sb_if_idle().Quoted 7 lines
This means we can remove the
flush_workandxfs_flush_inodes()- the VFS functionality completely replaces the internal flush queue for doing this writeback work in a separate context to avoid stack overruns..Signed-off-by Dave Chinner<[email protected]>---
I get a XFS hang on xfstest 205 - couple different machines:
Quoted 15 lines
# cat /proc/413/stack [<ffffffff810fa889>] sleep_on_page+0x9/0x10 [<ffffffff810fa874>] __lock_page+0x64/0x70 [<ffffffff81104f58>] write_cache_pages+0x368/0x510 [<ffffffff8110514c>] generic_writepages+0x4c/0x70 [<ffffffffa046d084>] xfs_vm_writepages+0x54/0x70 [xfs] [<ffffffff8110518b>] do_writepages+0x1b/0x40 [<ffffffff8117ad85>] __writeback_single_inode+0x45/0x160 [<ffffffff8117c0c7>] writeback_sb_inodes+0x2a7/0x490 [<ffffffff8117c539>] wb_writeback+0x119/0x2b0 [<ffffffff8117c7a4>] wb_do_writeback+0xd4/0x230 [<ffffffff8117c9db>] bdi_writeback_thread+0xdb/0x230 [<ffffffff810650be>] kthread+0x9e/0xb0 [<ffffffff81432dc4>] kernel_thread_helper+0x4/0x10 [<ffffffffffffffff>] 0xffffffffffffffff
Oh, curious. That implies that writeback has got stuck on the page we currently hold locked in this thread:
Quoted 10 lines
# cat /proc/12489/stack (dd command) [<ffffffff8117b415>] writeback_inodes_sb_nr+0x85/0xb0 [<ffffffff8117b77c>] writeback_inodes_sb+0x5c/0x80 [<ffffffff8117b7e2>] writeback_inodes_sb_if_idle+0x42/0x60 [<ffffffffa047b54e>] xfs_iomap_write_delay+0x28e/0x320 [xfs] [<ffffffffa046c738>] __xfs_get_blocks+0x2b8/0x500 [xfs] [<ffffffffa046c9ac>] xfs_get_blocks+0xc/0x10 [xfs] [<ffffffff811863df>] __block_write_begin+0x2af/0x5c0 [<ffffffffa046cfa1>] xfs_vm_write_begin+0x61/0xd0 [xfs] [<ffffffff810f9c02>] generic_perform_write+0xc2/0x1e0
Quoted 8 lines
[<ffffffff810f9d80>] generic_file_buffered_write+0x60/0xa0 [<ffffffffa047454d>] xfs_file_buffered_aio_write+0x11d/0x1b0 [xfs] [<ffffffffa04746f0>] xfs_file_aio_write+0x110/0x170 [xfs] [<ffffffff811530e1>] do_sync_write+0xa1/0xf0 [<ffffffff811536eb>] vfs_write+0xcb/0x130 [<ffffffff81153840>] sys_write+0x50/0x90 [<ffffffff81431d39>] system_call_fastpath+0x16/0x1b [<ffffffffffffffff>] 0xffffffffffffffff
Why didn't the current writeback code have this problem? It blocked waiting for writeback on dirty inodes.
Oh, it woul dhave found the xfs_inode with the IOLOCK already held, so it skipped writeback on the inode that triggered the flush. Bugger. Let me have a bit of a think about this.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 8/13 xfs: xfs_sync_fsdata is redundant
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
Why do we need to write the superblock to disk once we've written all the data? We don't actually - the reasons for doing this are lost in the mists of time, and go back to the way Irix used to drive VFS flushing.
On linux, this code is only called from two contexts: remount and .sync_fs. In the remount case, the call is followed by a metadata sync, which unpins and writes the superblock. In the sync_fs case, we only need to force the log to disk to ensure that the superblock is correctly on disk, so we don't actually need to write it. Hence the functionality is either redundant or superfluous and thus can be removed.
Seeing as xfs_quiesce_data is essentially now just a log force, remove it as well and fold the code back into the two callers. Neither of them need the log covering check, either, as that is redundant for the remount case, and unnecessary for the .sync_fs case.
--- fs/xfs/xfs_super.c | 19 +++++---------- fs/xfs/xfs_sync.c | 67 +++++++--------------------------------------------- 2 files changed, 14 insertions(+), 72 deletions(-)
fs/xfs/xfs_super.c +6 -13
@@ -934,7 +934,6 @@ xfs_fs_sync_fs( int wait) {struct xfs_mount *mp =XFS_M (sb); -int error;/* * Doing anything during the async pass would be counterproductive.
@@ -942,10 +941,7 @@ xfs_fs_sync_fs(if (!wait)return 0 ; - error =xfs_quiesce_data (mp); -if (error) -return -error; - +xfs_log_force (mp,XFS_LOG_SYNC );if (laptop_mode) {/* * The disk must be active because we're syncing.
@@ -1109,15 +1105,12 @@ xfs_fs_remount(/* rw -> ro */ if (!(mp->m_flags &XFS_MOUNT_RDONLY ) && (*flags &MS_RDONLY )) {/* -* After we have synced the data but before we sync the -* metadata, we need to free up the reserve block pool so that -* the used block count in the superblock on disk is correct at -* the end of the remount. Stash the current reserve pool size -* so that if we get remounted rw, we can return it to the same -* size. +* Before we sync the metadata, we need to free up the reserve +* block pool so that the used block count in the superblock on +* disk is correct at the end of the remount. Stash the current +* reserve pool size so that if we get remounted rw, we can +* return it to the same size. */ - -xfs_quiesce_data (mp);xfs_save_resvblks (mp);xfs_quiesce_attr (mp); mp->m_flags |=XFS_MOUNT_RDONLY ;
fs/xfs/xfs_sync.c +8 -59
84 lines not on this page · raw message
Signature · 6 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 08/13] xfs: xfs_sync_fsdata is redundant
Looks good,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 08/13] xfs: xfs_sync_fsdata is redundant
In reply to Dave ChinnerQuoted 15 lines · Dave Chinner
From: Dave Chinner<[email protected]>
Why do we need to write the superblock to disk once we've written all the data? We don't actually - the reasons for doing this are lost in the mists of time, and go back to the way Irix used to drive VFS flushing.
On linux, this code is only called from two contexts: remount and .sync_fs. In the remount case, the call is followed by a metadata sync, which unpins and writes the superblock. In the
sync_fscase, we only need to force the log to disk to ensure that the superblock is correctly on disk, so we don't actually need to write it. Hence the functionality is either redundant or superfluous and thus can be removed.
Quoted 8 lines
Seeing as
xfs_quiesce_datais essentially now just a log force, remove it as well and fold the code back into the two callers. Neither of them need the log covering check, either, as that is redundant for the remount case, and unnecessary for the .sync_fs case.Signed-off-by Dave Chinner<[email protected]>---
Looks good.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 9/13 xfs: move xfs_quiesce_attr() into xfs_super.c
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
Both callers of xfs_quiesce_attr() are in xfs_super.c, and there's nothing really sync-specific about this functionality so it doesn't really matter where it lives. Move it to benext to it's callers, so all the remount/sync_fs code is in the one place.
--- fs/xfs/xfs_super.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++ fs/xfs/xfs_sync.c | 65 -------------------------------------------------- fs/xfs/xfs_sync.h | 3 --- 3 files changed, 67 insertions(+), 68 deletions(-)
fs/xfs/xfs_super.c +67 -0
@@ -1025,6 +1025,73 @@ xfs_restore_resvblks(struct xfs_mount *mp)xfs_reserve_blocks (mp, &resblks,NULL ); } +/* +* Trigger writeback of all the dirty metadata in the file system. +* +* This ensures that the metadata is written to their location on disk rather +* than just existing in transactions in the log. This means after a quiesce +* there is no log replay required to write the inodes to disk (this is the main +* difference between a sync and a quiesce). +* +* This shoul deffectively mimic the code in xfs_unmountfs() and +* xfs_log_umount() but without tearing down any structures. +* XXX: bug fixes needed! +* +* Note: this stops background log work - the callers must ensure it is started +* again when appropriate. +*/ +void +xfs_quiesce_attr ( +struct xfs_mount *mp) +{ +int error =0 ; + +/* wait for all modifications to complete */ +while (atomic_read (&mp->m_active_trans) >0 ) +delay (100 ); + +/* force the log to unpin objects from the now complete transactions */ +xfs_log_force (mp,XFS_LOG_SYNC ); + +/* reclaim inodes to do any IO before the freeze completes */ +xfs_reclaim_inodes (mp,0 ); +xfs_reclaim_inodes (mp,SYNC_WAIT ); + +/* flush all pending changes from the AIL */ +xfs_ail_push_all_sync (mp->m_ail); + +/* stop background log work */ +cancel_delayed_work_sync (&mp->m_log->l_work); + +/* +* Just warn here till VFS can correctly support +* read-only remount without racing. +*/ +WARN_ON (atomic_read (&mp->m_active_trans) !=0 ); + +/* Push the superblock and write an unmount record */ + error =xfs_log_sbcount (mp); +if (error) +xfs_warn (mp,"xfs_attr_quiesce: failed to log sb changes. " +"Frozen image may not be consistent." ); +xfs_log_unmount_write (mp); + +/* +* At this point we might have modified the superblock again and thus +* added an item to the AIL, thus flush it again. +*/ +xfs_ail_push_all_sync (mp->m_ail); + +/* +* The superblock buffer is uncached and xfsaild_push() will lock and +* set the XBF_ASYNC flag on the buffer. We cannot do xfs_buf_iowait() +* here but a lock on the superblock buffer will block until iodone() +* has completed. +*/ +xfs_buf_lock (mp->m_sb_bp); +xfs_buf_unlock (mp->m_sb_bp); +} + STATIC int xfs_fs_remount( struct super_block *sb,
fs/xfs/xfs_sync.c +0 -65
71 lines not on this page · raw message
fs/xfs/xfs_sync.h +0 -3
@@ -26,9 +26,6 @@ struct xfs_perag;void xfs_reclaim_worker (struct work_struct *work); -int xfs_quiesce_data (struct xfs_mount *mp); -void xfs_quiesce_attr (struct xfs_mount *mp); -int xfs_reclaim_inodes (struct xfs_mount *mp,int mode);int xfs_reclaim_inodes_count (struct xfs_mount *mp);void xfs_reclaim_inodes_nr (struct xfs_mount *mp,int nr_to_scan);
Signature · 6 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 09/13] xfs: move xfs_quiesce_attr() into xfs_super.c
Looks good,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 09/13] xfs: move xfs_quiesce_attr() into xfs_super.c
In reply to Dave ChinnerQuoted 9 lines · Dave Chinner
From: Dave Chinner<[email protected]>
Both callers of
xfs_quiesce_attr()are inxfs_super.c, and there's nothing really sync-specific about this functionality so it doesn't really matter where it lives. Move it to benext to it's callers, so all the remount/sync_fs code is in the one place.Signed-off-by Dave Chinner<[email protected]>---
Looks good.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 10/13 xfs: xfs_quiesce_attr() should quiesce the log like unmount
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
xfs_quiesce_attr() is supposed to leave the log empty with an unmount record written. Right now it does not wait for the AIL to be emptied before writing the unmount record, not does it wait for metadata IO completion, either. Fix it to use the same method and code as xfs_log_unmount().
--- fs/xfs/xfs_log.c | 25 ++++++++++++++++++------- fs/xfs/xfs_log.h | 1 + fs/xfs/xfs_super.c | 41 ++++++++--------------------------------- 3 files changed, 27 insertions(+), 40 deletions(-)
fs/xfs/xfs_log.c +18 -7
@@ -853,20 +853,17 @@ xfs_log_unmount_write(xfs_mount_t *mp) }/* xfs_log_unmount_write */ /* -* Shut down and release the AIL and Log. -* -* During unmount, we need to ensure we flush all the dirty metadata objects -* from the AIL so that the log is empty before we write the unmount record to -* the log. +* Empty the log for unmount/freeze. * * To do this, we first need to shut down the background log work so it is not * trying to cover the log as we clean up. We then need to unpin all objects in * the log so we can then flush them out. Once they have completed their IO and * run the callbacks removing themselves from the AIL, we can write the unmount -* record, tear down the AIL and finally free the log. +* record. */ void -xfs_log_unmount (xfs_mount_t *mp) +xfs_log_quiesce ( +struct xfs_mount *mp) {cancel_delayed_work_sync (&mp->m_log->l_work);xfs_log_force (mp,XFS_LOG_SYNC );
@@ -884,6 +881,20 @@ xfs_log_unmount(xfs_mount_t *mp)xfs_buf_unlock (mp->m_sb_bp);xfs_log_unmount_write (mp); +} + +/* +* Shut down and release the AIL and Log. +* +* During unmount, we need to ensure we flush all the dirty metadata objects +* from the AIL so that the log is empty before we write the unmount record to +* the log. Once this is done, we can tear down the AIL and the log. +*/ +void +xfs_log_unmount ( +struct xfs_mount *mp) +{ +xfs_log_quiesce (mp);xfs_trans_ail_destroy (mp);xlog_dealloc_log (mp->m_log);
fs/xfs/xfs_log.h +1 -0
@@ -183,6 +183,7 @@ bool xfs_log_item_in_current_chkpt(struct xfs_log_item *lip);void xfs_log_work_queue (struct xfs_mount *mp);void xfs_log_worker (struct work_struct *work); +void xfs_log_quiesce (struct xfs_mount *mp);#endif #endif /* __XFS_LOG_H__ */
fs/xfs/xfs_super.c +8 -33
62 lines not on this page · raw message
Signature · 6 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 10/13] xfs: xfs_quiesce_attr() should quiesce the log like unmount
Looks good,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 10/13] xfs: xfs_quiesce_attr() should quiesce the log like unmount
In reply to Dave ChinnerQuoted 10 lines · Dave Chinner
From: Dave Chinner<[email protected]>
xfs_quiesce_attr()is supposed to leave the log empty with an unmount record written. Right now it does not wait for the AIL to be emptied before writing the unmount record, not does it wait for metadata IO completion, either. Fix it to use the same method and code asxfs_log_unmount().Signed-off-by Dave Chinner<[email protected]>---
Looks good.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 11/13 xfs: rename xfs_sync.[ch] to xfs_icache.[ch]
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
xfs_sync.c now only contains inode reclaim functions and inode cache iteration functions. It is not related to sync operations anymore. Rename to xfs_icache.c to reflect it's contents and prepare for consolidation with the other inode cache file that exists (xfs_iget.c).
--- fs/xfs/Makefile | 2 +- fs/xfs/xfs_icache.c | 716 ++++++++++++++++++++++++++++++++++++++++++++++ fs/xfs/xfs_icache.h | 43 +++ fs/xfs/xfs_iget.c | 1 + fs/xfs/xfs_mount.c | 1 + fs/xfs/xfs_mount.h | 2 - fs/xfs/xfs_qm_syscalls.c | 1 + fs/xfs/xfs_super.c | 2 +- fs/xfs/xfs_sync.c | 715 --------------------------------------------- fs/xfs/xfs_sync.h | 43 --- 10 files changed, 764 insertions(+), 762 deletions(-) create mode 100644 fs/xfs/xfs_icache.c create mode 100644 fs/xfs/xfs_icache.h delete mode 100644 fs/xfs/xfs_sync.c delete mode 100644 fs/xfs/xfs_sync.h
fs/xfs/Makefile +1 -1
@@ -39,6 +39,7 @@ xfs-y += xfs_aops.o \ xfs_fsops.o \ xfs_fs_subr.o \ xfs_globals.o \ + xfs_icache.o \ xfs_iget.o \ xfs_ioctl.o \ xfs_iomap.o \
@@ -47,7 +48,6 @@ xfs-y += xfs_aops.o \ xfs_message.o \ xfs_mru_cache.o \ xfs_super.o \ - xfs_sync.o \ xfs_xattr.o \ xfs_rename.o \ xfs_utils.o \
fs/xfs/xfs_icache.c new file +716 -0
716 lines not on this page · raw message
fs/xfs/xfs_icache.h new file +43 -0
43 lines not on this page · raw message
fs/xfs/xfs_iget.c +1 -0
@@ -38,6 +38,7 @@#include "xfs_inode_item.h" #include "xfs_bmap.h" #include "xfs_trace.h" +#include "xfs_icache.h" /*
fs/xfs/xfs_mount.c +1 -0
@@ -42,6 +42,7 @@#include "xfs_fsops.h" #include "xfs_utils.h" #include "xfs_trace.h" +#include "xfs_icache.h" struct workqueue_struct *xfs_mount_wq;
fs/xfs/xfs_mount.h +0 -2
@@ -51,8 +51,6 @@ typedef struct xfs_trans_reservations {#else /* __KERNEL__ */ -#include "xfs_sync.h" -struct xlog ;struct xfs_inode ;struct xfs_mru_cache ;
fs/xfs/xfs_qm_syscalls.c +1 -0
@@ -40,6 +40,7 @@#include "xfs_utils.h" #include "xfs_qm.h" #include "xfs_trace.h" +#include "xfs_icache.h" STATIC int xfs_qm_log_quotaoff(xfs_mount_t *, xfs_qoff_logitem_t **, uint); STATIC int xfs_qm_log_quotaoff_end(xfs_mount_t *, xfs_qoff_logitem_t *,
fs/xfs/xfs_super.c +1 -1
@@ -49,7 +49,7 @@#include "xfs_extfree_item.h" #include "xfs_mru_cache.h" #include "xfs_inode_item.h" -#include "xfs_sync.h" +#include "xfs_icache.h" #include "xfs_trace.h" #include <linux/namei.h>
fs/xfs/xfs_sync.c deleted +0 -715
715 lines not on this page · raw message
fs/xfs/xfs_sync.h deleted +0 -43
43 lines not on this page · raw message
Signature · 6 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 11/13] xfs: rename xfs_sync.[ch] to xfs_icache.[ch]
Looks good,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 11/13] xfs: rename xfs_sync.[ch] to xfs_icache.[ch]
In reply to Dave ChinnerQuoted 10 lines · Dave Chinner
From: Dave Chinner<[email protected]>
xfs_sync.cnow only contains inode reclaim functions and inode cache iteration functions. It is not related to sync operations anymore. Rename toxfs_icache.cto reflect it's contents and prepare for consolidation with the other inode cache file that exists (xfs_iget.c).Signed-off-by Dave Chinner<[email protected]>---
Looks good.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 12/13 xfs: move inode locking functions to xfs_inode.c
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
xfs_ilock() and friends really aren't related to the inode cache in any way, so move them to xfs_inode.c with all the other inode related functionality.
While doing this move, move the xfs_ilock() tracepoints to *before* the lock is taken so that when a hang on a lock occurs we have events to indicate which process and what inode we were trying to lock when the hang occurred. This is much better than the current silence we get on a hang...
--- fs/xfs/xfs_iget.c | 251 ---------------------------------------------------- fs/xfs/xfs_inode.c | 250 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 250 insertions(+), 251 deletions(-)
fs/xfs/xfs_iget.c +0 -251
@@ -453,254 +453,3 @@ out_error_or_again:return error; } -/* -* This is a wrapper routine around the xfs_ilock() routine -* used to centralize some grungy code. It is used in places -* that wish to lock the inode solely for reading the extents. -* The reason these places can't just call xfs_ilock(SHARED) -* is that the inode lock also guards to bringing in of the -* extents from disk for a file in b-tree format. If the inode -* is in b-tree format, then we need to lock the inode exclusively -* until the extents are read in. Locking it exclusively all -* the time would limit our parallelism unnecessarily, though. -* What we do instead is check to see if the extents have been -* read in yet, and only lock the inode exclusively if they -* have not. -* -* The function returns a value which should be given to the -* corresponding xfs_iunlock_map_shared(). This value is -* the mode in which the lock was actually taken. -*/ -uint -xfs_ilock_map_shared ( -xfs_inode_t *ip) -{ -uint lock_mode; - -if ((ip->i_d.di_format ==XFS_DINODE_FMT_BTREE ) && - ((ip->i_df.if_flags &XFS_IFEXTENTS ) ==0 )) { - lock_mode =XFS_ILOCK_EXCL ; - }else { - lock_mode =XFS_ILOCK_SHARED ; - } - -xfs_ilock (ip, lock_mode); - -return lock_mode; -} - -/* -* This is simply the unlock routine to go with xfs_ilock_map_shared(). -* All it does is call xfs_iunlock() with the given lock_mode. -*/ -void -xfs_iunlock_map_shared ( -xfs_inode_t *ip, -unsigned int lock_mode) -{ -xfs_iunlock (ip, lock_mode); -} - -/* -* The xfs inode contains 2 locks: a multi-reader lock called the -* i_iolock and a multi-reader lock called the i_lock. This routine -* allows either or both of the locks to be obtained. -* -* The 2 locks should always be ordered so that the IO lock is -* obtained first in order to prevent deadlock. -* -* ip -- the inode being locked -* lock_flags -- this parameter indicates the inode's locks -* to be locked. It can be: -* XFS_IOLOCK_SHARED, -* XFS_IOLOCK_EXCL, -* XFS_ILOCK_SHARED, -* XFS_ILOCK_EXCL, -* XFS_IOLOCK_SHARED | XFS_ILOCK_SHARED, -* XFS_IOLOCK_SHARED | XFS_ILOCK_EXCL, -* XFS_IOLOCK_EXCL | XFS_ILOCK_SHARED, -* XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL -*/ -void -xfs_ilock ( -xfs_inode_t *ip, -uint lock_flags) -{ -/* -* You can't set both SHARED and EXCL for the same lock, -* and only XFS_IOLOCK_SHARED, XFS_IOLOCK_EXCL, XFS_ILOCK_SHARED, -* and XFS_ILOCK_EXCL are valid values to set in lock_flags. -*/ -ASSERT ((lock_flags & (XFS_IOLOCK_SHARED |XFS_IOLOCK_EXCL )) != - (XFS_IOLOCK_SHARED |XFS_IOLOCK_EXCL )); -ASSERT ((lock_flags & (XFS_ILOCK_SHARED |XFS_ILOCK_EXCL )) != - (XFS_ILOCK_SHARED |XFS_ILOCK_EXCL )); -ASSERT ((lock_flags & ~(XFS_LOCK_MASK |XFS_LOCK_DEP_MASK )) ==0 ); - -if (lock_flags &XFS_IOLOCK_EXCL ) -mrupdate_nested (&ip->i_iolock,XFS_IOLOCK_DEP (lock_flags)); -else if (lock_flags &XFS_IOLOCK_SHARED ) -mraccess_nested (&ip->i_iolock,XFS_IOLOCK_DEP (lock_flags)); - -if (lock_flags &XFS_ILOCK_EXCL ) -mrupdate_nested (&ip->i_lock,XFS_ILOCK_DEP (lock_flags)); -else if (lock_flags &XFS_ILOCK_SHARED ) -mraccess_nested (&ip->i_lock,XFS_ILOCK_DEP (lock_flags)); - -trace_xfs_ilock (ip, lock_flags, _RET_IP_); -} - -/* -* This is just like xfs_ilock(), except that the caller -* is guaranteed not to sleep. It returns 1 if it gets -* the requested locks and 0 otherwise. If the IO lock is -* obtained but the inode lock cannot be, then the IO lock -* is dropped before returning. -* -* ip -- the inode being locked -* lock_flags -- this parameter indicates the inode's locks to be -* to be locked. See the comment for xfs_ilock() for a list -* of valid values. -*/ -int -xfs_ilock_nowait ( -xfs_inode_t *ip, -uint lock_flags) -{ -/* -* You can't set both SHARED and EXCL for the same lock, -* and only XFS_IOLOCK_SHARED, XFS_IOLOCK_EXCL, XFS_ILOCK_SHARED, -* and XFS_ILOCK_EXCL are valid values to set in lock_flags. -*/ -ASSERT ((lock_flags & (XFS_IOLOCK_SHARED |XFS_IOLOCK_EXCL )) != - (XFS_IOLOCK_SHARED |XFS_IOLOCK_EXCL )); -ASSERT ((lock_flags & (XFS_ILOCK_SHARED |XFS_ILOCK_EXCL )) != - (XFS_ILOCK_SHARED |XFS_ILOCK_EXCL )); -ASSERT ((lock_flags & ~(XFS_LOCK_MASK |XFS_LOCK_DEP_MASK )) ==0 ); - -if (lock_flags &XFS_IOLOCK_EXCL ) { -if (!mrtryupdate (&ip->i_iolock)) - goto out; - }else if (lock_flags &XFS_IOLOCK_SHARED ) { -if (!mrtryaccess (&ip->i_iolock)) - goto out; - } -if (lock_flags &XFS_ILOCK_EXCL ) { -if (!mrtryupdate (&ip->i_lock)) - goto out_undo_iolock; - }else if (lock_flags &XFS_ILOCK_SHARED ) { -if (!mrtryaccess (&ip->i_lock)) - goto out_undo_iolock; - } -trace_xfs_ilock_nowait (ip, lock_flags, _RET_IP_); -return 1 ; - - out_undo_iolock: -if (lock_flags &XFS_IOLOCK_EXCL ) -mrunlock_excl (&ip->i_iolock); -else if (lock_flags &XFS_IOLOCK_SHARED ) -mrunlock_shared (&ip->i_iolock); - out: -return 0 ; -} - -/* -* xfs_iunlock() is used to drop the inode locks acquired with -* xfs_ilock() and xfs_ilock_nowait(). The caller must pass -* in the flags given to xfs_ilock() or xfs_ilock_nowait() so -* that we know which locks to drop. -* -* ip -- the inode being unlocked -* lock_flags -- this parameter indicates the inode's locks to be -* to be unlocked. See the comment for xfs_ilock() for a list -* of valid values for this parameter. -* -*/ -void -xfs_iunlock ( -xfs_inode_t *ip, -uint lock_flags) -{ -/* -* You can't set both SHARED and EXCL for the same lock, -* and only XFS_IOLOCK_SHARED, XFS_IOLOCK_EXCL, XFS_ILOCK_SHARED, -* and XFS_ILOCK_EXCL are valid values to set in lock_flags. -*/ -ASSERT ((lock_flags & (XFS_IOLOCK_SHARED |XFS_IOLOCK_EXCL )) != - (XFS_IOLOCK_SHARED |XFS_IOLOCK_EXCL )); -ASSERT ((lock_flags & (XFS_ILOCK_SHARED |XFS_ILOCK_EXCL )) != - (XFS_ILOCK_SHARED |XFS_ILOCK_EXCL )); -ASSERT ((lock_flags & ~(XFS_LOCK_MASK |XFS_LOCK_DEP_MASK )) ==0 ); -ASSERT (lock_flags !=0 ); - -if (lock_flags &XFS_IOLOCK_EXCL ) -mrunlock_excl (&ip->i_iolock); -else if (lock_flags &XFS_IOLOCK_SHARED ) -mrunlock_shared (&ip->i_iolock); - -if (lock_flags &XFS_ILOCK_EXCL ) -mrunlock_excl (&ip->i_lock); -else if (lock_flags &XFS_ILOCK_SHARED ) -mrunlock_shared (&ip->i_lock); - -trace_xfs_iunlock (ip, lock_flags, _RET_IP_); -} - -/* -* give up write locks. the i/o lock cannot be held nested -* if it is being demoted. -*/ -void -xfs_ilock_demote ( -xfs_inode_t *ip, -uint lock_flags) -{ -ASSERT (lock_flags & (XFS_IOLOCK_EXCL |XFS_ILOCK_EXCL )); -ASSERT ((lock_flags & ~(XFS_IOLOCK_EXCL |XFS_ILOCK_EXCL )) ==0 ); - -if (lock_flags &XFS_ILOCK_EXCL ) -mrdemote (&ip->i_lock); -if (lock_flags &XFS_IOLOCK_EXCL ) -mrdemote (&ip->i_iolock); - -trace_xfs_ilock_demote (ip, lock_flags, _RET_IP_); -} - -#ifdef DEBUG -int -xfs_isilocked ( -xfs_inode_t *ip, -uint lock_flags) -{ -if (lock_flags & (XFS_ILOCK_EXCL |XFS_ILOCK_SHARED )) { -if (!(lock_flags &XFS_ILOCK_SHARED )) -return !!ip->i_lock.mr_writer; -return rwsem_is_locked (&ip->i_lock.mr_lock); - } - -if (lock_flags & (XFS_IOLOCK_EXCL |XFS_IOLOCK_SHARED )) { -if (!(lock_flags &XFS_IOLOCK_SHARED )) -return !!ip->i_iolock.mr_writer; -return rwsem_is_locked (&ip->i_iolock.mr_lock); - } - -ASSERT (0 ); -return 0 ; -} -#endif - -void -__xfs_iflock ( -struct xfs_inode *ip) -{ -wait_queue_head_t *wq =bit_waitqueue (&ip->i_flags, __XFS_IFLOCK_BIT); -DEFINE_WAIT_BIT (wait, &ip->i_flags, __XFS_IFLOCK_BIT); - -do { -prepare_to_wait_exclusive (wq, &wait.wait,TASK_UNINTERRUPTIBLE ); -if (xfs_isiflocked (ip)) -io_schedule (); - }while (!xfs_iflock_nowait (ip)); - -finish_wait (wq, &wait.wait); -}
fs/xfs/xfs_inode.c +250 -0
256 lines not on this page · raw message
Signature · 6 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 12/13] xfs: move inode locking functions to xfs_inode.c
Quoted 11 lines · Dave Chinner
From: Dave Chinner <[email protected]>
xfs_ilock()and friends really aren't related to the inode cache in any way, so move them toxfs_inode.cwith all the other inode related functionality.While doing this move, move the
xfs_ilock()tracepoints to *before* the lock is taken so that when a hang on a lock occurs we have events to indicate which process and what inode we were trying to lock when the hang occurred. This is much better than the current silence we get on a hang...
Looks good.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 12/13] xfs: move inode locking functions to xfs_inode.c
In reply to Dave ChinnerQuoted 6 lines · Dave Chinner
From: Dave Chinner<[email protected]>
xfs_ilock()and friends really aren't related to the inode cache in any way, so move them toxfs_inode.cwith all the other inode related functionality.
Quoted 8 lines
While doing this move, move the
xfs_ilock()tracepoints to *before* the lock is taken so that when a hang on a lock occurs we have events to indicate which process and what inode we were trying to lock when the hang occurred. This is much better than the current silence we get on a hang...Signed-off-by Dave Chinner<[email protected]>---
Looks good.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 13/13 xfs: remove xfs_iget.c
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
The inode cache functions remaining in xfs_iget.c can be moved to xfs_icache.c along with the other inode cache functions. This removes all functionality from xfs_iget.c, so the file can simply be removed.
This move results in various functions now only having the scope of a single file (e.g. xfs_inode_free()), so clean up all the definitions and exported prototypes in xfs_icache.[ch] and xfs_inode.h appropriately.
--- fs/xfs/Makefile | 1 - fs/xfs/xfs_export.c | 1 + fs/xfs/xfs_icache.c | 421 +++++++++++++++++++++++++++++++++++++++++- fs/xfs/xfs_icache.h | 6 +- fs/xfs/xfs_iget.c | 455 ---------------------------------------------- fs/xfs/xfs_inode.c | 1 + fs/xfs/xfs_inode.h | 10 +- fs/xfs/xfs_itable.c | 1 + fs/xfs/xfs_log_recover.c | 1 + fs/xfs/xfs_qm.c | 1 + fs/xfs/xfs_rtalloc.c | 1 + fs/xfs/xfs_vnodeops.c | 1 + 12 files changed, 430 insertions(+), 470 deletions(-) delete mode 100644 fs/xfs/xfs_iget.c
fs/xfs/Makefile +0 -1
@@ -40,7 +40,6 @@ xfs-y += xfs_aops.o \ xfs_fs_subr.o \ xfs_globals.o \ xfs_icache.o \ - xfs_iget.o \ xfs_ioctl.o \ xfs_iomap.o \ xfs_iops.o \
fs/xfs/xfs_export.c +1 -0
@@ -29,6 +29,7 @@#include "xfs_inode.h" #include "xfs_inode_item.h" #include "xfs_trace.h" +#include "xfs_icache.h" /* * Note that we only accept fileids which are long enough rather than allow
fs/xfs/xfs_icache.c +418 -3
445 lines not on this page · raw message
fs/xfs/xfs_icache.h +3 -3
@@ -24,6 +24,9 @@ struct xfs_perag;#define SYNC_WAIT 0x0001/* wait for i/o to complete */ #define SYNC_TRYLOCK 0x0002/* only try to lock inodes */ +int xfs_iget (struct xfs_mount *mp,struct xfs_trans *tp,xfs_ino_t ino, +uint flags,uint lock_flags,xfs_inode_t **ipp); +void xfs_reclaim_worker (struct work_struct *work);int xfs_reclaim_inodes (struct xfs_mount *mp,int mode);
@@ -31,9 +34,6 @@ int xfs_reclaim_inodes_count(struct xfs_mount *mp);void xfs_reclaim_inodes_nr (struct xfs_mount *mp,int nr_to_scan);void xfs_inode_set_reclaim_tag (struct xfs_inode *ip); -void __xfs_inode_set_reclaim_tag (struct xfs_perag *pag,struct xfs_inode *ip); -void __xfs_inode_clear_reclaim_tag (struct xfs_mount *mp,struct xfs_perag *pag, -struct xfs_inode *ip);int xfs_sync_inode_grab (struct xfs_inode *ip); int xfs_inode_ag_iterator(struct xfs_mount *mp,
fs/xfs/xfs_iget.c deleted +0 -455
455 lines not on this page · raw message
fs/xfs/xfs_inode.c +1 -0
@@ -45,6 +45,7 @@#include "xfs_filestream.h" #include "xfs_vnodeops.h" #include "xfs_trace.h" +#include "xfs_icache.h" kmem_zone_t *xfs_ifork_zone;kmem_zone_t *xfs_inode_zone;
fs/xfs/xfs_inode.h +2 -8
@@ -496,11 +496,10 @@ static inline int xfs_isiflocked(struct xfs_inode *ip) (((pip)->i_mount->m_flags &XFS_MOUNT_GRPID ) || \ ((pip)->i_d.di_mode &S_ISGID )) +/* -* xfs_iget.c prototypes. +* xfs_inode.c prototypes. */ -int xfs_iget (struct xfs_mount *,struct xfs_trans *,xfs_ino_t , -uint ,uint ,xfs_inode_t **);void xfs_ilock (xfs_inode_t *,uint );int xfs_ilock_nowait (xfs_inode_t *,uint );void xfs_iunlock (xfs_inode_t *,uint );
@@ -508,11 +507,6 @@ void xfs_ilock_demote(xfs_inode_t *, uint);int xfs_isilocked (xfs_inode_t *,uint );uint xfs_ilock_map_shared (xfs_inode_t *);void xfs_iunlock_map_shared (xfs_inode_t *,uint ); -void xfs_inode_free (struct xfs_inode *ip); - -/* -* xfs_inode.c prototypes. -*/ int xfs_ialloc (struct xfs_trans *,xfs_inode_t *,umode_t ,xfs_nlink_t ,xfs_dev_t ,prid_t ,int ,struct xfs_buf **,xfs_inode_t **);
fs/xfs/xfs_itable.c +1 -0
@@ -34,6 +34,7 @@#include "xfs_error.h" #include "xfs_btree.h" #include "xfs_trace.h" +#include "xfs_icache.h" STATIC int xfs_internal_inum(
fs/xfs/xfs_log_recover.c +1 -0
@@ -42,6 +42,7 @@#include "xfs_quota.h" #include "xfs_utils.h" #include "xfs_trace.h" +#include "xfs_icache.h" STATIC int xlog_find_zeroed(
fs/xfs/xfs_qm.c +1 -0
@@ -40,6 +40,7 @@#include "xfs_utils.h" #include "xfs_qm.h" #include "xfs_trace.h" +#include "xfs_icache.h" /* * The global quota manager. There is only one of these for the entire
fs/xfs/xfs_rtalloc.c +1 -0
@@ -38,6 +38,7 @@#include "xfs_utils.h" #include "xfs_trace.h" #include "xfs_buf.h" +#include "xfs_icache.h" /*
fs/xfs/xfs_vnodeops.c +1 -0
@@ -47,6 +47,7 @@#include "xfs_filestream.h" #include "xfs_vnodeops.h" #include "xfs_trace.h" +#include "xfs_icache.h" /* * The maximum pathlen is 1024 bytes. Since the minimum file system
Signature · 6 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 13/13] xfs: remove xfs_iget.c
Looks good,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 13/13] xfs: remove xfs_iget.c
In reply to Dave ChinnerQuoted 6 lines · Dave Chinner
From: Dave Chinner<[email protected]>
The inode cache functions remaining in
xfs_iget.ccan be moved toxfs_icache.calong with the other inode cache functions. This removes all functionality fromxfs_iget.c, so the file can simply be removed.
Quoted 6 lines
This move results in various functions now only having the scope of a single file (e.g.
xfs_inode_free()), so clean up all the definitions and exported prototypes in xfs_icache.[ch] andxfs_inode.happropriately.Signed-off-by Dave Chinner<[email protected]>---
Looks good.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 6 lines · Dave Chinner
Version 2 of the patchset I described here:
http://oss.sgi.com/archives/xfs/2012-06/msg00064.html
This version has run through xfstests completely once, so it's less likely to let smoke out....
Is there a publicly accessible git tree available where one could pull from? (This would be way easier than saving and hand-applying 13 patches.)
Signature · 6 lines
Markus _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 11 lines · Markus Trippelsdorf
Quoted 6 lines · Dave Chinner
Version 2 of the patchset I described here:
http://oss.sgi.com/archives/xfs/2012-06/msg00064.html
This version has run through xfstests completely once, so it's less likely to let smoke out....
Is there a publicly accessible git tree available where one could pull from? (This would be way easier than saving and hand-applying 13 patches.)
No.
Instead, save all the patches to a single mbox format file, then run:
$ git checkout -b umount-fix-test $ git am <mbox file>
And it will apply all the patches as separate commits to the umount-fix-test branch. This is how I take patch sets from my inbox to git. You can build and testing them from there.
I, personally, convert them to a guilt series first, usually integrating them into my working/test branch to test them along with everything else I'm working on at the same time. To do this, I generally do:
$ git checkout -b umount-fix-test working $ git am <mbox file> $ git checkout working $ git log --pretty=oneline working..umount-fix-test <record first and last commit id in applied series> $ guilt import-commits <first-commit..last-commit>
And that adds the commits from the umount-fix-test branch into individual patches in the guilt series preserving all the metadata from the original sender in them. i.e. if I then patchbomb them back to the mailing list, they will have the correct "From:" attribution and dates as found in the original emails....
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 15 lines · Dave Chinner
Quoted 11 lines · Markus Trippelsdorf
Quoted 6 lines · Dave Chinner
Version 2 of the patchset I described here:
http://oss.sgi.com/archives/xfs/2012-06/msg00064.html
This version has run through xfstests completely once, so it's less likely to let smoke out....
Is there a publicly accessible git tree available where one could pull from? (This would be way easier than saving and hand-applying 13 patches.)
No.
Quoted 8 lines
Instead, save all the patches to a single mbox format file, then run:
$ git checkout -b umount-fix-test $ git am <mbox file>And it will apply all the patches as separate commits to the umount-fix-test branch. This is how I take patch sets from my inbox to git. You can build and testing them from there.
Yeah. That works if you're using mbox format files. But if you're are a Maildir user like myself you're basically screwed, because
$ git am <Maildir> expects the Maildir to be *sorted* and because mails normally don't arrive in the right order, git-am will therefore try to apply the patches in the wrong order. (Maybe this should be reported to the git mailing-list)
So if there are mutt users out there who use "mbox_type=Maildir" and know how to save a thread to a single mbox file, then please let me know.
Thanks.
Signature · 6 lines
Markus _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 33 lines · Markus Trippelsdorf
Quoted 15 lines · Dave Chinner
Quoted 11 lines · Markus Trippelsdorf
Quoted 6 lines · Dave Chinner
Version 2 of the patchset I described here:
http://oss.sgi.com/archives/xfs/2012-06/msg00064.html
This version has run through xfstests completely once, so it's less likely to let smoke out....
Is there a publicly accessible git tree available where one could pull from? (This would be way easier than saving and hand-applying 13 patches.)
No.
Quoted 8 lines
Instead, save all the patches to a single mbox format file, then run:
$ git checkout -b umount-fix-test $ git am <mbox file>And it will apply all the patches as separate commits to the umount-fix-test branch. This is how I take patch sets from my inbox to git. You can build and testing them from there.
Yeah. That works if you're using mbox format files. But if you're are a Maildir user like myself you're basically screwed, because
$ git am <Maildir> expects the Maildir to be *sorted* and because mails normally don't arrive in the right order, git-am will therefore try to apply the patches in the wrong order. (Maybe this should be reported to the git mailing-list)
Quoted 3 lines
So if there are mutt users out there who use "mbox_type=Maildir" and know how to save a thread to a single mbox file, then please let me know.
I use mutt, and store all my mail folders in maildir format. However, I don't use mbox_type=Maildir - I simply create new maildir folders via the CLI when I need a new one. Mutt automatically recognises the directories as being in maildir format and uses it. Hence when I tag a thread and ;s to save all the tagged mail, it creates a mbox file as that is the default.
That probably doesn't help you, though. However, IIRC, you can get the same result regardless of your mbox_type by piping all the tagged mail through formail and redirecting that to a file like so:
;| formail -ds > $file
and that will result in $file being a mbox format file with all the tagged mail in it...
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 45 lines · Dave Chinner
Quoted 33 lines · Markus Trippelsdorf
Quoted 15 lines · Dave Chinner
Quoted 11 lines · Markus Trippelsdorf
Quoted 6 lines · Dave Chinner
Version 2 of the patchset I described here:
http://oss.sgi.com/archives/xfs/2012-06/msg00064.html
This version has run through xfstests completely once, so it's less likely to let smoke out....
Is there a publicly accessible git tree available where one could pull from? (This would be way easier than saving and hand-applying 13 patches.)
No.
Quoted 8 lines
Instead, save all the patches to a single mbox format file, then run:
$ git checkout -b umount-fix-test $ git am <mbox file>And it will apply all the patches as separate commits to the umount-fix-test branch. This is how I take patch sets from my inbox to git. You can build and testing them from there.
Yeah. That works if you're using mbox format files. But if you're are a Maildir user like myself you're basically screwed, because
$ git am <Maildir> expects the Maildir to be *sorted* and because mails normally don't arrive in the right order, git-am will therefore try to apply the patches in the wrong order. (Maybe this should be reported to the git mailing-list)Quoted 3 lines
So if there are mutt users out there who use "mbox_type=Maildir" and know how to save a thread to a single mbox file, then please let me know.
I use mutt, and store all my mail folders in maildir format. However, I don't use mbox_type=Maildir - I simply create new maildir folders via the CLI when I need a new one. Mutt automatically recognises the directories as being in maildir format and uses it. Hence when I tag a thread and ;s to save all the tagged mail, it creates a mbox file as that is the default.
Quoted 8 lines
That probably doesn't help you, though. However, IIRC, you can get the same result regardless of your
mbox_typeby piping all the tagged mail through formail and redirecting that to a file like so:;| formail -ds > $file
and that will result in $file being a mbox format file with all the tagged mail in it...
After some experimentation I came up with the following:
macro index <F5> "<enter-command>set mbox_type=mbox;set confirmcreate=no;set sort=subject<enter>;sPatches<enter><enter-command>set mbox_type=Maildir; set sort=threads;set confirmcreate=yes<enter>U.<enter>"
Basically you just tag the thread (esc-t) and then hit F5 and it will save it to a mbox file called "Patches" in your home dir.
Signature · 6 lines
Markus _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 7 lines · Dave Chinner
Version 2 of the patchset I described here:
http://oss.sgi.com/archives/xfs/2012-06/msg00064.html
This version has run through xfstests completely once, so it's less likely to let smoke out....
Quoted 8 lines
Version 2:
- fixwriteback_inodes_sb_if_idlecall inxfs_create()
- refreshed patch 13 before sending._______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
I wanted to get a fast look at your patch series. I am getting the following ASSERT on xfstest 179 when running the series with the latest OSS soruces.The ASSERT appears to start at patch number 3. Sorry these boxes won't kdump the top of tree kernels:
[17474.545964] XFS: Assertion failed: atomic_read(&bp->b_hold) > 0, file: /root/xfs/fs/xfs/xfs_buf.c, line: 896 [17474.555828] ------------[ cut here ]------------ [17474.559784] kernel BUG at /root/xfs/fs/xfs/xfs_message.c:100! [17474.559784] invalid opcode: 0000 [#1] SMP [17474.559784] Modules linked in: xfs(O) autofs4 binfmt_misc mperf fuse loop dm_mod qla2xxx ib_mthca ipv6 i5k_amb scsi_transport_fc ib_mad e1000e i2c_i801 i5000_edac scsi_tgt i2c_core ioatdma shpchp edac_core pci_hotplug sg lpc_ich mfd_core ib_core pcspkr dca microcode rtc_cmos button uhci_hcd ehci_hcd sd_mod crc_t10dif usbcore usb_common scsi_dh_emc scsi_dh_rdac scsi_dh_alua scsi_dh_hp_sw scsi_dh thermal sata_nv processor mptsas mptscsih scsi_transport_sas mptbase megaraid_sas fan thermal_sys hwmon ext3 jbd mbcache edd ata_piix ahci libahci libata scsi_mod [last unloaded: xfs] [17474.559784] CPU 0 [17474.559784] Pid: 26427, comm: umount Tainted: G O 3.6.0-rc1+ #1 SGI.COM AltixXE210/S5000PAL0 [17474.559784] RIP: 0010:[<ffffffffa05c544d>] [<ffffffffa05c544d>] assfail+0x1d/0x30 [xfs] [17474.559784] RSP: 0018:ffff8808379238a8 EFLAGS: 00010296 [17474.559784] RAX: 0000000000000060 RBX: ffff8807f8c0a564 RCX: 0000000000000082 [17474.559784] RDX: 0000000000004a61 RSI: 0000000000000086 RDI: 0000000000000246 [17474.559784] RBP: ffff8808379238a8 R08: 0000000000000811 R09: ffffffff818ba780 [17474.559784] R10: 0000000000000811 R11: 0000000000000006 R12: ffff8807f8c0a540 [17474.559784] R13: ffffffffa05b5b86 R14: ffff88083a8810c0 R15: 0000000000000000 [17474.559784] FS: 00007fc12646d740(0000) GS:ffff88085fc00000(0000) knlGS:0000000000000000 [17474.559784] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b [17474.559784] CR2: 00007fc125b29570 CR3: 000000085be2c000 CR4: 00000000000007f0 [17474.559784] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [17474.559784] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 [17474.559784] Process umount (pid: 26427, threadinfo ffff880837922000, task ffff88083d278600) [17474.559784] Stack: [17474.559784] ffff8808379238d8 ffffffffa05b4ed4 ffff8807f8c0a540 ffff8807f8c0a540 [17474.559784] ffffffffa061e939 0000000000000000 ffff8808379238f8 ffffffffa05b5b86 [17474.559784] ffff880837a436d0 ffff8807f8c0a540 ffff880837923928 ffffffffa05b5c26 [17474.559784] Call Trace: [17474.559784] [<ffffffffa05b4ed4>] xfs_buf_rele+0xa4/0x1b0 [xfs] [17474.559784] [<ffffffffa061e939>] ? xfs_buf_iodone_callbacks+0x59/0x230 [xfs] [17474.559784] [<ffffffffa05b5b86>] xfs_buf_iodone_work+0x46/0x50 [xfs] [17474.559784] [<ffffffffa05b5c26>] xfs_buf_ioend+0x96/0x120 [xfs] [17474.559784] [<ffffffffa05b5b61>] ? xfs_buf_iodone_work+0x21/0x50 [xfs] [17474.559784] [<ffffffffa061e939>] xfs_buf_iodone_callbacks+0x59/0x230 [xfs] [17474.559784] [<ffffffffa061f7e9>] ? xfs_buf_item_unpin+0x289/0x2d0 [xfs] [17474.559784] [<ffffffffa05b5b61>] xfs_buf_iodone_work+0x21/0x50 [xfs] [17474.559784] [<ffffffffa05b5c26>] xfs_buf_ioend+0x96/0x120 [xfs] [17474.559784] [<ffffffffa061f7e9>] xfs_buf_item_unpin+0x289/0x2d0 [xfs] [17474.559784] [<ffffffffa0617c33>] xfs_trans_committed_bulk+0x213/0x300 [xfs] [17474.559784] [<ffffffffa061bd33>] ? xlog_state_get_iclog_space+0x293/0x350 [xfs] [17474.559784] [<ffffffff8107f9e8>] ? idle_balance+0xe8/0x150 [17474.559784] [<ffffffffa061c9d5>] ? xlog_write+0x165/0x5d0 [xfs] [17474.559784] [<ffffffffa061dde6>] xlog_cil_committed+0x36/0x130 [xfs] [17474.559784] [<ffffffffa061e1e8>] xlog_cil_push+0x308/0x430 [xfs] [17474.559784] [<ffffffff81110522>] ? pagevec_lookup_tag+0x22/0x30 [17474.559784] [<ffffffff8105dbb9>] ? start_flush_work+0x29/0x100 [17474.559784] [<ffffffffa061e466>] xlog_cil_force_lsn+0x146/0x1b0 [xfs] [17474.559784] [<ffffffff81433a82>] ? wait_for_common+0xd2/0x190 [17474.559784] [<ffffffff81178285>] ? iput_final+0x145/0x1e0 [17474.559784] [<ffffffffa061c1e4>] _xfs_log_force+0x64/0x280 [xfs] [17474.559784] [<ffffffff811863fe>] ? sync_inodes_sb+0x9e/0xd0 [17474.559784] [<ffffffffa061c454>] xfs_log_force+0x54/0x80 [xfs] [17474.559784] [<ffffffffa05c65dd>] xfs_fs_sync_fs+0x2d/0x50 [xfs] [17474.559784] [<ffffffff8118c00b>] __sync_filesystem+0x2b/0x50 [17474.559784] [<ffffffff8118c073>] sync_filesystem+0x43/0x60 [17474.559784] [<ffffffff81160846>] generic_shutdown_super+0x36/0xe0 [17474.559784] [<ffffffff8116091c>] kill_block_super+0x2c/0x80 [17474.559784] [<ffffffff81160e78>] deactivate_locked_super+0x38/0x90 [17474.559784] [<ffffffff81161951>] deactivate_super+0x61/0x70 [17474.559784] [<ffffffff8117c659>] mntput_no_expire+0x149/0x1b0 [17474.559784] [<ffffffff8117d10e>] sys_umount+0x6e/0xd0 [17474.559784] [<ffffffff8143d479>] system_call_fastpath+0x16/0x1b [17474.559784] Code: 00 00 00 48 89 45 c8 e8 72 fc ff ff c9 c3 55 41 89 d0 48 89 f1 48 89 fa 48 c7 c6 58 b0 63 a0 31 ff 48 89 e5 31 c0 e8 93 ff ff ff <0f> 0b eb fe 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 55 4c [17474.559784] RIP [<ffffffffa05c544d>] assfail+0x1d/0x30 [xfs] [17474.559784] RSP <ffff8808379238a8>
I got this ASSERT when I ran it on the 8/27 OSS sources:
[188646.952426] XFS: Assertion failed: atomic_read(&iclog->ic_refcnt) == 0, file: /root/xfs/fs/xfs/xfs_log.c, line: 2590 [188646.963050] ------------[ cut here ]------------ [188646.967020] kernel BUG at /root/xfs/fs/xfs/xfs_message.c:100! [188646.967020] invalid opcode: 0000 [#1] SMP [188646.967020] Modules linked in: xfs(O) autofs4 binfmt_misc mperf fuse loop dm_
mod qla2xxx i5k_amb i5000_edac ipv6 scsi_transport_fc scsi_tgt e1000e edac_core i b_mthca shpchp sg ioatdma pci_hotplug lpc_ich mfd_core microcode ib_mad ib_core d ca pcspkr i2c_i801 i2c_core button rtc_cmos uhci_hcd ehci_hcd sd_mod crc_t10dif u sbcore usb_common scsi_dh_emc scsi_dh_rdac scsi_dh_alua scsi_dh_hp_sw scsi_dh the rmal sata_nv processor mptsas mptscsih scsi_transport_sas mptbase megaraid_sas fa n thermal_sys hwmon ext3 jbd mbcache edd ata_piix ahci libahci libata scsi_mod [l ast unloaded: xfs]
[188646.967020] CPU 2 [188646.967020] Pid: 356, comm: kworker/2:1H Tainted: G O 3.6.0-rc1+ #1 SGI.COM AltixXE210/S5000PAL0 [188646.967020] RIP: 0010:[<ffffffffa0188e2d>] [<ffffffffa0188e2d>] assfail+0x1d /0x30 [xfs] [188646.967020] RSP: 0018:ffff8808396a5d90 EFLAGS: 00010286 [188646.967020] RAX: 0000000000000068 RBX: ffff88083b4efdc0 RCX: 0000000000000086 [188646.967020] RDX: 0000000000000b04 RSI: 0000000000000082 RDI: 0000000000000246 [188646.967020] RBP: ffff8808396a5d90 R08: 0000000000002291 R09: ffffffff818ba780 [188646.967020] R10: 0000000000002291 R11: 0000000000000006 R12: ffff88053ff99800 [188646.967020] R13: ffff88053ff99928 R14: 0000000000000002 R15: ffff88085fc8d730 [188646.967020] FS: 0000000000000000(0000) GS:ffff88085fc80000(0000) knlGS:00000 00000000000 [188646.967020] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b [188646.967020] CR2: ffffffffff600400 CR3: 0000000838497000 CR4: 00000000000007e0 [188646.967020] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [188646.967020] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 [188646.967020] Process kworker/2:1H (pid: 356, threadinfo ffff8808396a4000, task ffff88083a9aa1c0) [188646.967020] Stack: [188646.967020] ffff8808396a5dc0 ffffffffa01dd2bf 0000000000000002 ffff88083ae67 980 [188646.967020] ffff88083b4efdc0 ffff88085fc8d400 ffff8808396a5df0 ffffffffa01dd bde [188646.967020] ffff88085fc8d400 ffff88083ae67980 ffff88083ae67a18 ffff88083a5e3 240 [188646.967020] Call Trace: [188646.967020] [<ffffffffa01dd2bf>] xlog_state_done_syncing+0x7f/0x110 [xfs] [188646.967020] [<ffffffffa01ddbde>] xlog_iodone+0x7e/0x100 [xfs] [188646.967020] [<ffffffffa0179b51>] xfs_buf_iodone_work+0x21/0x50 [xfs] [188646.967020] [<ffffffff8105d6b3>] process_one_work+0x1d3/0x370 [188646.967020] [<ffffffffa0179b30>] ? xfs_bioerror_relse+0x80/0x80 [xfs] [188646.967020] [<ffffffff810603e3>] worker_thread+0x133/0x390 [188646.967020] [<ffffffff810602b0>] ? manage_workers+0x70/0x70 [188646.967020] [<ffffffff810651ce>] kthread+0x9e/0xb0 [188646.967020] [<ffffffff8143e504>] kernel_thread_helper+0x4/0x10 [188646.967020] [<ffffffff81065130>] ? kthread_freezable_should_stop+0x70/0x70 [188646.967020] [<ffffffff8143e500>] ? gs_change+0x13/0x13 [188646.967020] Code: 00 00 00 48 89 45 c8 e8 72 fc ff ff c9 c3 55 41 89 d0 48 89 f1 48 89 fa 48 c7 c6 58 ef 1f a0 31 ff 48 89 e5 31 c0 e8 93 ff ff ff <0f> 0b eb fe 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 55 4c [188646.967020] RIP [<ffffffffa0188e2d>] assfail+0x1d/0x30 [xfs] [188646.967020] RSP <ffff8808396a5d90
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 17 lines · Mark Tinguely
Quoted 7 lines · Dave Chinner
Version 2 of the patchset I described here:
http://oss.sgi.com/archives/xfs/2012-06/msg00064.html
This version has run through xfstests completely once, so it's less likely to let smoke out....
Quoted 8 lines
Version 2:
- fixwriteback_inodes_sb_if_idlecall inxfs_create()
- refreshed patch 13 before sending._______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 7 lines
I wanted to get a fast look at your patch series. I am getting the following ASSERT on xfstest 179 when running the series with the latest OSS soruces.The ASSERT appears to start at patch number 3. Sorry these boxes won't kdump the top of tree kernels:
[17474.545964] XFS: Assertion failed: atomic_read(&bp->b_hold) > 0,file:
/root/xfs/fs/xfs/xfs_buf.c, line: 896
FWIW, when you paste stack traces, can you turn off line wrapping when you paste it so the crash is simple to quote in reply? (use :set paste in mutt, the :set nopaste when finished pasting it in).
Quoted 1 line
[17474.559784] Process umount (pid: 26427, threadinfo
...
Quoted 15 lines
[17474.559784] Call Trace: [17474.559784] [<ffffffffa05b4ed4>] xfs_buf_rele+0xa4/0x1b0 [xfs] [17474.559784] [<ffffffffa05b5b86>] xfs_buf_iodone_work+0x46/0x50 [xfs] [17474.559784] [<ffffffffa05b5c26>] xfs_buf_ioend+0x96/0x120 [xfs] [17474.559784] [<ffffffffa061e939>] xfs_buf_iodone_callbacks+0x59/0x230 [xfs] [17474.559784] [<ffffffffa05b5b61>] xfs_buf_iodone_work+0x21/0x50 [xfs] [17474.559784] [<ffffffffa05b5c26>] xfs_buf_ioend+0x96/0x120 [xfs] [17474.559784] [<ffffffffa061f7e9>] xfs_buf_item_unpin+0x289/0x2d0 [xfs] [17474.559784] [<ffffffffa0617c33>] xfs_trans_committed_bulk+0x213/0x300 [xfs] [17474.559784] [<ffffffffa061dde6>] xlog_cil_committed+0x36/0x130 [xfs] [17474.559784] [<ffffffffa061e1e8>] xlog_cil_push+0x308/0x430 [xfs] [17474.559784] [<ffffffffa061e466>] xlog_cil_force_lsn+0x146/0x1b0 [xfs] [17474.559784] [<ffffffffa061c1e4>] _xfs_log_force+0x64/0x280 [xfs] [17474.559784] [<ffffffffa061c454>] xfs_log_force+0x54/0x80 [xfs] [17474.559784] [<ffffffffa05c65dd>] xfs_fs_sync_fs+0x2d/0x50 [xfs]
Quoted 8 lines
[17474.559784] [<ffffffff8118c00b>] __sync_filesystem+0x2b/0x50 [17474.559784] [<ffffffff8118c073>] sync_filesystem+0x43/0x60 [17474.559784] [<ffffffff81160846>] generic_shutdown_super+0x36/0xe0 [17474.559784] [<ffffffff8116091c>] kill_block_super+0x2c/0x80 [17474.559784] [<ffffffff81160e78>] deactivate_locked_super+0x38/0x90 [17474.559784] [<ffffffff81161951>] deactivate_super+0x61/0x70 [17474.559784] [<ffffffff8117c659>] mntput_no_expire+0x149/0x1b0 [17474.559784] [<ffffffff8117d10e>] sys_umount+0x6e/0xd0
Nothing has been shut down in XFS at this point (i.e. .put_super() has not yet been called) so none of the shutdown changes could have caused this problem.
Indeed, it looks like this is during a forced shutdown here in xfs_buf_item_unpin:
} else if (freed && remove) {
xfs_buf_lock(bp);
xfs_buf_ioerror(bp, EIO);
XFS_BUF_UNDONE(bp);
xfs_buf_stale(bp);Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
xfs_buf_ioend(bp, 0);
}
Now, xfs_buf_stale() does this:
ASSERT(atomic_read(&bp->b_hold) >= 1);
Which means that in calling xfs_buf_ioend(), at least two references to the buffer are being dropped. Working out why that is occurring will find the root cause of this problem.
All that I can say at this point is that I find it highly unlikely that it is caused by the changes in this patchset.
Quoted 6 lines
I got this ASSERT when I ran it on the 8/27 OSS sources:
[188646.952426] XFS: Assertion failed:
atomic_read(&iclog->ic_refcnt)== 0, file:
/root/xfs/fs/xfs/xfs_log.c, line: 2590
Quoted 9 lines
[188646.967020] Process kworker/2:1H (pid: 356, threadinfo ffff8808396a4000, task ffff88083a9aa1c0) [188646.967020] Call Trace: [188646.967020] [<ffffffffa01dd2bf>] xlog_state_done_syncing+0x7f/0x110 [xfs] [188646.967020] [<ffffffffa01ddbde>] xlog_iodone+0x7e/0x100 [xfs] [188646.967020] [<ffffffffa0179b51>] xfs_buf_iodone_work+0x21/0x50 [xfs] [188646.967020] [<ffffffff8105d6b3>] process_one_work+0x1d3/0x370 [188646.967020] [<ffffffff810603e3>] worker_thread+0x133/0x390 [188646.967020] [<ffffffff810651ce>] kthread+0x9e/0xb0 [188646.967020] [<ffffffff8143e504>] kernel_thread_helper+0x4/0x10
I've never seen that ASSERT fire. That implies we've got a log buffer that is being actively modified under IO, but I cannot see how that would happen. Was this during an unmount? What test?
/me is starting to wonder about memory errors...
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 102 lines · Dave Chinner
Quoted 17 lines · Mark Tinguely
Quoted 7 lines · Dave Chinner
Version 2 of the patchset I described here:
http://oss.sgi.com/archives/xfs/2012-06/msg00064.html
This version has run through xfstests completely once, so it's less likely to let smoke out....
Quoted 8 lines
Version 2:
- fixwriteback_inodes_sb_if_idlecall inxfs_create()
- refreshed patch 13 before sending._______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfsQuoted 7 lines
I wanted to get a fast look at your patch series. I am getting the following ASSERT on xfstest 179 when running the series with the latest OSS soruces.The ASSERT appears to start at patch number 3. Sorry these boxes won't kdump the top of tree kernels:
[17474.545964] XFS: Assertion failed: atomic_read(&bp->b_hold)> 0,file:
/root/xfs/fs/xfs/xfs_buf.c, line: 896FWIW, when you paste stack traces, can you turn off line wrapping when you paste it so the crash is simple to quote in reply? (use :set paste in mutt, the :set nopaste when finished pasting it in).
Quoted 1 line
[17474.559784] Process umount (pid: 26427, threadinfo...
Quoted 15 lines
[17474.559784] Call Trace: [17474.559784] [<ffffffffa05b4ed4>] xfs_buf_rele+0xa4/0x1b0 [xfs] [17474.559784] [<ffffffffa05b5b86>] xfs_buf_iodone_work+0x46/0x50 [xfs] [17474.559784] [<ffffffffa05b5c26>] xfs_buf_ioend+0x96/0x120 [xfs] [17474.559784] [<ffffffffa061e939>] xfs_buf_iodone_callbacks+0x59/0x230 [xfs] [17474.559784] [<ffffffffa05b5b61>] xfs_buf_iodone_work+0x21/0x50 [xfs] [17474.559784] [<ffffffffa05b5c26>] xfs_buf_ioend+0x96/0x120 [xfs] [17474.559784] [<ffffffffa061f7e9>] xfs_buf_item_unpin+0x289/0x2d0 [xfs] [17474.559784] [<ffffffffa0617c33>] xfs_trans_committed_bulk+0x213/0x300 [xfs] [17474.559784] [<ffffffffa061dde6>] xlog_cil_committed+0x36/0x130 [xfs] [17474.559784] [<ffffffffa061e1e8>] xlog_cil_push+0x308/0x430 [xfs] [17474.559784] [<ffffffffa061e466>] xlog_cil_force_lsn+0x146/0x1b0 [xfs] [17474.559784] [<ffffffffa061c1e4>] _xfs_log_force+0x64/0x280 [xfs] [17474.559784] [<ffffffffa061c454>] xfs_log_force+0x54/0x80 [xfs] [17474.559784] [<ffffffffa05c65dd>] xfs_fs_sync_fs+0x2d/0x50 [xfs]Quoted 8 lines
[17474.559784] [<ffffffff8118c00b>] __sync_filesystem+0x2b/0x50 [17474.559784] [<ffffffff8118c073>] sync_filesystem+0x43/0x60 [17474.559784] [<ffffffff81160846>] generic_shutdown_super+0x36/0xe0 [17474.559784] [<ffffffff8116091c>] kill_block_super+0x2c/0x80 [17474.559784] [<ffffffff81160e78>] deactivate_locked_super+0x38/0x90 [17474.559784] [<ffffffff81161951>] deactivate_super+0x61/0x70 [17474.559784] [<ffffffff8117c659>] mntput_no_expire+0x149/0x1b0 [17474.559784] [<ffffffff8117d10e>] sys_umount+0x6e/0xd0Nothing has been shut down in XFS at this point (i.e. .put_super() has not yet been called) so none of the shutdown changes could have caused this problem.
Indeed, it looks like this is during a forced shutdown here in
xfs_buf_item_unpin:} else if (freed&& remove) { xfs_buf_lock(bp); xfs_buf_ioerror(bp, EIO); XFS_BUF_UNDONE(bp); xfs_buf_stale(bp);Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
xfs_buf_ioend(bp, 0);}Now,
xfs_buf_stale()does this:ASSERT(atomic_read(&bp->b_hold)>= 1);Which means that in calling
xfs_buf_ioend(), at least two references to the buffer are being dropped. Working out why that is occurring will find the root cause of this problem.All that I can say at this point is that I find it highly unlikely that it is caused by the changes in this patchset.
Quoted 6 lines
I got this ASSERT when I ran it on the 8/27 OSS sources:
[188646.952426] XFS: Assertion failed:
atomic_read(&iclog->ic_refcnt)== 0, file:
/root/xfs/fs/xfs/xfs_log.c, line: 2590Quoted 9 lines
[188646.967020] Process kworker/2:1H (pid: 356, threadinfo ffff8808396a4000, task ffff88083a9aa1c0) [188646.967020] Call Trace: [188646.967020] [<ffffffffa01dd2bf>] xlog_state_done_syncing+0x7f/0x110 [xfs] [188646.967020] [<ffffffffa01ddbde>] xlog_iodone+0x7e/0x100 [xfs] [188646.967020] [<ffffffffa0179b51>] xfs_buf_iodone_work+0x21/0x50 [xfs] [188646.967020] [<ffffffff8105d6b3>] process_one_work+0x1d3/0x370 [188646.967020] [<ffffffff810603e3>] worker_thread+0x133/0x390 [188646.967020] [<ffffffff810651ce>] kthread+0x9e/0xb0 [188646.967020] [<ffffffff8143e504>] kernel_thread_helper+0x4/0x10I've never seen that ASSERT fire. That implies we've got a log buffer that is being actively modified under IO, but I cannot see how that would happen. Was this during an unmount? What test?
Quoted 5 lines
/me is starting to wonder about memory errors...
Cheers,
Dave.
all panic on xfstest 179 - 3 different machines: 2 are x86_64 and one is x86_32. I believe all have XFS debug turned.
I will see what else I can find out.
--Mark.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Any chance to get a repost of this so that we can get it into 3.7?
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 1 line · Christoph Hellwig
Any chance to get a repost of this so that we can get it into 3.7?
That's tomorrow's job.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs