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
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
