[PATCH v3] xfs: introduce protection for drop nlink

[PATCH v3] xfs: introduce protection for drop nlink

[cheng.lin130]

From: Cheng Lin <cheng.lin130@zte.com.cn>

When abnormal drop_nlink are detected on the inode,
shutdown filesystem, to avoid corruption propagation.

Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn>
---
 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;
+		return -EFSCORRUPTED;
+	}
+
 	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);

 	drop_nlink(VFS_I(ip));
-- 
2.18.1

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

[Dave Chinner]

On Wed, Sep 13, 2023 at 05:44:45PM +0800, cheng.lin130@zte.com.cn wrote:
> From: Cheng Lin <cheng.lin130@zte.com.cn>
> 
> When abnormal drop_nlink are detected on the inode,
> shutdown filesystem, to avoid corruption propagation.
> 
> Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn>
> ---
>  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.

-Dave.

-- 
Dave Chinner
david@fromorbit.com

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

[cheng.lin130]

> On Wed, Sep 13, 2023 at 05:44:45PM +0800, cheng.lin130@zte.com.cn wrote:
> > From: Cheng Lin <cheng.lin130@zte.com.cn>
> >
> > When abnormal drop_nlink are detected on the inode,
> > shutdown filesystem, to avoid corruption propagation.
> >
> > Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn>
> > ---
> >  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.
I found the transaction maybe not dirty at this point. Dirty the transaction
can make sure to shutdown fs in subsequent xfs_trans_cancel().

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

[cheng.lin130]

> On Wed, Sep 13, 2023 at 05:44:45PM +0800, cheng.lin130@zte.com.cn wrote:
> > From: Cheng Lin <cheng.lin130@zte.com.cn>
> >
> > When abnormal drop_nlink are detected on the inode,
> > shutdown filesystem, to avoid corruption propagation.
> >
> > Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn>
> > ---
> >  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?
> -Dave.
> --
> Dave Chinner
> david@fromorbit.com

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

[Dave Chinner]

On Fri, Sep 15, 2023 at 05:50:56PM +0800, cheng.lin130@zte.com.cn wrote:
> > On Wed, Sep 13, 2023 at 05:44:45PM +0800, cheng.lin130@zte.com.cn wrote:
> > > From: Cheng Lin <cheng.lin130@zte.com.cn>
> > >
> > > When abnormal drop_nlink are detected on the inode,
> > > shutdown filesystem, to avoid corruption propagation.
> > >
> > > Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn>
> > > ---
> > >  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@fromorbit.com

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

[cheng.lin130]

> On Fri, Sep 15, 2023 at 05:50:56PM +0800, cheng.lin130@zte.com.cn wrote:
> > > On Wed, Sep 13, 2023 at 05:44:45PM +0800, cheng.lin130@zte.com.cn wrote:
> > > > From: Cheng Lin <cheng.lin130@zte.com.cn>
> > > >
> > > > When abnormal drop_nlink are detected on the inode,
> > > > shutdown filesystem, to avoid corruption propagation.
> > > >
> > > > Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn>
> > > > ---
> > > >  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@fromorbit.com

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

[Dave Chinner]

On Mon, Sep 18, 2023 at 11:44:53AM +0800, cheng.lin130@zte.com.cn wrote:
> > On Fri, Sep 15, 2023 at 05:50:56PM +0800, cheng.lin130@zte.com.cn wrote:
> > > > On Wed, Sep 13, 2023 at 05:44:45PM +0800, cheng.lin130@zte.com.cn wrote:
> > > > > From: Cheng Lin <cheng.lin130@zte.com.cn>
> > > > >
> > > > > When abnormal drop_nlink are detected on the inode,
> > > > > shutdown filesystem, to avoid corruption propagation.
> > > > >
> > > > > Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn>
> > > > > ---
> > > > >  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.

Yes, and that's perfectly fine. The transaction cancelling code has
worked this way for the past 20 years or so...

Indeed, you said your customer wants the system to stay alive if possible,
right? Well, so do we.

If the parent directory has a bogus nlink count, and that prevents
us from removing items from the directory, then as long as we
haven't dirtied anything and we can return a -EFSCORRUPTED error to
userspace to say the unlink failed and we don't have to shut the
filesystem down. All we now have is a directory that has objects in
it that can't be removed....

For a higher level perspective, we only need to shut the filesystem
down if we cannot safely back out of the modification operation that
was requested. Whilst the transaction is clean, we can safely return
errors to userspace and continue operation because everything in
memory and on disk is still consistent, even if we have found a
corruption in non-crtical the metadata. Just returning an error to
userspace can't make the problem any worse.

