[PATCH] xfs: synchronously write the superblock on unmount

[PATCH] xfs: synchronously write the superblock on unmount

[tinguely]

[-- Attachment #1: xfs-unmountfs_write_superblock.patch --]
[-- Type: text/plain, Size: 4086 bytes --]

xfs_wait_buftarg() does not wait for the completion of the write of the uncached
superblock. This write can race with the shutdown of the log and cause a panic if the
write does not win the race. Per Dave's suggestion, turn the final write of the
superblock into a syncronous write.

Signed-off-by: Mark Tinguely <tinguely@sgi.com>

---
 fs/xfs/xfs_mount.c |   43 +++++++++++++++++++------------------------
 fs/xfs/xfs_mount.h |    4 +++-
 fs/xfs/xfs_sync.c  |    8 +-------
 3 files changed, 23 insertions(+), 32 deletions(-)

Index: b/fs/xfs/xfs_mount.c
===================================================================
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -1517,11 +1517,6 @@ xfs_unmountfs(
 		xfs_warn(mp, "Unable to free reserved block pool. "
 				"Freespace may not be correct on next mount.");
 
-	error = xfs_log_sbcount(mp);
-	if (error)
-		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.
@@ -1529,6 +1524,11 @@ xfs_unmountfs(
 	xfs_ail_push_all_sync(mp->m_ail);
 	xfs_wait_buftarg(mp->m_ddev_targp);
 
+	error = xfs_write_sbcount(mp);
+	if (error)
+		xfs_warn(mp, "Unable to update superblock counters. "
+				"Freespace may not be correct on next mount.");
+
 	xfs_log_unmount_write(mp);
 	xfs_log_unmount(mp);
 	xfs_uuid_unmount(mp);
@@ -1547,19 +1547,17 @@ xfs_fs_writable(xfs_mount_t *mp)
 }
 
 /*
- * xfs_log_sbcount
+ * xfs_write_sbcount
  *
  * Sync the superblock counters to disk.
  *
- * Note this code can be called during the process of freezing, so
- * we may need to use the transaction allocator which does not
- * block when the transaction subsystem is in its frozen state.
  */
 int
-xfs_log_sbcount(xfs_mount_t *mp)
+xfs_write_sbcount(
+	struct xfs_mount	*mp)
 {
-	xfs_trans_t	*tp;
-	int		error;
+	struct xfs_buf		*bp;
+	int			error;
 
 	if (!xfs_fs_writable(mp))
 		return 0;
@@ -1571,19 +1569,16 @@ xfs_log_sbcount(xfs_mount_t *mp)
 	 * counters on every modification.
 	 */
 	if (!xfs_sb_version_haslazysbcount(&mp->m_sb))
-		return 0;
+	return 0;
 
-	tp = _xfs_trans_alloc(mp, XFS_TRANS_SB_COUNT, KM_SLEEP);
-	error = xfs_trans_reserve(tp, 0, mp->m_sb.sb_sectsize + 128, 0, 0,
-					XFS_DEFAULT_LOG_COUNT);
-	if (error) {
-		xfs_trans_cancel(tp, 0);
-		return error;
-	}
-
-	xfs_mod_sb(tp, XFS_SB_IFREE | XFS_SB_ICOUNT | XFS_SB_FDBLOCKS);
-	xfs_trans_set_sync(tp);
-	error = xfs_trans_commit(tp, 0);
+	bp = xfs_getsb(mp, 0);
+	xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb,
+		XFS_SB_IFREE | XFS_SB_ICOUNT | XFS_SB_FDBLOCKS);
+
+	if (xfs_buf_ispinned(bp))
+		xfs_log_force(mp, 0);
+	error = xfs_bwrite(bp);
+	xfs_buf_relse(bp);
 	return error;
 }
 
Index: b/fs/xfs/xfs_mount.h
===================================================================
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -371,7 +371,9 @@ typedef struct xfs_mod_sb {
 	int64_t		msb_delta;	/* Change to make to specified field */
 } xfs_mod_sb_t;
 
-extern int	xfs_log_sbcount(xfs_mount_t *);
+extern int
+xfs_write_sbcount(
+	struct xfs_mount *mp);
 extern __uint64_t xfs_default_resblks(xfs_mount_t *mp);
 extern int	xfs_mountfs(xfs_mount_t *mp);
 
Index: b/fs/xfs/xfs_sync.c
===================================================================
--- a/fs/xfs/xfs_sync.c
+++ b/fs/xfs/xfs_sync.c
@@ -348,17 +348,11 @@ xfs_quiesce_attr(
 	WARN_ON(atomic_read(&mp->m_active_trans) != 0);
 
 	/* Push the superblock and write an unmount record */
-	error = xfs_log_sbcount(mp);
+	error = xfs_write_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);
 }
 
 static void

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: synchronously write the superblock on unmount

[Carlos Maiolino]

Hi,

comments inlined below

> @@ -1571,19 +1569,16 @@ xfs_log_sbcount(xfs_mount_t *mp)
>  	 * counters on every modification.
>  	 */
>  	if (!xfs_sb_version_haslazysbcount(&mp->m_sb))
> -		return 0;
> +	return 0;
	^^^
I'd like to point this cosmetic thing about properly alignment of the
statement with its parent if condition.
>  
> ===================================================================
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -371,7 +371,9 @@ typedef struct xfs_mod_sb {
>  	int64_t		msb_delta;	/* Change to make to specified field */
>  } xfs_mod_sb_t;
>  
> -extern int	xfs_log_sbcount(xfs_mount_t *);
> +extern int
> +xfs_write_sbcount(
> +	struct xfs_mount *mp);
I would also write it in one single line, but this is just my point of view

Otherwise, looks good

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
-- 
--Carlos

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: synchronously write the superblock on unmount

[Christoph Hellwig]

>  /*
> - * xfs_log_sbcount
> + * xfs_write_sbcount

Please drop these function name comment line in anything you touch.

>   *
>   * Sync the superblock counters to disk.
>   *
> - * Note this code can be called during the process of freezing, so
> - * we may need to use the transaction allocator which does not
> - * block when the transaction subsystem is in its frozen state.
>   */

Can you add a little commt here on why we write it out synchronously?
Basically a shortened version of the commit message.

>  	if (!xfs_sb_version_haslazysbcount(&mp->m_sb))
> -		return 0;
> +	return 0;

As mentioned by carlos the indendation here got messed up a bit.

> -extern int	xfs_log_sbcount(xfs_mount_t *);
> +extern int
> +xfs_write_sbcount(
> +	struct xfs_mount *mp);
>  extern __uint64_t xfs_default_resblks(xfs_mount_t *mp);

For the header I'd suggest to keep the simple indentation style.

Otherwise the changes look good to, thanks a lot!


Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: synchronously write the superblock on unmount

[Dave Chinner]

On Tue, Jun 26, 2012 at 11:00:52AM -0500, tinguely@sgi.com wrote:
> xfs_wait_buftarg() does not wait for the completion of the write of the uncached
> superblock. This write can race with the shutdown of the log and cause a panic if the
> write does not win the race. Per Dave's suggestion, turn the final write of the
> superblock into a syncronous write.

Not quite what I suggested. You removed the transaction that logs
the changes first. That needs to stay, otherwise we are writing
unlogged changes to the superblock, and have the possibility of a
crash before the unmount record is written causing log recovery to
modify the superblock and not have all the correct changes in the
log.

So it first needs to log the changes (synchronously), then write the
changes (synchronously).

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs