On 1/28/22 5:04 PM, Darrick J. Wong wrote:
On Fri, Jan 28, 2022 at 02:53:02PM -0600, Eric Sandeen wrote:
On 1/19/22 6:21 PM, Darrick J. Wong wrote:
From: Darrick J. Wong <djwong@xxxxxxxxxx>
Replace XFS_BUF_SET_ADDR with a new function that will set the buffer
block number correctly, then port the two users to it.
Ok, this is in preparation for later adding more to the
function (saying "set it correctly" confused me a little, because
the function looks equivalent to the macro....)
...
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 63895f28..057b3b09 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3505,8 +3505,8 @@ alloc_write_buf(
error);
exit(1);
}
- bp->b_bn = daddr;
- bp->b_maps[0].bm_bn = daddr;
+
+ xfs_buf_set_daddr(bp, daddr);
This *looks* a little like a functional change, since you dropped
setting of the bp->b_maps[0].bm_bn. But since we get here with a
single buffer, not a map of buffers, I ... think that at this point,
nobody will be looking at b_maps[0].bm_bn anyway?
But I'm not quite sure. I also notice xfs_get_aghdr_buf() in the kernel
setting both b_bn and b_maps[0].bm_bn upstream, for similar purposes.
Can you sanity-check me a little here?
This whole thing is as twisty as a pretzel driving into the mountains.
The end game is that b_bn is actually the cache key for the xfs_buf
structure, so ultimately we don't want anyone accessing it other than
the cache management functions. Hence we spend the next two patches
kicking everybody off of b_bn and then rename it to b_cache_key. Anyone
who wants the daddr address of an xfs_buf (cached or uncached) is
supposed to use bp->b_maps[0].bm_bn (or xfs_buf_daddr/xfs_buf_set_daddr)
after that point.
xfs_get_aghdr_buf (in xfs_ag.c) encodes that rather than setting up a
one-liner helper because that's the only place in the kernel where we
call xfs_buf_get_uncached. By contrast, userspace needs to set a
buffer's daddr(ess) from mkfs and libxlog, so (I guess) that's why the
helper is still useful.
*However* at this point in the game, most people still use b_bn
(incorrectly) so that's probably why alloc_write_buf sets both.
I guess this patch should have replaced only the "b_bn = daddr" part,
and in the next patch removed the "bp->b_maps[0].bm_bn = daddr" line.
Ah, that makes sense, sorry for not assimilating the next patch in
my brain before asking.
I'll make that minor change and,
Reviewed-by: Eric Sandeen <sandeen@xxxxxxxxxx>
After all that, b_bn of the uncached buffer will be NULLDADDR, like it's
supposed to be.
--D
Thanks,
-Eric
return bp;
}
