* [PATCH] xfs: synchronously write the superblock on unmount [not found] <20120626160051.364635296@sgi.com> @ 2012-06-26 16:00 ` tinguely 2012-06-26 18:09 ` Carlos Maiolino ` (2 more replies) 0 siblings, 3 replies; 4+ messages in thread From: tinguely @ 2012-06-26 16:00 UTC (permalink / raw) To: xfs [-- 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs: synchronously write the superblock on unmount 2012-06-26 16:00 ` [PATCH] xfs: synchronously write the superblock on unmount tinguely @ 2012-06-26 18:09 ` Carlos Maiolino 2012-06-26 21:06 ` Christoph Hellwig 2012-06-27 10:08 ` Dave Chinner 2 siblings, 0 replies; 4+ messages in thread From: Carlos Maiolino @ 2012-06-26 18:09 UTC (permalink / raw) To: xfs 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs: synchronously write the superblock on unmount 2012-06-26 16:00 ` [PATCH] xfs: synchronously write the superblock on unmount tinguely 2012-06-26 18:09 ` Carlos Maiolino @ 2012-06-26 21:06 ` Christoph Hellwig 2012-06-27 10:08 ` Dave Chinner 2 siblings, 0 replies; 4+ messages in thread From: Christoph Hellwig @ 2012-06-26 21:06 UTC (permalink / raw) To: tinguely; +Cc: xfs > /* > - * 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs: synchronously write the superblock on unmount 2012-06-26 16:00 ` [PATCH] xfs: synchronously write the superblock on unmount tinguely 2012-06-26 18:09 ` Carlos Maiolino 2012-06-26 21:06 ` Christoph Hellwig @ 2012-06-27 10:08 ` Dave Chinner 2 siblings, 0 replies; 4+ messages in thread From: Dave Chinner @ 2012-06-27 10:08 UTC (permalink / raw) To: tinguely; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-06-27 10:09 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20120626160051.364635296@sgi.com>
2012-06-26 16:00 ` [PATCH] xfs: synchronously write the superblock on unmount tinguely
2012-06-26 18:09 ` Carlos Maiolino
2012-06-26 21:06 ` Christoph Hellwig
2012-06-27 10:08 ` Dave Chinner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox