| Message ID | 20180515204721.GM4933@magnolia (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
On 5/15/18 3:47 PM, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Use the new helper to read secondary superblocks instead of opencoding > it ourselves. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> Thanks, sorry I forgot^Wdidn't know that existed. Reviewed-by: Eric Sandeen <sandeen@redhat.com> > --- > 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", > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 15, 2018 at 01:47:21PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Use the new helper to read secondary superblocks instead of opencoding > it ourselves. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > 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? Cheers, Dave.
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 <darrick.wong@oracle.com> > > > > Use the new helper to read secondary superblocks instead of opencoding > > it ourselves. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > 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. 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. Label wants a function to read each secondary sb, format the incore sb into the buffer, and write it to disk. 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. Does that sound right? --D > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <darrick.wong@oracle.com> > > > > > > Use the new helper to read secondary superblocks instead of opencoding > > > it ourselves. > > > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > > --- > > > 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.
On Wed, May 16, 2018 at 10:23:46AM +1000, Dave Chinner wrote: > 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 <darrick.wong@oracle.com> > > > > > > > > Use the new helper to read secondary superblocks instead of opencoding > > > > it ourselves. > > > > > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > > > --- > > > > 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* Aha, just s/xfs_update_secondary_supers/xfs_update_secondary_sbs/ in Eric's online label patch, and drop the one where he adds xfs_update_secondary_supers. > > 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? Yes. It's likely we only want to update a single secondary sb, and updating all of them is overkill, so maybe it's fine just to open code this one use? Seeing as I don't think we'd ordinarily be updating only a single secondary sb. > > 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? Online fsck checks both sb_fname and sb_uuid and complains if the secondaries are out of date. xfs_repair doesn't check at all. > > 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. Ok, I had wondered myself if we should unconditionally rewrite the secondary sbs, because why would we care what the previous contents were? --D > Cheers, > > Dave. > > -- > Dave Chinner > david@fromorbit.com > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 15, 2018 at 05:41:29PM -0700, Darrick J. Wong wrote: > On Wed, May 16, 2018 at 10:23:46AM +1000, Dave Chinner wrote: > > 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 <darrick.wong@oracle.com> > > > > > > > > > > Use the new helper to read secondary superblocks instead of opencoding > > > > > it ourselves. > > > > > > > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > > > > --- > > > > > 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* > > Aha, just s/xfs_update_secondary_supers/xfs_update_secondary_sbs/ in > Eric's online label patch, and drop the one where he adds > xfs_update_secondary_supers. > > > > 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? > > Yes. It's likely we only want to update a single secondary sb, and > updating all of them is overkill, so maybe it's fine just to open code > this one use? Seeing as I don't think we'd ordinarily be updating only > a single secondary sb. Ok, so that means - initially, at least - they will be separate functions. In time, I'd like to explore making the secondary superblock updates ordered buffers attached to a special superblock update transaction (i.e. like the inode extent swap intent in the inode log item flags), and that would probably allow us to factor it all down to a single set of transaction aware functions... > > > 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? > > Online fsck checks both sb_fname and sb_uuid and complains if the > secondaries are out of date. xfs_repair doesn't check at all. OK, so I'm guessing that will get added to the growing list of things that repair needs to check? > > > 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. > > Ok, I had wondered myself if we should unconditionally rewrite the > secondary sbs, because why would we care what the previous contents > were? Exactly :) Cheers, Dave.
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",