Re: [PATCH 42/45] libxfs: replace XFS_BUF_SET_ADDR with a function

["Darrick J. Wong" <djwong@kernel.org>]

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@kernel.org>
> > 
> > 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;
> >   }
> >