On Fri, Jan 28, 2022 at 02:31:11PM -0600, Eric Sandeen wrote:
> On 1/19/22 6:21 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> >
> > The __initbuf function is responsible for initializing the fields of an
> > xfs_buf. Buffers are always required to have a mapping, though in the
> > typical case there's only one mapping, so we can use the internal one.
> >
> > The single-mapping b_maps init code at the end of the function doesn't
> > quite get this right though -- if a single-mapping buffer in the cache
> > was allowed to expire and now is being repurposed, it'll come out with
> > b_maps == &__b_map, in which case we incorrectly skip initializing the
> > map.
>
> In this case b_nmaps must already be 1, right. And it's the bn and
> length in b_maps[0] that fail to be initialized?
>
> I wonder, then, if it's any more clear to reorganize it just a little bit,
> like:
>
> if (!bp->b_maps) {
> bp->b_maps = &bp->__b_map;
> bp->b_nmaps = 1;
> }
>
> if (bp->b_maps == &bp->__b_map) {
> bp->b_maps[0].bm_bn = bp->b_bn;
> bp->b_maps[0].bm_len = bp->b_length;
> }
>
> because AFAICT b_nmaps only needs to be reset to 1 if we didn't already
> get here with b_maps == &__b_map?
That would also work, though it's less obvious (to me anyway) that
b_nmaps is always 1 when bp->b_maps == &bp->__b_map.
--D
> If this is just navel-gazing I can leave it as is. If you think it's
> any clearer, I'll make the change. (or if I've gotten it completely wrong,
> sorry!)
>
> Thanks,
> -Eric
>
> > This has gone unnoticed until now because (AFAICT) the code paths
> > that use b_maps are the same ones that are called with multi-mapping
> > buffers, which are initialized correctly.
> >
> > Anyway, the improperly initialized single-mappings will cause problems
> > in upcoming patches where we turn b_bn into the cache key and require
> > the use of b_maps[0].bm_bn for the buffer LBA. Fix this.
> >
> > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > ---
> > libxfs/rdwr.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> >
> > diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> > index 5086bdbc..a55e3a79 100644
> > --- a/libxfs/rdwr.c
> > +++ b/libxfs/rdwr.c
> > @@ -251,9 +251,11 @@ __initbuf(struct xfs_buf *bp, struct xfs_buftarg *btp, xfs_daddr_t bno,
> > bp->b_ops = NULL;
> > INIT_LIST_HEAD(&bp->b_li_list);
> > - if (!bp->b_maps) {
> > - bp->b_nmaps = 1;
> > + if (!bp->b_maps)
> > bp->b_maps = &bp->__b_map;
> > +
> > + if (bp->b_maps == &bp->__b_map) {
> > + bp->b_nmaps = 1;
> > bp->b_maps[0].bm_bn = bp->b_bn;
> > bp->b_maps[0].bm_len = bp->b_length;
> > }
> >
