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

[dd <zhongling0719@126.com>]

At 2026-09-13 16:31:30, "liubaolin" <liubaolin12138@163.com> wrote:
>
>
>在 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.
>
>>  
Hi Baolin,

  Thanks for the review. You're right: the patch does not serialize the
  dirty-bit update with the whole metadata operation.

  v9 2/4 only closes the skipped-set variant. The unlocked check could skip
  ntfs_set_volume_flags() entirely; the unconditional call always attempts
  the update under the $Volume mrec_lock. The remaining mark-at-start window
  is pre-existing and is already documented in the last paragraph of the
  commit message.

  The ntfs_file_write_iter() comment describes the failure mode of the
  removed unlocked check, not a lifecycle guarantee, but I agree it reads
  like one. I'll post a follow-up patch against ntfs-next that stops
  ntfs_sync_fs() from clearing VOLUME_IS_DIRTY while the volume is mounted
  read-write. The bit is then only cleared at a clean unmount from
  ntfs_put_super(), after inode eviction. This avoids holding the $Volume
  mrec_lock across write operations, at the cost of a possible extra chkdsk
  if the machine crashes between a sync and the unmount.

  Thanks,
  Hongling
 
>>   	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);