Re: [PATCH v3 0/3] ntfs: fix volume flag update races and persist the recorded error state

[liubaolin <liubaolin12138@163.com>]

在 2026/9/7 15:08, Hongling Zeng 写道:
> Hi all,
> 
> First of all, my apologies for the many versions of this patch.  I
> should have folded the reviewer feedback in and resent a single coherent
> series sooner; the split across v1/v2 and follow-ups made the thread
> harder to follow than it needed to be.  This v3 consolidates everything
> into one series of three patches.
> 
> To recap the review discussion:
> 
> The original patch (now 1/3) fixed the lost-update race where
> ntfs_set_volume_flags() and ntfs_clear_volume_flags() computed the new
> value from vol->vol_flags outside any lock.  The review correctly
> pointed out that a race remained in ntfs_sync_fs(): it checked
> NVolErrors() outside the critical section, so a concurrent error path
> could still record an error after the check and before the clear, and
> the volume would end up persisted as clean despite the recorded error,
> meaning chkdsk would not run on the next mount.
> 
> That race is now fixed in 1/3 itself: the NVolErrors() check and the
> clearing of VOLUME_IS_DIRTY both happen inside the same mrec_lock
> critical section, via a dedicated ntfs_clear_volume_dirty_if_no_errors()
> helper.  With that, every interleaving of the sync thread and the error
> path leaves the volume dirty on disk.
> 
> Patches 2/3 then close the writer-side half of the window: the runtime
> metadata-corruption paths in fs/ntfs recorded only the in-memory
> NVolErrors() flag and never persisted VOLUME_IS_DIRTY at all, so a
> volume could unmount with a clean on-disk flag despite recorded
> corruption.  Persisting from the error paths themselves is not an
> option: they run under a wide variety of ntfs locks (runlist locks,
> vol->lcnbmp_lock, vol->mftbmp_lock, mrec_locks) while the dirty-bit
> write needs the $Volume mrec_lock and can take the $MFT runlist lock,
> which self-deadlocks or forms ABBA cycles.  Instead, 2/3 makes
> persistence a property of the lock-free sync contexts: the new
> ntfs_sync_volume_dirty_state() sets VOLUME_IS_DIRTY when NVolErrors()
> is recorded and clears it otherwise, evaluating the error flag under
> the $Volume mrec_lock.  It is called from ntfs_sync_fs(), the
> remount-to-read-only path and ntfs_put_super(), with NV_Hibernated
> gating so a hibernated volume is never written by these paths.  The
> guarantee is eventual rather than instantaneous (a crash between the
> error record and the next persistence point remains a window);
> ntfs_put_super() runs on a quiesced filesystem after evict_inodes(),
> so a volume that is read-write at unmount time cannot unmount clean.
> 
> Patch 3/3 is pure hygiene found during review: the load_system_files()
> error unwind leaves a stale vol->vol_ino pointer in place, and carries
> a dead iput() inside the IS_ERR(vol->vol_ino) branch; NULL the pointer
> like ntfs_put_super() and the ntfs_fill_super() error path already do.
> 
> One housekeeping note: this series was rebased onto the current ntfs
> tree after the first posting failed to apply there.  Dennis Tighe's
> "ntfs: do not mark the volume clean in sync_fs when errors were
> recorded" landed in the meantime and fixed part of the same sync_fs
> problem this series addresses; patch 2/3 supersedes that fix with the
> ntfs_sync_volume_dirty_state() helper, which guards the clear in every
> persistence context, not just sync_fs.  No conflict with its intent.
> 
> Patch layout:
> 
>    1/3 ntfs: fix volume flag update races
>        Move the read-modify-write of vol->vol_flags inside the
>        $Volume mrec_lock; ntfs_sync_fs() clears the dirty bit under
>        the lock with the NVolErrors() check.
> 
>    2/3 ntfs: sync the volume dirty bit with the recorded error state
>        New ntfs_sync_volume_dirty_state(), called from the lock-free
>        sync contexts (sync_fs / remount-ro / put_super); NV_Hibernated
>        gating.
> 
>    3/3 ntfs: NULL vol->vol_ino in the load_system_files() error
>        teardown
>        Drop the stale pointer and the dead iput().
> 
> The series builds cleanly with W=1.
> 
> Thanks,
> Hongling

Hi Hongling,
    Thanks for putting this together. I left a couple of comments on 1/3 
and 2/3. Patch 3/3 looks good to me.

Thanks,
Baolin.