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

[dd <zhongling0719@126.com>]

At 2026-09-15 13:38:41, "Hyunchul Lee" <hyc.lee@gmail.com> wrote:
>Hi Hongling,
>
>2026년 9월 14일 (월) 오후 5:37, Hongling Zeng <zenghongling@kylinos.cn>님이 작성:
>>
>> 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");
>>
>
>
>The initial sync_filesystem() call in this function  still ignores
>the return value.
>
>And the emergency remount(SB_FORCE) does not drain writers
>that are already in progress. This function can commit the
>clean flag while a writer is still modifying metadata.
>
>It seems fine not to clear the flag for SB_FORCE.
>
Hi Hyunchul, 

Thanks for the review.  Both points are valid and are addressed by the 
appended 7/7 patch.
The sync_filesystem() error is now returned and fails the remount, so
the unmount can retry.  It is only checked in the read-only direction,
as ext4 does.

For SB_FORCE the dirty bit is never cleared.  The sync runs only when 
errors are recorded, where it can only set the bit; the commit and 
the flush stay, since a writer marking the volume concurrently can
only make the on-disk bit more set, not less.  Note that a forced 
remount ignores the returned error and turns the superblock read-only 
anyway, so the return value matters for the regular remount only.

Thanks, 
Hongling

>> @@ -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);
>>         }
>>
>>         /*
>> --
>> 2.25.1
>>
>
>
>-- 
>Thanks,
>Hyunchul