| Message ID | 164263807467.860211.13040036268013928337.stgit@magnolia (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfsprogs: sync libxfs with 5.15 | expand |
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? Thanks, -Eric > return bp; > } > >
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; > > } > >
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@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. 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@redhat.com> > After all that, b_bn of the uncached buffer will be NULLDADDR, like it's > supposed to be. > > --D > >> Thanks, >> -Eric >> >>> return bp; >>> } >>> > >
diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h index 3cc4f4ee..bf489259 100644 --- a/libxfs/libxfs_io.h +++ b/libxfs/libxfs_io.h @@ -116,7 +116,10 @@ typedef unsigned int xfs_buf_flags_t; #define xfs_buf_offset(bp, offset) ((bp)->b_addr + (offset)) #define XFS_BUF_ADDR(bp) ((bp)->b_bn) -#define XFS_BUF_SET_ADDR(bp,blk) ((bp)->b_bn = (blk)) +static inline void xfs_buf_set_daddr(struct xfs_buf *bp, xfs_daddr_t blkno) +{ + bp->b_bn = blkno; +} void libxfs_buf_set_priority(struct xfs_buf *bp, int priority); int libxfs_buf_priority(struct xfs_buf *bp); diff --git a/libxlog/xfs_log_recover.c b/libxlog/xfs_log_recover.c index d43914b9..3c24c021 100644 --- a/libxlog/xfs_log_recover.c +++ b/libxlog/xfs_log_recover.c @@ -114,7 +114,7 @@ xlog_bread_noalign( ASSERT(nbblks > 0); ASSERT(nbblks <= bp->b_length); - XFS_BUF_SET_ADDR(bp, log->l_logBBstart + blk_no); + xfs_buf_set_daddr(bp, log->l_logBBstart + blk_no); bp->b_length = nbblks; bp->b_error = 0; 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); return bp; }