diff mbox series

[01/26] xfs: use unsigned ints for non-negative quantities in xfs_attr_remote.c

Message ID 171444680378.957659.14973171617153243991.stgit@frogsfrogsfrogs (mailing list archive)
State New, archived
Headers show
Series [01/26] xfs: use unsigned ints for non-negative quantities in xfs_attr_remote.c | expand

Commit Message

Darrick J. Wong April 30, 2024, 3:24 a.m. UTC
From: Darrick J. Wong <djwong@kernel.org>

In the next few patches we're going to refactor the attr remote code so
that we can support headerless remote xattr values for storing merkle
tree blocks.  For now, let's change the code to use unsigned int to
describe quantities of bytes and blocks that cannot be negative.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Andrey Albershteyn <aalbersh@redhat.com>
---
 fs/xfs/libxfs/xfs_attr_remote.c |   61 +++++++++++++++++++--------------------
 fs/xfs/libxfs/xfs_attr_remote.h |    2 +
 2 files changed, 31 insertions(+), 32 deletions(-)

Comments

Christoph Hellwig May 1, 2024, 6:55 a.m. UTC | #1
On Mon, Apr 29, 2024 at 08:24:22PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> In the next few patches we're going to refactor the attr remote code so
> that we can support headerless remote xattr values for storing merkle
> tree blocks.  For now, let's change the code to use unsigned int to
> describe quantities of bytes and blocks that cannot be negative.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Andrey Albershteyn <aalbersh@redhat.com>

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Can we please get this included ASAP instead of having it linger around?
Darrick J. Wong May 1, 2024, 10:39 p.m. UTC | #2
On Tue, Apr 30, 2024 at 11:55:26PM -0700, Christoph Hellwig wrote:
> On Mon, Apr 29, 2024 at 08:24:22PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > In the next few patches we're going to refactor the attr remote code so
> > that we can support headerless remote xattr values for storing merkle
> > tree blocks.  For now, let's change the code to use unsigned int to
> > describe quantities of bytes and blocks that cannot be negative.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > Reviewed-by: Andrey Albershteyn <aalbersh@redhat.com>
> 
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks!

> Can we please get this included ASAP instead of having it linger around?

Chandan, how many more patches are you willing to take for 6.10?  I
think Christoph has a bunch of fully-reviewed cleanups lurking on the
list, and then there's this one.

--D
Christoph Hellwig May 2, 2024, 4:56 a.m. UTC | #3
On Wed, May 01, 2024 at 03:39:27PM -0700, Darrick J. Wong wrote:
> > Can we please get this included ASAP instead of having it linger around?
> 
> Chandan, how many more patches are you willing to take for 6.10?  I
> think Christoph has a bunch of fully-reviewed cleanups lurking on the
> list, and then there's this one.

Also a bunch of more bugfixes for the log recovery out of bounds
access and the racy iext accesses.  Although I need to respin that
last series for the name change requested by Dave unless someone
disagrees with that.

But even if it's not or 6.10 I would so love if we could feed this kind
of generally useful cleanups upstream ASAP instead of reposting it
hundreds of times or have it linger in a branch somewhere.  That just
leads to frustranting rebases, conflicts and reinventions.  You have
quite a few more candidates like that in your patch stack.
Chandan Babu R May 2, 2024, 5:56 a.m. UTC | #4
On Wed, May 01, 2024 at 03:39:27 PM -0700, Darrick J. Wong wrote:
> On Tue, Apr 30, 2024 at 11:55:26PM -0700, Christoph Hellwig wrote:
>> On Mon, Apr 29, 2024 at 08:24:22PM -0700, Darrick J. Wong wrote:
>> > From: Darrick J. Wong <djwong@kernel.org>
>> > 
>> > In the next few patches we're going to refactor the attr remote code so
>> > that we can support headerless remote xattr values for storing merkle
>> > tree blocks.  For now, let's change the code to use unsigned int to
>> > describe quantities of bytes and blocks that cannot be negative.
>> > 
>> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
>> > Reviewed-by: Andrey Albershteyn <aalbersh@redhat.com>
>> 
>> Looks good:
>> 
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
> Thanks!
>
>> Can we please get this included ASAP instead of having it linger around?
>
> Chandan, how many more patches are you willing to take for 6.10?  I
> think Christoph has a bunch of fully-reviewed cleanups lurking on the
> list, and then there's this one.

I have pushed a set of new patches to for-next a few hours ago.

Also, I have queued the following patchsets for internal testing,
1. fix h_size validation v2
2. quota (un)reservation cleanups
3. Removal of duplicate includes
   i.e. https://lore.kernel.org/linux-xfs/20240430034728.86811-1-jiapeng.chong@linux.alibaba.com/ 