This also is how we treat corruption that is found during read
operations - we return -EFSCORRUPTED to userspace because something in
the directory or inode we were trying to read from was corrupted. We
do not need to shut down the filesystem because there is
no risk of making things worse or the in-memory filesystem state
getting out of sync with the on-disk state.

It is only when we are trying to modify something that corruption
becomes a problem with fatal consequences. Once we've made a
modification, the in-memory state is different to the on-disk state
and whilst we are in that state any corruption we discover becomes
fatal. That is because there is no way to reconcile the changes
we've already made in memory with what is on-disk - we don't know
that the in-memory changes are good because we tripped over
corruption, and so we must not propagate bad in-memory state and
metadata to disk over the top of what may be still be uncorrupted
metadata on disk.

This "in memory matches on disk" state is effectively what the dirty
flag in the transaction tracks, and it's done as part of the normal
running of a transaction as items are tagged for logging. Marking a
transaction dirty that has nothign tagged for logging is actually an
incorrect state; we may handle it correctly, but it should never
actually occur and we should definitely not be open coding dirtying
of transactions to create this state.

IOWs, the transaction modification error handling paths already do
the right thing according to the state carried by the transaction at
the time the error was encountered.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[cheng.lin130]

> On Mon, Sep 18, 2023 at 11:44:53AM +0800, cheng.lin130@zte.com.cn wrote:
> > > On Fri, Sep 15, 2023 at 05:50:56PM +0800, cheng.lin130@zte.com.cn wrote:
> > > > > On Wed, Sep 13, 2023 at 05:44:45PM +0800, cheng.lin130@zte.com.cn wrote:
> > > > > > From: Cheng Lin <cheng.lin130@zte.com.cn>
> > > > > >
> > > > > > When abnormal drop_nlink are detected on the inode,
> > > > > > shutdown filesystem, to avoid corruption propagation.
> > > > > >
> > > > > > Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn>
> > > > > > ---
> > > > > >  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.
> Yes, and that's perfectly fine. The transaction cancelling code has
> worked this way for the past 20 years or so...
> Indeed, you said your customer wants the system to stay alive if possible,
> right? Well, so do we.
> If the parent directory has a bogus nlink count, and that prevents
> us from removing items from the directory, then as long as we
> haven't dirtied anything and we can return a -EFSCORRUPTED error to
> userspace to say the unlink failed and we don't have to shut the
> filesystem down. All we now have is a directory that has objects in
> it that can't be removed....
> For a higher level perspective, we only need to shut the filesystem
> down if we cannot safely back out of the modification operation that
> was requested. Whilst the transaction is clean, we can safely return
> errors to userspace and continue operation because everything in
> memory and on disk is still consistent, even if we have found a
> corruption in non-crtical the metadata. Just returning an error to
> userspace can't make the problem any worse.
> This also is how we treat corruption that is found during read
> operations - we return -EFSCORRUPTED to userspace because something in
> the directory or inode we were trying to read from was corrupted. We
> do not need to shut down the filesystem because there is
> no risk of making things worse or the in-memory filesystem state
> getting out of sync with the on-disk state.
> It is only when we are trying to modify something that corruption
> becomes a problem with fatal consequences. Once we've made a
> modification, the in-memory state is different to the on-disk state
> and whilst we are in that state any corruption we discover becomes
> fatal. That is because there is no way to reconcile the changes
> we've already made in memory with what is on-disk - we don't know
> that the in-memory changes are good because we tripped over
> corruption, and so we must not propagate bad in-memory state and
> metadata to disk over the top of what may be still be uncorrupted
> metadata on disk.
> This "in memory matches on disk" state is effectively what the dirty
> flag in the transaction tracks, and it's done as part of the normal
> running of a transaction as items are tagged for logging. Marking a
> transaction dirty that has nothign tagged for logging is actually an
> incorrect state; we may handle it correctly, but it should never
> actually occur and we should definitely not be open coding dirtying
> of transactions to create this state.
> IOWs, the transaction modification error handling paths already do
> the right thing according to the state carried by the transaction at
> the time the error was encountered.
Thanks for your detailed explainations, I get it.
> -Dave.
> --
> Dave Chinner
> david@fromorbit.com

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

[Darrick J. Wong]

On Mon, Sep 18, 2023 at 03:48:38PM +1000, Dave Chinner wrote:
> On Mon, Sep 18, 2023 at 11:44:53AM +0800, cheng.lin130@zte.com.cn wrote:
> > > On Fri, Sep 15, 2023 at 05:50:56PM +0800, cheng.lin130@zte.com.cn wrote:
> > > > > On Wed, Sep 13, 2023 at 05:44:45PM +0800, cheng.lin130@zte.com.cn wrote:
> > > > > > From: Cheng Lin <cheng.lin130@zte.com.cn>
> > > > > >
> > > > > > When abnormal drop_nlink are detected on the inode,
> > > > > > shutdown filesystem, to avoid corruption propagation.
> > > > > >
> > > > > > Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn>
> > > > > > ---
> > > > > >  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.
> 
> Yes, and that's perfectly fine. The transaction cancelling code has
> worked this way for the past 20 years or so...
> 
> Indeed, you said your customer wants the system to stay alive if possible,
> right? Well, so do we.
> 
> If the parent directory has a bogus nlink count, and that prevents
> us from removing items from the directory, then as long as we
> haven't dirtied anything and we can return a -EFSCORRUPTED error to
> userspace to say the unlink failed and we don't have to shut the
> filesystem down. All we now have is a directory that has objects in
> it that can't be removed....
> 
> For a higher level perspective, we only need to shut the filesystem
> down if we cannot safely back out of the modification operation that
> was requested. Whilst the transaction is clean, we can safely return
> errors to userspace and continue operation because everything in
> memory and on disk is still consistent, even if we have found a
> corruption in non-crtical the metadata. Just returning an error to
> userspace can't make the problem any worse.
> 
> This also is how we treat corruption that is found during read
> operations - we return -EFSCORRUPTED to userspace because something in
> the directory or inode we were trying to read from was corrupted. We
> do not need to shut down the filesystem because there is
> no risk of making things worse or the in-memory filesystem state
> getting out of sync with the on-disk state.
> 
> It is only when we are trying to modify something that corruption
> becomes a problem with fatal consequences. Once we've made a
> modification, the in-memory state is different to the on-disk state
> and whilst we are in that state any corruption we discover becomes
> fatal. That is because there is no way to reconcile the changes
> we've already made in memory with what is on-disk - we don't know
> that the in-memory changes are good because we tripped over
> corruption, and so we must not propagate bad in-memory state and
> metadata to disk over the top of what may be still be uncorrupted
> metadata on disk.

It'd be a massive effort, but wouldn't it be fun if one could attach
defer ops to a transaction that updated incore state on commit but
otherwise never appeared on disk?

Let me cogitate on that during part 2 of vacation...

--D

> This "in memory matches on disk" state is effectively what the dirty
> flag in the transaction tracks, and it's done as part of the normal
> running of a transaction as items are tagged for logging. Marking a
> transaction dirty that has nothign tagged for logging is actually an
> incorrect state; we may handle it correctly, but it should never
> actually occur and we should definitely not be open coding dirtying
> of transactions to create this state.
> 
> IOWs, the transaction modification error handling paths already do
> the right thing according to the state carried by the transaction at
> the time the error was encountered.
> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

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

[Dave Chinner]

On Mon, Sep 18, 2023 at 08:33:35PM -0700, Darrick J. Wong wrote:
> On Mon, Sep 18, 2023 at 03:48:38PM +1000, Dave Chinner wrote:
> > It is only when we are trying to modify something that corruption
> > becomes a problem with fatal consequences. Once we've made a
> > modification, the in-memory state is different to the on-disk state
> > and whilst we are in that state any corruption we discover becomes
> > fatal. That is because there is no way to reconcile the changes
> > we've already made in memory with what is on-disk - we don't know
> > that the in-memory changes are good because we tripped over
> > corruption, and so we must not propagate bad in-memory state and
> > metadata to disk over the top of what may be still be uncorrupted
> > metadata on disk.
> 
> It'd be a massive effort, but wouldn't it be fun if one could attach
> defer ops to a transaction that updated incore state on commit but
> otherwise never appeared on disk?
>
> Let me cogitate on that during part 2 of vacation...

Sure, I'm interested to see what you might come up with.

My thoughts on rollback of dirty transactions come from a different
perspective.

Conceptually being able to roll back individual transactions isn't
that difficult. All it takes is a bit more memory and CPU - when we
join the item to the transaction we take a copy of the item we are
about to modify.

If we then cancel a dirty transaction, we then roll back all the
dirty items to their original state before we unlock them.  This
works fine for all the on-disk stuff we track in log items.

I have vague thoughts about how this could potentially be tied into
the shadow buffers we already use for keeping a delta copy of all
the committed in-memory changes in the CIL that we haven't yet
committed to the journal - that's actually the entire delta between
what is on disk and what we've changed prior to the current
transaction we are cancelling.

Hence, in theory, a rollback for a dirty log item is simply "read it
from disk again, copy the CIL shadow buffer delta into it".

