From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ipmail07.adl2.internode.on.net ([150.101.137.131]:61814 "EHLO ipmail07.adl2.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751917AbeEPAXt (ORCPT ); Tue, 15 May 2018 20:23:49 -0400 Date: Wed, 16 May 2018 10:23:46 +1000 From: Dave Chinner Subject: Re: [PATCH] xfs: change xfs_update_secondary_supers to use xfS_sb_read_secondary Message-ID: <20180516002346.GL23861@dastard> References: <20180515204721.GM4933@magnolia> <20180515230811.GJ23861@dastard> <20180515233656.GO4933@magnolia> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180515233656.GO4933@magnolia> Sender: linux-xfs-owner@vger.kernel.org List-ID: List-Id: xfs To: "Darrick J. Wong" Cc: Eric Sandeen , xfs On Tue, May 15, 2018 at 04:36:56PM -0700, Darrick J. Wong wrote: > On Wed, May 16, 2018 at 09:08:11AM +1000, Dave Chinner wrote: > > On Tue, May 15, 2018 at 01:47:21PM -0700, Darrick J. Wong wrote: > > > From: Darrick J. Wong > > > > > > Use the new helper to read secondary superblocks instead of opencoding > > > it ourselves. > > > > > > Signed-off-by: Darrick J. Wong > > > --- > > > fs/xfs/xfs_fsops.c | 10 ++++------ > > > 1 file changed, 4 insertions(+), 6 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c > > > index 056a76689197..40c131ebf772 100644 > > > --- a/fs/xfs/xfs_fsops.c > > > +++ b/fs/xfs/xfs_fsops.c > > > @@ -191,18 +191,15 @@ xfs_growfs_data_private( > > > */ > > > int > > > xfs_update_secondary_supers( > > > - xfs_mount_t *mp) > > > + struct xfs_mount *mp) > > > { > > > - int error, saved_error; > > > + struct xfs_buf *bp; > > > xfs_agnumber_t agno; > > > - xfs_buf_t *bp; > > > + int error, saved_error; > > > > > > error = saved_error = 0; > > > > > > for (agno = 1; agno < mp->m_sb.sb_agcount; agno++) { > > > - error = xfs_trans_read_buf(mp, NULL, mp->m_ddev_targp, > > > - XFS_AGB_TO_DADDR(mp, agno, XFS_SB_BLOCK(mp)), > > > - XFS_FSS_TO_BB(mp, 1), 0, &bp, &xfs_sb_buf_ops); > > > /* > > > * If we get an error reading or writing alternate superblocks, > > > * continue. xfs_repair chooses the "best" superblock based > > > @@ -210,6 +207,7 @@ xfs_update_secondary_supers( > > > * superblocks un-updated than updated, and xfs_repair may > > > * pick them over the properly-updated primary. > > > */ > > > + error = xfs_sb_read_secondary(mp, NULL, agno, &bp); > > > if (error) { > > > xfs_warn(mp, > > > "error %d reading secondary superblock for ag %d", > > > > Why change this now when my growfs patchset ireworks it and then > > moves the function entirely? Doing this now just breaks that > > patchset unnecessarily and forces another rebase - can we move this > > to the end of the growfs patchset? > > The label stuff will come after your growfs stuff... maybe I'll try this > again tomorrow when I'm more awake. *nod* > Scrub wants a function to read a secondary sb, and a second one to clone > the primary to fix damage by getting the sb, formatting the incore sb > into the buffer, and writing it to disk. This is all done in a transaction context during scrub, isn't it? Which means it's mostly separate to the non-transactional "update all secondary SB's? > Label wants a function to read each secondary sb, format the incore sb > into the buffer, and write it to disk. Yup - it can just use the function I added to rewrite all the secondary superblocks. Hmmmm - just a thought here: does repair check and repair inconsistent labels across secondary superblocks? What about UUID updates? Do we have a generic need for some superblock updates to be atomic across all superblocks? > Growfs wants to (read an existing ag's secondary sb | get a new ag's > sb), format the incore sb into the buffer, and write it to disk. After my changes, growfs no longer reads the secondary superblocks. It just rewrites them all completely by rewriting the primary SB into them. The existing code rewrites them completely, too, so the read is actually redundant and, potentially, can cause the grow to fail. Cheers, Dave. -- Dave Chinner david@fromorbit.com