The remaining patchsets from Christoph i.e.
1. optimize COW end I/O remapping v2
2. optimize local for and shortform directory handling
2. iext handling fixes and cleanup
... are either missing RVBs or need to address review comments.

Darrick, I will update for-next sometime tomorrow evening my time. Can you
please send me a pull request containing fs-verity patchset based on
tomorrow's updated for-next branch by end of Friday? This will be last
patchset I will be applying for 6.10 merge window since I would like to test
linux-next during next week.
Christoph Hellwig May 2, 2024, 6:34 a.m. UTC | #5
On Thu, May 02, 2024 at 11:26:08AM +0530, Chandan Babu R wrote:
> 1. optimize COW end I/O remapping v2

This has been retracted and split.

> 2. iext handling fixes and cleanup
> ... are either missing RVBs or need to address review comments.

I'll resend this with the rename in a bit.
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index a8de9dc1e998a..1d44ab3e0a506 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -47,13 +47,13 @@ 
  * Each contiguous block has a header, so it is not just a simple attribute
  * length to FSB conversion.
  */
-int
+unsigned int
 xfs_attr3_rmt_blocks(
-	struct xfs_mount *mp,
-	int		attrlen)
+	struct xfs_mount	*mp,
+	unsigned int		attrlen)
 {
 	if (xfs_has_crc(mp)) {
-		int buflen = XFS_ATTR3_RMT_BUF_SPACE(mp, mp->m_sb.sb_blocksize);
+		unsigned int buflen = XFS_ATTR3_RMT_BUF_SPACE(mp, mp->m_sb.sb_blocksize);
 		return (attrlen + buflen - 1) / buflen;
 	}
 	return XFS_B_TO_FSB(mp, attrlen);
@@ -92,7 +92,6 @@  xfs_attr3_rmt_verify(
 	struct xfs_mount	*mp,
 	struct xfs_buf		*bp,
 	void			*ptr,
-	int			fsbsize,
 	xfs_daddr_t		bno)
 {
 	struct xfs_attr3_rmt_hdr *rmt = ptr;
@@ -103,7 +102,7 @@  xfs_attr3_rmt_verify(
 		return __this_address;
 	if (be64_to_cpu(rmt->rm_blkno) != bno)
 		return __this_address;
-	if (be32_to_cpu(rmt->rm_bytes) > fsbsize - sizeof(*rmt))
+	if (be32_to_cpu(rmt->rm_bytes) > mp->m_attr_geo->blksize - sizeof(*rmt))
 		return __this_address;
 	if (be32_to_cpu(rmt->rm_offset) +
 				be32_to_cpu(rmt->rm_bytes) > XFS_XATTR_SIZE_MAX)
@@ -122,9 +121,9 @@  __xfs_attr3_rmt_read_verify(
 {
 	struct xfs_mount *mp = bp->b_mount;
 	char		*ptr;
-	int		len;
+	unsigned int	len;
 	xfs_daddr_t	bno;
-	int		blksize = mp->m_attr_geo->blksize;
+	unsigned int	blksize = mp->m_attr_geo->blksize;
 
 	/* no verification of non-crc buffers */
 	if (!xfs_has_crc(mp))
@@ -141,7 +140,7 @@  __xfs_attr3_rmt_read_verify(
 			*failaddr = __this_address;
 			return -EFSBADCRC;
 		}
-		*failaddr = xfs_attr3_rmt_verify(mp, bp, ptr, blksize, bno);
+		*failaddr = xfs_attr3_rmt_verify(mp, bp, ptr, bno);
 		if (*failaddr)
 			return -EFSCORRUPTED;
 		len -= blksize;
@@ -186,7 +185,7 @@  xfs_attr3_rmt_write_verify(
 {
 	struct xfs_mount *mp = bp->b_mount;
 	xfs_failaddr_t	fa;
-	int		blksize = mp->m_attr_geo->blksize;
+	unsigned int	blksize = mp->m_attr_geo->blksize;
 	char		*ptr;
 	int		len;
 	xfs_daddr_t	bno;
@@ -203,7 +202,7 @@  xfs_attr3_rmt_write_verify(
 	while (len > 0) {
 		struct xfs_attr3_rmt_hdr *rmt = (struct xfs_attr3_rmt_hdr *)ptr;
 
-		fa = xfs_attr3_rmt_verify(mp, bp, ptr, blksize, bno);
+		fa = xfs_attr3_rmt_verify(mp, bp, ptr, bno);
 		if (fa) {
 			xfs_verifier_error(bp, -EFSCORRUPTED, fa);
 			return;
@@ -281,20 +280,20 @@  xfs_attr_rmtval_copyout(
 	struct xfs_buf		*bp,
 	struct xfs_inode	*dp,
 	xfs_ino_t		owner,
-	int			*offset,
-	int			*valuelen,
+	unsigned int		*offset,
+	unsigned int		*valuelen,
 	uint8_t			**dst)
 {
 	char			*src = bp->b_addr;
 	xfs_daddr_t		bno = xfs_buf_daddr(bp);
-	int			len = BBTOB(bp->b_length);
-	int			blksize = mp->m_attr_geo->blksize;
+	unsigned int		len = BBTOB(bp->b_length);
+	unsigned int		blksize = mp->m_attr_geo->blksize;
 
 	ASSERT(len >= blksize);
 
 	while (len > 0 && *valuelen > 0) {
-		int hdr_size = 0;
-		int byte_cnt = XFS_ATTR3_RMT_BUF_SPACE(mp, blksize);
+		unsigned int hdr_size = 0;
+		unsigned int byte_cnt = XFS_ATTR3_RMT_BUF_SPACE(mp, blksize);
 
 		byte_cnt = min(*valuelen, byte_cnt);
 
@@ -330,20 +329,20 @@  xfs_attr_rmtval_copyin(
 	struct xfs_mount *mp,
 	struct xfs_buf	*bp,
 	xfs_ino_t	ino,
-	int		*offset,
-	int		*valuelen,
+	unsigned int	*offset,
+	unsigned int	*valuelen,
 	uint8_t		**src)
 {
 	char		*dst = bp->b_addr;
 	xfs_daddr_t	bno = xfs_buf_daddr(bp);
-	int		len = BBTOB(bp->b_length);
-	int		blksize = mp->m_attr_geo->blksize;
+	unsigned int	len = BBTOB(bp->b_length);
+	unsigned int	blksize = mp->m_attr_geo->blksize;
 
 	ASSERT(len >= blksize);
 
 	while (len > 0 && *valuelen > 0) {
-		int hdr_size;
-		int byte_cnt = XFS_ATTR3_RMT_BUF_SPACE(mp, blksize);
+		unsigned int hdr_size;
+		unsigned int byte_cnt = XFS_ATTR3_RMT_BUF_SPACE(mp, blksize);
 
 		byte_cnt = min(*valuelen, byte_cnt);
 		hdr_size = xfs_attr3_rmt_hdr_set(mp, dst, ino, *offset,
@@ -389,12 +388,12 @@  xfs_attr_rmtval_get(
 	struct xfs_buf		*bp;
 	xfs_dablk_t		lblkno = args->rmtblkno;
 	uint8_t			*dst = args->value;
-	int			valuelen;
+	unsigned int		valuelen;
 	int			nmap;
 	int			error;
-	int			blkcnt = args->rmtblkcnt;
+	unsigned int		blkcnt = args->rmtblkcnt;
 	int			i;
-	int			offset = 0;
+	unsigned int		offset = 0;
 
 	trace_xfs_attr_rmtval_get(args);
 
@@ -452,7 +451,7 @@  xfs_attr_rmt_find_hole(
 	struct xfs_inode	*dp = args->dp;
 	struct xfs_mount	*mp = dp->i_mount;
 	int			error;
-	int			blkcnt;
+	unsigned int		blkcnt;
 	xfs_fileoff_t		lfileoff = 0;
 
 	/*
@@ -481,11 +480,11 @@  xfs_attr_rmtval_set_value(
 	struct xfs_bmbt_irec	map;
 	xfs_dablk_t		lblkno;
 	uint8_t			*src = args->value;
-	int			blkcnt;
-	int			valuelen;
+	unsigned int		blkcnt;
+	unsigned int		valuelen;
 	int			nmap;
 	int			error;
-	int			offset = 0;
+	unsigned int		offset = 0;
 
 	/*
 	 * Roll through the "value", copying the attribute value to the
@@ -645,7 +644,7 @@  xfs_attr_rmtval_invalidate(
 	struct xfs_da_args	*args)
 {
 	xfs_dablk_t		lblkno;
-	int			blkcnt;
+	unsigned int		blkcnt;
 	int			error;
 
 	/*
diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
index d097ec6c4dc35..c64b04f91cafd 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.h
+++ b/fs/xfs/libxfs/xfs_attr_remote.h
@@ -6,7 +6,7 @@ 
 #ifndef __XFS_ATTR_REMOTE_H__
 #define	__XFS_ATTR_REMOTE_H__
 
-int xfs_attr3_rmt_blocks(struct xfs_mount *mp, int attrlen);
+unsigned int xfs_attr3_rmt_blocks(struct xfs_mount *mp, unsigned int attrlen);
 
 int xfs_attr_rmtval_get(struct xfs_da_args *args);
 int xfs_attr_rmtval_stale(struct xfs_inode *ip, struct xfs_bmbt_irec *map,