xfs: remove the xfssyncd mess
v4 cover xfs: remove the xfssyncd mess
Hi Dave,
Here I am reposting your xfssyncd series. I want to make sure that we're all on the same page. In particular, are we all happy with patch 6, 'xfs: xfs_sync_data is redundant'?
Version 4:
- updated 'xfs: xfs_sync_data is redundant' with cleanups to the xfs_flush_inodes interface as per Christoph's request,
- updated 'xfs: xfs_sync_data is redundant', folding in changes from
http://oss.sgi.com/archives/xfs/2012-10/msg00036.html
- fixed a minor typo in xfs: 'syncd workqueue is no more', renaming the log worker from 'xfs-reclaim' to 'xfs-log'.
I was going to rush this in for the 3.7 merge window. However in the light of the issues with patch 6 and Linus's comment here:
http://lkml.org/lkml/2012/9/30/152 and Stephen's comment here:
https://lkml.org/lkml/2012/9/23/144, I think it wiser to behave. 3.7 is stable without this series, so I will merge it for 3.8.
Once we have an agreement that patch 6 is ready I will pull this in to the master branch first thing after the 3.7-rc1 merge from upstream.
Regards,
Ben
Version 3 of the patchset I described here:
http://oss.sgi.com/archives/xfs/2012-06/msg00064.html
This patchset reliably exposed the log->l_last_sync_lsn problem I just posted a fix for.
Version 3:
- per-mount log and reclaim workqueues instead of a generic mount workqueue.
- reordering of some of the patches as Christoph requested.
- fixed the writeback_inodes_sb_if_idle deadlock by moving it all the way back up the write stack to xfs_file_aio_buffered_write where we were just flushing the current file to avoid deadlocking on it anyway.
- reintroduced xfs_flush_inodes() as a wrapper for writeback_inodes_sb_if_idle().
- rebased on a current TOT.
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 v4 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
@@ -921,7 +921,11 @@ xfs_fs_put_super(xfs_filestream_unmount (mp);cancel_delayed_work_sync (&mp->m_sync_work);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);
@@ -1291,9 +1295,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)
@@ -1316,8 +1322,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);
@@ -1336,7 +1344,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 · 12 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH v4 2/13 xfs: rationalise xfs_mount_wq users
In reply to Ben MyersFrom: 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 | 34 ++++++++++++++-------------------- fs/xfs/xfs_sync.c | 21 +++++---------------- 3 files changed, 23 insertions(+), 38 deletions(-)
fs/xfs/xfs_mount.c +4 -2
@@ -1483,9 +1483,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 +14 -20
@@ -918,14 +918,12 @@ xfs_fs_put_super( {struct xfs_mount *mp =XFS_M (sb); -xfs_filestream_unmount (mp); -cancel_delayed_work_sync (&mp->m_sync_work); -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);
@@ -1232,6 +1230,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;
@@ -1295,15 +1296,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_syncd_queue_sync (mp); - error =xfs_mountfs (mp);if (error) - goto out_syncd_stop; + goto out_filestream_unmount; root =igrab (VFS_I (mp->m_rootip));if (!root) {
@@ -1320,12 +1315,15 @@ xfs_fs_fill_super( goto out_unmount; } +/* +* The filesystem is successfully mounted, so we can start background +* sync work now. +*/ +xfs_syncd_queue_sync (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);
@@ -1344,10 +1342,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
@@ -379,9 +379,9 @@ xfs_syncd_queue_sync( }/* -* 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(
@@ -391,17 +391,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))
@@ -409,8 +399,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 · 12 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH v4 3/13 xfs: dont run the sync work if the filesystem is
In reply to Ben MyersFrom: Dave Chinner <[email protected]>
If the filesystem 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 | 29 ++++++++++++++++------------- 2 files changed, 18 insertions(+), 13 deletions(-)
fs/xfs/xfs_super.c +2 -0
@@ -1107,6 +1107,7 @@ xfs_fs_remount(* value if it is non-zero, otherwise go with the default. */ xfs_restore_resvblks (mp); +xfs_syncd_queue_sync (mp); }/* rw -> ro */
@@ -1152,6 +1153,7 @@ xfs_fs_unfreeze(struct xfs_mount *mp =XFS_M (sb);xfs_restore_resvblks (mp); +xfs_syncd_queue_sync (mp);return 0 ; }
fs/xfs/xfs_sync.c +16 -13
@@ -323,6 +323,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(
@@ -341,6 +344,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.
@@ -379,9 +385,8 @@ xfs_syncd_queue_sync( }/* -* 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. +* Every sync period we need to push dirty metadata and try to cover the log +* to indicate the filesystem is idle and not frozen. */ void xfs_sync_worker(
@@ -391,17 +396,15 @@ xfs_sync_worker( 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 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 ); -/* 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_syncd_queue_sync (mp);
Signature · 12 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH v4 4/13 xfs: sync work is now only periodic log work
In reply to Ben MyersFrom: 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 | 39 +++------------------------------------ fs/xfs/xfs_sync.h | 2 -- 7 files changed, 51 insertions(+), 52 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_syncd_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
@@ -197,7 +197,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);
@@ -953,10 +952,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 ;
@@ -1107,7 +1106,7 @@ xfs_fs_remount(* value if it is non-zero, otherwise go with the default. */ xfs_restore_resvblks (mp); -xfs_syncd_queue_sync (mp); +xfs_log_work_queue (mp); }/* rw -> ro */
@@ -1153,7 +1152,7 @@ xfs_fs_unfreeze(struct xfs_mount *mp =XFS_M (sb);xfs_restore_resvblks (mp); -xfs_syncd_queue_sync (mp); +xfs_log_work_queue (mp);return 0 ; }
@@ -1233,7 +1232,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;
@@ -1317,12 +1315,6 @@ xfs_fs_fill_super( goto out_unmount; } -/* -* The filesystem is successfully mounted, so we can start background -* sync work now. -*/ -xfs_syncd_queue_sync (mp); -return 0 ; out_filestream_unmount:
fs/xfs/xfs_sync.c +3 -36
@@ -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"
@@ -344,8 +345,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
@@ -376,40 +377,6 @@ xfs_quiesce_attr(xfs_buf_unlock (mp->m_sb_bp); } -void -xfs_syncd_queue_sync ( -struct xfs_mount *mp) -{ -queue_delayed_work (xfs_syncd_wq, &mp->m_sync_work, -msecs_to_jiffies (xfs_syncd_centisecs *10 )); -} - -/* -* Every sync period we need to push dirty metadata and try to cover the log -* to indicate the filesystem is idle and not frozen. -*/ -void -xfs_sync_worker ( -struct work_struct *work) -{ -struct xfs_mount *mp =container_of (to_delayed_work (work), - struct xfs_mount, m_sync_work); -int error; - -/* 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 ); - -/* start pushing all the metadata that is currently dirty */ -xfs_ail_push_all (mp->m_ail); - -/* queue us up again */ -xfs_syncd_queue_sync (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
@@ -26,8 +26,6 @@ struct xfs_perag;extern struct workqueue_struct *xfs_syncd_wq;/* sync workqueue */ -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);
Signature · 12 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [patch v4 04/13] [PATCH 04/13] xfs: sync work is now only periodic log work
fs/xfs/xfs_log.c xfs_log_mount_finish(xfs_mount_t *mp) · line 704
Quoted 6 lines · Ben Myers
fs/xfs/xfs_log.c +2
-0ASSERT (mp->m_flags &XFS_MOUNT_RDONLY ); } +xfs_log_work_queue (mp); +return error;
I still think queueing the work item here if we return a failure is the wrong thing to do.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [patch v4 04/13] [PATCH 04/13] xfs: sync work is now only periodic log work
Quoted 11 lines · Christoph Hellwig
fs/xfs/xfs_log.c xfs_log_mount_finish(xfs_mount_t *mp) · line 704
Quoted 6 lines · Ben Myers
ASSERT(mp->m_flags& XFS_MOUNT_RDONLY); }fs/xfs/xfs_log.c +2
-0+xfs_log_work_queue (mp); +return error;I still think queueing the work item here if we return a failure is the wrong thing to do.
Quoted 4 lines
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
You are correct. I did not see that it was not moved when mentioned in the first series.
--Mark.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v4 5/13 xfs: Bring some sanity to log unmounting
In reply to Ben MyersFrom: 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
@@ -1493,13 +1493,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
@@ -1524,23 +1517,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 · 12 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH v4 6/13 xfs: xfs_sync_data is redundant.
In reply to Ben MyersFrom: 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 could 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(). However, writeback_inodes_sb_if_idle() is not sufficient to trigger delalloc conversion fast enough to prevent spurious ENOSPC when there are hundreds of writers, thousands of small files and GBs of free RAM. Use sync_sb_inodes() instead to block callers while we wait for writeback.
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.
This does have one complication - it cannot be called with page locks held. Hence move the flushing of delalloc space when ENOSPC occurs back up into xfs_file_aio_buffered_write when we don't hold any locks that will stall writeback.
Note that we always need to pass a count of zero to generic_file_buffered_write() as the previously written byte count. We only do this by accident before this patch by the virtue of ret always being zero when there are no errors. Make this explicit rather than needing to specifically zero ret in the ENOSPC retry case.
---
v2: cleaned up the xfs_flush_inodes interface as per Christoph's request. -bpm
v3: updated the xfs_flush_inode implementation with the patch from
http://oss.sgi.com/archives/xfs/2012-10/msg00036.html. Folded the commit header comments into this commit header and into the comment above xfs_flush_inode. -bpm
fs/xfs/xfs_file.c | 13 ++++---- fs/xfs/xfs_iomap.c | 23 ++++---------- fs/xfs/xfs_mount.h | 22 +++++++++++++- fs/xfs/xfs_super.c | 3 - fs/xfs/xfs_sync.c | 78 -------------------------------------------------- fs/xfs/xfs_sync.h | 3 - fs/xfs/xfs_vnodeops.c | 2 - 7 files changed, 36 insertions(+), 108 deletions(-)
fs/xfs/xfs_file.c +7 -6
@@ -728,16 +728,17 @@ xfs_file_buffered_aio_write( write_retry:trace_xfs_file_buffered_write (ip, count, iocb->ki_pos,0 ); ret =generic_file_buffered_write (iocb, iovp, nr_segs, - pos, &iocb->ki_pos, count, ret); + pos, &iocb->ki_pos, count,0 ); +/* -* if we just got an ENOSPC, flush the inode now we aren't holding any -* page locks and retry *once* +* If we just got an ENOSPC, try to write back all dirty inodes to +* convert delalloc space to free up some of the excess reserved +* metadata space. */ if (ret == -ENOSPC && !enospc) { enospc =1 ; - ret = -xfs_flush_pages (ip,0 ,-1 ,0 ,FI_NONE ); -if (!ret) - goto write_retry; +xfs_flush_inodes (ip->i_mount); + goto write_retry; } current->backing_dev_info =NULL ;
fs/xfs/xfs_iomap.c +7 -16
@@ -373,7 +373,7 @@ xfs_iomap_write_delay(xfs_extlen_t extsz;int nimaps;xfs_bmbt_irec_t imap[XFS_WRITE_IMAPS ]; -int prealloc, flushed =0 ; +int prealloc;int error;ASSERT (xfs_isilocked (ip,XFS_ILOCK_EXCL ));
@@ -434,26 +434,17 @@ retry: }/* -* If bmapi returned us nothing, we got either ENOSPC or EDQUOT. For -* ENOSPC, * flush all other inodes with delalloc blocks to free up -* some of the excess reserved metadata space. For both cases, retry +* If bmapi returned us nothing, we got either ENOSPC or EDQUOT. Retry * without EOF preallocation. */ if (nimaps ==0 ) {trace_xfs_delalloc_enospc (ip, offset, count); -if (flushed) -return XFS_ERROR (error ? error :ENOSPC ); - -if (error ==ENOSPC ) { -xfs_iunlock (ip,XFS_ILOCK_EXCL ); -xfs_flush_inodes (ip); -xfs_ilock (ip,XFS_ILOCK_EXCL ); +if (prealloc) { + prealloc =0 ; + error =0 ; + goto retry; } - - flushed =1 ; - error =0 ; - prealloc =0 ; - goto retry; +return XFS_ERROR (error ? error :ENOSPC ); }if (!(imap[0 ].br_startblock ||XFS_IS_REALTIME_INODE (ip)))
fs/xfs/xfs_mount.h +21 -1
@@ -198,7 +198,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 */
@@ -381,6 +380,27 @@ extern int xfs_dev_is_read_only(struct xextern void xfs_set_low_space_thresholds (struct xfs_mount *); +/* +* Flush all dirty data to disk. Must not be called while holding an XFS_ILOCK +* or a page lock. +* +* We have to hold the s_umount lock here, but because this call can nest +* inside i_mutex (the parent directory in the create case, held by the VFS), +* we have to use down_read_trylock() to avoid potential deadlocks. In +* practice, this trylock will succeed on almost every attempt as +* unmount/remount type operations are exceedingly rare. +*/ +static inline void +xfs_flush_inodes (struct xfs_mount *mp) +{ +struct super_block *sb = mp->m_super; + +if (down_read_trylock (&sb->s_umount)) { +sync_inodes_sb (sb); +up_read (&sb->s_umount); + } +} +#endif /* __KERNEL__ */ extern void xfs_mod_sb (struct xfs_trans *,__int64_t );
fs/xfs/xfs_super.c +0 -3
@@ -1005,8 +1005,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);
@@ -1324,7 +1322,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
@@ -26,14 +26,11 @@ struct xfs_perag;extern struct workqueue_struct *xfs_syncd_wq;/* sync workqueue */ -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 +1 -1
@@ -777,7 +777,7 @@ xfs_create( XFS_TRANS_PERM_LOG_RES, log_count);if (error ==ENOSPC ) {/* flush outstanding delalloc blocks and retry */ -xfs_flush_inodes (dp); +xfs_flush_inodes (dp->i_mount); error =xfs_trans_reserve (tp, resblks, log_res,0 ,XFS_TRANS_PERM_LOG_RES , log_count); }
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [patch v4 06/13] [PATCH 06/13] xfs: xfs_sync_data is redundant.
Quoted 27 lines · Ben Myers
Index:
xfs/fs/xfs/xfs_mount.h
===================================================================fs/xfs/xfs_mount.h +11
-1@@ -198,7 +198,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 */ @@ -381,6 +380,27 @@ extern int xfs_dev_is_read_only(struct xextern void xfs_set_low_space_thresholds (struct xfs_mount *); +/* +* Flush all dirty data to disk. Must not be called while holding an XFS_ILOCK +* or a page lock. +* +* We have to hold the s_umount lock here, but because this call can nest +* inside i_mutex (the parent directory in the create case, held by the VFS), +* we have to use down_read_trylock() to avoid potential deadlocks. In +* practice, this trylock will succeed on almost every attempt as +* unmount/remount type operations are exceedingly rare. +*/ +static inline void
fs/xfs/xfs_mount.h extern int xfs_dev_is_read_only(struct x · line 403
Quoted 10 lines
fs/xfs/xfs_mount.h +10
-0+xfs_flush_inodes(struct xfs_mount *mp) +{ +struct super_block *sb = mp->m_super; + +if (down_read_trylock (&sb->s_umount)) { +sync_inodes_sb (sb); +up_read (&sb->s_umount); + } +} +
Was this suppose to be in xfs_inode.h? Otherwise....
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [patch v4 06/13] [PATCH 06/13] xfs: xfs_sync_data is redundant.
Hey Mark,
Quoted 33 lines · Mark Tinguely
Quoted 32 lines · Ben Myers
Index:
xfs/fs/xfs/xfs_mount.h
===================================================================fs/xfs/xfs_mount.h +16
-1@@ -198,7 +198,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 */ @@ -381,6 +380,27 @@ extern int xfs_dev_is_read_only(struct xextern void xfs_set_low_space_thresholds (struct xfs_mount *); +/* +* Flush all dirty data to disk. Must not be called while holding an XFS_ILOCK +* or a page lock. +* +* We have to hold the s_umount lock here, but because this call can nest +* inside i_mutex (the parent directory in the create case, held by the VFS), +* we have to use down_read_trylock() to avoid potential deadlocks. In +* practice, this trylock will succeed on almost every attempt as +* unmount/remount type operations are exceedingly rare. +*/ +static inline void +xfs_flush_inodes (struct xfs_mount *mp) +{ +struct super_block *sb = mp->m_super; + +if (down_read_trylock (&sb->s_umount)) {
Quoted 8 lines
fs/xfs/xfs_mount.h extern int xfs_dev_is_read_only(struct x · line 403
Quoted 5 lines
fs/xfs/xfs_mount.h +5
-0+sync_inodes_sb (sb); +up_read (&sb->s_umount); + } +} +Was this suppose to be in
xfs_inode.h? Otherwise....
Christoph suggested that xfs_flush_inodes should take an xfs_mount instead of an inode. It is operating on the super_block level to flush multiple inodes rather than the single inode whose pointer was passed in, so I moved it from xfs_inode.h to xfs_mount.h.
Regards,
Ben
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [patch v4 06/13] [PATCH 06/13] xfs: xfs_sync_data is redundant.
Quoted 4 lines · Ben Myers
Christoph suggested that
xfs_flush_inodesshould take anxfs_mountinstead of an inode. It is operating on thesuper_blocklevel to flush multiple inodes rather than the single inode whose pointer was passed in, so I moved it fromxfs_inode.htoxfs_mount.h.
It's more than a trivial wrapper now, so I'd suggest to move out of line to e.g. xfs_super.c
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [patch v4 06/13] [PATCH 06/13] xfs: xfs_sync_data is redundant.
In reply to Ben MyersQuoted 45 lines · Ben Myers
Hey Mark,
Quoted 33 lines · Mark Tinguely
Quoted 32 lines · Ben Myers
Index:
xfs/fs/xfs/xfs_mount.h
===================================================================fs/xfs/xfs_mount.h +16
-1@@ -198,7 +198,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 */ @@ -381,6 +380,27 @@ extern int xfs_dev_is_read_only(struct xextern void xfs_set_low_space_thresholds (struct xfs_mount *); +/* +* Flush all dirty data to disk. Must not be called while holding an XFS_ILOCK +* or a page lock. +* +* We have to hold the s_umount lock here, but because this call can nest +* inside i_mutex (the parent directory in the create case, held by the VFS), +* we have to use down_read_trylock() to avoid potential deadlocks. In +* practice, this trylock will succeed on almost every attempt as +* unmount/remount type operations are exceedingly rare. +*/ +static inline void +xfs_flush_inodes (struct xfs_mount *mp) +{ +struct super_block *sb = mp->m_super; + +if (down_read_trylock (&sb->s_umount)) {Quoted 8 lines
fs/xfs/xfs_mount.h extern int xfs_dev_is_read_only(struct x · line 403
Quoted 5 lines
fs/xfs/xfs_mount.h +5
-0+sync_inodes_sb (sb); +up_read (&sb->s_umount); + } +} +Was this suppose to be in
xfs_inode.h? Otherwise....
Quoted 7 lines
Christoph suggested that
xfs_flush_inodesshould take anxfs_mountinstead of an inode. It is operating on thesuper_blocklevel to flush multiple inodes rather than the single inode whose pointer was passed in, so I moved it fromxfs_inode.htoxfs_mount.h.Regards,
Ben
You are right - that is where it belongs with the inode->mount parameter change. Sorry for the noise.
--Mark.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v4 7/13 xfs: syncd workqueue is no more
In reply to Ben MyersFrom: Dave Chinner <[email protected]>
With the syncd functions moved to the log and/or removed, the syncd workqueue is the only remaining bit left. It is used by the log covering/ail pushing work, as well as by the inode reclaim work.
Given how cheap workqueues are these days, give the log and inode reclaim work their own work queues and kill the syncd work queue.
---
v2: changed the name of the log_workqueue from 'xfs-reclaim' to 'xfs-log' -bpm
fs/xfs/xfs_log.c | 2 +- fs/xfs/xfs_mount.h | 2 ++ fs/xfs/xfs_super.c | 38 ++++++++++++++++++-------------------- fs/xfs/xfs_sync.c | 20 +++++++++----------- fs/xfs/xfs_sync.h | 2 -- 5 files changed, 30 insertions(+), 34 deletions(-)
fs/xfs/xfs_log.c +1 -1
@@ -1191,7 +1191,7 @@ voidxfs_log_work_queue ( struct xfs_mount *mp) { -queue_delayed_work (xfs_syncd_wq, &mp->m_log->l_work, +queue_delayed_work (mp->m_log_workqueue, &mp->m_log->l_work,msecs_to_jiffies (xfs_syncd_centisecs *10 )); }
fs/xfs/xfs_mount.h +2 -0
@@ -207,6 +207,8 @@ typedef struct xfs_mount {struct workqueue_struct *m_data_workqueue;struct workqueue_struct *m_unwritten_workqueue;struct workqueue_struct *m_cil_workqueue; +struct workqueue_struct *m_reclaim_workqueue; +struct workqueue_struct *m_log_workqueue; } xfs_mount_t;/*
fs/xfs/xfs_super.c +18 -20
@@ -776,8 +776,23 @@ xfs_init_mount_workqueues( WQ_MEM_RECLAIM, 0, mp->m_fsname);if (!mp->m_cil_workqueue) goto out_destroy_unwritten; + + mp->m_reclaim_workqueue =alloc_workqueue ("xfs-reclaim/%s" , +WQ_NON_REENTRANT ,0 , mp->m_fsname); +if (!mp->m_reclaim_workqueue) + goto out_destroy_cil; + + mp->m_log_workqueue =alloc_workqueue ("xfs-reclaim/%s" , +WQ_NON_REENTRANT ,0 , mp->m_fsname); +if (!mp->m_log_workqueue) + goto out_destroy_reclaim; +return 0 ; +out_destroy_reclaim: +destroy_workqueue (mp->m_reclaim_workqueue); +out_destroy_cil: +destroy_workqueue (mp->m_cil_workqueue); out_destroy_unwritten:destroy_workqueue (mp->m_unwritten_workqueue); out_destroy_data_iodone_queue:
@@ -790,6 +805,8 @@ STATIC voidxfs_destroy_mount_workqueues ( struct xfs_mount *mp) { +destroy_workqueue (mp->m_log_workqueue); +destroy_workqueue (mp->m_reclaim_workqueue);destroy_workqueue (mp->m_cil_workqueue);destroy_workqueue (mp->m_data_workqueue);destroy_workqueue (mp->m_unwritten_workqueue);
@@ -1280,10 +1297,6 @@ xfs_fs_fill_super(/* * we must configure the block size in the superblock before we run the * full mount process as the mount process can lookup and cache inodes. -* For the same reason we must also initialise the syncd and register -* the inode cache shrinker so that inodes can be reclaimed during -* operations like a quotacheck that iterate all inodes in the -* filesystem. */ sb->s_magic =XFS_SB_MAGIC ; sb->s_blocksize = mp->m_sb.sb_blocksize;
@@ -1523,16 +1536,6 @@ STATIC int __initxfs_init_workqueues (void) {/* -* We never want to the same work item to run twice, reclaiming inodes -* or idling the log is not going to get any faster by multiple CPUs -* 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) -return -ENOMEM ; - -/* * The allocation workqueue can be used in memory reclaim situations * (writepage path), and parallelism is only limited by the number of * AGs in all the filesystems mounted. Hence use the default large
@@ -1540,20 +1543,15 @@ xfs_init_workqueues(void)*/ xfs_alloc_wq =alloc_workqueue ("xfsalloc" ,WQ_MEM_RECLAIM ,0 );if (!xfs_alloc_wq) - goto out_destroy_syncd; +return -ENOMEM ;return 0 ; - -out_destroy_syncd: -destroy_workqueue (xfs_syncd_wq); -return -ENOMEM ; } STATIC voidxfs_destroy_workqueues (void ) {destroy_workqueue (xfs_alloc_wq); -destroy_workqueue (xfs_syncd_wq); }STATIC int__init
fs/xfs/xfs_sync.c +9 -11
@@ -40,8 +40,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 * radix tree lookups to a minimum. The batch size is a trade off between
@@ -335,18 +333,18 @@ xfs_quiesce_attr(/* * 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 -* on the xfs syncd work default of 30s. Perhaps this should have it's own +* on the xfs periodic sync default of 30s. Perhaps this should have it's own * tunable, but that can be done if this method proves to be ineffective or too * aggressive. */ static void -xfs_syncd_queue_reclaim ( +xfs_reclaim_work_queue (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 (mp->m_reclaim_workqueue, &mp->m_reclaim_work,msecs_to_jiffies (xfs_syncd_centisecs /6 *10 )); }rcu_read_unlock ();
@@ -367,7 +365,7 @@ xfs_reclaim_worker( struct xfs_mount, m_reclaim_work);xfs_reclaim_inodes (mp,SYNC_TRYLOCK ); -xfs_syncd_queue_reclaim (mp); +xfs_reclaim_work_queue (mp); }void
@@ -388,7 +386,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_work_queue (ip->i_mount);trace_xfs_perag_set_reclaim (ip->i_mount, pag->pag_agno,-1 , _RET_IP_);
@@ -646,9 +644,9 @@ out:/* * We could return EAGAIN here to make reclaim rescan the inode tree in * a short while. However, this just burns CPU time scanning the tree -* waiting for IO to complete and xfssyncd never goes back to the idle -* state. Instead, return 0 to let the next scheduled background reclaim -* attempt to reclaim the inode again. +* waiting for IO to complete and the reclaim work never goes back to +* the idle state. Instead, return 0 to let the next scheduled +* background reclaim attempt to reclaim the inode again. */ return 0 ; }
@@ -804,7 +802,7 @@ xfs_reclaim_inodes_nr( int nr_to_scan) {/* kick background reclaimer and push the AIL */ -xfs_syncd_queue_reclaim (mp); +xfs_reclaim_work_queue (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 +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 */ -extern struct workqueue_struct *xfs_syncd_wq;/* sync workqueue */ -void xfs_reclaim_worker (struct work_struct *work);int xfs_quiesce_data (struct xfs_mount *mp);
Signature · 12 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [patch v4 07/13] [PATCH 07/13] xfs: syncd workqueue is no more
Quoted 11 lines · Ben Myers
From: Dave Chinner<[email protected]>
With the syncd functions moved to the log and/or removed, the syncd workqueue is the only remaining bit left. It is used by the log covering/ail pushing work, as well as by the inode reclaim work.
Given how cheap workqueues are these days, give the log and inode reclaim work their own work queues and kill the syncd work queue.
Signed-off-by Dave Chinner<[email protected]>---
Looks good.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v4 8/13 xfs: xfs_sync_fsdata is redundant
In reply to Ben MyersFrom: 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
@@ -952,7 +952,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.
@@ -960,10 +959,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.
@@ -1127,15 +1123,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 · 12 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH v4 9/13 xfs: move xfs_quiesce_attr() into xfs_super.c
In reply to Ben MyersFrom: 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
@@ -1043,6 +1043,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 · 12 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH v4 10/13 xfs: xfs_quiesce_attr() should quiesce the log like
In reply to Ben MyersFrom: 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 · 12 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH v4 11/13 xfs: rename xfs_sync.[ch] to xfs_icache.[ch]
In reply to Ben MyersFrom: 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 | 715 ++++++++++++++++++++++++++++++++++++++++++++++ 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 | 714 --------------------------------------------- fs/xfs/xfs_sync.h | 43 --- 10 files changed, 763 insertions(+), 761 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 +715 -0
715 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" #ifdef HAVE_PERCPU_SB
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 -714
714 lines not on this page · raw message
fs/xfs/xfs_sync.h deleted +0 -43
43 lines not on this page · raw message
Signature · 12 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH v4 12/13 xfs: move inode locking functions to xfs_inode.c
In reply to Ben MyersFrom: 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 · 12 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH v4 13/13 xfs: remove xfs_iget.c
In reply to Ben MyersFrom: 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
@@ -506,11 +506,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 );
@@ -518,11 +517,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 · 12 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 14 lines · Ben Myers
Hi Dave,
Here I am reposting your xfssyncd series. I want to make sure that we're all on the same page. In particular, are we all happy with patch 6, 'xfs:
xfs_sync_datais redundant'?Version 4:
- updated 'xfs:xfs_sync_datais redundant' with cleanups to thexfs_flush_inodesinterface as per Christoph's request,
- updated 'xfs:xfs_sync_datais redundant', folding in changes from
http://oss.sgi.com/archives/xfs/2012-10/msg00036.html
- fixed a minor typo in xfs: 'syncd workqueue is no more', renaming the log worker from 'xfs-reclaim' to 'xfs-log'.
Quoted 8 lines
I was going to rush this in for the 3.7 merge window. However in the light of the issues with patch 6 and Linus's comment here:
http://lkml.org/lkml/2012/9/30/152 and Stephen's comment here:
https://lkml.org/lkml/2012/9/23/144, I think it wiser to behave. 3.7 is stable without this series, so I will merge it for 3.8.Once we have an agreement that patch 6 is ready I will pull this in to the master branch first thing after the 3.7-rc1 merge from upstream.
<sigh>
Seriously? Why am I only finding out that there needs to be more rework to patches in this series after someone else reposted them? And that this is the cause of it missing the merge window?
So, a week ago on IRC I said to you (Ben):
[27/09/12 09:42] <dchinner_> I'm not really surprised you missed me
[27/09/12 09:42] <dchinner_> in fact, that's what I wanted to talk about
[27/09/12 09:43] <dchinner_> I'm getting really frustrated by the 24-hour turnaround time for any sort of discussion with you and mark
[27/09/12 09:44] <dchinner_> You and mark only seem to respond to the mailing list in Eagan business hours (roughly 10am-4:30pm according to the time stamps on your emails) [27/09/12 09:44] <dchinner_> and that corresponds pretty closely with the 7-8 hours I sleep every night.
[27/09/12 09:45] <dchinner_> I tend to check and respond to mailing list traffic over a 15-16 window each day, but even doing that I get zero overlap with your online times. [27/09/12 09:45] <dchinner_> So discussions that should take an hour or 2 and be solved take a week or more
[27/09/12 09:49] <dchinner_> I can't lengthen my day any more than it already is to try to get some overlap with you and Mark....
[27/09/12 11:16] * dchinner_ suspects he's going to be waiting until tomorrow to get a response....
If I was frustrated by this communication problem a week ago....
If you simply said that there's more than just folding a fix into the series, I could have done all the changes and had it turned around in about 2 hours (modification, build and test cycle time), and all these issues could have been shaken out several days ago. If the first I hear that there is significant rework needed to one of my patchsets is someone else reposting it with changes that need discussion in it, then there's a serious comminucation breakdown occurring.
Seriously, if there's more than one less-than-trivial problem with a patch set, through it back to the owner of the patchset to fix. The maintainers job is to communicate and co-ordinate, not fix other people's patch sets for them. It just doesn't scale - push problems back to the patch owner to deal with.
FWIW, I'll quote from another patchset description I posted yesterday (allocation workqueue deadlock fixes) to point out that I'm not just commenting on this patch set:
"This is a followup from the last conversation with Mark about this deadlock. I haven't heard anything in the last couple of days, so I figured I'd just write the patches that did what I thought is needed."
The discussion of that patch series made 4-5 round trips between Mark and myself over a period of more than 2 weeks, with me always waiting on answers from Mark so that I could understand the problem he was seeing and trying to fix. Realistically, it should have taken 2-3 days at most. And then when I don't get a response for 2-3 days in the middle of a working week, I'm left to wonder WTF is going on. I end up just doing the change instead of letting it get dropped on the ground, without knowing whether I'm duplicating work that is already being done by someone else.
As I said, I can't do any more than I already do to try to get lower latency and more frequent communications, so there's little I can do to reduce the amount of frustration I'm having. So I'm reduced to making more public noise about the problem in the hope that we can come up with a solution....
Not a happy camper.
-Dave
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Hi Dave,
Quoted 21 lines · Dave Chinner
Quoted 20 lines · Ben Myers
Hi Dave,
Here I am reposting your xfssyncd series. I want to make sure that we're all on the same page. In particular, are we all happy with patch 6, 'xfs:
xfs_sync_datais redundant'?Version 4:
- updated 'xfs:xfs_sync_datais redundant' with cleanups to thexfs_flush_inodesinterface as per Christoph's request,
- updated 'xfs:xfs_sync_datais redundant', folding in changes from
http://oss.sgi.com/archives/xfs/2012-10/msg00036.html
- fixed a minor typo in xfs: 'syncd workqueue is no more', renaming the log worker from 'xfs-reclaim' to 'xfs-log'.I was going to rush this in for the 3.7 merge window. However in the light of the issues with patch 6 and Linus's comment here:
http://lkml.org/lkml/2012/9/30/152 and Stephen's comment here:
https://lkml.org/lkml/2012/9/23/144, I think it wiser to behave. 3.7 is stable without this series, so I will merge it for 3.8.
Quoted 6 lines
Quoted 2 lines
Once we have an agreement that patch 6 is ready I will pull this in to the master branch first thing after the 3.7-rc1 merge from upstream.
<sigh>
Seriously?
Take it as good news. Brian found the regression and put the brakes on before I pulled it in. My regret in this is that SGI didn't find it first. We have a very stable 3.7 release, one less regression, and a full release cycle to test this series in the master branch. That is a pretty good outcome for XFS users.
Quoted 2 lines
Why am I only finding out that there needs to be more rework to patches in this series after someone else reposted them?
You aren't. There are currently two pending suggestions for the series, and apparently one has been around for awhile:
1) 'xfs: sync work is now only periodic log work'
HCH: "I still think queueing the work item here if we return a failure is the wrong thing to do."
This issue was mentioned before I reposted your series:
http://oss.sgi.com/archives/xfs/2012-09/msg00046.html From Mark on Sep 4
http://oss.sgi.com/archives/xfs/2012-09/msg00465.html From HCH on Sep 28
That is just a missed opportunity after the initial suggestion from Mark. It happens. I had planned to pull it in anyway. That is probably not the best judgement on my part.
2) 'xfs: xfs_sync_data is redundant.'
HCH: (on xfs_flush_inodes) "It's more than a trivial wrapper now, so I'd suggest to move out of line to e.g. xfs_super.c"
This one is my fault. I might have considered moving xfs_flush_inodes out of xfs_mount.h when I folded your other patch in. I hope you don't mind fixing that up.
Quoted 1 line
And that this is the cause of it missing the merge window?
The regression is the only reason this missed the merge window. I was ready to push the series to -next when Brian pulled the cord.
According Linus and Stephens comments we should have content in -next by -rc7. Clearly there is some flex in that system but I have no desire to find its limit, and I think that this experience proves their point. Lesson learned! I will be more conservative regarding the merge window in the future.
I'm giving this series some soak time with the new fix. I can't pull it in with the new fix for the regression until we have some testing. Once we have some confidence in it I will push it up to oss.
My understanding is that work for 3.8 should not be added to -next during the merge window for 3.7-rc1, so you have plenty of time to address Mark&Christoph's concern.
Thanks,
Ben
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Dave Chinner
Reviewed-by Mark TinguelyQuoted 24 lines · Ben Myers
Hi Dave,
Quoted 21 lines · Dave Chinner
Quoted 20 lines · Ben Myers
Hi Dave,
Here I am reposting your xfssyncd series. I want to make sure that we're all on the same page. In particular, are we all happy with patch 6, 'xfs:
xfs_sync_datais redundant'?Version 4:
- updated 'xfs:xfs_sync_datais redundant' with cleanups to thexfs_flush_inodesinterface as per Christoph's request,
- updated 'xfs:xfs_sync_datais redundant', folding in changes from
http://oss.sgi.com/archives/xfs/2012-10/msg00036.html
- fixed a minor typo in xfs: 'syncd workqueue is no more', renaming the log worker from 'xfs-reclaim' to 'xfs-log'.I was going to rush this in for the 3.7 merge window. However in the light of the issues with patch 6 and Linus's comment here:
http://lkml.org/lkml/2012/9/30/152 and Stephen's comment here:
https://lkml.org/lkml/2012/9/23/144, I think it wiser to behave. 3.7 is stable without this series, so I will merge it for 3.8.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
Once we have an agreement that patch 6 is ready I will pull this in to the master branch first thing after the 3.7-rc1 merge from upstream.
<sigh>
Seriously?
Take it as good news.
Not really.
Quoted 2 lines
Brian found the regression and put the brakes on before I pulled it in.
Right, and I posted an initial fix for it 4 hours after Brian reported it. That was 10am Tuesday, my time. Brian reported the warning overnight, and the version of the patch that fixed that I posted at 6:50am Wednesday morning. You asked about folding it at 10am Thursday morning, and that's the last I heard until Saturday morning after you reposted the entire series....
Indeed, the reason you wanted to fold it was this:
"I'd prefer to apply this to patch 6 itself rather than check in the regression followed by the fix."
Which really isn't something that should be a concern. regressions get checked in all the time, and fixes for them get checked in later all the time. Indeed, I think that having commits that explain regressions and how they were found and fixed is far more important to have in the revision history than to try to avoid them altogether. People learn more from mistakes and documenting (so that others can learn from them, too) than trying pretending they never occurred.
Quoted 3 lines
My regret in this is that SGI didn't find it first. We have a very stable 3.7 release, one less regression, and a full release cycle to test this series in the master branch. That is a pretty good outcome for XFS users.
Actually, it's not.
It now will be 5-6 months before the change gets to release and users will actually start testing it, rather than using the 2 months after the -rc1 merge to send small fixes to Linus to fix minor regressions. In 6 months time, I expect that the code base is going to be fully CRC enabled, and finding and fixing problems with stuff we committed now is going to be the least of our problem.
Upstream is supposed to be agile, fast and respond quickly to changes and challenges. If you want something that is "very stable", then that's what the long-term stable releases (like 3.0.x) and downstream kernel consumers like enterprise distros are for. Indeed, in software terms "very stable" is the equivalent of "did not change", and that is a fitting description of the 3.7 merge:
Release diffstat
v3.6..xfs-oss/master 7 files changed, 447 insertions(+), 80 deletions(-)
v3.5..v3.6 51 files changed, 2607 insertions(+), 2455 deletions(-)
v3.4..v3.5 81 files changed, 2633 insertions(+), 3004 deletions(-)
v3.3..v3.4 61 files changed, 1692 insertions(+), 2356 deletions(-)
v3.2..v3.3 45 files changed, 954 insertions(+), 2182 deletions(-)
v3.1..v3.2 54 files changed, 2414 insertions(+), 2625 deletions(-)
v3.0..v3.1 111 files changed, 3237 insertions(+), 5169 deletions(-)
An order of magnitude lower changes than the average over the past 6 releases. IOWs, "did not change" is a good description for this release.
Quoted 9 lines
Quoted 2 lines
Why am I only finding out that there needs to be more rework to patches in this series after someone else reposted them?
You aren't. There are currently two pending suggestions for the series, and apparently one has been around for awhile:
1) 'xfs: sync work is now only periodic log work'
HCH: "I still think queueing the work item here if we return a failure is the wrong thing to do."
Sure, but it also has:
on it. Which meant when I last went through the series, I saw this tag and didn't look any further. I figured that I'd already addressed the review comments. That's my mistake, but one you, as a maintainer, need to point out and ask me to address.
Quoted 6 lines
2) 'xfs:
xfs_sync_datais redundant.'
HCH: (onxfs_flush_inodes) "It's more than a trivial wrapper now, so I'd suggest to move out of line to e.g.xfs_super.c"This one is my fault. I might have considered moving
xfs_flush_inodesout ofxfs_mount.hwhen I folded your other patch in.
And this is precisely what I'm concerned about. The moment there's a "do I need to?" type of concern about folding two patches, then it should be punted straight back to the patch series owner with a relevant direction/question.
Quoted 1 line
From my perspective, I'd addressed the regression and everything was
good to go, but in this case silence for 3 days obviously doesn't mean that.
Quoted 1 line
I hope you don't mind fixing that up.
Of course I'll fix it. There's never been a question about that. My problem is that being told I need to fix it came too late....
Quoted 4 lines
Quoted 1 line
And that this is the cause of it missing the merge window?
The regression is the only reason this missed the merge window. I was ready to push the series to -next when Brian pulled the cord.
There's 2-3 months after the merge window to fix minor regressions like this. More below on this.
Quoted 4 lines
According Linus and Stephens comments we should have content in -next by -rc7. Clearly there is some flex in that system but I have no desire to find its limit, and I think that this experience proves their point. Lesson learned! I will be more conservative regarding the merge window in the future.
But that's not the issue I raised - the missing of the release window is a result of a more fundamental problem - the high communication latency that prolongs the review process.
And I think being *more conservative* is exactly the wrong way to fix the problem.
Quoted 3 lines
I'm giving this series some soak time with the new fix. I can't pull it in with the new fix for the regression until we have some testing. Once we have some confidence in it I will push it up to oss.
You are treating a development tree like it is a precious child that must be protected from the evil developers that might do something nasty to it. The development tree is there to benefit the developers, not the users. We can break the dev tree however we like as long as it is fixed before the code gets to users (i.e. final kernel release). There is nothing wrong with having the master branch of the xfs development tree having regressions in it. IOWs, keeping changes out of the dev tree because "they need soak time" is exactly the wrong thing to do. Changes should be "soaking" *in the dev tree*, not in some private black hole that nobody but people at SGI know about.
Indeed, the faster the changes get into the dev tree, the more widespread the testing of the changes is going to be. All my development use the dev tree as it's base, so unless I merge every patchset in the list into my current dev tree, I'm not testing those changes during development. I think the same goes for most other developers as well. The dev-tree is the tree we use and expect to find regressions in - that's exactly what a dev tree is for.
Further, holding the changes out of tree can be actively harmful for co-ordination of work between developers. e.g Brian's prealloc reclaim work is dependent on this patch set, and while it is held out of the tree for "soak" he has to privately manage the patches in his tree, and whenever he posts updates to his work has to reference the version of the patch set he has based his work on. If it was in the dev tree, he wouldn't have had to do this.
Hence, IMO, taking a "we can't commit a change until it has passed some [unknown test] criteria" approach to managing the dev tree is, at best, misguided and at worst is completely wrong. The dev tree should move as fast as possible, and regressions be fixed with commits just as quickly.
This doesn't change how quickly changes get published from the dev-tree to for-next or for-linus branches - that will still happen after soak testing occurs.
e.g. A typical release cycle might look like:
Merge window opens:
- move for-next -> for_linus, pull request
- move -dev -> for-next
At -rc4 (or earlier and repeatedly if for->next needs fixing):
- move -dev -> for-next
- freeze for-next
- start soak tests for next release
- merge regressions fixes from for-next -> for-linus, pull request
At -rc6/7 (final -rc):
- merge regression fixes from -dev -> for-next
- run final release tests, ready for merge window.
I don't know what your current plan is. It seems, from the outside, to be completely ad hoc and driven by the realisation that the merge window is approaching rather than being planned and consistent. Combine this will long communication latencies, and we have a recipe for work that was posted a couple of weeks before the merge window opened missing it because a minor regression wasn't sorted out fast enough.
If you were committing code the moment it had a reviewed-by tag on each patch in the series, then committing small patches to fix regressions and other late review changes, then problems with latency in commication are minimised. A regression fix with a "tested-by" tag on it can be committed immediately to the dev tree, and we move on. We find the problems faster, we fix them faster, and we don't miss merge windows going around in circles trying to work out who is or isn't doing something.
Quoted 3 lines
My understanding is that work for 3.8 should not be added to -next during the merge window for 3.7-rc1, so you have plenty of time to address Mark&Christoph's concern.
Case in point: you are comingling the release rules for the for-next branch with the commit rules for the development branch (master). We control the master branch completely, and it can move independently of the for-next branch. IOWs, There is no reason why you can't commit stuff to the master branch during a merge window because that branch is *completely independent* of the for-next/for-linus branches and release cycles.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs