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
