| Message ID | 20250219040813.GL21808@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] mkfs,xfs_repair: don't pass a daddr as the flags argument | expand |
On Tue, Feb 18, 2025 at 08:08:13PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > libxfs_buf_get_uncached doesn't take a daddr argument, so don't pass one > as the flags argument. Also take the opportunity to use > xfs_buf_set_daddr to set the actual disk address. Should it take a daddr argument? I've been wondering that a bit as the interface that doesn't pass one seems a bit odd. The patch itself looks fine, although I don't really see the point in the xfsprogs-only xfs_buf_set_daddr (including the current two callers).
On Wed, Feb 19, 2025 at 06:37:17AM +0100, Christoph Hellwig wrote: > On Tue, Feb 18, 2025 at 08:08:13PM -0800, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > libxfs_buf_get_uncached doesn't take a daddr argument, so don't pass one > > as the flags argument. Also take the opportunity to use > > xfs_buf_set_daddr to set the actual disk address. > > Should it take a daddr argument? I've been wondering that a bit as the > interface that doesn't pass one seems a bit odd. We'd have to change the kernel first, there are libxfs callers with that signature. > The patch itself looks fine, although I don't really see the point in > the xfsprogs-only xfs_buf_set_daddr (including the current two callers). Eh, yeah. Want me to resend with those bits cut out? --D
On Tue, Feb 18, 2025 at 09:45:15PM -0800, Darrick J. Wong wrote: > > The patch itself looks fine, although I don't really see the point in > > the xfsprogs-only xfs_buf_set_daddr (including the current two callers). > > Eh, yeah. Want me to resend with those bits cut out? As long as the helper is around there's probably no reason not to use it. Removing it would probably pair pretty well with passing a daddr to xfs_get_buf_uncached. Or maybe killing xfs_{get,read}_buf_uncached entirely in favor of just using xfs_buf_oneshot more..
On Wed, Feb 19, 2025 at 06:48:51AM +0100, Christoph Hellwig wrote: > On Tue, Feb 18, 2025 at 09:45:15PM -0800, Darrick J. Wong wrote: > > > The patch itself looks fine, although I don't really see the point in > > > the xfsprogs-only xfs_buf_set_daddr (including the current two callers). > > > > Eh, yeah. Want me to resend with those bits cut out? > > As long as the helper is around there's probably no reason not to use > it. Removing it would probably pair pretty well with passing a daddr > to xfs_get_buf_uncached. Or maybe killing xfs_{get,read}_buf_uncached > entirely in favor of just using xfs_buf_oneshot more.. <nod> I think I'd rather rid of it entirely and fix the _uncached API to take a daddr. It's not like we can't pass in DADDR_NULL if we *really* don't know where it's going. --D
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index f5556fcc4040ed..79ce68e96bd2a5 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -4989,15 +4989,14 @@ write_rtsb( } error = -libxfs_buf_get_uncached(mp->m_rtdev_targp, - XFS_FSB_TO_BB(mp, 1), XFS_RTSB_DADDR, - &rtsb_bp); + XFS_FSB_TO_BB(mp, 1), 0, &rtsb_bp); if (error) { fprintf(stderr, _("%s: couldn't grab realtime superblock buffer\n"), progname); exit(1); } - rtsb_bp->b_maps[0].bm_bn = XFS_RTSB_DADDR; + xfs_buf_set_daddr(rtsb_bp, XFS_RTSB_DADDR); rtsb_bp->b_ops = &xfs_rtsb_buf_ops; libxfs_update_rtsb(rtsb_bp, sb_bp); diff --git a/repair/rt.c b/repair/rt.c index 5ba04919bc3ccf..12cc9bb8a88aeb 100644 --- a/repair/rt.c +++ b/repair/rt.c @@ -616,12 +616,12 @@ rewrite_rtsb( _("couldn't grab primary sb to update realtime sb\n")); error = -libxfs_buf_get_uncached(mp->m_rtdev_targp, - XFS_FSB_TO_BB(mp, 1), XFS_RTSB_DADDR, &rtsb_bp); + XFS_FSB_TO_BB(mp, 1), 0, &rtsb_bp); if (error) do_error( _("couldn't grab realtime superblock\n")); - rtsb_bp->b_maps[0].bm_bn = XFS_RTSB_DADDR; + xfs_buf_set_daddr(rtsb_bp, XFS_RTSB_DADDR); rtsb_bp->b_ops = &xfs_rtsb_buf_ops; libxfs_update_rtsb(rtsb_bp, sb_bp);