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
