Skip to contentSkip to conversation

xfs: remove the xfssyncd mess

PATCH Superseded Dave Chinner 5 participants 37 messages Sep 28, 2012 – Oct 4, 2012 XFS filesystem

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

v3 cover xfs: remove the xfssyncd mess

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

#1 lore raw

PATCH v3 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
index 19e2380..59d76bd 100644
@@ -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
index 9654817..13830e4 100644
@@ -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
index 941202e..3f59e5b 100644
@@ -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
#2 lore raw

PATCH v3 2/13 xfs: rationalise xfs_mount_wq users

In reply to Dave Chinner

From: 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
index 29c2f83..3152924 100644
@@ -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
index 59d76bd..935b2c7 100644
@@ -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
index 13830e4..509190f 100644
@@ -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 · 6 lines
1.7.10

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#4 lore raw

PATCH v3 3/13 xfs: don't run the sync work if the filesystem is read-only

In reply to Dave Chinner

From: 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
index 935b2c7..f6147ae 100644
@@ -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
index 509190f..5d69f23 100644
@@ -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 · 6 lines
1.7.10

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#6 lore raw

PATCH v3 4/13 xfs: sync work is now only periodic log work

In reply to 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.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
index 7f4f937..a1fe7a7 100644
@@ -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
index 748d312..26ed7de 100644
@@ -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
index 18a801d..9a4e0e5 100644
@@ -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
index deee09e..26e46ae 100644
@@ -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
index f6147ae..b85ca2d 100644
@@ -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
index 5d69f23..7527610 100644
@@ -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
index 3f59e5b..8d58fab 100644
@@ -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 · 6 lines
1.7.10

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#7 lore raw

Re: [PATCH 04/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.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.

You'll get an OK for me as I'd like this series in 3.7, but I'd really prefer if we could stop waking up xfsaild from here ASAP, and instead just do a schedule_timeout in xfsaild where we currently sleep forever.

Any chance to get a follow on patch for that?

fs/xfs/xfs_log.c xfs_log_mount_finish(xfs_mount_t *mp) · line 704

Quoted 11 lines
fs/xfs/xfs_log.c +3 -0
+#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;

I think this should only be called when xlog_recover_finish didn't return an error.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#8 lore raw

PATCH v3 5/13 xfs: Bring some sanity to log unmounting

In reply to 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.

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
index a1fe7a7..a400e71 100644
@@ -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
index 3152924..149b601 100644
@@ -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 · 6 lines
1.7.10

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#9 lore raw

PATCH v3 6/13 xfs: xfs_sync_data is redundant.

In reply to 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().

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.

---
 fs/xfs/xfs_file.c  |   13 +++++----
 fs/xfs/xfs_inode.h |   10 +++++++
 fs/xfs/xfs_iomap.c |   23 +++++-----------
 fs/xfs/xfs_mount.h |    1 -
 fs/xfs/xfs_super.c |    3 --
 fs/xfs/xfs_sync.c  |   78 ----------------------------------------------------
 fs/xfs/xfs_sync.h  |    3 --
 7 files changed, 24 insertions(+), 107 deletions(-)
fs/xfs/xfs_file.c +7 -6
index 1eaeb8b..2cadcf7 100644
@@ -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);
+		goto write_retry;
 	}
 
 	current->backing_dev_info = NULL;
fs/xfs/xfs_inode.h +10 -0
index 94b32f9..a12fe18 100644
@@ -288,6 +288,16 @@ xfs_new_eof(struct xfs_inode *ip, xfs_fsize_t new_size)
 }
 
 /*
+ * Flush all dirty data to disk. Must not be called while holding an XFS_ILOCK
+ * or a page lock.
+ */
+static inline void
+xfs_flush_inodes(struct xfs_inode *ip)
+{
+	writeback_inodes_sb_if_idle(VFS_I(ip)->i_sb, WB_REASON_FS_FREE_SPACE);
+}
+
+/*
  * i_flags helper functions
  */
 static inline void
fs/xfs/xfs_iomap.c +7 -16
index 973dff6..f858b90 100644
@@ -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 +0 -1
index 26e46ae..a54b5aa 100644
@@ -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 */
fs/xfs/xfs_super.c +0 -3
index b85ca2d..fed1eb2 100644
@@ -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);
 
@@ -1231,7 +1229,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
index 8d58fab..0018e84 100644
@@ -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);
Signature · 6 lines
1.7.10

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Reviewed-by on v3 6/13
#10 lore raw

Re: [PATCH 06/13] xfs: xfs_sync_data is redundant.

Quoted 2 lines

/*
+ * Flush all dirty data to disk. Must not be called while holding an XFS_ILOCK

slightly too long line.

fs/xfs/xfs_inode.h xfs_new_eof(struct xfs_inode *ip, xfs_fsize_t new_size) · line 298

Quoted 7 lines

+ * or a page lock.
+ */

fs/xfs/xfs_inode.h +5 -0
+static inline void
+xfs_flush_inodes(struct xfs_inode *ip)
+{
+	writeback_inodes_sb_if_idle(VFS_I(ip)->i_sb, WB_REASON_FS_FREE_SPACE);
+}

Passing an inode here seems very odd, an xfs_mount would be the proper interface.

Looks fine modulo these cosmetic changes, so:

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Reviewed-by Christoph Hellwig Reviewed-by Mark Tinguely
#11 lore raw

Re: [PATCH 06/13] xfs: xfs_sync_data is redundant.

Hi Dave,

Quoted 4 lines · Christoph Hellwig
Quoted 2 lines

/*
+ * Flush all dirty data to disk. Must not be called while holding an XFS_ILOCK

slightly too long line.

It comes out at 78 columns when the patch is applied. Not a problem.

Quoted 11 lines

Review

Quoted 7 lines

+ * or a page lock.
+ */

(quoted diff) +5 -0
+static inline void
+xfs_flush_inodes(struct xfs_inode *ip)
+{
+	writeback_inodes_sb_if_idle(VFS_I(ip)->i_sb, WB_REASON_FS_FREE_SPACE);
+}

Passing an inode here seems very odd, an xfs_mount would be the proper interface.

Quoted 3 lines

Looks fine modulo these cosmetic changes, so:

I cleaned up these cosmetic changes on your behalf. Here is the result:

Quoted 1 line

From [email protected] Fri Sep 28 16:27:23 2012

From: Dave Chinner <[email protected]>
To: [email protected]
Subject: [PATCH 06/13] xfs: xfs_sync_data is redundant.
Date: Fri, 28 Sep 2012 14:44:38 +1000
Message-Id: <[email protected]>
X-Patchwork-Id: 4147

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().

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.

---
 fs/xfs/xfs_file.c  |   13 +++++----
 fs/xfs/xfs_inode.h |   10 +++++++
 fs/xfs/xfs_iomap.c |   23 +++++-----------
 fs/xfs/xfs_mount.h |    1 -
 fs/xfs/xfs_super.c |    3 --
 fs/xfs/xfs_sync.c  |   78 ----------------------------------------------------
 fs/xfs/xfs_sync.h  |    3 --
 7 files changed, 24 insertions(+), 107 deletions(-)
fs/xfs/xfs_file.c +7 -6
Index: xfs/fs/xfs/xfs_file.c
===================================================================
@@ -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
Index: xfs/fs/xfs/xfs_iomap.c
===================================================================
@@ -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 +10 -1
Index: xfs/fs/xfs/xfs_mount.h
===================================================================
@@ -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,16 @@ extern int	xfs_dev_is_read_only(struct x
 
 extern 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.
+ */
+static inline void
+xfs_flush_inodes(struct xfs_mount *mp)
+{
+	writeback_inodes_sb_if_idle(mp->m_super, WB_REASON_FS_FREE_SPACE);
+}
+
 #endif	/* __KERNEL__ */
 
 extern void	xfs_mod_sb(struct xfs_trans *, __int64_t);
fs/xfs/xfs_super.c +0 -3
Index: xfs/fs/xfs/xfs_super.c
===================================================================
@@ -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);
 
@@ -1235,7 +1233,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
Index: xfs/fs/xfs/xfs_sync.h
===================================================================
@@ -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
Index: xfs/fs/xfs/xfs_vnodeops.c
===================================================================
@@ -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

#12 lore raw

Re: [PATCH 06/13] xfs: xfs_sync_data is redundant.

Quoted 21 lines · Ben Myers

Hi Dave,

Quoted 4 lines · Christoph Hellwig
Quoted 2 lines

/*
+ * Flush all dirty data to disk. Must not be called while holding an XFS_ILOCK

slightly too long line.

It comes out at 78 columns when the patch is applied. Not a problem.

Quoted 11 lines

fs/xfs/xfs_mount.h extern int xfs_dev_is_read_only(struct x · line 391

Quoted 7 lines

+ * or a page lock.
+ */

fs/xfs/xfs_mount.h +5 -0
+static inline void
+xfs_flush_inodes(struct xfs_inode *ip)
+{
+	writeback_inodes_sb_if_idle(VFS_I(ip)->i_sb, WB_REASON_FS_FREE_SPACE);
+}

Passing an inode here seems very odd, an xfs_mount would be the proper interface.

Quoted 5 lines
Quoted 3 lines

Looks fine modulo these cosmetic changes, so:

I cleaned up these cosmetic changes on your behalf. Here is the result:

Looks good. Thanks for doing this, Ben.

FWIW, The patch I just posted to change writeback_inodes_sb_if_idle() to sync_inodes_sb() will need massaging to apply to your tree now. I'll update it if you push your tree out to oss.sgi.com before taking it...

Cheers,

Dave.

Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#13 lore raw

Re: [PATCH 06/13] xfs: xfs_sync_data is redundant.

Hey Dave,

Quoted 22 lines · Dave Chinner
Quoted 21 lines · Ben Myers

Hi Dave,

Quoted 4 lines · Christoph Hellwig
Quoted 2 lines

/*
+ * Flush all dirty data to disk. Must not be called while holding an XFS_ILOCK

slightly too long line.

It comes out at 78 columns when the patch is applied. Not a problem.

Quoted 11 lines

fs/xfs/xfs_mount.h extern int xfs_dev_is_read_only(struct x · line 391

Quoted 7 lines

+ * or a page lock.
+ */

fs/xfs/xfs_mount.h +5 -0
+static inline void
+xfs_flush_inodes(struct xfs_inode *ip)
+{
+	writeback_inodes_sb_if_idle(VFS_I(ip)->i_sb, WB_REASON_FS_FREE_SPACE);
+}

Passing an inode here seems very odd, an xfs_mount would be the proper interface.

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

Looks fine modulo these cosmetic changes, so:

I cleaned up these cosmetic changes on your behalf. Here is the result:

Looks good. Thanks for doing this, Ben.

No problem. I wanted to pull this in before the merge window opened. Otherwise I'd have let you go after it.

Quoted 4 lines

FWIW, The patch I just posted to change writeback_inodes_sb_if_idle() to sync_inodes_sb() will need massaging to apply to your tree now. I'll update it if you push your tree out to oss.sgi.com before taking it...

Sure. You can find it here:

git://oss.sgi.com/xfs/xfs.git, tmp-for-dave-worker-changes branch

Can you repost the full patch rather than a delta? That would be cleaner.

Regards,
Ben

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Reviewed-by on v3 6/13
#14 lore raw

Re: [PATCH 06/13] xfs: xfs_sync_data is redundant.

In reply to Dave Chinner
Quoted 25 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().

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.

Quoted 2 lines

---

Looks good.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#15 lore raw

Re: [PATCH 06/13] xfs: xfs_sync_data is redundant.

In reply to Dave Chinner
Quoted 18 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().

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.

Quoted 8 lines

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.

Heads up... I was doing some testing against my eofblocks set rebased against this patchset and I'm reproducing a new 273 failure. The failure bisects down to this patch.

With the bisection, I'm running xfs top of tree plus the following patch:

xfs: only update the last_sync_lsn when a transaction completes

... and patches 1-6 of this set on top of that. i.e.:

xfs: xfs_sync_data is redundant.
xfs: Bring some sanity to log unmounting
xfs: sync work is now only periodic log work
xfs: don't run the sync work if the filesystem is read-only
xfs: rationalise xfs_mount_wq users
xfs: xfs_syncd_stop must die
xfs: only update the last_sync_lsn when a transaction completes
xfs: Make inode32 a remountable option

