[PATCH V2 0/2] erofs: refactor fast_symlink and read_inode

[PATCH V2 0/2] erofs: refactor fast_symlink and read_inode

[Yiyang Wu]

This patchset is response to the original suggestions posted here[1].
Since my later work is somehow related to this issue, i think it's
better to deal with it first.

Changes in V2:
  1. Lift the erofs_fill_symlink patch to 1/2
  2. Fix code styles problems in read_inode patch.
  3. Fix the formatting problems caused by clang-format.

[1]: https://lore.kernel.org/all/20240425222847.GN2118490@ZenIV/

Yiyang Wu (2):
  erofs: use kmemdup_nul in erofs_fill_symlink
  erofs: refactor read_inode calling convention

 fs/erofs/inode.c | 128 ++++++++++++++++++++++++-----------------------
 1 file changed, 65 insertions(+), 63 deletions(-)

-- 
2.46.0

[PATCH V2 1/2] erofs: use kmemdup_nul in erofs_fill_symlink

[Yiyang Wu]

Remove open coding in erofs_fill_symlink.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Link: https://lore.kernel.org/all/20240425222847.GN2118490@ZenIV
Signed-off-by: Yiyang Wu <toolmanp@tlmp.cc>
---
 fs/erofs/inode.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
index 419432be3223..d051afe39670 100644
--- a/fs/erofs/inode.c
+++ b/fs/erofs/inode.c
@@ -188,22 +188,20 @@ static int erofs_fill_symlink(struct inode *inode, void *kaddr,
 		return 0;
 	}
 
-	lnk = kmalloc(inode->i_size + 1, GFP_KERNEL);
-	if (!lnk)
-		return -ENOMEM;
-
 	m_pofs += vi->xattr_isize;
 	/* inline symlink data shouldn't cross block boundary */
 	if (m_pofs + inode->i_size > bsz) {
-		kfree(lnk);
 		erofs_err(inode->i_sb,
 			  "inline data cross block boundary @ nid %llu",
 			  vi->nid);
 		DBG_BUGON(1);
 		return -EFSCORRUPTED;
 	}
-	memcpy(lnk, kaddr + m_pofs, inode->i_size);
-	lnk[inode->i_size] = '\0';
+
+	lnk = kmemdup_nul(kaddr + m_pofs, inode->i_size, GFP_KERNEL);
+
+	if (!lnk)
+		return -ENOMEM;
 
 	inode->i_link = lnk;
 	inode->i_op = &erofs_fast_symlink_iops;
-- 
2.46.0

Re: [PATCH V2 1/2] erofs: use kmemdup_nul in erofs_fill_symlink

[Gao Xiang]

On 2024/9/2 15:00, Yiyang Wu via Linux-erofs wrote:
> Remove open coding in erofs_fill_symlink.
> 
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Link: https://lore.kernel.org/all/20240425222847.GN2118490@ZenIV
> Signed-off-by: Yiyang Wu <toolmanp@tlmp.cc>
> ---
>   fs/erofs/inode.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
> index 419432be3223..d051afe39670 100644
> --- a/fs/erofs/inode.c
> +++ b/fs/erofs/inode.c
> @@ -188,22 +188,20 @@ static int erofs_fill_symlink(struct inode *inode, void *kaddr,
>   		return 0;
>   	}
>   
> -	lnk = kmalloc(inode->i_size + 1, GFP_KERNEL);
> -	if (!lnk)
> -		return -ENOMEM;
> -
>   	m_pofs += vi->xattr_isize;
>   	/* inline symlink data shouldn't cross block boundary */
>   	if (m_pofs + inode->i_size > bsz) {
> -		kfree(lnk);
>   		erofs_err(inode->i_sb,
>   			  "inline data cross block boundary @ nid %llu",
>   			  vi->nid);
>   		DBG_BUGON(1);
>   		return -EFSCORRUPTED;
>   	}
> -	memcpy(lnk, kaddr + m_pofs, inode->i_size);
> -	lnk[inode->i_size] = '\0';
> +
> +	lnk = kmemdup_nul(kaddr + m_pofs, inode->i_size, GFP_KERNEL);
> +

Unnecessary new line.

Also I wonder if it's possible to just
	inode->i_link = kmemdup_nul(kaddr + m_pofs, inode->i_size, GFP_KERNEL);
	if (!inode->i_link)
		return -ENOMEM;

here, and get rid of variable lnk.

