On Mon, Apr 15, 2024 at 10:12:12PM -0700, Christoph Hellwig wrote:
> > + if (op != XFS_ATTRUPDATE_REMOVE || xfs_inode_hasattr(dp)) {
> > error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK,
> > XFS_IEXT_ATTR_MANIP_CNT(rmt_blks));
> > if (error == -EFBIG)
>
> No need to change this in the current patch, but checking for a remove
> on an inodes without attrs just to skip the extent count upgrade
> and not fail the whole operation is a little silly :)
Yeah, I don't think there's a point to checking @op here. The only
code we need here is skipping the overflow checks if !xfs_inode_hasattr.
Separate patch.
> > @@ -203,7 +203,8 @@ __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> > xfs_acl_to_disk(args.value, acl);
> > }
> >
> > - error = xfs_attr_change(&args, XFS_ATTRUPDATE_UPSERT);
> > + error = xfs_attr_change(&args, args.value ? XFS_ATTRUPDATE_UPSERT :
> > + XFS_ATTRUPDATE_REMOVE);
>
> Given that we have a conditional for removing vs setting right above
> i'd clean this up a bit:
>
> xfs_acl_to_disk(args.value, acl);
> error = xfs_attr_change(&args, XFS_ATTRUPDATE_UPSERT)
> kvfree(args.value);
> } else {
> error = xfs_attr_change(&args, XFS_ATTRUPDATE_REMOVE)
> /*
> * If the attribute didn't exist to start with that's fine.
> */
> if (error == -ENOATTR)
> error = 0;
> }
Done.
> Otherwise looks good:
>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
Thanks!
--D
