Re: [PATCH v13 4/7] ntfs: persist the dirty state after the final put_super() commits

[liubaolin <liubaolin12138@163.com>]

在 2026/9/15 16:32, Hongling Zeng 写道:
> The just-in-case mftmirr/mft commits and the final write_inode_now()
> in ntfs_put_super() can record NVolErrors() after the dirty state has
> been persisted, so errors from those points would leave the volume
> unmounted with a clean on-disk dirty bit - contradicting the "cannot
> unmount clean" guarantee ntfs_sync_volume_dirty_state() is meant to
> provide.
> 
> Move the persistence to the end of ntfs_put_super(): keep the gated
> re-commits and the tail commits where they are, run
> ntfs_sync_volume_dirty_state() and the $Volume commit after the last
> write_inode_now(), and release vol->vol_ino only after the sync.
> 
> The release order of the special inodes matters for the $Volume
> commit: writing the $Volume record mirrors it through
> ntfs_sync_mft_mirror() (record number 3 is below vol->mftmirr_size),
> which fails with -EIO and leaves the mirror stale once
> vol->mftmirr_ino is gone, so the mirror inode is released only after
> that commit.  vol->vol_ino is then put before vol->mft_ino is dropped:
> if the commit failed before it could clear the dirty flag,
> ntfs_evict_big_inode() commits the inode again on its way out, and
> __ntfs_write_inode() resolves the runlist through vol->mft_ino.
> 
> Reported-by: Baolin Liu <liubaolin@kylinos.cn>
> Cc: stable@vger.kernel.org
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
> ---
>   fs/ntfs/super.c | 75 ++++++++++++++++++++++++++++++++++---------------
>   1 file changed, 52 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
> index 733565953302..3574c224fe28 100644
> --- a/fs/ntfs/super.c
> +++ b/fs/ntfs/super.c
> @@ -1812,26 +1812,13 @@ static void ntfs_put_super(struct super_block *sb)
>   	ntfs_commit_inode(vol->mft_ino);
>   
>   	/*
> -	 * If a read-write mount, persist the error state in the volume flags:
> -	 * mark the volume clean if no volume errors have occurred, and make
> -	 * sure VOLUME_IS_DIRTY is on disk if any have, so chkdsk runs on the
> -	 * next mount.  Also, re-commit all affected inodes.
> +	 * If a read-write mount, re-commit all affected inodes once more.
> +	 * The dirty state itself is persisted at the end of ntfs_put_super(),
> +	 * after the last commits and the final write_inode_now(): those can
> +	 * still record errors via __ntfs_write_inode(), and the sync must
> +	 * evaluate NVolErrors() with the last setter already run.
>   	 */
>   	if (!sb_rdonly(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)) {
> -			/*
> -			 * The dirty bit is on disk now; only warn when the
> -			 * sync actually succeeded, or this message would
> -			 * contradict the one above.
> -			 */
> -			ntfs_warning(sb,
> -				"Volume has errors.  Leaving volume marked dirty.  Run chkdsk.");
> -		}
> -		/* Commits the updated volume flags if they were written. */
> -		ntfs_commit_inode(vol->vol_ino);
>   		if (!NVolErrors(vol)) {
>   			ntfs_commit_inode(vol->root_ino);
>   			if (vol->mftmirr_ino)
> @@ -1840,9 +1827,6 @@ static void ntfs_put_super(struct super_block *sb)
>   		}
>   	}
>   
> -	iput(vol->vol_ino);
> -	vol->vol_ino = NULL;
> -
>   	/* NTFS 3.0+ specific clean up. */
>   	if (vol->major_ver >= 3) {
>   		if (vol->extend_ino) {
> @@ -1872,8 +1856,6 @@ static void ntfs_put_super(struct super_block *sb)
>   		/* Re-commit the mft mirror and mft just in case. */
>   		ntfs_commit_inode(vol->mftmirr_ino);
>   		ntfs_commit_inode(vol->mft_ino);
> -		iput(vol->mftmirr_ino);
> -		vol->mftmirr_ino = NULL;
>   	}
>   	/*
>   	 * We should have no dirty inodes left, due to
> @@ -1883,6 +1865,53 @@ static void ntfs_put_super(struct super_block *sb)
>   	ntfs_commit_inode(vol->mft_ino);
>   	write_inode_now(vol->mft_ino, 1);
>   
> +	/*
> +	 * If a read-write mount, persist the error state in the volume flags:
> +	 * mark the volume clean if no volume errors have occurred, and make
> +	 * sure VOLUME_IS_DIRTY is on disk if any have, so chkdsk runs on the
> +	 * next mount.
> +	 */
> +	if (!sb_rdonly(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)) {
> +			/*
> +			 * The dirty bit is on disk now; only warn when the
> +			 * sync actually succeeded, or this message would
> +			 * contradict the one 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);
> +	}
> +
> +	/*
> +	 * Release $Volume while the mft inode is still available: if the
> +	 * commit above failed before it could clear the dirty flag,
> +	 * ntfs_evict_big_inode() commits the inode again on its way out,
> +	 * and __ntfs_write_inode() needs vol->mft_ino to look up the
> +	 * runlist of the record to write.
> +	 */
> +	iput(vol->vol_ino);
> +	vol->vol_ino = NULL;
> +
> +	if (vol->mftmirr_ino) {
> +		iput(vol->mftmirr_ino);
> +		vol->mftmirr_ino = NULL;
> +	}
> +
>   	iput(vol->mft_ino);
>   	vol->mft_ino = NULL;
>   	blkdev_issue_flush(sb->s_bdev);

Looks good to me.

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