However, the complexity comes with trying to roll back associated
in-memory state changes that we don't track as log items.  e.g.
incore extent list changes, in memory inode flag state (e.g.
XFS_ISTALE), etc. that's where all the hard problems to solve lie, I
think.

Another problem is how do we rollback from the middle of an intent
(defer ops) chain? We have to complete that chain for things to end
up consistent on disk, so we can't just cancel the current
transaction and say we are done and everything is clean.  Maybe
that's what you are thinking of here - each chain has an "undo"
intent chain that can roll back all the changes already made?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Darrick J. Wong]

On Wed, Sep 20, 2023 at 03:53:40PM +1000, Dave Chinner wrote:
> On Mon, Sep 18, 2023 at 08:33:35PM -0700, Darrick J. Wong wrote:
> > On Mon, Sep 18, 2023 at 03:48:38PM +1000, Dave Chinner wrote:
> > > It is only when we are trying to modify something that corruption
> > > becomes a problem with fatal consequences. Once we've made a
> > > modification, the in-memory state is different to the on-disk state
> > > and whilst we are in that state any corruption we discover becomes
> > > fatal. That is because there is no way to reconcile the changes
> > > we've already made in memory with what is on-disk - we don't know
> > > that the in-memory changes are good because we tripped over
> > > corruption, and so we must not propagate bad in-memory state and
> > > metadata to disk over the top of what may be still be uncorrupted
> > > metadata on disk.
> > 
> > It'd be a massive effort, but wouldn't it be fun if one could attach
> > defer ops to a transaction that updated incore state on commit but
> > otherwise never appeared on disk?
> >
> > Let me cogitate on that during part 2 of vacation...
> 
> Sure, I'm interested to see what you might come up with.
> 
> My thoughts on rollback of dirty transactions come from a different
> perspective.
> 
> Conceptually being able to roll back individual transactions isn't
> that difficult. All it takes is a bit more memory and CPU - when we
> join the item to the transaction we take a copy of the item we are
> about to modify.
> 
> If we then cancel a dirty transaction, we then roll back all the
> dirty items to their original state before we unlock them.  This
> works fine for all the on-disk stuff we track in log items.
> 
> I have vague thoughts about how this could potentially be tied into
> the shadow buffers we already use for keeping a delta copy of all
> the committed in-memory changes in the CIL that we haven't yet
> committed to the journal - that's actually the entire delta between
> what is on disk and what we've changed prior to the current
> transaction we are cancelling.
> 
> Hence, in theory, a rollback for a dirty log item is simply "read it
> from disk again, copy the CIL shadow buffer delta into it".

<nod> That's more or less the same as what I was thinking.

> However, the complexity comes with trying to roll back associated
> in-memory state changes that we don't track as log items.  e.g.
> incore extent list changes, in memory inode flag state (e.g.
> XFS_ISTALE), etc. that's where all the hard problems to solve lie, I
> think.

Yeah.  I was thinking that each of those incore state changes could be
implemented as a defer_ops that have NOP ->create_intent and
->create_done functions.  The ->finish_item would actually update the
incore structure.  This would be a very large project, and I'm not sure
that it wouldn't be easier to snapshot the xfs_inode fields themselves,
similar to how inode log items snapshot xfs_dinode fields.

(Snapshotting probably doesn't work for the more complex incore
inode structures.)

Kent has been wrangling with this problem for a while in bcachefs and I
think he's actually gotten the rollbacks to work more or less correctly.
He told me that it was a significant restructuring of his codebase even
though *everything* is tracked in btrees and the cursor abstraction
there is more robust than xfs.

> Another problem is how do we rollback from the middle of an intent
> (defer ops) chain? We have to complete that chain for things to end
> up consistent on disk, so we can't just cancel the current
> transaction and say we are done and everything is clean.  Maybe
> that's what you are thinking of here - each chain has an "undo"
> intent chain that can roll back all the changes already made?

Yes.  Every time we call ->finish_item on a log intent item, we also log
a new intent item that undoes whatever that step did.  These items we'll
call "log undo intent" items, and put them on a separate list, e.g.
tp->t_undoops.  If the chain completes successfully then the last step
is to abort everything on t_undoops to release all that memory.

If the chain does not succeed, then we'd abort the intents on t_dfops,
splice t_undoops onto t_dfops, and call xfs_defer_finish to write the
log undo intent items to disk and finish them.  If /that/ fails then we
have to shutdown.

I think this also means that buffer updates that are logged from a
->finish_item function should not be cancelled per above, since the undo
intent item will take care of that.  That would be easy if btree updates
made by an efi/cui/rui items used ordered buffers instead of logging
them directly like we do now.

For bui items, I think we'd need ordered buffers for bmbt updates and
snapshotting inode items for the inode updates themselves.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com