diff mbox series

[1/5] xfs: refactor remote attr value buffer invalidation

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

Commit Message

Darrick J. Wong Jan. 9, 2020, 6:44 p.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

Hoist the code that invalidates remote extended attribute value buffers
into a separate helper function.  This prepares us for a memory
corruption fix in the next patch.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_attr_remote.c |   44 +++++++++++++++++++++++++--------------
 fs/xfs/libxfs/xfs_attr_remote.h |    1 +
 2 files changed, 29 insertions(+), 16 deletions(-)

Comments

Christoph Hellwig Jan. 10, 2020, 11:55 a.m. UTC | #1 
> +	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.
Darrick J. Wong Jan. 14, 2020, 12:43 a.m. UTC | #2 
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 mbox series

Patch

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__ */