Re: [PATCH] xfs: introduce protection for drop nlink

[Darrick J. Wong]

On Fri, Aug 25, 2023 at 05:09:20PM +0800, cheng.lin130@xxxxxxxxxx wrote:
> > On Thu, Aug 24, 2023 at 03:43:52PM +0800, cheng.lin130@xxxxxxxxxx wrote:
> >> From: Cheng Lin <cheng.lin130@xxxxxxxxxx>
> >> An dir nlinks overflow which down form 0 to 0xffffffff, cause the
> >> directory to become unusable until the next xfs_repair run.
> > Hmmm.  How does this ever happen?
> > IMO, if it does happen, we need to fix whatever bug that causes it
> > to happen, not issue a warning and do nothing about the fact we
> > just hit a corrupt inode state...
> Yes, I'm very agree with your opinion. But I don't know how it happened,
> and how to reproduce it.

Wait, is this the result of a customer problem?  Or static analysis?

> >> Introduce protection for drop nlink to reduce the impact of this.
> >> And produce a warning for directory nlink error during remove.
> >>
> >> Signed-off-by: Cheng Lin <cheng.lin130@xxxxxxxxxx>
> >> ---
> >>  fs/xfs/xfs_inode.c | 16 +++++++++++++++-
> >>  1 file changed, 15 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> >> index 9e62cc5..536dbe4 100644
> >> --- a/fs/xfs/xfs_inode.c
> >> +++ b/fs/xfs/xfs_inode.c
> >> @@ -919,6 +919,15 @@ STATIC int xfs_iunlink_remove(struct xfs_trans *tp, struct xfs_perag *pag,

I'm not sure why your diff program thinks this hunk is from
xfs_iunlink_remove, seeing as the line numbers of the chunk point to
xfs_droplink.  Maybe that's what's going on in this part of the thread?

> >>      xfs_trans_t *tp,
> >>      xfs_inode_t *ip)
> >>  {
> >> +    xfs_mount_t     *mp;
> >> +
> >> +    if (VFS_I(ip)->i_nlink == 0) {
> >> +        mp = ip->i_mount;
> >> +        xfs_warn(mp, "%s: Deleting inode %llu with no links.",
> >> +             __func__, ip->i_ino);
> >> +        return 0;
> >> +    }
> > This is obviously incorrect - whiteout inodes (RENAME_WHITEOUT) have an
> > i_nlink of zero when they are removed from the unlinked list. As do
> > O_TMPFILE inodes - when they are linked into the filesystem, we
> > explicitly check for i_nlink being zero before calling
> > xfs_iunlink_remove().
> I am not familiar with the above process. You means there is such a
> scenario, even if it is (i_nlink==0), it still needs to run drop_nlink()
> in xfs_droplink()? But this will cause i_nlink to underflow to 0xffffffff.

xfs_iunlink_remove doesn't touch the link count.  I think Dave is
confused because of the misleading --show-c-function output.

> >> +
> >>      xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
> >>
> >>      drop_nlink(VFS_I(ip));
> > Wait a second - this code doesn't match an upstream kernel. What
> > kernel did you make this patch against?
> It's kernel mainline linux-6.5-rc7

...and what did you use to generate the patch?  git diff?

--D

> 
> Thanks.
> > -Dave.
> > --
> > Dave Chinner
> > david@xxxxxxxxxxxxx