Otherwise it looks good to me.

Thanks,
Gao Xiang

> +	if (!lnk)
> +		return -ENOMEM;
>   
>   	inode->i_link = lnk;
>   	inode->i_op = &erofs_fast_symlink_iops;

Re: [PATCH V2 1/2] erofs: use kmemdup_nul in erofs_fill_symlink

[Yiyang Wu]

On Mon, Sep 02, 2024 at 03:11:45PM GMT, Gao Xiang wrote:

> 
> 
> On 2024/9/2 15:00, Yiyang Wu via Linux-erofs wrote:
> > Remove open coding in erofs_fill_symlink.
> > 
> > Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> > Link: https://lore.kernel.org/all/20240425222847.GN2118490@ZenIV
> > Signed-off-by: Yiyang Wu <toolmanp@tlmp.cc>
> > ---
> >   fs/erofs/inode.c | 12 +++++-------
> >   1 file changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
> > index 419432be3223..d051afe39670 100644
> > --- a/fs/erofs/inode.c
> > +++ b/fs/erofs/inode.c
> > @@ -188,22 +188,20 @@ static int erofs_fill_symlink(struct inode *inode, void *kaddr,
> >   		return 0;
> >   	}
> > -	lnk = kmalloc(inode->i_size + 1, GFP_KERNEL);
> > -	if (!lnk)
> > -		return -ENOMEM;
> > -
> >   	m_pofs += vi->xattr_isize;
> >   	/* inline symlink data shouldn't cross block boundary */
> >   	if (m_pofs + inode->i_size > bsz) {
> > -		kfree(lnk);
> >   		erofs_err(inode->i_sb,
> >   			  "inline data cross block boundary @ nid %llu",
> >   			  vi->nid);
> >   		DBG_BUGON(1);
> >   		return -EFSCORRUPTED;
> >   	}
> > -	memcpy(lnk, kaddr + m_pofs, inode->i_size);
> > -	lnk[inode->i_size] = '\0';
> > +
> > +	lnk = kmemdup_nul(kaddr + m_pofs, inode->i_size, GFP_KERNEL);
> > +
> 
> Unnecessary new line.
> 
> Also I wonder if it's possible to just
> 	inode->i_link = kmemdup_nul(kaddr + m_pofs, inode->i_size, GFP_KERNEL);
> 	if (!inode->i_link)
> 		return -ENOMEM;
> 
> here, and get rid of variable lnk.
> 
> Otherwise it looks good to me.
> 
> Thanks,
> Gao Xiang
>  
Yeah, it looks good to me. Fixed.
> > +	if (!lnk)
> > +		return -ENOMEM;
> >   	inode->i_link = lnk;
> >   	inode->i_op = &erofs_fast_symlink_iops;
> 

Best Regards,
Yiyang Wu

[PATCH V2 2/2] erofs: refactor read_inode calling convention

[Yiyang Wu]

Refactor out the iop binding behavior out of the erofs_fill_symlink
and move erofs_buf into the erofs_read_inode, so that erofs_fill_inode
can only deal with inode operation bindings and can be decoupled from
metabuf operations. This results in better calling conventions.

Note that after this patch, we do not need erofs_buf and ofs as
parameters any more when calling erofs_read_inode as
all the data operations are now included in itself.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Link: https://lore.kernel.org/all/20240425222847.GN2118490@ZenIV/
Signed-off-by: Yiyang Wu <toolmanp@tlmp.cc>
---
 fs/erofs/inode.c | 126 ++++++++++++++++++++++++-----------------------
 1 file changed, 65 insertions(+), 61 deletions(-)

diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
index d051afe39670..d854509bb082 100644
--- a/fs/erofs/inode.c
+++ b/fs/erofs/inode.c
@@ -8,8 +8,39 @@
 
 #include <trace/events/erofs.h>
 
