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

[liubaolin <liubaolin12138@163.com>]

在 2026/9/15 16:32, Hongling Zeng 写道:
> 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@kylinos.cn>
> Cc: stable@vger.kernel.org
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
> ---
>   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");
>   
> @@ -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);
>   	}
>   
>   	/*

Looks good to me.

Reviewed-by: Baolin Liu <liubaolin@kylinos.cn>