This is on a 16p (according to /proc/cpuinfo) x86-64 system with 32GB RAM. The test and scratch volumes are both 500GB lvm volumes on top of a hardware raid. I haven't looked into this at all yet but I wanted to drop it on the list for now. The 273 output is attached.

Brian

Quoted 244 lines
---
 fs/xfs/xfs_file.c  |   13 +++++----
 fs/xfs/xfs_inode.h |   10 +++++++
 fs/xfs/xfs_iomap.c |   23 +++++-----------
 fs/xfs/xfs_mount.h |    1 -
 fs/xfs/xfs_super.c |    3 --
 fs/xfs/xfs_sync.c  |   78 ----------------------------------------------------
 fs/xfs/xfs_sync.h  |    3 --
 7 files changed, 24 insertions(+), 107 deletions(-)
fs/xfs/xfs_file.c +7 -6
index 1eaeb8b..2cadcf7 100644
@@ -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);
+		goto write_retry;
 	}
 
 	current->backing_dev_info = NULL;
fs/xfs/xfs_inode.h +10 -0
index 94b32f9..a12fe18 100644
@@ -288,6 +288,16 @@ xfs_new_eof(struct xfs_inode *ip, xfs_fsize_t new_size)
 }
 
 /*
+ * Flush all dirty data to disk. Must not be called while holding an XFS_ILOCK
+ * or a page lock.
+ */
+static inline void
+xfs_flush_inodes(struct xfs_inode *ip)
+{
+	writeback_inodes_sb_if_idle(VFS_I(ip)->i_sb, WB_REASON_FS_FREE_SPACE);
+}
+
+/*
  * i_flags helper functions
  */
 static inline void
fs/xfs/xfs_iomap.c +7 -16
index 973dff6..f858b90 100644
@@ -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 +0 -1
index 26e46ae..a54b5aa 100644
@@ -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 */
fs/xfs/xfs_super.c +0 -3
index b85ca2d..fed1eb2 100644
@@ -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);
 
@@ -1231,7 +1229,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
index 7527610..6a2ada3 100644
@@ -217,51 +217,6 @@ xfs_inode_ag_iterator(
 }
 
 STATIC int
-xfs_sync_inode_data(
-	struct xfs_inode	*ip,
-	struct xfs_perag	*pag,
-	int			flags)
-{
-	struct inode		*inode = VFS_I(ip);
-	struct address_space *mapping = inode->i_mapping;
-	int			error = 0;
-
-	if (!mapping_tagged(mapping, PAGECACHE_TAG_DIRTY))
-		return 0;
-
-	if (!xfs_ilock_nowait(ip, XFS_IOLOCK_SHARED)) {
-		if (flags & SYNC_TRYLOCK)
-			return 0;
-		xfs_ilock(ip, XFS_IOLOCK_SHARED);
-	}
-
-	error = xfs_flush_pages(ip, 0, -1, (flags & SYNC_WAIT) ?
-				0 : XBF_ASYNC, FI_NONE);
-	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
-	return error;
-}
-
-/*
- * Write out pagecache data for the whole filesystem.
- */
-STATIC int
-xfs_sync_data(
-	struct xfs_mount	*mp,
-	int			flags)
-{
-	int			error;
-
-	ASSERT((flags & ~(SYNC_TRYLOCK|SYNC_WAIT)) == 0);
-
-	error = xfs_inode_ag_iterator(mp, xfs_sync_inode_data, flags);
-	if (error)
-		return XFS_ERROR(error);
-
-	xfs_log_force(mp, (flags & SYNC_WAIT) ? XFS_LOG_SYNC : 0);
-	return 0;
-}
-
-STATIC int
 xfs_sync_fsdata(
 	struct xfs_mount	*mp)
 {
@@ -415,39 +370,6 @@ xfs_reclaim_worker(
 	xfs_syncd_queue_reclaim(mp);
 }
 
-/*
- * Flush delayed allocate data, attempting to free up reserved space
- * from existing allocations.  At this point a new allocation attempt
- * has failed with ENOSPC and we are in the process of scratching our
- * heads, looking about for more room.
- *
- * Queue a new data flush if there isn't one already in progress and
- * wait for completion of the flush. This means that we only ever have one
- * inode flush in progress no matter how many ENOSPC events are occurring and
- * so will prevent the system from bogging down due to every concurrent
- * ENOSPC event scanning all the active inodes in the system for writeback.
- */
-void
-xfs_flush_inodes(
-	struct xfs_inode	*ip)
-{
-	struct xfs_mount	*mp = ip->i_mount;
-
-	queue_work(xfs_syncd_wq, &mp->m_flush_work);
-	flush_work_sync(&mp->m_flush_work);
-}
-
-void
-xfs_flush_worker(
-	struct work_struct *work)
-{
-	struct xfs_mount *mp = container_of(work,
-					struct xfs_mount, m_flush_work);
-
-	xfs_sync_data(mp, SYNC_TRYLOCK);
-	xfs_sync_data(mp, SYNC_TRYLOCK | SYNC_WAIT);
-}
-
 void
 __xfs_inode_set_reclaim_tag(
 	struct xfs_perag	*pag,
fs/xfs/xfs_sync.h +0 -1
index 8d58fab..0018e84 100644
@@ -26,14 +26,11 @@ struct xfs_perag;
 
 extern struct workqueue_struct	*xfs_syncd_wq;	/* sync workqueue */
 
-void xfs_flush_worker(struct work_struct *work);

fs/xfs/xfs_sync.h struct xfs_perag; · line 36

Quoted 10 lines
fs/xfs/xfs_sync.h +0 -2
 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);

QA output created by 273
------------------------------
start the workload
------------------------------
_porter 31 not complete
_porter 79 not complete
_porter 149 not complete_porter 74 not complete
_porter 161 not complete
_porter 54 not complete
_porter 98 not complete
_porter 99 not complete
_porter 167 not complete
_porter 76 not complete
_porter 45 not complete
_porter 152 not complete
_porter 173 not complete_porter 24 not complete

_porter 6 not complete
_porter 104 not complete
_porter 117 not complete
_porter 181 not complete
_porter 30 not complete
_porter 148 not complete
_porter 147 not complete
_porter 77 not complete
_porter 111 not complete
_porter 56 not complete

_porter 49 not complete
_porter 165 not complete
_porter 97 not complete
_porter 158 not complete
_porter 157 not complete
_porter 179 not complete
_porter 191 not complete
_porter 57 not complete
_porter 123 not complete
_porter 160 not complete
_porter 100 not complete
_porter 9 not complete
_porter 95 not complete
_porter 10 not complete
_porter 53 not complete
_porter 73 not complete
_porter 27 not complete
_porter 4 not complete
_porter 5 not complete
_porter 39 not complete
_porter 43 not complete
_porter 13 not complete
_porter 48 not complete
_porter 35 not complete
_porter 70 not complete
_porter 29 not complete
_porter 7 not complete
_porter 71 not complete
_porter 93 not complete
_porter 78 not complete

_porter 135 not complete
_porter 174 not complete
_porter 80 not complete
_porter 102 not complete
_porter 183 not complete
_porter 185 not complete_porter 168 not complete
_porter 178 not complete
_porter 129 not complete
_porter 193 not complete
_porter 192 not complete
_porter 199 not complete
_porter 120 not complete
_porter 125 not complete
_porter 126 not complete
_porter 145 not complete
_porter 124 not complete
_porter 172 not complete

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#16 lore raw

Re: [PATCH 06/13] xfs: xfs_sync_data is redundant.

On 10/01/12 15:14, Brian Foster wrote:
<deletes by mt>

Quoted 19 lines

Heads up... I was doing some testing against my eofblocks set rebased against this patchset and I'm reproducing a new 273 failure. The failure bisects down to this patch.

With the bisection, I'm running xfs top of tree plus the following patch:

xfs: only update the last_sync_lsn when a transaction completes

... and patches 1-6 of this set on top of that. i.e.:

xfs: xfs_sync_data is redundant.
xfs: Bring some sanity to log unmounting
xfs: sync work is now only periodic log work
xfs: don't run the sync work if the filesystem is read-only
xfs: rationalise xfs_mount_wq users
xfs: xfs_syncd_stop must die
xfs: only update the last_sync_lsn when a transaction completes
xfs: Make inode32 a remountable option

Quoted 6 lines

This is on a 16p (according to /proc/cpuinfo) x86-64 system with 32GB RAM. The test and scratch volumes are both 500GB lvm volumes on top of a hardware raid. I haven't looked into this at all yet but I wanted to drop it on the list for now. The 273 output is attached.

Brian

<deletes by mt>

Quoted 5 lines

273.out.bad

Quoted 16 lines

QA output created by 273
------------------------------
start the workload
------------------------------
_porter 31 not complete
_porter 79 not complete
_porter 149 not complete_porter 74 not complete
_porter 161 not complete
_porter 54 not complete
_porter 98 not complete
_porter 99 not complete
_porter 167 not complete
_porter 76 not complete
_porter 45 not complete
_porter 152 not complete
_porter 173 not complete_porter 24 not complete

<deletes by mt>

I see it too on a single machine. It looks like an interaction between patch 06 and the "...update the last_sync_lsn...".

I like the "...update the last_sync_lsn ..." patch because it fixes the "xlog_verify_tail_lsn: tail wrapped" and "xlog_verify_tail_lsn: ran out of log space" messages that I am getting on that machine.

--Mark.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#17 lore raw

Re: [PATCH 06/13] xfs: xfs_sync_data is redundant.

In reply to Brian Foster

Hi, Brian.

Quoted 3 lines · Brian Foster

Warning: This message has had one or more attachments removed
Warning: (273.out.bad).
Warning: Please read the "boprocket-Attachment-Warning.txt" attachment(s) for more information.

Which says:

Quoted 2 lines

At Mon Oct 1 20:14:58 2012 the virus scanner said:
MailScanner: Attempt to hide real filename extension (273.out.bad)

Looks like your mailer did something wrong with the attachment....

Quoted 19 lines

Heads up... I was doing some testing against my eofblocks set rebased against this patchset and I'm reproducing a new 273 failure. The failure bisects down to this patch.

With the bisection, I'm running xfs top of tree plus the following patch:

xfs: only update the last_sync_lsn when a transaction completes

... and patches 1-6 of this set on top of that. i.e.:

xfs: xfs_sync_data is redundant.
xfs: Bring some sanity to log unmounting
xfs: sync work is now only periodic log work
xfs: don't run the sync work if the filesystem is read-only
xfs: rationalise xfs_mount_wq users
xfs: xfs_syncd_stop must die
xfs: only update the last_sync_lsn when a transaction completes
xfs: Make inode32 a remountable option

Quoted 5 lines

This is on a 16p (according to /proc/cpuinfo) x86-64 system with 32GB RAM. The test and scratch volumes are both 500GB lvm volumes on top of a hardware raid.
I haven't looked into this at all yet but I wanted to
drop it on the list for now. The 273 output is attached.

I bet you had writes fail with ENOSPC - 201 * 426
= 85626 files of 8k each, that gives 685MB. When the test is running, I see upwards of 1.5GB of space consumed, which then slowly drops again as data files are closed and data is written.

Some of that space is specualtive preallocation (4k per file, I think), but also a significant amount of it is metadata reservation for delayed allocation (4 blocks per file, IIRC). If I've only got 2GB RAM on my machine, then writeback starts at 200MB written, and so well before the fs runs out of space the metadata reservations are being released.

I just upped the VM to 8GB RAM, and immediately I see the test starting to fail. And this is in 273.full:

cp: cannot create regular file `/mnt/scratch/sub_198/origin/file_141': No space left on device
cp: cannot create regular file `/mnt/scratch/sub_198/origin/file_142': No space left on device
cp: cannot create regular file /mnt/scratch/sub_198/origin/file_1cp: cannot create regular filcp: cannot create regular file /mnt/scratch/sub_198/origin/file_147': No space left on device
cp: cannot create regular file /mnt/scratch/sub_198/origin/file_1cp: cannot create regular filcp: writing /mnt/scratch/sub_198/origin/file_149': No space left cp: writing `/mnt/scratch/sub_156/origin/file_275': No space left on device
cp: failed to extencp: writing /mnt/scratch/sub_198/origin/file_150': No space left cp: writing /mnt/scratch/sub_156/origin/file_276': No space left on device
cp: failed to extencp: cannot create regular file /mnt/scratch/sub_124/origin/file_3cp: cannot create regular filcp: writing /mnt/scratch/sub_124/origin/file_378': No space left cp: writing `/mnt/scratch/sub_173/origin/file_250': No space left on device
cp: failed to extencp: writing /mnt/scratch/sub_124/origin/file_379': No space left cp: cannot create regular file /mnt/scratch/sub_173/origin/file_2cp: cannot create regular file `/mnt/scratch/sub_134/origin/file_337': No space left on device
cp: cannot create regular filcp: cannot create regular filcp: writing `/mnt/scratch/sub_159/origin/file_307': No space left on device
cp: failed to extend `/mnt/scratch/sub_159/origin/file_307': No space left on device
cp: writing `/mnt/scratch/sub_159/origin/file_308': No space left on device
cp: failed to extend `/mnt/scratch/sub_159/origin/file_308': No space left on device
cp: cannot create regular file `/mnt/scratch/sub_159/origin/file_309': No space left on device
cp: cannot create regular file `/mnt/scratch/sub_159/origin/file_310': No space left on device
cp: cannot create regular file `/mnt/scratch/sub_159/origin/file_311': No space left on device
cp: cannot create regular file `/mnt/scratch/sub_159/origin/file_312': No space left on device
cp: cannot create regular file `/mnt/scratch/sub_159/origin/file_313': No space left on device
cp: cannot create regular file `/mnt/scratch/sub_159/origin/file_314': No space left on device
.....

So, turning off speculative preallocation via the allocsize mount option doesn't fix the problem. IOWs, the problem is too much active metadata reservation. If we are caching 685MB, that's less than the writeback thresholds of a large memory machine, so the metadata reservations won't be trimmed at all until ENOSPC actually occurs and writeback is then started.

The problem is that writeback_inodes_sb_if_idle() does not block if there is already writeback in progress, so the callers just keep hitting ENOSPC rather than being throttled waiting for delalloc conversion.

The patch below should fix this - it changes xfs_flush_inodes() to us sync_inodes_sb(), which will issue IO and block waiting for it to complete, just like xfs_flush_inodes() used to. Indeed, it passes again on my VM with 8GB RAM....

Cheers,

Dave.

Signature · 17 lines
Dave Chinner
[email protected]

xfs: make inode writeback at ENOSPC blocking.

From: Dave Chinner <[email protected]>

writeback_inodes_sb_if_idle() is not sufficient to trigger delalloc
conversion fast enough to prevent spurious ENOSPC whent here are
hundreds of writers, thousands of small files and GBs of free RAM.
Change this to use sync_sb_inodes() to block callers while we wait
for writeback like the previous xfs_flush_inodes implementation did.

Signed-off-by: Dave Chinner <[email protected]>
---
 fs/xfs/xfs_inode.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/xfs/xfs_inode.h +1 -1
index da69c18..0ec7a46 100644
@@ -294,7 +294,7 @@ xfs_new_eof(struct xfs_inode *ip, xfs_fsize_t new_size)
 static inline void
 xfs_flush_inodes(struct xfs_inode *ip)
 {
-	writeback_inodes_sb_if_idle(VFS_I(ip)->i_sb, WB_REASON_FS_FREE_SPACE);
+	sync_inodes_sb(VFS_I(ip)->i_sb);
 }
 
 /*

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#18 lore raw

Re: [PATCH 06/13] xfs: xfs_sync_data is redundant.

Quoted 7 lines · Dave Chinner

Hi, Brian.

Quoted 3 lines · Brian Foster

Warning: This message has had one or more attachments removed
Warning: (273.out.bad).
Warning: Please read the "boprocket-Attachment-Warning.txt" attachment(s) for more information.

Quoted 7 lines

Which says:

Quoted 2 lines

At Mon Oct 1 20:14:58 2012 the virus scanner said:
MailScanner: Attempt to hide real filename extension (273.out.bad)

Looks like your mailer did something wrong with the attachment....

Ugh, sorry. The output file was filled with messages like so:

_porter 31 not complete
_porter 79 not complete
_porter 149 not complete
_porter 74 not complete
_porter 161 not complete
_porter 54 not complete
_porter 98 not complete
_porter 99 not complete
...

Quoted 51 lines
Quoted 20 lines

Heads up... I was doing some testing against my eofblocks set rebased against this patchset and I'm reproducing a new 273 failure. The failure bisects down to this patch.

With the bisection, I'm running xfs top of tree plus the following patch:

xfs: only update the last_sync_lsn when a transaction completes

... and patches 1-6 of this set on top of that. i.e.:

xfs: xfs_sync_data is redundant.
xfs: Bring some sanity to log unmounting
xfs: sync work is now only periodic log work
xfs: don't run the sync work if the filesystem is read-only
xfs: rationalise xfs_mount_wq users
xfs: xfs_syncd_stop must die
xfs: only update the last_sync_lsn when a transaction completes
xfs: Make inode32 a remountable option

Quoted 5 lines

This is on a 16p (according to /proc/cpuinfo) x86-64 system with 32GB RAM. The test and scratch volumes are both 500GB lvm volumes on top of a hardware raid.
I haven't looked into this at all yet but I wanted to
drop it on the list for now. The 273 output is attached.

I bet you had writes fail with ENOSPC - 201 * 426
= 85626 files of 8k each, that gives 685MB. When the test is running, I see upwards of 1.5GB of space consumed, which then slowly drops again as data files are closed and data is written.

Some of that space is specualtive preallocation (4k per file, I think), but also a significant amount of it is metadata reservation for delayed allocation (4 blocks per file, IIRC). If I've only got 2GB RAM on my machine, then writeback starts at 200MB written, and so well before the fs runs out of space the metadata reservations are being released.

I just upped the VM to 8GB RAM, and immediately I see the test starting to fail. And this is in 273.full:

cp: cannot create regular file `/mnt/scratch/sub_198/origin/file_141': No space left on device
cp: cannot create regular file `/mnt/scratch/sub_198/origin/file_142': No space left on device
cp: cannot create regular file /mnt/scratch/sub_198/origin/file_1cp: cannot create regular filcp: cannot create regular file /mnt/scratch/sub_198/origin/file_147': No space left on device
cp: cannot create regular file /mnt/scratch/sub_198/origin/file_1cp: cannot create regular filcp: writing /mnt/scratch/sub_198/origin/file_149': No space left cp: writing `/mnt/scratch/sub_156/origin/file_275': No space left on device
cp: failed to extencp: writing /mnt/scratch/sub_198/origin/file_150': No space left cp: writing /mnt/scratch/sub_156/origin/file_276': No space left on device
cp: failed to extencp: cannot create regular file /mnt/scratch/sub_124/origin/file_3cp: cannot create regular filcp: writing /mnt/scratch/sub_124/origin/file_378': No space left cp: writing `/mnt/scratch/sub_173/origin/file_250': No space left on device
cp: failed to extencp: writing /mnt/scratch/sub_124/origin/file_379': No space left cp: cannot create regular file /mnt/scratch/sub_173/origin/file_2cp: cannot create regular file `/mnt/scratch/sub_134/origin/file_337': No space left on device
cp: cannot create regular filcp: cannot create regular filcp: writing `/mnt/scratch/sub_159/origin/file_307': No space left on device
cp: failed to extend `/mnt/scratch/sub_159/origin/file_307': No space left on device
cp: writing `/mnt/scratch/sub_159/origin/file_308': No space left on device

Quoted 9 lines

cp: failed to extend `/mnt/scratch/sub_159/origin/file_308': No space left on device
cp: cannot create regular file `/mnt/scratch/sub_159/origin/file_309': No space left on device
cp: cannot create regular file `/mnt/scratch/sub_159/origin/file_310': No space left on device
cp: cannot create regular file `/mnt/scratch/sub_159/origin/file_311': No space left on device
cp: cannot create regular file `/mnt/scratch/sub_159/origin/file_312': No space left on device
cp: cannot create regular file `/mnt/scratch/sub_159/origin/file_313': No space left on device
cp: cannot create regular file `/mnt/scratch/sub_159/origin/file_314': No space left on device
.....

Yep, I see the same thing...

Quoted 12 lines

So, turning off speculative preallocation via the allocsize mount option doesn't fix the problem. IOWs, the problem is too much active metadata reservation. If we are caching 685MB, that's less than the writeback thresholds of a large memory machine, so the metadata reservations won't be trimmed at all until ENOSPC actually occurs and writeback is then started.

The problem is that writeback_inodes_sb_if_idle() does not block if there is already writeback in progress, so the callers just keep hitting ENOSPC rather than being throttled waiting for delalloc conversion.

Quoted 5 lines

The patch below should fix this - it changes xfs_flush_inodes() to us sync_inodes_sb(), which will issue IO and block waiting for it to complete, just like xfs_flush_inodes() used to. Indeed, it passes again on my VM with 8GB RAM....

I gave this a quick couple runs against 273 and it passes (on top of the entire die-xfssyncd-die patchset). I'll kick off another full run on this box overnight. Thanks!

Brian

Quoted 4 lines

Cheers,

Dave.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#19 lore raw

Re: [PATCH 06/13] xfs: xfs_sync_data is redundant.

Quoted 1 line · Brian Foster

On 10/01/2012 08:10 PM, Dave Chinner wrote:

...

Quoted 5 lines

I gave this a quick couple runs against 273 and it passes (on top of the entire die-xfssyncd-die patchset). I'll kick off another full run on this box overnight. Thanks!

And I spoke a bit too soon... I hit the following warning with this change:

WARNING: at fs/fs-writeback.c:1401 sync_inodes_sb+0xc0/0xd0()

The inline patch addresses it. I also see the following message during 273 but it doesn't appear related to this set:

kernel: XFS (dm-4): xlog_verify_grant_tail: space > BBTOB(tail_blocks)

Brian

fs/xfs/xfs_inode.h +3 -1
index da69c18..f11133b 100644
@@ -294,7 +294,9 @@ xfs_new_eof(struct xfs_inode *ip, xfs_fsize_t new_size)
 static inline void
 xfs_flush_inodes(struct xfs_inode *ip)
 {
-	writeback_inodes_sb_if_idle(VFS_I(ip)->i_sb, WB_REASON_FS_FREE_SPACE);
+	down_read(&VFS_I(ip)->i_sb->s_umount);
+	sync_inodes_sb(VFS_I(ip)->i_sb);
+	up_read(&VFS_I(ip)->i_sb->s_umount);
 }
 
 /*
Quoted 10 lines

Brian

Quoted 4 lines

Cheers,

Dave.

_______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#20 lore raw

Re: [PATCH 06/13] xfs: xfs_sync_data is redundant.

Quoted 25 lines · Brian Foster
Quoted 1 line · Brian Foster

On 10/01/2012 08:10 PM, Dave Chinner wrote:

...

Quoted 5 lines

I gave this a quick couple runs against 273 and it passes (on top of the entire die-xfssyncd-die patchset). I'll kick off another full run on this box overnight. Thanks!

And I spoke a bit too soon... I hit the following warning with this change:

WARNING: at fs/fs-writeback.c:1401 sync_inodes_sb+0xc0/0xd0()

The inline patch addresses it. I also see the following message during 273 but it doesn't appear related to this set:

kernel: XFS (dm-4): xlog_verify_grant_tail: space > BBTOB(tail_blocks)

Brian

fs/xfs/xfs_inode.h +0 -0
index da69c18..f11133b 100644

fs/xfs/xfs_inode.h xfs_new_eof(struct xfs_inode *ip, xfs_fsize_t new_size) · line 300

Quoted 8 lines
fs/xfs/xfs_inode.h +3 -1
@@ -294,7 +294,9 @@ xfs_new_eof(struct xfs_inode *ip, xfs_fsize_t new_size)
 static inline void
 xfs_flush_inodes(struct xfs_inode *ip)
 {
-	writeback_inodes_sb_if_idle(VFS_I(ip)->i_sb, WB_REASON_FS_FREE_SPACE);
+	down_read(&VFS_I(ip)->i_sb->s_umount);
+	sync_inodes_sb(VFS_I(ip)->i_sb);
+	up_read(&VFS_I(ip)->i_sb->s_umount);
 }

I don't think we can do an unconditional down_read() there, as the caller from xfs_create() already holds an i_mutex (the VFS holds the directory inode lock) and I'm pretty sure that s_umount is supposed to be outside per-inode locks.

Given that where we are called we are inside a transaction for the create case, and inside mnt_want_write() protection for the buffered write case, the likelyhood of s_umount being held for write at ENOSPC is going to be non-existent at these call sites. Hence a down_read_trylock() will avoid lock ordering issues, but will almost always succeed and so be equivalent to down_read()....

/me modifies and runs 273 and the enospc xfstests group...

Seems to work just fine, and no warnings. Patch below.

Cheers,

Dave.

Signature · 23 lines
Dave Chinner
[email protected]

xfs: make inode writeback at ENOSPC blocking.

From: Dave Chinner <[email protected]>

writeback_inodes_sb_if_idle() is not sufficient to trigger delalloc
conversion fast enough to prevent spurious ENOSPC whent here are
hundreds of writers, thousands of small files and GBs of free RAM.
Change this to use sync_sb_inodes() to block callers while we wait
for writeback like the previous xfs_flush_inodes implementation did.

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.

Signed-off-by: Dave Chinner <[email protected]>
---
 fs/xfs/xfs_inode.h |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
fs/xfs/xfs_inode.h +6 -1
index da69c18..b3dabe9 100644
@@ -294,7 +294,12 @@ xfs_new_eof(struct xfs_inode *ip, xfs_fsize_t new_size)
 static inline void
 xfs_flush_inodes(struct xfs_inode *ip)
 {
-	writeback_inodes_sb_if_idle(VFS_I(ip)->i_sb, WB_REASON_FS_FREE_SPACE);
+	struct super_block *sb = VFS_I(ip)->i_sb;
+
+	if (down_read_trylock(&sb->s_umount)) {
+		sync_inodes_sb(sb);
+		up_read(&sb->s_umount);
+	}
 }
 
 /*

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#21 lore raw

Re: [PATCH 06/13] xfs: xfs_sync_data is redundant.

Quoted 5 lines · Dave Chinner
Quoted 4 lines · Brian Foster
Quoted 1 line · Brian Foster

On 10/01/2012 08:10 PM, Dave Chinner wrote:

...

Quoted 1 line

...

Quoted 6 lines

I don't think we can do an unconditional down_read() there, as the caller from xfs_create() already holds an i_mutex (the VFS holds the directory inode lock) and I'm pretty sure that s_umount is supposed to be outside per-inode locks.

Quoted 7 lines

Given that where we are called we are inside a transaction for the create case, and inside mnt_want_write() protection for the buffered write case, the likelyhood of s_umount being held for write at ENOSPC is going to be non-existent at these call sites. Hence a down_read_trylock() will avoid lock ordering issues, but will almost always succeed and so be equivalent to down_read()....

Ok...

Quoted 4 lines

/me modifies and runs 273 and the enospc xfstests group...

Seems to work just fine, and no warnings. Patch below.

This passes 273 clean (save the xlog_verify_grant_tail message) for me as well. Thanks.

Brian

Quoted 4 lines

Cheers,

Dave.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#22 lore raw

Re: [PATCH 06/13] xfs: xfs_sync_data is redundant.

In reply to Dave Chinner

Hey Dave,

Quoted 28 lines · Dave Chinner

From: Dave Chinner <[email protected]>

writeback_inodes_sb_if_idle() is not sufficient to trigger delalloc conversion fast enough to prevent spurious ENOSPC whent here are hundreds of writers, thousands of small files and GBs of free RAM. Change this to use sync_sb_inodes() to block callers while we wait for writeback like the previous xfs_flush_inodes implementation did.

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.

---
 fs/xfs/xfs_inode.h |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
fs/xfs/xfs_inode.h +0 -0
index da69c18..b3dabe9 100644
@@ -294,7 +294,12 @@ xfs_new_eof(struct xfs_inode *ip, xfs_fsize_t new_size)
 static inline void
 xfs_flush_inodes(struct xfs_inode *ip)
 {

fs/xfs/xfs_inode.h xfs_new_eof(struct xfs_inode *ip, xfs_fsize_t new_size) · line 305

Quoted 10 lines
fs/xfs/xfs_inode.h +6 -1
-	writeback_inodes_sb_if_idle(VFS_I(ip)->i_sb, WB_REASON_FS_FREE_SPACE);
+	struct super_block *sb = VFS_I(ip)->i_sb;
+
+	if (down_read_trylock(&sb->s_umount)) {
+		sync_inodes_sb(sb);
+		up_read(&sb->s_umount);
+	}
 }
 
 /*

Is this the one you want to go with? I'd prefer to apply this to patch 6 itself rather than check in the regression followed by the fix. If you'd like resubmit this patch that's great, otherwise I'll be happy to repost the result.

Thanks again Brian.

Regards,
Ben

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#23 lore raw

Re: [PATCH 06/13] xfs: xfs_sync_data is redundant.

Quoted 18 lines · Ben Myers

Hey Dave,

Quoted 15 lines · Dave Chinner

From: Dave Chinner <[email protected]>

writeback_inodes_sb_if_idle() is not sufficient to trigger delalloc conversion fast enough to prevent spurious ENOSPC whent here are hundreds of writers, thousands of small files and GBs of free RAM. Change this to use sync_sb_inodes() to block callers while we wait for writeback like the previous xfs_flush_inodes implementation did.

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.

<snip>

Quoted 3 lines

Is this the one you want to go with? I'd prefer to apply this to patch 6 itself rather than check in the regression followed by the fix. If you'd like resubmit this patch that's great, otherwise I'll be happy to repost the result.

Yes, this is the version that is necessary. Folding it back into the original patch is fine as long as you fold the commit message as well to explain why it was implemented this way.

Cheers,

Dave.

Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#24 lore raw

Re: [PATCH 06/13] xfs: xfs_sync_data is redundant.

In reply to Dave Chinner

fs/xfs/xfs_inode.h xfs_new_eof(struct xfs_inode *ip, xfs_fsize_t new_size) · line 297

Quoted 10 lines · Dave Chinner
fs/xfs/xfs_inode.h +1 -1
index da69c18..0ec7a46 100644
@@ -294,7 +294,7 @@ xfs_new_eof(struct xfs_inode *ip, xfs_fsize_t new_size)
 static inline void
 xfs_flush_inodes(struct xfs_inode *ip)
 {
-	writeback_inodes_sb_if_idle(VFS_I(ip)->i_sb, WB_REASON_FS_FREE_SPACE);
+	sync_inodes_sb(VFS_I(ip)->i_sb);

sync_inodes_sb needs s_umount held and asserts that, while our callers usually won't have it.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#25 lore raw

Re: [PATCH 06/13] xfs: xfs_sync_data is redundant.

Quoted 14 lines · Christoph Hellwig

fs/xfs/xfs_inode.h xfs_new_eof(struct xfs_inode *ip, xfs_fsize_t new_size) · line 297

Quoted 10 lines · Dave Chinner
fs/xfs/xfs_inode.h +1 -1
index da69c18..0ec7a46 100644
@@ -294,7 +294,7 @@ xfs_new_eof(struct xfs_inode *ip, xfs_fsize_t new_size)
 static inline void
 xfs_flush_inodes(struct xfs_inode *ip)
 {
-	writeback_inodes_sb_if_idle(VFS_I(ip)->i_sb, WB_REASON_FS_FREE_SPACE);
+	sync_inodes_sb(VFS_I(ip)->i_sb);

sync_inodes_sb needs s_umount held and asserts that, while our callers usually won't have it.

Ah, bugger. I didn't notice that, and hadn't checked dmesg after I ran 273 a few times with this patch. I just figured that because it waited for completion, it didn't need it.

Adding s_umount is fine for the wirte path, but the create path holds a directory i_mutex so I'd guess that means it has to be a trylock to avoid lock inversion warnings from lockdep....

Cheers,

Dave.

Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#26 lore raw

Re: [PATCH 06/13] xfs: xfs_sync_data is redundant.

In reply to Brian Foster
Quoted 26 lines · Brian Foster
Quoted 25 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().

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.

Quoted 5 lines
Quoted 1 line

Heads up... I was doing some testing against my eofblocks set rebased against this patchset and I'm reproducing a new 273 failure. The failure bisects down to this patch.

Nice catch Brian!

Regards,
Ben

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#27 lore raw

PATCH v3 7/13 xfs: syncd workqueue is no more

In reply to Dave Chinner

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.

---
 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
index a400e71..96ea9ed 100644
@@ -1191,7 +1191,7 @@ void
 xfs_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
index a54b5aa..7c417b6 100644
@@ -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
index fed1eb2..65cf42c 100644
@@ -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 void
 xfs_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 __init
 xfs_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 void
 xfs_destroy_workqueues(void)
 {
 	destroy_workqueue(xfs_alloc_wq);
-	destroy_workqueue(xfs_syncd_wq);
 }
 
 STATIC int __init
fs/xfs/xfs_sync.c +9 -11
index 6a2ada3..15be21f 100644
@@ -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
index 0018e84..0beabea 100644
@@ -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 · 6 lines
1.7.10

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Reviewed-by on v3 7/13
#28 lore raw

Re: [PATCH 07/13] xfs: syncd workqueue is no more

Quoted 10 lines · Dave Chinner

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.

Looks good,

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Reviewed-by on v3 7/13
#29 lore raw

Re: [PATCH 07/13] xfs: syncd workqueue is no more

In reply to Dave Chinner
Quoted 11 lines · Dave Chinner

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.

---

<deleted info>

Quoted 6 lines
fs/xfs/xfs_super.c +0 -0
index fed1eb2..65cf42c 100644
@@ -776,8 +776,23 @@ xfs_init_mount_workqueues(
  			WQ_MEM_RECLAIM, 0, mp->m_fsname);

fs/xfs/xfs_super.c xfs_init_mount_workqueues( · line 786

Quoted 10 lines
fs/xfs/xfs_super.c +8 -0
  	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);

Cut/paste error for the worker name?

Looks good.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#30 lore raw

Re: [PATCH 07/13] xfs: syncd workqueue is no more

Quoted 15 lines · Mark Tinguely
Quoted 11 lines · Dave Chinner

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.

---

<deleted info>

Quoted 16 lines
Quoted 6 lines
fs/xfs/xfs_super.c +0 -0
index fed1eb2..65cf42c 100644
@@ -776,8 +776,23 @@ xfs_init_mount_workqueues(
 			WQ_MEM_RECLAIM, 0, mp->m_fsname);

fs/xfs/xfs_super.c xfs_init_mount_workqueues( · line 786

Quoted 10 lines
fs/xfs/xfs_super.c +8 -0
 	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);

Changed this to "xfs-log/%s" on Dave's behalf.

-Ben

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#31 lore raw

PATCH v3 8/13 xfs: xfs_sync_fsdata is redundant

In reply to 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_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
index 65cf42c..1ee7b09 100644
@@ -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 · 6 lines
1.7.10

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#32 lore raw

PATCH v3 9/13 xfs: move xfs_quiesce_attr() into xfs_super.c

In reply to Dave Chinner

From: 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
index 1ee7b09..4806e46 100644
@@ -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
index 0beabea..0ba9c89 100644
@@ -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
#33 lore raw

PATCH v3 10/13 xfs: xfs_quiesce_attr() should quiesce the log like unmount

In reply to 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 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
index 96ea9ed..67d63f6 100644
@@ -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
index 26ed7de..5caee96 100644
@@ -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
#34 lore raw

PATCH v3 11/13 xfs: rename xfs_sync.[ch] to xfs_icache.[ch]

In reply to Dave Chinner

From: 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
index d2bf974..442f256 100644
@@ -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
index 784a803..069c5ce 100644
@@ -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
index 149b601..7dfbf07 100644
@@ -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
index 7c417b6..a631ca3 100644
@@ -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
index 858a3b1..7a9071f 100644
@@ -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
index 81de924..ce52520 100644
@@ -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 · 6 lines
1.7.10

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#35 lore raw

PATCH v3 12/13 xfs: move inode locking functions to xfs_inode.c

In reply to 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 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
index 069c5ce..ea9a5fa 100644
@@ -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
#36 lore raw

PATCH v3 13/13 xfs: remove xfs_iget.c

In reply to Dave Chinner

From: 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
index 442f256..e65357b 100644
@@ -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
index 4267922..9b6e330 100644
@@ -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
index 0ba9c89..222e22f 100644
@@ -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
index ba404e4..bba8f37 100644
@@ -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
index a12fe18..da69c18 100644
@@ -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
index 01d10a6..3998fd2 100644
@@ -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
index 5da3ace..651c988 100644
@@ -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
index 2e86fa0..48c750b 100644
@@ -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
index ca28a4b..a69e0b4 100644
@@ -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
index 2a5c6373..e9cae13 100644
@@ -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