-static void *erofs_read_inode(struct erofs_buf *buf,
-			      struct inode *inode, unsigned int *ofs)
+static int erofs_fill_symlink(struct inode *inode, void *kaddr,
+			      unsigned int m_pofs)
+{
+	struct erofs_inode *vi = EROFS_I(inode);
+	unsigned int bsz = i_blocksize(inode);
+	char *lnk;
+
+	/* if it cannot be handled with fast symlink scheme */
+	if (vi->datalayout != EROFS_INODE_FLAT_INLINE ||
+	    inode->i_size >= bsz || inode->i_size < 0) {
+		return 0;
+	}
+
+	m_pofs += vi->xattr_isize;
+	/* inline symlink data shouldn't cross block boundary */
+	if (m_pofs + inode->i_size > bsz) {
+		erofs_err(inode->i_sb,
+			  "inline data cross block boundary @ nid %llu",
+			  vi->nid);
+		DBG_BUGON(1);
+		return -EFSCORRUPTED;
+	}
+
+	lnk = kmemdup_nul(kaddr + m_pofs, inode->i_size, GFP_KERNEL);
+
+	if (!lnk)
+		return -ENOMEM;
+
+	inode->i_link = lnk;
+	return 0;
+}
+
+static int erofs_read_inode(struct inode *inode)
 {
 	struct super_block *sb = inode->i_sb;
 	struct erofs_sb_info *sbi = EROFS_SB(sb);
@@ -20,20 +51,21 @@ static void *erofs_read_inode(struct erofs_buf *buf,
 	struct erofs_inode_compact *dic;
 	struct erofs_inode_extended *die, *copied = NULL;
 	union erofs_inode_i_u iu;
-	unsigned int ifmt;
+	struct erofs_buf buf;
+	unsigned int ifmt, ofs;
 	int err;
 
 	blkaddr = erofs_blknr(sb, inode_loc);
-	*ofs = erofs_blkoff(sb, inode_loc);
+	ofs = erofs_blkoff(sb, inode_loc);
 
-	kaddr = erofs_read_metabuf(buf, sb, erofs_pos(sb, blkaddr), EROFS_KMAP);
+	kaddr = erofs_read_metabuf(&buf, sb, erofs_pos(sb, blkaddr), EROFS_KMAP);
 	if (IS_ERR(kaddr)) {
 		erofs_err(sb, "failed to get inode (nid: %llu) page, err %ld",
 			  vi->nid, PTR_ERR(kaddr));
-		return kaddr;
+		return PTR_ERR(kaddr);
 	}
 
-	dic = kaddr + *ofs;
+	dic = kaddr + ofs;
 	ifmt = le16_to_cpu(dic->i_format);
 	if (ifmt & ~EROFS_I_ALL) {
 		erofs_err(sb, "unsupported i_format %u of nid %llu",
@@ -54,11 +86,11 @@ static void *erofs_read_inode(struct erofs_buf *buf,
 	case EROFS_INODE_LAYOUT_EXTENDED:
 		vi->inode_isize = sizeof(struct erofs_inode_extended);
 		/* check if the extended inode acrosses block boundary */
-		if (*ofs + vi->inode_isize <= sb->s_blocksize) {
-			*ofs += vi->inode_isize;
+		if (ofs + vi->inode_isize <= sb->s_blocksize) {
+			ofs += vi->inode_isize;
 			die = (struct erofs_inode_extended *)dic;
 		} else {
-			const unsigned int gotten = sb->s_blocksize - *ofs;
+			const unsigned int gotten = sb->s_blocksize - ofs;
 
 			copied = kmalloc(vi->inode_isize, GFP_KERNEL);
 			if (!copied) {
@@ -66,16 +98,16 @@ static void *erofs_read_inode(struct erofs_buf *buf,
 				goto err_out;
 			}
 			memcpy(copied, dic, gotten);
-			kaddr = erofs_read_metabuf(buf, sb, erofs_pos(sb, blkaddr + 1),
+			kaddr = erofs_read_metabuf(&buf, sb, erofs_pos(sb, blkaddr + 1),
 						   EROFS_KMAP);
 			if (IS_ERR(kaddr)) {
 				erofs_err(sb, "failed to get inode payload block (nid: %llu), err %ld",
 					  vi->nid, PTR_ERR(kaddr));
 				kfree(copied);
-				return kaddr;
+				return PTR_ERR(kaddr);
 			}
-			*ofs = vi->inode_isize - gotten;
-			memcpy((u8 *)copied + gotten, kaddr, *ofs);
+			ofs = vi->inode_isize - gotten;
+			memcpy((u8 *)copied + gotten, kaddr, ofs);
 			die = copied;
 		}
 		vi->xattr_isize = erofs_xattr_ibody_size(die->i_xattr_icount);
@@ -95,7 +127,7 @@ static void *erofs_read_inode(struct erofs_buf *buf,
 		break;
 	case EROFS_INODE_LAYOUT_COMPACT:
 		vi->inode_isize = sizeof(struct erofs_inode_compact);
-		*ofs += vi->inode_isize;
+		ofs += vi->inode_isize;
 		vi->xattr_isize = erofs_xattr_ibody_size(dic->i_xattr_icount);
 
 		inode->i_mode = le16_to_cpu(dic->i_mode);
@@ -119,6 +151,11 @@ static void *erofs_read_inode(struct erofs_buf *buf,
 	case S_IFREG:
 	case S_IFDIR:
 	case S_IFLNK:
+		if(S_ISLNK(inode->i_mode)) {
+			err = erofs_fill_symlink(inode, kaddr, ofs);
+			if(err)
+				goto err_out;
+		}
 		vi->raw_blkaddr = le32_to_cpu(iu.raw_blkaddr);
 		break;
 	case S_IFCHR:
@@ -165,63 +202,29 @@ static void *erofs_read_inode(struct erofs_buf *buf,
 		inode->i_blocks = round_up(inode->i_size, sb->s_blocksize) >> 9;
 	else
 		inode->i_blocks = nblks << (sb->s_blocksize_bits - 9);
-	return kaddr;
+
+	erofs_put_metabuf(&buf);
+	return 0;
 
 err_out:
 	DBG_BUGON(1);
 	kfree(copied);
-	erofs_put_metabuf(buf);
-	return ERR_PTR(err);
+	erofs_put_metabuf(&buf);
+	return err;
 }
 
-static int erofs_fill_symlink(struct inode *inode, void *kaddr,
-			      unsigned int m_pofs)
-{
-	struct erofs_inode *vi = EROFS_I(inode);
-	unsigned int bsz = i_blocksize(inode);
-	char *lnk;
-
-	/* if it cannot be handled with fast symlink scheme */
-	if (vi->datalayout != EROFS_INODE_FLAT_INLINE ||
-	    inode->i_size >= bsz || inode->i_size < 0) {
-		inode->i_op = &erofs_symlink_iops;
-		return 0;
-	}
-
-	m_pofs += vi->xattr_isize;
-	/* inline symlink data shouldn't cross block boundary */
-	if (m_pofs + inode->i_size > bsz) {
-		erofs_err(inode->i_sb,
-			  "inline data cross block boundary @ nid %llu",
-			  vi->nid);
-		DBG_BUGON(1);
-		return -EFSCORRUPTED;
-	}
-
-	lnk = kmemdup_nul(kaddr + m_pofs, inode->i_size, GFP_KERNEL);
-
-	if (!lnk)
-		return -ENOMEM;
-
-	inode->i_link = lnk;
-	inode->i_op = &erofs_fast_symlink_iops;
-	return 0;
-}
 
 static int erofs_fill_inode(struct inode *inode)
 {
 	struct erofs_inode *vi = EROFS_I(inode);
-	struct erofs_buf buf = __EROFS_BUF_INITIALIZER;
-	void *kaddr;
-	unsigned int ofs;
 	int err = 0;
 
 	trace_erofs_fill_inode(inode);
 
 	/* read inode base data from disk */
-	kaddr = erofs_read_inode(&buf, inode, &ofs);
-	if (IS_ERR(kaddr))
-		return PTR_ERR(kaddr);
+	err = erofs_read_inode(inode);
+	if (err)
+		goto out_unlock;
 
 	/* setup the new inode */
 	switch (inode->i_mode & S_IFMT) {
@@ -238,9 +241,11 @@ static int erofs_fill_inode(struct inode *inode)
 		inode_nohighmem(inode);
 		break;
 	case S_IFLNK:
-		err = erofs_fill_symlink(inode, kaddr, ofs);
-		if (err)
-			goto out_unlock;
+		if (inode->i_link)
+			inode->i_op = &erofs_fast_symlink_iops;
+		else
+			inode->i_op = &erofs_symlink_iops;
+
 		inode_nohighmem(inode);
 		break;
 	case S_IFCHR:
@@ -273,7 +278,6 @@ static int erofs_fill_inode(struct inode *inode)
 #endif
 	}
 out_unlock:
-	erofs_put_metabuf(&buf);
 	return err;
 }
 
-- 
2.46.0

Re: [PATCH V2 2/2] erofs: refactor read_inode calling convention

[Gao Xiang]

On 2024/9/2 15:00, Yiyang Wu via Linux-erofs wrote:
> Refactor out the iop binding behavior out of the erofs_fill_symlink
> and move erofs_buf into the erofs_read_inode, so that erofs_fill_inode
> can only deal with inode operation bindings and can be decoupled from
> metabuf operations. This results in better calling conventions.
> 
> Note that after this patch, we do not need erofs_buf and ofs as
> parameters any more when calling erofs_read_inode as
> all the data operations are now included in itself.
> 
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Link: https://lore.kernel.org/all/20240425222847.GN2118490@ZenIV/
> Signed-off-by: Yiyang Wu <toolmanp@tlmp.cc>
> ---
>   fs/erofs/inode.c | 126 ++++++++++++++++++++++++-----------------------
>   1 file changed, 65 insertions(+), 61 deletions(-)
> 
> diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
> index d051afe39670..d854509bb082 100644
> --- a/fs/erofs/inode.c
> +++ b/fs/erofs/inode.c
> @@ -8,8 +8,39 @@
>   
>   #include <trace/events/erofs.h>
>   
> -static void *erofs_read_inode(struct erofs_buf *buf,
> -			      struct inode *inode, unsigned int *ofs)
> +static int erofs_fill_symlink(struct inode *inode, void *kaddr,
> +			      unsigned int m_pofs)
> +{
> +	struct erofs_inode *vi = EROFS_I(inode);
> +	unsigned int bsz = i_blocksize(inode);
> +	char *lnk;
> +
> +	/* if it cannot be handled with fast symlink scheme */
> +	if (vi->datalayout != EROFS_INODE_FLAT_INLINE ||
> +	    inode->i_size >= bsz || inode->i_size < 0) {
> +		return 0;
> +	}
> +
> +	m_pofs += vi->xattr_isize;
> +	/* inline symlink data shouldn't cross block boundary */
> +	if (m_pofs + inode->i_size > bsz) {
> +		erofs_err(inode->i_sb,
> +			  "inline data cross block boundary @ nid %llu",
> +			  vi->nid);

Since you move the code block of erofs_fill_symlink,

I think it'd be better to update this statement to
		erofs_err(inode->i_sb, "inline data cross block boundary @ nid %llu",
			  vi->nid);

As I mentioned, the print message doesn't have line limitation.

> +		DBG_BUGON(1);
> +		return -EFSCORRUPTED;
> +	}
> +
> +	lnk = kmemdup_nul(kaddr + m_pofs, inode->i_size, GFP_KERNEL);
> +
> +	if (!lnk)
> +		return -ENOMEM;

As I mentioned in the previous patch, so it could become
	inode->i_link = kmemdup_nul(kaddr + m_pofs, inode->i_size, GFP_KERNEL);
	return inode->i_link ? 0 : -ENOMEM;

> +
> +	inode->i_link = lnk;
> +	return 0;
> +}
> +
> +static int erofs_read_inode(struct inode *inode)
>   {
>   	struct super_block *sb = inode->i_sb;
>   	struct erofs_sb_info *sbi = EROFS_SB(sb);
> @@ -20,20 +51,21 @@ static void *erofs_read_inode(struct erofs_buf *buf,
>   	struct erofs_inode_compact *dic;
>   	struct erofs_inode_extended *die, *copied = NULL;
>   	union erofs_inode_i_u iu;
> -	unsigned int ifmt;
> +	struct erofs_buf buf;
> +	unsigned int ifmt, ofs;
>   	int err;
>   
>   	blkaddr = erofs_blknr(sb, inode_loc);
> -	*ofs = erofs_blkoff(sb, inode_loc);
> +	ofs = erofs_blkoff(sb, inode_loc);
>   
> -	kaddr = erofs_read_metabuf(buf, sb, erofs_pos(sb, blkaddr), EROFS_KMAP);
> +	kaddr = erofs_read_metabuf(&buf, sb, erofs_pos(sb, blkaddr), EROFS_KMAP);
>   	if (IS_ERR(kaddr)) {
>   		erofs_err(sb, "failed to get inode (nid: %llu) page, err %ld",
>   			  vi->nid, PTR_ERR(kaddr));
> -		return kaddr;
> +		return PTR_ERR(kaddr);
>   	}
>   
> -	dic = kaddr + *ofs;
> +	dic = kaddr + ofs;
>   	ifmt = le16_to_cpu(dic->i_format);
>   	if (ifmt & ~EROFS_I_ALL) {
>   		erofs_err(sb, "unsupported i_format %u of nid %llu",
> @@ -54,11 +86,11 @@ static void *erofs_read_inode(struct erofs_buf *buf,
>   	case EROFS_INODE_LAYOUT_EXTENDED:
>   		vi->inode_isize = sizeof(struct erofs_inode_extended);
>   		/* check if the extended inode acrosses block boundary */
> -		if (*ofs + vi->inode_isize <= sb->s_blocksize) {
> -			*ofs += vi->inode_isize;
> +		if (ofs + vi->inode_isize <= sb->s_blocksize) {
> +			ofs += vi->inode_isize;
>   			die = (struct erofs_inode_extended *)dic;
>   		} else {
> -			const unsigned int gotten = sb->s_blocksize - *ofs;
> +			const unsigned int gotten = sb->s_blocksize - ofs;
>   
>   			copied = kmalloc(vi->inode_isize, GFP_KERNEL);
>   			if (!copied) {
> @@ -66,16 +98,16 @@ static void *erofs_read_inode(struct erofs_buf *buf,
>   				goto err_out;
>   			}
>   			memcpy(copied, dic, gotten);
> -			kaddr = erofs_read_metabuf(buf, sb, erofs_pos(sb, blkaddr + 1),
> +			kaddr = erofs_read_metabuf(&buf, sb, erofs_pos(sb, blkaddr + 1),
>   						   EROFS_KMAP);
>   			if (IS_ERR(kaddr)) {
>   				erofs_err(sb, "failed to get inode payload block (nid: %llu), err %ld",
>   					  vi->nid, PTR_ERR(kaddr));
>   				kfree(copied);
> -				return kaddr;
> +				return PTR_ERR(kaddr);
>   			}
> -			*ofs = vi->inode_isize - gotten;
> -			memcpy((u8 *)copied + gotten, kaddr, *ofs);
> +			ofs = vi->inode_isize - gotten;
> +			memcpy((u8 *)copied + gotten, kaddr, ofs);
>   			die = copied;
>   		}
>   		vi->xattr_isize = erofs_xattr_ibody_size(die->i_xattr_icount);
> @@ -95,7 +127,7 @@ static void *erofs_read_inode(struct erofs_buf *buf,
>   		break;
>   	case EROFS_INODE_LAYOUT_COMPACT:
>   		vi->inode_isize = sizeof(struct erofs_inode_compact);
> -		*ofs += vi->inode_isize;
> +		ofs += vi->inode_isize;
>   		vi->xattr_isize = erofs_xattr_ibody_size(dic->i_xattr_icount);
>   
>   		inode->i_mode = le16_to_cpu(dic->i_mode);
> @@ -119,6 +151,11 @@ static void *erofs_read_inode(struct erofs_buf *buf,
>   	case S_IFREG:
>   	case S_IFDIR:
>   	case S_IFLNK:
> +		if(S_ISLNK(inode->i_mode)) {
> +			err = erofs_fill_symlink(inode, kaddr, ofs);
> +			if(err)

missing a blank:
			if (err)


> +				goto err_out;
> +		}
>   		vi->raw_blkaddr = le32_to_cpu(iu.raw_blkaddr);
>   		break;
>   	case S_IFCHR:
> @@ -165,63 +202,29 @@ static void *erofs_read_inode(struct erofs_buf *buf,
>   		inode->i_blocks = round_up(inode->i_size, sb->s_blocksize) >> 9;
>   	else
>   		inode->i_blocks = nblks << (sb->s_blocksize_bits - 9);
> -	return kaddr;
> +
> +	erofs_put_metabuf(&buf);
> +	return 0;
>   
>   err_out:

I wonder this can be unified too as:

err_out:
	DBG_BUGON(err);
	kfree(copied);			I'm not sure if it's really needed now.
	erofs_put_metabuf(&buf);
	return err;

>   	DBG_BUGON(1);
>   	kfree(copied);
> -	erofs_put_metabuf(buf);
> -	return ERR_PTR(err);
> +	erofs_put_metabuf(&buf);
> +	return err;
>   }
>   
> -static int erofs_fill_symlink(struct inode *inode, void *kaddr,
> -			      unsigned int m_pofs)
> -{
> -	struct erofs_inode *vi = EROFS_I(inode);
> -	unsigned int bsz = i_blocksize(inode);
> -	char *lnk;
> -
> -	/* if it cannot be handled with fast symlink scheme */
> -	if (vi->datalayout != EROFS_INODE_FLAT_INLINE ||
> -	    inode->i_size >= bsz || inode->i_size < 0) {
> -		inode->i_op = &erofs_symlink_iops;
> -		return 0;
> -	}
> -
> -	m_pofs += vi->xattr_isize;
> -	/* inline symlink data shouldn't cross block boundary */
> -	if (m_pofs + inode->i_size > bsz) {
> -		erofs_err(inode->i_sb,
> -			  "inline data cross block boundary @ nid %llu",
> -			  vi->nid);
> -		DBG_BUGON(1);
> -		return -EFSCORRUPTED;
> -	}
> -
> -	lnk = kmemdup_nul(kaddr + m_pofs, inode->i_size, GFP_KERNEL);
> -
> -	if (!lnk)
> -		return -ENOMEM;
> -
> -	inode->i_link = lnk;
> -	inode->i_op = &erofs_fast_symlink_iops;
> -	return 0;
> -}
>   
>   static int erofs_fill_inode(struct inode *inode)
>   {
>   	struct erofs_inode *vi = EROFS_I(inode);
> -	struct erofs_buf buf = __EROFS_BUF_INITIALIZER;
> -	void *kaddr;
> -	unsigned int ofs;
>   	int err = 0;
>   
>   	trace_erofs_fill_inode(inode);
>   
>   	/* read inode base data from disk */
> -	kaddr = erofs_read_inode(&buf, inode, &ofs);
> -	if (IS_ERR(kaddr))
> -		return PTR_ERR(kaddr);
> +	err = erofs_read_inode(inode);
> +	if (err)
> +		goto out_unlock;

out_unlock can be avoided too, just
		return err;

>   
>   	/* setup the new inode */
>   	switch (inode->i_mode & S_IFMT) {
> @@ -238,9 +241,11 @@ static int erofs_fill_inode(struct inode *inode)
>   		inode_nohighmem(inode);
>   		break;
>   	case S_IFLNK:
> -		err = erofs_fill_symlink(inode, kaddr, ofs);
> -		if (err)
> -			goto out_unlock;
> +		if (inode->i_link)
> +			inode->i_op = &erofs_fast_symlink_iops;
> +		else
> +			inode->i_op = &erofs_symlink_iops;
> +
>   		inode_nohighmem(inode);
>   		break;
>   	case S_IFCHR:
> @@ -273,7 +278,6 @@ static int erofs_fill_inode(struct inode *inode)
>   #endif
>   	}
>   out_unlock:

Remove this.

Thanks,
Gao Xiang

Re: [PATCH V2 2/2] erofs: refactor read_inode calling convention

[Yiyang Wu]

On Mon, Sep 02, 2024 at 03:18:54PM GMT, Gao Xiang wrote:
> 
> 
> > +	m_pofs += vi->xattr_isize;
> > +	/* inline symlink data shouldn't cross block boundary */
> > +	if (m_pofs + inode->i_size > bsz) {
> > +		erofs_err(inode->i_sb,
> > +			  "inline data cross block boundary @ nid %llu",
> > +			  vi->nid);
> 
> Since you move the code block of erofs_fill_symlink,
> 
> I think it'd be better to update this statement to
> 		erofs_err(inode->i_sb, "inline data cross block boundary @ nid %llu",
> 			  vi->nid);
> 
> As I mentioned, the print message doesn't have line limitation.

I just simply cannot tell the difference here. But since you put it
here, I will change this in the next version.

> > +		DBG_BUGON(1);
> > +		return -EFSCORRUPTED;
> > +	}
> > +
> > +	lnk = kmemdup_nul(kaddr + m_pofs, inode->i_size, GFP_KERNEL);
> > +
> > +	if (!lnk)
> > +		return -ENOMEM;
> 
> As I mentioned in the previous patch, so it could become
> 	inode->i_link = kmemdup_nul(kaddr + m_pofs, inode->i_size, GFP_KERNEL);
> 	return inode->i_link ? 0 : -ENOMEM;
> 

Looks good to me.

> > +		if(S_ISLNK(inode->i_mode)) {
> > +			err = erofs_fill_symlink(inode, kaddr, ofs);
> > +			if(err)
> 
> missing a blank:
> 			if (err)
> 
> 

Fixed here.
> > +				goto err_out;
> > +		}
> >   		vi->raw_blkaddr = le32_to_cpu(iu.raw_blkaddr);
> >   		break;
> >   	case S_IFCHR:
> > @@ -165,63 +202,29 @@ static void *erofs_read_inode(struct erofs_buf *buf,
> >   		inode->i_blocks = round_up(inode->i_size, sb->s_blocksize) >> 9;
> >   	else
> >   		inode->i_blocks = nblks << (sb->s_blocksize_bits - 9);
> > -	return kaddr;
> > +
> > +	erofs_put_metabuf(&buf);
> > +	return 0;
> >   err_out:
> 
> I wonder this can be unified too as:
> 
> err_out:
> 	DBG_BUGON(err);
> 	kfree(copied);			I'm not sure if it's really needed now.
I agree. copied doesn't need to be freed anymore as it's already freed when error happens in the first switch block of inode format part. 
Will be fixed in the next version.
> 	erofs_put_metabuf(&buf);
> 	return err;
> 
> >   	DBG_BUGON(1);
> >   	kfree(copied);
> > -	erofs_put_metabuf(buf);
> > -	return ERR_PTR(err);
> > +	erofs_put_metabuf(&buf);
> > +	return err;
> >   }
> > -static int erofs_fill_symlink(struct inode *inode, void *kaddr,
> > -			      unsigned int m_pofs)
> > -{
> > -	struct erofs_inode *vi = EROFS_I(inode);
> > -	unsigned int bsz = i_blocksize(inode);
> > -	char *lnk;
> > -
> > -	/* if it cannot be handled with fast symlink scheme */
> > -	if (vi->datalayout != EROFS_INODE_FLAT_INLINE ||
> > -	    inode->i_size >= bsz || inode->i_size < 0) {
> > -		inode->i_op = &erofs_symlink_iops;
> > -		return 0;
> > -	}
> > -
> > -	m_pofs += vi->xattr_isize;
> > -	/* inline symlink data shouldn't cross block boundary */
> > -	if (m_pofs + inode->i_size > bsz) {
> > -		erofs_err(inode->i_sb,
> > -			  "inline data cross block boundary @ nid %llu",
> > -			  vi->nid);
> > -		DBG_BUGON(1);
> > -		return -EFSCORRUPTED;
> > -	}
> > -
> > -	lnk = kmemdup_nul(kaddr + m_pofs, inode->i_size, GFP_KERNEL);
> > -
> > -	if (!lnk)
> > -		return -ENOMEM;
> > -
> > -	inode->i_link = lnk;
> > -	inode->i_op = &erofs_fast_symlink_iops;
> > -	return 0;
> > -}
> >   static int erofs_fill_inode(struct inode *inode)
> >   {
> >   	struct erofs_inode *vi = EROFS_I(inode);
> > -	struct erofs_buf buf = __EROFS_BUF_INITIALIZER;
> > -	void *kaddr;
> > -	unsigned int ofs;
> >   	int err = 0;
> >   	trace_erofs_fill_inode(inode);
> >   	/* read inode base data from disk */
> > -	kaddr = erofs_read_inode(&buf, inode, &ofs);
> > -	if (IS_ERR(kaddr))
> > -		return PTR_ERR(kaddr);
> > +	err = erofs_read_inode(inode);
> > +	if (err)
> > +		goto out_unlock;
> 
> out_unlock can be avoided too, just
> 		return err;
> 
> >   	/* setup the new inode */
> >   	switch (inode->i_mode & S_IFMT) {
> > @@ -238,9 +241,11 @@ static int erofs_fill_inode(struct inode *inode)
> >   		inode_nohighmem(inode);
> >   		break;
> >   	case S_IFLNK:
> > -		err = erofs_fill_symlink(inode, kaddr, ofs);
> > -		if (err)
> > -			goto out_unlock;
> > +		if (inode->i_link)
> > +			inode->i_op = &erofs_fast_symlink_iops;
> > +		else
> > +			inode->i_op = &erofs_symlink_iops;
> > +
> >   		inode_nohighmem(inode);
> >   		break;
> >   	case S_IFCHR:
> > @@ -273,7 +278,6 @@ static int erofs_fill_inode(struct inode *inode)
> >   #endif
> >   	}
> >   out_unlock:
> 
> Remove this.

I'm not sure whether we need to remove this becasue device inodes do not
need other operation bindings below and still needs the out_unlock to be
early out. Another solution is to use the early return but i deem the
goto solution is clearer.

> 
> Thanks,
> Gao Xiang

Best Regards,
Yiyang Wu