Message ID | 157859548668.164065.18078635787497973193.stgit@magnolia (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | xfs: fix buf log item memory corruption on non-amd64 | expand |
> + struct xfs_mount *mp = ip->i_mount; > + struct xfs_buf *bp; > + xfs_daddr_t dblkno; > + int dblkcnt; > + > + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL)); > + > + dblkno = XFS_FSB_TO_DADDR(mp, map->br_startblock), > + dblkcnt = XFS_FSB_TO_BB(mp, map->br_blockcount); > + > + /* > + * If the "remote" value is in the cache, remove it. > + */ > + bp = xfs_buf_incore(mp->m_ddev_targp, dblkno, dblkcnt, XBF_TRYLOCK); Do we really need the dblkno and dblkcnt local variables here? > @@ -592,18 +614,8 @@ xfs_attr_rmtval_remove( > ASSERT((map.br_startblock != DELAYSTARTBLOCK) && > (map.br_startblock != HOLESTARTBLOCK)); > > - dblkno = XFS_FSB_TO_DADDR(mp, map.br_startblock), > - dblkcnt = XFS_FSB_TO_BB(mp, map.br_blockcount); > - > - /* > - * If the "remote" value is in the cache, remove it. > - */ > - bp = xfs_buf_incore(mp->m_ddev_targp, dblkno, dblkcnt, XBF_TRYLOCK); > - if (bp) { > - xfs_buf_stale(bp); > - xfs_buf_relse(bp); > - bp = NULL; > - } > + if (map.br_startblock != HOLESTARTBLOCK) > + xfs_attr_rmtval_stale(args->dp, &map); I don't think we need the HOLESTARTBLOCK check here, given that we have the asserts above. I also think the assert should move into xfs_attr_rmtval_stale and be split into two asserts, one each for the invalid values.
On Fri, Jan 10, 2020 at 03:55:40AM -0800, Christoph Hellwig wrote: > > + struct xfs_mount *mp = ip->i_mount; > > + struct xfs_buf *bp; > > + xfs_daddr_t dblkno; > > + int dblkcnt; > > + > > + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL)); > > + > > + dblkno = XFS_FSB_TO_DADDR(mp, map->br_startblock), > > + dblkcnt = XFS_FSB_TO_BB(mp, map->br_blockcount); > > + > > + /* > > + * If the "remote" value is in the cache, remove it. > > + */ > > + bp = xfs_buf_incore(mp->m_ddev_targp, dblkno, dblkcnt, XBF_TRYLOCK); > > Do we really need the dblkno and dblkcnt local variables here? Eh, not really. > > @@ -592,18 +614,8 @@ xfs_attr_rmtval_remove( > > ASSERT((map.br_startblock != DELAYSTARTBLOCK) && > > (map.br_startblock != HOLESTARTBLOCK)); > > > > - dblkno = XFS_FSB_TO_DADDR(mp, map.br_startblock), > > - dblkcnt = XFS_FSB_TO_BB(mp, map.br_blockcount); > > - > > - /* > > - * If the "remote" value is in the cache, remove it. > > - */ > > - bp = xfs_buf_incore(mp->m_ddev_targp, dblkno, dblkcnt, XBF_TRYLOCK); > > - if (bp) { > > - xfs_buf_stale(bp); > > - xfs_buf_relse(bp); > > - bp = NULL; > > - } > > + if (map.br_startblock != HOLESTARTBLOCK) > > + xfs_attr_rmtval_stale(args->dp, &map); > > I don't think we need the HOLESTARTBLOCK check here, given that we have > the asserts above. I also think the assert should move into > xfs_attr_rmtval_stale and be split into two asserts, one each for the > invalid values. <nod> I'll upgrade them to proper fs corruption messages while I'm at it. --D
diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c index a6ef5df42669..6babdaac6057 100644 --- a/fs/xfs/libxfs/xfs_attr_remote.c +++ b/fs/xfs/libxfs/xfs_attr_remote.c @@ -552,6 +552,32 @@ xfs_attr_rmtval_set( return 0; } +/* Mark stale any incore buffers for the remote value. */ +void +xfs_attr_rmtval_stale( + struct xfs_inode *ip, + struct xfs_bmbt_irec *map) +{ + struct xfs_mount *mp = ip->i_mount; + struct xfs_buf *bp; + xfs_daddr_t dblkno; + int dblkcnt; + + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL)); + + dblkno = XFS_FSB_TO_DADDR(mp, map->br_startblock), + dblkcnt = XFS_FSB_TO_BB(mp, map->br_blockcount); + + /* + * If the "remote" value is in the cache, remove it. + */ + bp = xfs_buf_incore(mp->m_ddev_targp, dblkno, dblkcnt, XBF_TRYLOCK); + if (bp) { + xfs_buf_stale(bp); + xfs_buf_relse(bp); + } +} + /* * Remove the value associated with an attribute by deleting the * out-of-line buffer that it is stored on. @@ -560,7 +586,6 @@ int xfs_attr_rmtval_remove( struct xfs_da_args *args) { - struct xfs_mount *mp = args->dp->i_mount; xfs_dablk_t lblkno; int blkcnt; int error; @@ -575,9 +600,6 @@ xfs_attr_rmtval_remove( blkcnt = args->rmtblkcnt; while (blkcnt > 0) { struct xfs_bmbt_irec map; - struct xfs_buf *bp; - xfs_daddr_t dblkno; - int dblkcnt; int nmap; /* @@ -592,18 +614,8 @@ xfs_attr_rmtval_remove( ASSERT((map.br_startblock != DELAYSTARTBLOCK) && (map.br_startblock != HOLESTARTBLOCK)); - dblkno = XFS_FSB_TO_DADDR(mp, map.br_startblock), - dblkcnt = XFS_FSB_TO_BB(mp, map.br_blockcount); - - /* - * If the "remote" value is in the cache, remove it. - */ - bp = xfs_buf_incore(mp->m_ddev_targp, dblkno, dblkcnt, XBF_TRYLOCK); - if (bp) { - xfs_buf_stale(bp); - xfs_buf_relse(bp); - bp = NULL; - } + if (map.br_startblock != HOLESTARTBLOCK) + xfs_attr_rmtval_stale(args->dp, &map); lblkno += map.br_blockcount; blkcnt -= map.br_blockcount; diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h index 9d20b66ad379..ce7287ac5b1f 100644 --- a/fs/xfs/libxfs/xfs_attr_remote.h +++ b/fs/xfs/libxfs/xfs_attr_remote.h @@ -11,5 +11,6 @@ int xfs_attr3_rmt_blocks(struct xfs_mount *mp, int attrlen); int xfs_attr_rmtval_get(struct xfs_da_args *args); int xfs_attr_rmtval_set(struct xfs_da_args *args); int xfs_attr_rmtval_remove(struct xfs_da_args *args); +void xfs_attr_rmtval_stale(struct xfs_inode *ip, struct xfs_bmbt_irec *map); #endif /* __XFS_ATTR_REMOTE_H__ */