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

[cheng.lin130]

> On Fri, Sep 15, 2023 at 05:50:56PM +0800, cheng.lin130@xxxxxxxxxx wrote:
> > > On Wed, Sep 13, 2023 at 05:44:45PM +0800, cheng.lin130@xxxxxxxxxx wrote:
> > > > From: Cheng Lin <cheng.lin130@xxxxxxxxxx>
> > > >
> > > > When abnormal drop_nlink are detected on the inode,
> > > > shutdown filesystem, to avoid corruption propagation.
> > > >
> > > > Signed-off-by: Cheng Lin <cheng.lin130@xxxxxxxxxx>
> > > > ---
> > > >  fs/xfs/xfs_inode.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > > index 9e62cc500..40cc106ae 100644
> > > > --- a/fs/xfs/xfs_inode.c
> > > > +++ b/fs/xfs/xfs_inode.c
> > > > @@ -919,6 +919,15 @@ xfs_droplink(
> > > >      xfs_trans_t *tp,
> > > >      xfs_inode_t *ip)
> > > >  {
> > > > +
> > > > +    if (VFS_I(ip)->i_nlink == 0) {
> > > > +        xfs_alert(ip->i_mount,
> > > > +              "%s: Deleting inode %llu with no links.",
> > > > +              __func__, ip->i_ino);
> > > > +        tp->t_flags |= XFS_TRANS_DIRTY;
> > > Marking the transaction dirty is not necessary.
> > > Otherwise this seems fine.
> > Another strategy:
> > Set nlink to an invalid value(like XFS_NLINK_PINNED), and
> > Complete this transaction before shutdown fs. To make sure
> > nlink not be zero. If the nlink of a directory are zero, it may
> > be cleaned up.
> > Is that appropriate?
> No, all I'm asking you to do is drop dirtying of the transaction
> from this patch because it is a) unnecessary and b) a layering
> violation.
> It is unnecessary because the transaction will almost always be
> dirty before we get to xfs_droplink(). In the cases where it isn't
> dirty (e.g. xfs_remove() on a directory) we explicitly check that
> nlink == 2 before proceeding to call xfs_droplink(). Hence we can't
> actually get to xfs_droplink() with a clean transaction, and so
If the corrupted inode is a parent directory, when remove its
subdirectory, the parent's nlink will be decreased to 0. But the
transaction of subdirectory removing is not dirty (There are not
check about the parent directory). In this situation, the transaction
will be failed and the filesystem will be alive.

> marking it dirty here on underrun is unnecessary as returning an
> error from xfs_droplink() will result in shutting down the
> filesystem when the transaction is cancelled.
> -Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx