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. After all that, b_bn of the uncached buffer will be NULLDADDR, like it's supposed to be. --D > Thanks, > -Eric > > > return bp; > > } > >
