Re: [PATCH v2 3/3] ntfs: convert runtime error paths to mark volume dirty after recording errors

[liubaolin <liubaolin12138@163.com>]

在 2026/9/3 15:54, Hongling Zeng 写道:
> Follow-up to the helper introduction: the runtime metadata-corruption
> paths in mft.c, lcnalloc.c, bitmap.c and inode.c still record the error
> flag with a bare NVolSetErrors(), leaving the same window in which
> ntfs_sync_fs() can acquire ni->mrec_lock, find NVolErrors() unset, and
> persist the volume as clean before the erroring thread records its
> error.
> 
> Convert the call sites that can run concurrently with sync and hold no
> mrec_lock:
> 
>   - the mft record allocation and $MFT data/$BITMAP extend rollback
>     paths (mft.c), which hold the runlist, mftbmp and lcnbmp locks;
> 
>   - the cluster allocation/free rollback paths (lcnalloc.c), which
>     hold vol->lcnbmp_lock;
> 
>   - the bitmap rollback failure path (bitmap.c), which holds no
>     relevant lock;
> 
>   - the inode read and writeback failure paths (inode.c), which run
>     under I_NEW or with the mrec_lock already released at err_out.
> 
> Blocking on the $Volume mrec_lock from those paths is safe: it is a
> leaf lock, its critical sections never acquire the locks the callers
> hold, and it carries its own lockdep class so the validator does not
> report an inversion against the shared mrec_lock class.
> 
> The remaining call sites keep calling NVolSetErrors() directly, now
> with comments explaining why:
> 
>   - the mft record writeback paths (write_mft_record_nolock() and
>     ntfs_write_mft_block()) can run with a caller's mrec_lock held,
>     which for the $Volume inode itself would self-deadlock the helper;
> 
>   - the ntfs_attr_lookup() failure paths (ntfs_attr_find(),
>     ntfs_external_attr_find() and the ntfs_attr_make_non_resident()
>     rollback) run with the caller's mrec_lock held for the same
>     reason.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
> ---
>   fs/ntfs/attrib.c   | 18 ++++++++++++++-
>   fs/ntfs/bitmap.c   |  2 +-
>   fs/ntfs/inode.c    |  8 +++----
>   fs/ntfs/lcnalloc.c |  4 ++--
>   fs/ntfs/mft.c      | 55 ++++++++++++++++++++++++++++------------------
>   5 files changed, 58 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> index 60264833bb63..5965a67b5971 100644
> --- a/fs/ntfs/attrib.c
> +++ b/fs/ntfs/attrib.c
> @@ -964,6 +964,11 @@ static int ntfs_attr_find(const __le32 type, const __le16 *name,
>   	}
>   	ntfs_error(vol->sb, "mft %#llx, type %#x is corrupt. Run chkdsk.",
>   		   (long long)ctx->ntfs_ino->mft_no, le32_to_cpu(type));
> +	/*
> +	 * ntfs_attr_lookup() runs with the caller's mrec_lock held, so the
> +	 * dirty bit cannot be written here (it would self-deadlock for the
> +	 * $Volume inode); record the error flag only.
> +	 */
>   	NVolSetErrors(vol);
>   	return -EIO;
>   }
> @@ -1501,8 +1506,14 @@ static int ntfs_external_attr_find(const __le32 type,
>   		err = -EIO;
>   	}
>   
> -	if (err != -ENOMEM)
> +	if (err != -ENOMEM) {
> +		/*
> +		 * ntfs_attr_lookup() runs with the caller's mrec_lock
> +		 * held, so the dirty bit cannot be written here; record
> +		 * the error flag only.
> +		 */
>   		NVolSetErrors(vol);
> +	}
>   	return err;
>   not_found:
>   	/*
> @@ -2236,6 +2247,11 @@ int ntfs_attr_make_non_resident(struct ntfs_inode *ni, const u32 data_size)
>   		if (ntfs_cluster_free_from_rl(vol, rl) < 0) {
>   			ntfs_error(vol->sb,
>   				"Failed to release allocated cluster(s) in error code path.  Run chkdsk to recover the lost cluster(s).");
> +			/*
> +			 * The caller may hold the mrec_lock of the inode
> +			 * being modified, so the dirty bit cannot be
> +			 * written here; record the error flag only.
> +			 */
>   			NVolSetErrors(vol);
>   		}
>   		kvfree(rl);
> diff --git a/fs/ntfs/bitmap.c b/fs/ntfs/bitmap.c
> index b1436b3151b9..6120cb301665 100644
> --- a/fs/ntfs/bitmap.c
> +++ b/fs/ntfs/bitmap.c
> @@ -284,7 +284,7 @@ int __ntfs_bitmap_set_bits_in_run(struct inode *vi, const s64 start_bit,
>   		ntfs_error(vi->i_sb,
>   			"Failed to map subsequent page (error %i) and rollback failed (error %i). Aborting and leaving inconsistent metadata. Unmount and run chkdsk.",
>   			err, pos);
> -		NVolSetErrors(NTFS_SB(vi->i_sb));
> +		ntfs_mark_volume_dirty_with_error(NTFS_SB(vi->i_sb));

Hi Hongling,
   There may still be a case where the dirty bit is not persisted.  This 
call site invokes ntfs_error() before 
ntfs_mark_volume_dirty_with_error().  When errors=remount-ro is in
   use, ntfs_error() sets SB_RDONLY through the following path:

   ntfs_error()
     -> ntfs_handle_error()
       -> sb->s_flags |= SB_RDONLY

   Therefore, when ntfs_mark_volume_dirty_with_error() is called,
NVolErrors() is set first, but sb_rdonly() is already true.  The helper 
returns without writing VOLUME_IS_DIRTY to disk.

   For example, the following sequence still appears possible:

   ntfs_sync_fs()                    error path
   --------------                    ----------
   acquire $Volume mrec_lock
   observe NVolErrors() == false
                                     ntfs_error()
                                       -> set SB_RDONLY
                                     set NVolErrors()
                                     sb_rdonly() == true
                                     skip the dirty-bit update
   clear VOLUME_IS_DIRTY
   release $Volume mrec_lock

   This leaves NVolErrors() set while the on-disk VOLUME_IS_DIRTY bit 
has been cleared.

   The same issue should apply to the other call sites in this patch 
that invoke ntfs_error() before ntfs_mark_volume_dirty_with_error().
   Should the dirty bit be persisted before setting SB_RDONLY?

Thanks,
Baolin.

>   	}
>   	return err;
>   }
> diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
> index 32edb4045178..0288f6a101dc 100644
> --- a/fs/ntfs/inode.c
> +++ b/fs/ntfs/inode.c
> @@ -1245,7 +1245,7 @@ static int ntfs_read_locked_inode(struct inode *vi)
>   		ntfs_error(vol->sb,
>   			"Failed with error code %i.  Marking corrupt inode 0x%llx as bad.  Run chkdsk.",
>   			err, ni->mft_no);
> -		NVolSetErrors(vol);
> +		ntfs_mark_volume_dirty_with_error(vol);
>   	}
>   	return err;
>   }
> @@ -1473,7 +1473,7 @@ static int ntfs_read_locked_attr_inode(struct inode *base_vi, struct inode *vi)
>   			err, ni->mft_no, ni->type, ni->name_len,
>   			base_ni->mft_no);
>   	if (err != -ENOENT && err != -ENOMEM)
> -		NVolSetErrors(vol);
> +		ntfs_mark_volume_dirty_with_error(vol);
>   	return err;
>   }
>   
> @@ -1725,7 +1725,7 @@ static int ntfs_read_locked_index_inode(struct inode *base_vi, struct inode *vi)
>   		"Failed with error code %i while reading index inode (mft_no 0x%llx, name_len %i.",
>   		err, ni->mft_no, ni->name_len);
>   	if (err != -EOPNOTSUPP && err != -ENOMEM)
> -		NVolSetErrors(vol);
> +		ntfs_mark_volume_dirty_with_error(vol);
>   	return err;
>   }
>   
> @@ -2883,7 +2883,7 @@ int __ntfs_write_inode(struct inode *vi, int sync)
>   		mark_inode_dirty(vi);
>   	else {
>   		ntfs_error(vi->i_sb, "Failed (error %i):  Run chkdsk.", -err);
> -		NVolSetErrors(ni->vol);
> +		ntfs_mark_volume_dirty_with_error(ni->vol);
>   	}
>   	if (need_iput)
>   		iput(vi);
> diff --git a/fs/ntfs/lcnalloc.c b/fs/ntfs/lcnalloc.c
> index aa2e017a4384..dd6854f16136 100644
> --- a/fs/ntfs/lcnalloc.c
> +++ b/fs/ntfs/lcnalloc.c
> @@ -763,7 +763,7 @@ switch_to_data1_zone:		search_zone = 2;
>   			ntfs_error(vol->sb,
>   				"Failed to rollback (error %i). Leaving inconsistent metadata! Unmount and run chkdsk.",
>   				err2);
> -			NVolSetErrors(vol);
> +			ntfs_mark_volume_dirty_with_error(vol);
>   		}
>   		/* Free the runlist. */
>   		kvfree(rl);
> @@ -1044,7 +1044,7 @@ s64 __ntfs_cluster_free(struct ntfs_inode *ni, const s64 start_vcn, s64 count,
>   		ntfs_error(vol->sb,
>   			"Failed to rollback (error %i).  Leaving inconsistent metadata!  Unmount and run chkdsk.",
>   			(int)delta);
> -		NVolSetErrors(vol);
> +		ntfs_mark_volume_dirty_with_error(vol);
>   	}
>   	ntfs_dec_free_clusters(vol, delta);
>   	up_write(&vol->lcnbmp_lock);
> diff --git a/fs/ntfs/mft.c b/fs/ntfs/mft.c
> index 984a0827f9ac..d8791d7b9678 100644
> --- a/fs/ntfs/mft.c
> +++ b/fs/ntfs/mft.c
> @@ -657,8 +657,15 @@ int write_mft_record_nolock(struct ntfs_inode *ni, struct mft_record *m, int syn
>   			"Not enough memory to write mft record. Redirtying so the write is retried later.");
>   		mark_mft_record_dirty(ni);
>   		err = 0;
> -	} else
> +	} else {
> +		/*
> +		 * The writeback path can run with the caller's mrec_lock
> +		 * held, so the dirty bit cannot be written here (it would
> +		 * self-deadlock for the $Volume inode); record the error
> +		 * flag only.
> +		 */
>   		NVolSetErrors(vol);
> +	}
>   	return err;
>   }
>   
> @@ -1150,7 +1157,7 @@ static int ntfs_mft_bitmap_extend_allocation_nolock(struct ntfs_volume *vol)
>   			if (ntfs_cluster_free_from_rl(vol, rl2)) {
>   				ntfs_error(vol->sb, "Failed to deallocate allocated cluster.%s",
>   						es);
> -				NVolSetErrors(vol);
> +				ntfs_mark_volume_dirty_with_error(vol);
>   			}
>   			kvfree(rl2);
>   			return PTR_ERR(rl);
> @@ -1285,7 +1292,7 @@ static int ntfs_mft_bitmap_extend_allocation_nolock(struct ntfs_volume *vol)
>   		 * The only thing that is now wrong is ->allocated_size of the
>   		 * base attribute extent which chkdsk should be able to fix.
>   		 */
> -		NVolSetErrors(vol);
> +		ntfs_mark_volume_dirty_with_error(vol);
>   		return ret;
>   	}
>   	a = ctx->attr;
> @@ -1306,7 +1313,7 @@ static int ntfs_mft_bitmap_extend_allocation_nolock(struct ntfs_volume *vol)
>   	down_write(&vol->lcnbmp_lock);
>   	if (ntfs_bitmap_clear_bit(vol->lcnbmp_ino, lcn)) {
>   		ntfs_error(vol->sb, "Failed to free allocated cluster.%s", es);
> -		NVolSetErrors(vol);
> +		ntfs_mark_volume_dirty_with_error(vol);
>   	} else
>   		ntfs_inc_free_clusters(vol, 1);
>   	up_write(&vol->lcnbmp_lock);
> @@ -1317,16 +1324,16 @@ static int ntfs_mft_bitmap_extend_allocation_nolock(struct ntfs_volume *vol)
>   				a->data.non_resident.mapping_pairs_offset),
>   				rl2, ll, -1, NULL, NULL, NULL)) {
>   			ntfs_error(vol->sb, "Failed to restore mapping pairs array.%s", es);
> -			NVolSetErrors(vol);
> +			ntfs_mark_volume_dirty_with_error(vol);
>   		}
>   		if (ntfs_attr_record_resize(ctx->mrec, a, old_alen)) {
>   			ntfs_error(vol->sb, "Failed to restore attribute record.%s", es);
> -			NVolSetErrors(vol);
> +			ntfs_mark_volume_dirty_with_error(vol);
>   		}
>   		mark_mft_record_dirty(ctx->ntfs_ino);
>   	} else if (status.mp_extended && ntfs_attr_update_mapping_pairs(mftbmp_ni, 0)) {
>   		ntfs_error(vol->sb, "Failed to restore mapping pairs.%s", es);
> -		NVolSetErrors(vol);
> +		ntfs_mark_volume_dirty_with_error(vol);
>   	}
>   	if (ctx)
>   		ntfs_attr_put_search_ctx(ctx);
> @@ -1420,20 +1427,20 @@ static int ntfs_mft_bitmap_extend_initialized_nolock(struct ntfs_volume *vol)
>   	mrec = map_mft_record(mft_ni);
>   	if (IS_ERR(mrec)) {
>   		ntfs_error(vol->sb, "Failed to map mft record.%s", es);
> -		NVolSetErrors(vol);
> +		ntfs_mark_volume_dirty_with_error(vol);
>   		return ret;
>   	}
>   	ctx = ntfs_attr_get_search_ctx(mft_ni, mrec);
>   	if (unlikely(!ctx)) {
>   		ntfs_error(vol->sb, "Failed to get search context.%s", es);
> -		NVolSetErrors(vol);
> +		ntfs_mark_volume_dirty_with_error(vol);
>   		goto unm_err_out;
>   	}
>   	if (ntfs_attr_lookup(mftbmp_ni->type, mftbmp_ni->name,
>   			mftbmp_ni->name_len, CASE_SENSITIVE, 0, NULL, 0, ctx)) {
>   		ntfs_error(vol->sb,
>   			"Failed to find first attribute extent of mft bitmap attribute.%s", es);
> -		NVolSetErrors(vol);
> +		ntfs_mark_volume_dirty_with_error(vol);
>   put_err_out:
>   		ntfs_attr_put_search_ctx(ctx);
>   unm_err_out:
> @@ -1587,7 +1594,7 @@ static int ntfs_mft_data_extend_allocation_nolock(struct ntfs_volume *vol)
>   		if (ntfs_cluster_free_from_rl(vol, rl2)) {
>   			ntfs_error(vol->sb,
>   				"Failed to deallocate clusters from the mft data attribute.%s", es);
> -			NVolSetErrors(vol);
> +			ntfs_mark_volume_dirty_with_error(vol);
>   		}
>   		kvfree(rl2);
>   		return PTR_ERR(rl);
> @@ -1721,7 +1728,7 @@ static int ntfs_mft_data_extend_allocation_nolock(struct ntfs_volume *vol)
>   		 * The only thing that is now wrong is ->allocated_size of the
>   		 * base attribute extent which chkdsk should be able to fix.
>   		 */
> -		NVolSetErrors(vol);
> +		ntfs_mark_volume_dirty_with_error(vol);
>   		return ret;
>   	}
>   	ctx->attr->data.non_resident.highest_vcn =
> @@ -1729,17 +1736,17 @@ static int ntfs_mft_data_extend_allocation_nolock(struct ntfs_volume *vol)
>   undo_alloc:
>   	if (ntfs_cluster_free(mft_ni, old_last_vcn, -1, ctx) < 0) {
>   		ntfs_error(vol->sb, "Failed to free clusters from mft data attribute.%s", es);
> -		NVolSetErrors(vol);
> +		ntfs_mark_volume_dirty_with_error(vol);
>   	}
>   
>   	if (ntfs_rl_truncate_nolock(vol, &mft_ni->runlist, old_last_vcn)) {
>   		ntfs_error(vol->sb, "Failed to truncate mft data attribute runlist.%s", es);
> -		NVolSetErrors(vol);
> +		ntfs_mark_volume_dirty_with_error(vol);
>   	}
>   	if (mp_extended && ntfs_attr_update_mapping_pairs(mft_ni, 0)) {
>   		ntfs_error(vol->sb, "Failed to restore mapping pairs.%s",
>   			   es);
> -		NVolSetErrors(vol);
> +		ntfs_mark_volume_dirty_with_error(vol);
>   	}
>   	if (ctx) {
>   		a = ctx->attr;
> @@ -1750,16 +1757,16 @@ static int ntfs_mft_data_extend_allocation_nolock(struct ntfs_volume *vol)
>   					a->data.non_resident.mapping_pairs_offset),
>   				rl2, ll, -1, NULL, NULL, NULL)) {
>   				ntfs_error(vol->sb, "Failed to restore mapping pairs array.%s", es);
> -				NVolSetErrors(vol);
> +				ntfs_mark_volume_dirty_with_error(vol);
>   			}
>   			if (ntfs_attr_record_resize(ctx->mrec, a, old_alen)) {
>   				ntfs_error(vol->sb, "Failed to restore attribute record.%s", es);
> -				NVolSetErrors(vol);
> +				ntfs_mark_volume_dirty_with_error(vol);
>   			}
>   			mark_mft_record_dirty(ctx->ntfs_ino);
>   		} else if (IS_ERR(ctx->mrec)) {
>   			ntfs_error(vol->sb, "Failed to restore attribute search context.%s", es);
> -			NVolSetErrors(vol);
> +			ntfs_mark_volume_dirty_with_error(vol);
>   		}
>   		ntfs_attr_put_search_ctx(ctx);
>   	}
> @@ -2322,7 +2329,7 @@ int ntfs_mft_record_alloc(struct ntfs_volume *vol, const int mode,
>   			folio_unlock(folio);
>   			kunmap_local(m);
>   			folio_put(folio);
> -			NVolSetErrors(vol);
> +			ntfs_mark_volume_dirty_with_error(vol);
>   			goto search_free_rec;
>   		}
>   		/*
> @@ -2475,7 +2482,7 @@ int ntfs_mft_record_alloc(struct ntfs_volume *vol, const int mode,
>   undo_mftbmp_alloc_nolock:
>   	if (ntfs_bitmap_clear_bit(vol->mftbmp_ino, bit)) {
>   		ntfs_error(vol->sb, "Failed to clear bit in mft bitmap.%s", es);
> -		NVolSetErrors(vol);
> +		ntfs_mark_volume_dirty_with_error(vol);
>   	}
>   	if (!base_ni || base_ni->mft_no != FILE_MFT)
>   		up_write(&vol->mftbmp_lock);
> @@ -2815,8 +2822,14 @@ static int ntfs_write_mft_block(struct folio *folio, struct writeback_control *w
>   			iput(ref_inos[nr_ref_inos]);
>   	}
>   
> -	if (unlikely(err && err != -ENOMEM))
> +	if (unlikely(err && err != -ENOMEM)) {
> +		/*
> +		 * The writeback path can run with a caller's mrec_lock
> +		 * held, so the dirty bit cannot be written here; record
> +		 * the error flag only.
> +		 */
>   		NVolSetErrors(vol);
> +	}
>   	if (likely(!err))
>   		ntfs_debug("Done.");
>   	return err;