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

[Hongling Zeng]

Metadata-changing paths skipped ntfs_set_volume_flags() when the
locklessly-read volume flags already contained VOLUME_IS_DIRTY.  A
concurrent ntfs_sync_fs() could clear the bit after that check and before
the metadata update, leaving the modified volume clean on disk.

Always call ntfs_set_volume_flags(); the locked update avoids an
unnecessary write when the bit is already set.

For IOCB_NOWAIT writes, use a non-blocking variant that returns -EAGAIN
when acquiring the $Volume mrec_lock, allocating the search context, or
handling an attribute list would require sleeping.  Use GFP_NOWAIT for
the search-context allocation, and keep expected -EAGAIN results out of
the filesystem error-reporting path.

Reported-by: Baolin Liu <liubaolin@xxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Hongling Zeng <zenghongling@xxxxxxxxxx>
---
Change in v7:
- Use a separate cleanup path for expected NOWAIT failures so they do not
  call ntfs_error().
- Use GFP_NOWAIT instead of GFP_ATOMIC for search-context allocation.
---
 fs/ntfs/attrib.c | 30 +++++++++++++----
 fs/ntfs/attrib.h |  2 ++
 fs/ntfs/file.c   | 29 ++++++++++++-----
 fs/ntfs/namei.c  | 24 +++++---------
 fs/ntfs/ntfs.h   |  1 +
 fs/ntfs/super.c  | 83 +++++++++++++++++++++++++++++++++++++++++-------
 6 files changed, 128 insertions(+), 41 deletions(-)

diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index c7c09a751c6a..b51f8abeaacf 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -1678,20 +1678,24 @@ void ntfs_attr_reinit_search_ctx(struct ntfs_attr_search_ctx *ctx)
 }
 
 /*
- * ntfs_attr_get_search_ctx - allocate/initialize a new attribute search context
+ * ntfs_attr_get_search_ctx_gfp - allocate/initialize a new attribute search
+ * context using the given allocation flags
  * @ni:		ntfs inode with which to initialize the search context
  * @mrec:	mft record with which to initialize the search context
+ * @gfp:	allocation flags for the search context
  *
- * Allocate a new attribute search context, initialize it with @ni and @mrec,
- * and return it. Return NULL if allocation failed.
+ * Allocate a new attribute search context with kmem_cache_alloc(@gfp),
+ * initialize it with @ni and @mrec, and return it. Return NULL if allocation
+ * failed.  Callers that must not sleep, e.g. those servicing an IOCB_NOWAIT
+ * request, pass GFP_ATOMIC so the allocation cannot enter direct reclaim.
  */
