Re: [PATCH v9 2/4] ntfs: set the volume dirty bit unconditionally on metadata changes

[liubaolin <liubaolin12138@163.com>]

在 2026/9/11 10:09, Hongling Zeng 写道:
> The callers in file.c and namei.c skip ntfs_set_volume_flags() when
> the in-memory vol_flags already show VOLUME_IS_DIRTY, but that check
> runs without any lock: if it observes the bit set and ntfs_sync_fs()
> clears it under the mrec_lock before the caller's metadata update
> completes, the set is skipped and the volume can end up clean on disk
> despite the modification, so chkdsk will not run on the next mount.
> 
> Drop the caller-side checks and call ntfs_set_volume_flags()
> unconditionally: ntfs_write_volume_flags() already skips the write
> under the mrec_lock when the combined value is unchanged.  That
> unconditional call costs one mrec_lock acquisition per metadata
> operation even in the already-dirty steady state; it cannot be
> avoided, because deciding to skip the call without the lock is itself
> what allows a concurrent ntfs_sync_fs() clear to lose the set.
> 
> The IOCB_NOWAIT path in ntfs_file_write_iter() goes through the same
> sleeping call: a RWF_NOWAIT write can block in the marking, as it
> already could before this change whenever the volume appeared clean.
> Giving that path a non-blocking variant is left as follow-up work.
> The callers keep the pre-existing behavior of proceeding when the
> marking fails, so the dirty bit remains best-effort.
> 
> This closes the variant where the set is skipped outright.  A clear
> for a concurrent, error-free sync can still land between the set and
> the end of the metadata operation; that mark-at-start lifecycle is
> pre-existing and is not changed by this patch.
> 
> Reported-by: Baolin Liu <liubaolin@kylinos.cn>
> Cc: stable@vger.kernel.org
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
> ---
>   fs/ntfs/file.c  | 20 +++++++++++---------
>   fs/ntfs/namei.c | 24 ++++++++----------------
>   2 files changed, 19 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/ntfs/file.c b/fs/ntfs/file.c
> index 007d1614b9ac..cfc7b36b7dff 100644
> --- a/fs/ntfs/file.c
> +++ b/fs/ntfs/file.c
> @@ -325,8 +325,7 @@ int ntfs_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>   		goto out;
>   	}
>   
> -	if (!(vol->vol_flags & VOLUME_IS_DIRTY))
> -		ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
> +	ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
>   
>   	if (ia_valid & ATTR_SIZE) {
>   		err = ntfs_setattr_size(vi, attr);
> @@ -620,8 +619,13 @@ static ssize_t ntfs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
>   		goto out_lock;
>   	}
>   
> -	if (!(vol->vol_flags & VOLUME_IS_DIRTY))
> -		ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
> +	/*
> +	 * The volume must be marked dirty before the modification is made,
> +	 * without an unlocked check of the in-memory flag: ntfs_sync_fs()
> +	 * can clear the bit concurrently and the modification would then
> +	 * land on a volume that is clean on disk.
> +	 */
> +	ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);

Hi Hongling,
   The added call removes the unlocked check, but it does not close the 
writer/sync lifecycle race. ntfs_set_volume_flags() protects only the 
read-modify-write of the $Volume record while mrec_lock is held, and 
releases the lock before ntfs_file_write_iter() enters the actual write 
path (ntfs_file_buffered_write() or the direct-I/O path) that modifies 
file data and related MFT metadata.

   For example, the following execution is still possible:

   Writer                                      ntfs_sync_fs()
   ------                                      ------------
   ntfs_set_volume_flags()
       acquire $Volume mrec_lock
       set VOLUME_IS_DIRTY
       update $Volume record
       release $Volume mrec_lock

                                                acquire $Volume mrec_lock
                                            observe NVolErrors() == false
                                                clear VOLUME_IS_DIRTY
                                                release $Volume mrec_lock
                                                commit clean volume flags

   continue ntfs_file_write_iter()
   enter ntfs_file_buffered_write()
   or the direct-I/O write path
   modify file data and related MFT metadata
   commit dirty pages/MFT records

   Thus, the metadata modification can still reach disk after 
ntfs_sync_fs() has persisted a clean on-disk dirty bit. The mrec_lock 
serializes only individual $Volume flag updates; it does not cover the 
subsequent write operation. Therefore, the guarantee described in the 
comment above is incomplete, and the same race applies to the other 
unconditional dirty-bit calls added by this patch.

