Re: [PATCH v12 6/6] ntfs: check the dirty-state commit on remount and unmount

[Hyunchul Lee]

Hi Hongling,

2026년 9월 14일 (월) 오후 5:37, Hongling Zeng <zenghongling@xxxxxxxxxx>님이 작성:
>
> The remount-to-read-only path commits the updated volume flags with
> ntfs_commit_inode(), a void wrapper around __ntfs_write_inode(), and
> ignores the blkdev_issue_flush() return value, so a failed commit or
> flush is reported as success.  Once the remount has succeeded no
> persistence point is ever reached again: ntfs_put_super() skips
> read-only superblocks and the VFS never syncs one, so fail the
> remount unless the commit and the flush succeed.  The superblock
> then stays read-write and ntfs_put_super() retries the persistence
> at unmount.  The errors=remount-ro downgrade does not go through
> ntfs_reconfigure() and is unchanged.
>
> A zero-return commit is not trusted blindly: write_mft_record()
> redirties the record on allocation failure and reports success, so
> the $Volume inode is required to be clean afterwards.
>
> ntfs_put_super() discards the same commit error.  Call
> __ntfs_write_inode() there with the same dirty re-check and warn on
> failure, as put_super() cannot return an error.  The commit is
> skipped when the dirty-state sync itself failed, as that could write
> back an inconsistent flag state; a record left dirty by an earlier
> update is still committed at evict time.
>
> NVolErrors() is deliberately not used to detect the failure: it is
> sticky for the lifetime of the mount, so it cannot distinguish a
> fresh commit failure from errors recorded before the remount.
>
> Hibernated volumes: ntfs_sync_volume_dirty_state() is a no-op for
> them and never dirties the $Volume inode on such a mount, since the
> on-disk flags are already dirty and ntfs_set_volume_flags() has
> nothing to change.  The commit only runs if something dirtied the
> inode independently, as before this patch; mounting hibernated
> volumes read-only removes even that.
>
> Reported-by: Baolin Liu <liubaolin@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Hongling Zeng <zenghongling@xxxxxxxxxx>
> ---
>  fs/ntfs/super.c | 76 +++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 54 insertions(+), 22 deletions(-)
>
> diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
> index 463050ae1b0e..0228d7429596 100644
> --- a/fs/ntfs/super.c
> +++ b/fs/ntfs/super.c
> @@ -268,6 +268,7 @@ static int ntfs_reconfigure(struct fs_context *fc)
>  {
>         struct super_block *sb = fc->root->d_sb;
>         struct ntfs_volume *vol = NTFS_SB(sb);
> +       int err;
>
>         ntfs_debug("Entering with remount");
>


The initial sync_filesystem() call in this function  still ignores
the return value.

And the emergency remount(SB_FORCE) does not drain writers
that are already in progress. This function can commit the
clean flag while a writer is still modifying metadata.

It seems fine not to clear the flag for SB_FORCE.

> @@ -324,14 +325,39 @@ static int ntfs_reconfigure(struct fs_context *fc)
>                  * and ntfs_put_super() skips them, so the only remaining
>                  * write would be the evict-time commit at unmount, which
>                  * a crash never reaches.  An error recorded only after
> -                * the remount is still never persisted.
> +                * the remount is still never persisted; a failed commit
> +                * or flush fails the remount, leaving the superblock
> +                * read-write so ntfs_put_super() retries at unmount.
>                  */
> -               if (ntfs_sync_volume_dirty_state(vol)) {
> +               err = ntfs_sync_volume_dirty_state(vol);
> +               if (err) {
>                         ntfs_warning(sb,
>                                 "Failed to update dirty bit in volume information flags.  Run chkdsk.");
> -               } else if (NInoDirty(NTFS_I(vol->vol_ino))) {
> -                       ntfs_commit_inode(vol->vol_ino);
> -                       blkdev_issue_flush(sb->s_bdev);
> +                       return err;
> +               }
> +               if (NInoDirty(NTFS_I(vol->vol_ino))) {
> +                       /* ntfs_commit_inode() would discard the error. */
> +                       err = __ntfs_write_inode(vol->vol_ino, 1);
> +                       if (err) {
> +                               ntfs_warning(sb,
> +                                       "Failed to commit volume information flags.  Run chkdsk.");
> +                               return err;
> +                       }
> +                       /*
> +                        * write_mft_record() redirties the record on
> +                        * -ENOMEM and still reports success.
> +                        */
> +                       if (NInoDirty(NTFS_I(vol->vol_ino))) {
> +                               ntfs_warning(sb,
> +                                       "Volume information flags remain dirty after commit.  Run chkdsk.");
> +                               return -EIO;
> +                       }
> +                       err = blkdev_issue_flush(sb->s_bdev);
> +                       if (err) {
> +                               ntfs_warning(sb,
> +                                       "Failed to flush volume information flags.  Run chkdsk.");
> +                               return err;
> +                       }
>                 }
>         }
>
> @@ -1876,26 +1902,32 @@ static void ntfs_put_super(struct super_block *sb)
>                 if (ntfs_sync_volume_dirty_state(vol)) {
>                         ntfs_warning(sb,
>                                 "Failed to sync dirty bit in volume information flags.  Run chkdsk.");
> -               } else if (NVolErrors(vol)) {
> +               } else {
>                         /*
> -                        * The dirty bit is on disk now; only warn when the
> -                        * sync actually succeeded, or this message would
> -                        * contradict the one above.
> +                        * __ntfs_write_inode(), not the void
> +                        * ntfs_commit_inode() wrapper: the error can only
> +                        * be warned about here.  The mirror inode is only
> +                        * released below: writing the $Volume record (mft
> +                        * record number 3, below vol->mftmirr_size) mirrors
> +                        * it through ntfs_sync_mft_mirror(), which fails
> +                        * with -EIO once vol->mftmirr_ino is gone.
>                          */
> -                       ntfs_warning(sb,
> -                               "Volume has errors.  Leaving volume marked dirty.  Run chkdsk.");
> +                       if (__ntfs_write_inode(vol->vol_ino, 1)) {
> +                               ntfs_warning(sb,
> +                                       "Failed to commit volume information flags.  Run chkdsk.");
> +                       } else if (NInoDirty(NTFS_I(vol->vol_ino))) {
> +                               ntfs_warning(sb,
> +                                       "Volume information flags remain dirty after commit.  Run chkdsk.");
> +                       } else if (NVolErrors(vol)) {
> +                               /*
> +                                * Only warn once the commit has succeeded,
> +                                * or this could contradict a failure
> +                                * reported above.
> +                                */
> +                               ntfs_warning(sb,
> +                                       "Volume has errors.  Leaving volume marked dirty.  Run chkdsk.");
> +                       }
>                 }
> -               /*
> -                * Commits the updated volume flags if they were written.
> -                * The mft mirror must still be around for this: the
> -                * $Volume record (mft record number 3, below
> -                * vol->mftmirr_size) is mirrored by write_mft_record()
> -                * through ntfs_sync_mft_mirror(), which fails with -EIO
> -                * and leaves the mirror stale once vol->mftmirr_ino is
> -                * gone, so the mirror inode is only released after this
> -                * commit.
> -                */
> -               ntfs_commit_inode(vol->vol_ino);
>         }
>
>         /*
> --
> 2.25.1
>


-- 
Thanks,
Hyunchul