-struct ntfs_attr_search_ctx *ntfs_attr_get_search_ctx(struct ntfs_inode *ni,
-		struct mft_record *mrec)
+struct ntfs_attr_search_ctx *ntfs_attr_get_search_ctx_gfp(
+		struct ntfs_inode *ni, struct mft_record *mrec, gfp_t gfp)
 {
 	struct ntfs_attr_search_ctx *ctx;
 	bool init;
 
-	ctx = kmem_cache_alloc(ntfs_attr_ctx_cache, GFP_NOFS);
+	ctx = kmem_cache_alloc(ntfs_attr_ctx_cache, gfp);
 	if (ctx) {
 		init = ntfs_attr_init_search_ctx(ctx, ni, mrec);
 		if (init == false) {
@@ -1703,6 +1707,20 @@ struct ntfs_attr_search_ctx *ntfs_attr_get_search_ctx(struct ntfs_inode *ni,
 	return ctx;
 }
 
+/*
+ * ntfs_attr_get_search_ctx - allocate/initialize a new attribute search context
+ * @ni:		ntfs inode with which to initialize the search context
+ * @mrec:	mft record with which to initialize the search context
+ *
+ * Allocate a new attribute search context, initialize it with @ni and @mrec,
+ * and return it. Return NULL if allocation failed.
+ */
+struct ntfs_attr_search_ctx *ntfs_attr_get_search_ctx(
+		struct ntfs_inode *ni, struct mft_record *mrec)
+{
+	return ntfs_attr_get_search_ctx_gfp(ni, mrec, GFP_NOFS);
+}
+
 /*
  * ntfs_attr_put_search_ctx - release an attribute search context
  * @ctx:	attribute search context to free
diff --git a/fs/ntfs/attrib.h b/fs/ntfs/attrib.h
index 6b4fa9f57640..dd3c39668eff 100644
--- a/fs/ntfs/attrib.h
+++ b/fs/ntfs/attrib.h
@@ -88,6 +88,8 @@ static inline s64 ntfs_attr_size(const struct attr_record *a)
 void ntfs_attr_reinit_search_ctx(struct ntfs_attr_search_ctx *ctx);
 struct ntfs_attr_search_ctx *ntfs_attr_get_search_ctx(struct ntfs_inode *ni,
 		struct mft_record *mrec);
+struct ntfs_attr_search_ctx *ntfs_attr_get_search_ctx_gfp(
+		struct ntfs_inode *ni, struct mft_record *mrec, gfp_t gfp);
 void ntfs_attr_put_search_ctx(struct ntfs_attr_search_ctx *ctx);
 int ntfs_attr_size_bounds_check(const struct ntfs_volume *vol,
 		const __le32 type, const s64 size);
diff --git a/fs/ntfs/file.c b/fs/ntfs/file.c
index 007d1614b9ac..32a644ff21b6 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,24 @@ static ssize_t ntfs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		goto out_lock;
 	}
 
-	if (!(vol->vol_flags & 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.  In the IOCB_NOWAIT case
+	 * the marking must not sleep, so it uses the nowait variant, and
+	 * any failure of the marking fails the request instead of letting
+	 * the write proceed on a volume that may be clean on disk.
+	 */
+	if (iocb->ki_flags & IOCB_NOWAIT) {
+		err = ntfs_set_volume_flags_nowait(vol, VOLUME_IS_DIRTY);
+		if (err) {
+			ret = err;
+			goto out_lock;
+		}
+	} else {
 		ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
+	}
 
 	pos = iocb->ki_pos;
 	count = ret;
@@ -1153,11 +1168,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);
diff --git a/fs/ntfs/ntfs.h b/fs/ntfs/ntfs.h
index 45f77848a9cf..b76501a91143 100644
--- a/fs/ntfs/ntfs.h
+++ b/fs/ntfs/ntfs.h
@@ -219,6 +219,7 @@ struct option_t {
 };
 extern const struct option_t on_errors_arr[];
 int ntfs_set_volume_flags(struct ntfs_volume *vol, __le16 flags);
+int ntfs_set_volume_flags_nowait(struct ntfs_volume *vol, __le16 flags);
 int ntfs_clear_volume_flags(struct ntfs_volume *vol, __le16 flags);
 int ntfs_write_volume_label(struct ntfs_volume *vol, char *label);
 
diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
index 6ba19986a598..5f12197104cc 100644
--- a/fs/ntfs/super.c
+++ b/fs/ntfs/super.c
@@ -357,6 +357,7 @@ void ntfs_handle_error(struct super_block *sb)
  * @vol:	ntfs volume on which to modify the flags
  * @set_bits:	bits to set in the volume information flags
  * @clear_bits:	bits to clear in the volume information flags
+ * @nowait:	do not wait for the $Volume mrec_lock
  *
  * Internal function.  You probably want to use ntfs_{set,clear}_volume_flags()
  * instead (see below).
@@ -368,19 +369,33 @@ void ntfs_handle_error(struct super_block *sb)
  * All bit manipulation is done on CPU-endian values, and the result is
  * converted back to little-endian before storing it.
  *
+ * When @nowait is set, the call must not sleep: the mrec_lock is acquired
+ * with mutex_trylock() and a contended lock fails with -EAGAIN, the search
+ * context is allocated with GFP_ATOMIC and exhausted memory fails with
+ * -EAGAIN, and a $Volume inode carrying an attribute list, whose extent
+ * mapping could block, is rejected with -EAGAIN as well.  The remaining
+ * transition work is non-blocking: the mft record of $Volume stays mapped
+ * for the lifetime of the mount, so the attribute lookup is an in-memory
+ * scan, and marking the record dirty does not submit I/O.
+ *
  * Return 0 on success and -errno on error.
  */
 static int ntfs_write_volume_flags(struct ntfs_volume *vol,
 		const __le16 set_bits, const __le16 clear_bits,
-		const bool skip_if_errors)
+		const bool skip_if_errors, const bool nowait)
 {
 	struct ntfs_inode *ni = NTFS_I(vol->vol_ino);
 	struct volume_information *vi;
-	struct ntfs_attr_search_ctx *ctx;
+	struct ntfs_attr_search_ctx *ctx = NULL;
 	u16 flags;
 	int err;
 
-	mutex_lock(&ni->mrec_lock);
+	if (nowait) {
+		if (!mutex_trylock(&ni->mrec_lock))
+			return -EAGAIN;
+	} else {
+		mutex_lock(&ni->mrec_lock);
+	}
 
 	if (skip_if_errors && NVolErrors(vol))
 		goto done;
@@ -394,16 +409,28 @@ static int ntfs_write_volume_flags(struct ntfs_volume *vol,
 	if (le16_to_cpu(vol->vol_flags) == flags)
 		goto done;
 
-	ctx = ntfs_attr_get_search_ctx(ni, NULL);
+	/*
+	 * The nowait path must not sleep past this point either.  An
+	 * attribute list on $Volume would make ntfs_attr_lookup() map
+	 * extent mft records, which can block; sane volumes never have
+	 * one, so refuse such a volume instead of risking the sleep.
+	 */
+	if (nowait && NInoAttrList(ni)) {
+		err = -EAGAIN;
+		goto out_unlock;
+	}
+
+	ctx = ntfs_attr_get_search_ctx_gfp(ni, NULL,
+			nowait ? GFP_ATOMIC : GFP_NOFS);
 	if (!ctx) {
-		err = -ENOMEM;
-		goto put_unm_err_out;
+		err = nowait ? -EAGAIN : -ENOMEM;
+		goto out_unlock;
 	}
 
 	err = ntfs_attr_lookup(AT_VOLUME_INFORMATION, NULL, 0, 0, 0, NULL, 0,
 			ctx);
 	if (err)
-		goto put_unm_err_out;
+		goto out_unlock;
 
 	vi = (struct volume_information *)((u8 *)ctx->attr +
 			le16_to_cpu(ctx->attr->data.resident.value_offset));
@@ -414,10 +441,19 @@ static int ntfs_write_volume_flags(struct ntfs_volume *vol,
 	mutex_unlock(&ni->mrec_lock);
 	ntfs_debug("Done.");
 	return 0;
-put_unm_err_out:
+out_unlock:
 	if (ctx)
 		ntfs_attr_put_search_ctx(ctx);
 	mutex_unlock(&ni->mrec_lock);
+	/*
+	 * -EAGAIN from the NOWAIT path means that the operation would
+	 *  have to sleep.  It is an expected result, not a filesystem
+	 *  error, so do not pass it to ntfs_error().
+	 */
+	if (nowait && err == -EAGAIN) {
+		ntfs_debug("Failed with error code %i.", -err);
+		return err;
+	}
 	ntfs_error(vol->sb, "Failed with error code %i.", -err);
 	return err;
 }
@@ -435,7 +471,32 @@ static int ntfs_write_volume_flags(struct ntfs_volume *vol,
  */
 int ntfs_set_volume_flags(struct ntfs_volume *vol, __le16 flags)
 {
-	return ntfs_write_volume_flags(vol, flags, 0, false);
+	return ntfs_write_volume_flags(vol, flags, 0, false, false);
+}
+
+/*
+ * ntfs_set_volume_flags_nowait - set bits without sleeping
+ * @vol:	ntfs volume on which to modify the flags
+ * @flags:	flags to set on the volume
+ *
+ * Same as ntfs_set_volume_flags(), except that the call does not sleep.
+ * Contended mrec_lock, exhausted atomic memory and an attribute list on
+ * $Volume (whose extent mapping could block) each fail with -EAGAIN; see
+ * ntfs_write_volume_flags() for why the remaining path is non-blocking.
+ * For callers servicing an IOCB_NOWAIT request, which must fail with
+ * -EAGAIN rather than sleep.  That -EAGAIN is an expected retry signal
+ * rather than a filesystem error, so it is not reported through
+ * ntfs_error() and never triggers the errors= handling.
+ *
+ * A failure means the flags were NOT set; such callers should fail the
+ * request rather than proceed with the modification.
+ *
+ * Return 0 on success, -EAGAIN when the operation would have to sleep and
+ * -errno on other errors.
+ */
+int ntfs_set_volume_flags_nowait(struct ntfs_volume *vol, __le16 flags)
+{
+	return ntfs_write_volume_flags(vol, flags, 0, false, true);
 }
 
 /*
@@ -451,7 +512,7 @@ int ntfs_set_volume_flags(struct ntfs_volume *vol, __le16 flags)
  */
 int ntfs_clear_volume_flags(struct ntfs_volume *vol, __le16 flags)
 {
-	return ntfs_write_volume_flags(vol, 0, flags, false);
+	return ntfs_write_volume_flags(vol, 0, flags, false, false);
 }
 
 /*
@@ -464,7 +525,7 @@ int ntfs_clear_volume_flags(struct ntfs_volume *vol, __le16 flags)
  */
 static int ntfs_clear_volume_dirty_if_no_errors(struct ntfs_volume *vol)
 {
-	return ntfs_write_volume_flags(vol, 0, VOLUME_IS_DIRTY, true);
+	return ntfs_write_volume_flags(vol, 0, VOLUME_IS_DIRTY, true, false);
 }
 
 int ntfs_write_volume_label(struct ntfs_volume *vol, char *label)
-- 
2.25.1