Thanks,
Baolin.

>   
>   	pos = iocb->ki_pos;
>   	count = ret;
> @@ -1153,11 +1157,9 @@ static long ntfs_fallocate(struct file *file, int mode, loff_t offset, loff_t le
>   			return err;
>   	}
>   
> -	if (!(vol->vol_flags & VOLUME_IS_DIRTY)) {
> -		err = ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
> -		if (err)
> -			return err;
> -	}
> +	err = ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
> +	if (err)
> +		return err;
>   
>   	old_size = i_size_read(vi);
>   
> diff --git a/fs/ntfs/namei.c b/fs/ntfs/namei.c
> index fdf52fac4329..3e0adb9a0ea4 100644
> --- a/fs/ntfs/namei.c
> +++ b/fs/ntfs/namei.c
> @@ -757,8 +757,7 @@ static int ntfs_create(struct mnt_idmap *idmap, struct inode *dir,
>   		return err;
>   	}
>   
> -	if (!(vol->vol_flags & VOLUME_IS_DIRTY))
> -		ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
> +	ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
>   
>   	ni = __ntfs_create(idmap, dir, uname, uname_len, S_IFREG | mode, 0, NULL, 0);
>   	kmem_cache_free(ntfs_name_cache, uname);
> @@ -1032,8 +1031,7 @@ static int ntfs_unlink(struct inode *dir, struct dentry *dentry)
>   		return err;
>   	}
>   
> -	if (!(vol->vol_flags & VOLUME_IS_DIRTY))
> -		ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
> +	ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
>   
>   	err = ntfs_delete(ni, NTFS_I(dir), uname, uname_len, true);
>   	if (err)
> @@ -1076,8 +1074,7 @@ static struct dentry *ntfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>   		return ERR_PTR(err);
>   	}
>   
> -	if (!(vol->vol_flags & VOLUME_IS_DIRTY))
> -		ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
> +	ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
>   
>   	ni = __ntfs_create(idmap, dir, uname, uname_len, mode, 0, NULL, 0);
>   	kmem_cache_free(ntfs_name_cache, uname);
> @@ -1118,8 +1115,7 @@ static int ntfs_rmdir(struct inode *dir, struct dentry *dentry)
>   		return err;
>   	}
>   
> -	if (!(vol->vol_flags & VOLUME_IS_DIRTY))
> -		ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
> +	ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
>   
>   	err = ntfs_delete(ni, NTFS_I(dir), uname, uname_len, true);
>   	if (err)
> @@ -1305,8 +1301,7 @@ static int ntfs_rename(struct mnt_idmap *idmap, struct inode *old_dir,
>   		new_dir_first = is_subdir(new_dentry->d_parent,
>   					  old_dentry->d_parent);
>   
> -	if (!(vol->vol_flags & VOLUME_IS_DIRTY))
> -		ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
> +	ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
>   
>   	mutex_lock_nested(&old_ni->mrec_lock, NTFS_INODE_MUTEX_NORMAL);
>   	if (new_ni)
> @@ -1429,8 +1424,7 @@ static int ntfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
>   		goto out;
>   	}
>   
> -	if (!(vol->vol_flags & VOLUME_IS_DIRTY))
> -		ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
> +	ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
>   
>   	ni = __ntfs_create(idmap, dir, usrc, usrc_len, S_IFLNK | 0777, 0,
>   			   symname, symlen);
> @@ -1474,8 +1468,7 @@ static int ntfs_mknod(struct mnt_idmap *idmap, struct inode *dir,
>   		return err;
>   	}
>   
> -	if (!(vol->vol_flags & VOLUME_IS_DIRTY))
> -		ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
> +	ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
>   
>   	switch (mode & S_IFMT) {
>   	case S_IFCHR:
> @@ -1521,8 +1514,7 @@ static int ntfs_link(struct dentry *old_dentry, struct inode *dir,
>   		return -ENOMEM;
>   	}
>   
> -	if (!(vol->vol_flags & VOLUME_IS_DIRTY))
> -		ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
> +	ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
>   
>   	ihold(vi);
>   	mutex_lock_nested(&ni->mrec_lock, NTFS_INODE_MUTEX_NORMAL);