[PATCH 0/6] fs/ntfs3: Refactoring of several functions

[PATCH 0/6] fs/ntfs3: Refactoring of several functions

[Konstantin Komarov]

6 commits, that hopefully improves code (tests are ok).
While working on attr_set_size, attr_punch_hole and attr_insert_range
I've noticed how convoluted the code is.
I'll think about how to make code less complex.

Konstantin Komarov (6):
   fs/ntfs3: New function ntfs_bad_inode
   fs/ntfs3: Refactoring attr_set_size to restore after errors
   fs/ntfs3: Refactoring attr_punch_hole to restore after errors
   fs/ntfs3: Refactoring attr_insert_range to restore after errors
   fs/ntfs3: Create MFT zone only if length is large enough
   fs/ntfs3: ni_ins_new_attr now returns error

  fs/ntfs3/attrib.c  | 416 +++++++++++++++++++++++++++++++--------------
  fs/ntfs3/frecord.c |  35 +++-
  fs/ntfs3/fsntfs.c  |  25 ++-
  fs/ntfs3/inode.c   |   6 +-
  fs/ntfs3/namei.c   |   4 +-
  fs/ntfs3/ntfs_fs.h |   3 +
  fs/ntfs3/run.c     |  25 +++
  7 files changed, 368 insertions(+), 146 deletions(-)

-- 
2.37.0

[PATCH 1/6] fs/ntfs3: New function ntfs_bad_inode

[Konstantin Komarov]

There are repetitive steps in case of bad inode
This commit wraps them in function

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
  fs/ntfs3/attrib.c  | 10 ++++------
  fs/ntfs3/frecord.c |  6 ++----
  fs/ntfs3/fsntfs.c  | 14 ++++++++++++++
  fs/ntfs3/inode.c   |  6 ++----
  fs/ntfs3/namei.c   |  4 +---
  fs/ntfs3/ntfs_fs.h |  2 ++
  6 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/fs/ntfs3/attrib.c b/fs/ntfs3/attrib.c
index 68a210bb54fe..d096d77ea042 100644
--- a/fs/ntfs3/attrib.c
+++ b/fs/ntfs3/attrib.c
@@ -1912,7 +1912,7 @@ int attr_collapse_range(struct ntfs_inode *ni, u64 vbo, u64 bytes)
  out:
  	up_write(&ni->file.run_lock);
  	if (err)
-		make_bad_inode(&ni->vfs_inode);
+		_ntfs_bad_inode(&ni->vfs_inode);
  
  	return err;
  }
@@ -2092,10 +2092,8 @@ int attr_punch_hole(struct ntfs_inode *ni, u64 vbo, u64 bytes, u32 *frame_size)
  
  out:
  	up_write(&ni->file.run_lock);
-	if (err) {
-		ntfs_set_state(sbi, NTFS_DIRTY_ERROR);
-		make_bad_inode(&ni->vfs_inode);
-	}
+	if (err)
+		_ntfs_bad_inode(&ni->vfs_inode);
  
  	return err;
  }
@@ -2280,7 +2278,7 @@ int attr_insert_range(struct ntfs_inode *ni, u64 vbo, u64 bytes)
  
  	up_write(&ni->file.run_lock);
  	if (err)
-		make_bad_inode(&ni->vfs_inode);
+		_ntfs_bad_inode(&ni->vfs_inode);
  
  	return err;
  }
diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
index bc48923693a9..bdc568053fae 100644
--- a/fs/ntfs3/frecord.c
+++ b/fs/ntfs3/frecord.c
@@ -2328,10 +2328,8 @@ int ni_decompress_file(struct ntfs_inode *ni)
  
  out:
  	kfree(pages);
-	if (err) {
-		make_bad_inode(inode);
-		ntfs_set_state(sbi, NTFS_DIRTY_ERROR);
-	}
+	if (err)
+		_ntfs_bad_inode(inode);
  
  	return err;
  }
diff --git a/fs/ntfs3/fsntfs.c b/fs/ntfs3/fsntfs.c
index ed9a1b851ce9..e6f5bf684da4 100644
--- a/fs/ntfs3/fsntfs.c
+++ b/fs/ntfs3/fsntfs.c
@@ -878,6 +878,20 @@ int ntfs_update_mftmirr(struct ntfs_sb_info *sbi, int wait)
  	return err;
  }
  
+/*
+ * ntfs_bad_inode
+ *
+ * Marks inode as bad and marks fs as 'dirty'
+ */
+void ntfs_bad_inode(struct inode *inode, const char *hint)
+{
+	struct ntfs_sb_info *sbi = inode->i_sb->s_fs_info;
+
+	ntfs_inode_err(inode, "%s", hint);
+	make_bad_inode(inode);
+	ntfs_set_state(sbi, NTFS_DIRTY_ERROR);
+}
+
  /*
   * ntfs_set_state
   *
diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c
index 3ed319663747..cd48687127c1 100644
--- a/fs/ntfs3/inode.c
+++ b/fs/ntfs3/inode.c
@@ -501,7 +501,7 @@ struct inode *ntfs_iget5(struct super_block *sb, const struct MFT_REF *ref,
  		inode = ntfs_read_mft(inode, name, ref);
  	else if (ref->seq != ntfs_i(inode)->mi.mrec->seq) {
  		/* Inode overlaps? */
-		make_bad_inode(inode);
+		_ntfs_bad_inode(inode);
  	}
  
  	return inode;
@@ -1725,9 +1725,7 @@ int ntfs_unlink_inode(struct inode *dir, const struct dentry *dentry)
  		if (inode->i_nlink)
  			mark_inode_dirty(inode);
  	} else if (!ni_remove_name_undo(dir_ni, ni, de, de2, undo_remove)) {
-		make_bad_inode(inode);
-		ntfs_inode_err(inode, "failed to undo unlink");
-		ntfs_set_state(sbi, NTFS_DIRTY_ERROR);
+		_ntfs_bad_inode(inode);
  	} else {
  		if (ni_is_dirty(dir))
  			mark_inode_dirty(dir);
diff --git a/fs/ntfs3/namei.c b/fs/ntfs3/namei.c
index 1cc700760c7e..bc22cc321a74 100644
--- a/fs/ntfs3/namei.c
+++ b/fs/ntfs3/namei.c
@@ -308,9 +308,7 @@ static int ntfs_rename(struct user_namespace *mnt_userns, struct inode *dir,
  	err = ni_rename(dir_ni, new_dir_ni, ni, de, new_de, &is_bad);
  	if (is_bad) {
  		/* Restore after failed rename failed too. */
-		make_bad_inode(inode);
-		ntfs_inode_err(inode, "failed to undo rename");
-		ntfs_set_state(sbi, NTFS_DIRTY_ERROR);
+		_ntfs_bad_inode(inode);
  	} else if (!err) {
  		inode->i_ctime = dir->i_ctime = dir->i_mtime =
  			current_time(dir);
diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h
index 54c20700afd3..c3e17090effc 100644
--- a/fs/ntfs3/ntfs_fs.h
+++ b/fs/ntfs3/ntfs_fs.h
@@ -593,6 +593,8 @@ void ntfs_mark_rec_free(struct ntfs_sb_info *sbi, CLST rno, bool is_mft);
  int ntfs_clear_mft_tail(struct ntfs_sb_info *sbi, size_t from, size_t to);
  int ntfs_refresh_zone(struct ntfs_sb_info *sbi);
  int ntfs_update_mftmirr(struct ntfs_sb_info *sbi, int wait);
+void ntfs_bad_inode(struct inode *inode, const char *hint);
+#define _ntfs_bad_inode(i) ntfs_bad_inode(i, __func__)
  enum NTFS_DIRTY_FLAGS {
  	NTFS_DIRTY_CLEAR = 0,
  	NTFS_DIRTY_DIRTY = 1,
-- 
2.37.0

[PATCH 2/6] fs/ntfs3: Refactoring attr_set_size to restore after errors

[Konstantin Komarov]

Added comments to code
Added two undo labels for restoring after errors

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
  fs/ntfs3/attrib.c | 180 ++++++++++++++++++++++++++++++++--------------
  1 file changed, 126 insertions(+), 54 deletions(-)

diff --git a/fs/ntfs3/attrib.c b/fs/ntfs3/attrib.c
index d096d77ea042..7bcae3094712 100644
--- a/fs/ntfs3/attrib.c
+++ b/fs/ntfs3/attrib.c
@@ -173,7 +173,6 @@ int attr_allocate_clusters(struct ntfs_sb_info *sbi, struct runs_tree *run,
  {
  	int err;
  	CLST flen, vcn0 = vcn, pre = pre_alloc ? *pre_alloc : 0;
-	struct wnd_bitmap *wnd = &sbi->used.bitmap;
  	size_t cnt = run->count;
  
  	for (;;) {
@@ -196,9 +195,7 @@ int attr_allocate_clusters(struct ntfs_sb_info *sbi, struct runs_tree *run,
  		/* Add new fragment into run storage. */
  		if (!run_add_entry(run, vcn, lcn, flen, opt == ALLOCATE_MFT)) {
  			/* Undo last 'ntfs_look_for_free_space' */
-			down_write_nested(&wnd->rw_lock, BITMAP_MUTEX_CLUSTERS);
-			wnd_set_free(wnd, lcn, flen);
-			up_write(&wnd->rw_lock);
+			mark_as_free_ex(sbi, lcn, len, false);
  			err = -ENOMEM;
  			goto out;
  		}
@@ -419,40 +416,44 @@ int attr_set_size(struct ntfs_inode *ni, enum ATTR_TYPE type,
  	struct mft_inode *mi, *mi_b;
  	CLST alen, vcn, lcn, new_alen, old_alen, svcn, evcn;
  	CLST next_svcn, pre_alloc = -1, done = 0;
-	bool is_ext;
+	bool is_ext, is_bad = false;
  	u32 align;
  	struct MFT_REC *rec;
  
  again:
+	alen = 0;
  	le_b = NULL;
  	attr_b = ni_find_attr(ni, NULL, &le_b, type, name, name_len, NULL,
  			      &mi_b);
  	if (!attr_b) {
  		err = -ENOENT;
-		goto out;
+		goto bad_inode;
  	}
  
  	if (!attr_b->non_res) {
  		err = attr_set_size_res(ni, attr_b, le_b, mi_b, new_size, run,
  					&attr_b);
-		if (err || !attr_b->non_res)
-			goto out;
+		if (err)
+			return err;
+
+		/* Return if file is still resident. */
+		if (!attr_b->non_res)
+			goto ok1;
  
  		/* Layout of records may be changed, so do a full search. */
  		goto again;
  	}
  
  	is_ext = is_attr_ext(attr_b);
-
-again_1:
  	align = sbi->cluster_size;
-
  	if (is_ext)
  		align <<= attr_b->nres.c_unit;
  
  	old_valid = le64_to_cpu(attr_b->nres.valid_size);
  	old_size = le64_to_cpu(attr_b->nres.data_size);
  	old_alloc = le64_to_cpu(attr_b->nres.alloc_size);
+
+again_1:
  	old_alen = old_alloc >> cluster_bits;
  
  	new_alloc = (new_size + align - 1) & ~(u64)(align - 1);
@@ -475,24 +476,27 @@ int attr_set_size(struct ntfs_inode *ni, enum ATTR_TYPE type,
  		mi = mi_b;
  	} else if (!le_b) {
  		err = -EINVAL;
-		goto out;
+		goto bad_inode;
  	} else {
  		le = le_b;
  		attr = ni_find_attr(ni, attr_b, &le, type, name, name_len, &vcn,
  				    &mi);
  		if (!attr) {
  			err = -EINVAL;
-			goto out;
+			goto bad_inode;
  		}
  
  next_le_1:
  		svcn = le64_to_cpu(attr->nres.svcn);
  		evcn = le64_to_cpu(attr->nres.evcn);
  	}
-
+	/*
+	 * Here we have:
+	 * attr,mi,le - last attribute segment (containing 'vcn').
+	 * attr_b,mi_b,le_b - base (primary) attribute segment.
+	 */
  next_le:
  	rec = mi->mrec;
-
  	err = attr_load_runs(attr, ni, run, NULL);
  	if (err)
  		goto out;
@@ -507,6 +511,13 @@ int attr_set_size(struct ntfs_inode *ni, enum ATTR_TYPE type,
  			goto ok;
  		}
  
+		/*
+		 * Add clusters. In simple case we have to:
+		 *  - allocate space (vcn, lcn, len)
+		 *  - update packed run in 'mi'
+		 *  - update attr->nres.evcn
+		 *  - update attr_b->nres.data_size/attr_b->nres.alloc_size
+		 */
  		to_allocate = new_alen - old_alen;
  add_alloc_in_same_attr_seg:
  		lcn = 0;
@@ -520,9 +531,11 @@ int attr_set_size(struct ntfs_inode *ni, enum ATTR_TYPE type,
  			pre_alloc = 0;
  			if (type == ATTR_DATA && !name_len &&
  			    sbi->options->prealloc) {
-				CLST new_alen2 = bytes_to_cluster(
-					sbi, get_pre_allocated(new_size));
-				pre_alloc = new_alen2 - new_alen;
+				pre_alloc =
+					bytes_to_cluster(
+						sbi,
+						get_pre_allocated(new_size)) -
+					new_alen;
  			}
  
  			/* Get the last LCN to allocate from. */
@@ -580,7 +593,7 @@ int attr_set_size(struct ntfs_inode *ni, enum ATTR_TYPE type,
  pack_runs:
  		err = mi_pack_runs(mi, attr, run, vcn - svcn);
  		if (err)
-			goto out;
+			goto undo_1;
  
  		next_svcn = le64_to_cpu(attr->nres.evcn) + 1;
  		new_alloc_tmp = (u64)next_svcn << cluster_bits;
@@ -614,7 +627,7 @@ int attr_set_size(struct ntfs_inode *ni, enum ATTR_TYPE type,
  		if (type == ATTR_LIST) {
  			err = ni_expand_list(ni);
  			if (err)
-				goto out;
+				goto undo_2;
  			if (next_svcn < vcn)
  				goto pack_runs;
  
@@ -624,8 +637,9 @@ int attr_set_size(struct ntfs_inode *ni, enum ATTR_TYPE type,
  
  		if (!ni->attr_list.size) {
  			err = ni_create_attr_list(ni);
+			/* In case of error layout of records is not changed. */
  			if (err)
-				goto out;
+				goto undo_2;
  			/* Layout of records is changed. */
  		}
  
@@ -638,47 +652,56 @@ int attr_set_size(struct ntfs_inode *ni, enum ATTR_TYPE type,
  		err = ni_insert_nonresident(ni, type, name, name_len, run,
  					    next_svcn, vcn - next_svcn,
  					    attr_b->flags, &attr, &mi, NULL);
-		if (err)
-			goto out;
-
-		if (!is_mft)
-			run_truncate_head(run, evcn + 1);
  
-		svcn = le64_to_cpu(attr->nres.svcn);
-		evcn = le64_to_cpu(attr->nres.evcn);
-
-		le_b = NULL;
  		/*
  		 * Layout of records maybe changed.
  		 * Find base attribute to update.
  		 */
+		le_b = NULL;
  		attr_b = ni_find_attr(ni, NULL, &le_b, type, name, name_len,
  				      NULL, &mi_b);
  		if (!attr_b) {
-			err = -ENOENT;
-			goto out;
+			err = -EINVAL;
+			goto bad_inode;
  		}
  
-		attr_b->nres.alloc_size = cpu_to_le64((u64)vcn << cluster_bits);
-		attr_b->nres.data_size = attr_b->nres.alloc_size;
-		attr_b->nres.valid_size = attr_b->nres.alloc_size;
+		if (err) {
+			/* ni_insert_nonresident failed. */
+			attr = NULL;
+			goto undo_2;
+		}
+
+		if (!is_mft)
+			run_truncate_head(run, evcn + 1);
+
+		svcn = le64_to_cpu(attr->nres.svcn);
+		evcn = le64_to_cpu(attr->nres.evcn);
+
+		/*
+		 * Attribute is in consistency state.
+		 * Save this point to restore to if next steps fail.
+		 */
+		old_valid = old_size = old_alloc = (u64)vcn << cluster_bits;
+		attr_b->nres.valid_size = attr_b->nres.data_size =
+			attr_b->nres.alloc_size = cpu_to_le64(old_size);
  		mi_b->dirty = true;
  		goto again_1;
  	}
  
  	if (new_size != old_size ||
  	    (new_alloc != old_alloc && !keep_prealloc)) {
+		/*
+		 * Truncate clusters. In simple case we have to:
+		 *  - update packed run in 'mi'
+		 *  - update attr->nres.evcn
+		 *  - update attr_b->nres.data_size/attr_b->nres.alloc_size
+		 *  - mark and trim clusters as free (vcn, lcn, len)
+		 */
+		CLST dlen = 0;
+
  		vcn = max(svcn, new_alen);
  		new_alloc_tmp = (u64)vcn << cluster_bits;
  
-		alen = 0;
-		err = run_deallocate_ex(sbi, run, vcn, evcn - vcn + 1, &alen,
-					true);
-		if (err)
-			goto out;
-
-		run_truncate(run, vcn);
-
  		if (vcn > svcn) {
  			err = mi_pack_runs(mi, attr, run, vcn - svcn);
  			if (err)
@@ -697,7 +720,7 @@ int attr_set_size(struct ntfs_inode *ni, enum ATTR_TYPE type,
  
  			if (!al_remove_le(ni, le)) {
  				err = -EINVAL;
-				goto out;
+				goto bad_inode;
  			}
  
  			le = (struct ATTR_LIST_ENTRY *)((u8 *)le - le_sz);
@@ -723,12 +746,20 @@ int attr_set_size(struct ntfs_inode *ni, enum ATTR_TYPE type,
  				attr_b->nres.valid_size =
  					attr_b->nres.alloc_size;
  		}
+		mi_b->dirty = true;
  
-		if (is_ext)
+		err = run_deallocate_ex(sbi, run, vcn, evcn - vcn + 1, &dlen,
+					true);
+		if (err)
+			goto out;
+
+		if (is_ext) {
+			/* dlen - really deallocated clusters. */
  			le64_sub_cpu(&attr_b->nres.total_size,
-				     ((u64)alen << cluster_bits));
+				     ((u64)dlen << cluster_bits));
+		}
  
-		mi_b->dirty = true;
+		run_truncate(run, vcn);
  
  		if (new_alloc_tmp <= new_alloc)
  			goto ok;
@@ -747,7 +778,7 @@ int attr_set_size(struct ntfs_inode *ni, enum ATTR_TYPE type,
  		if (le->type != type || le->name_len != name_len ||
  		    memcmp(le_name(le), name, name_len * sizeof(short))) {
  			err = -EINVAL;
-			goto out;
+			goto bad_inode;
  		}
  
  		err = ni_load_mi(ni, le, &mi);
@@ -757,7 +788,7 @@ int attr_set_size(struct ntfs_inode *ni, enum ATTR_TYPE type,
  		attr = mi_find_attr(mi, NULL, type, name, name_len, &le->id);
  		if (!attr) {
  			err = -EINVAL;
-			goto out;
+			goto bad_inode;
  		}
  		goto next_le_1;
  	}
@@ -772,13 +803,13 @@ int attr_set_size(struct ntfs_inode *ni, enum ATTR_TYPE type,
  		}
  	}
  
-out:
-	if (!err && attr_b && ret)
+ok1:
+	if (ret)
  		*ret = attr_b;
  
  	/* Update inode_set_bytes. */
-	if (!err && ((type == ATTR_DATA && !name_len) ||
-		     (type == ATTR_ALLOC && name == I30_NAME))) {
+	if (((type == ATTR_DATA && !name_len) ||
+	     (type == ATTR_ALLOC && name == I30_NAME))) {
  		bool dirty = false;
  
  		if (ni->vfs_inode.i_size != new_size) {
@@ -786,7 +817,7 @@ int attr_set_size(struct ntfs_inode *ni, enum ATTR_TYPE type,
  			dirty = true;
  		}
  
-		if (attr_b && attr_b->non_res) {
+		if (attr_b->non_res) {
  			new_alloc = le64_to_cpu(attr_b->nres.alloc_size);
  			if (inode_get_bytes(&ni->vfs_inode) != new_alloc) {
  				inode_set_bytes(&ni->vfs_inode, new_alloc);
@@ -800,6 +831,47 @@ int attr_set_size(struct ntfs_inode *ni, enum ATTR_TYPE type,
  		}
  	}
  
+	return 0;
+
+undo_2:
+	vcn -= alen;
+	attr_b->nres.data_size = cpu_to_le64(old_size);
+	attr_b->nres.valid_size = cpu_to_le64(old_valid);
+	attr_b->nres.alloc_size = cpu_to_le64(old_alloc);
+
+	/* Restore 'attr' and 'mi'. */
+	if (attr)
+		goto restore_run;
+
+	if (le64_to_cpu(attr_b->nres.svcn) <= svcn &&
+	    svcn <= le64_to_cpu(attr_b->nres.evcn)) {
+		attr = attr_b;
+		le = le_b;
+		mi = mi_b;
+	} else if (!le_b) {
+		err = -EINVAL;
+		goto bad_inode;
+	} else {
+		le = le_b;
+		attr = ni_find_attr(ni, attr_b, &le, type, name, name_len,
+				    &svcn, &mi);
+		if (!attr)
+			goto bad_inode;
+	}
+
+restore_run:
+	if (mi_pack_runs(mi, attr, run, evcn - svcn + 1))
+		is_bad = true;
+
+undo_1:
+	run_deallocate_ex(sbi, run, vcn, alen, NULL, false);
+
+	run_truncate(run, vcn);
+out:
+	if (is_bad) {
+bad_inode:
+		_ntfs_bad_inode(&ni->vfs_inode);
+	}
  	return err;
  }
  
-- 
2.37.0

[PATCH 3/6] fs/ntfs3: Refactoring attr_punch_hole to restore after errors

[Konstantin Komarov]

Added comments to code
Added new function run_clone to make a copy of run
Added done and undo labels for restoring after errors

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
  fs/ntfs3/attrib.c  | 117 ++++++++++++++++++++++++++++++---------------
  fs/ntfs3/ntfs_fs.h |   1 +
  fs/ntfs3/run.c     |  25 ++++++++++
  3 files changed, 105 insertions(+), 38 deletions(-)

diff --git a/fs/ntfs3/attrib.c b/fs/ntfs3/attrib.c
index 7bcae3094712..24d545041787 100644
--- a/fs/ntfs3/attrib.c
+++ b/fs/ntfs3/attrib.c
@@ -140,7 +140,10 @@ static int run_deallocate_ex(struct ntfs_sb_info *sbi, struct runs_tree *run,
  		}
  
  		if (lcn != SPARSE_LCN) {
-			mark_as_free_ex(sbi, lcn, clen, trim);
+			if (sbi) {
+				/* mark bitmap range [lcn + clen) as free and trim clusters. */
+				mark_as_free_ex(sbi, lcn, clen, trim);
+			}
  			dn += clen;
  		}
  
@@ -2002,10 +2005,11 @@ int attr_punch_hole(struct ntfs_inode *ni, u64 vbo, u64 bytes, u32 *frame_size)
  	struct ATTRIB *attr = NULL, *attr_b;
  	struct ATTR_LIST_ENTRY *le, *le_b;
  	struct mft_inode *mi, *mi_b;
-	CLST svcn, evcn1, vcn, len, end, alen, dealloc, next_svcn;
+	CLST svcn, evcn1, vcn, len, end, alen, hole, next_svcn;
  	u64 total_size, alloc_size;
  	u32 mask;
  	__le16 a_flags;
+	struct runs_tree run2;
  
  	if (!bytes)
  		return 0;
@@ -2057,6 +2061,9 @@ int attr_punch_hole(struct ntfs_inode *ni, u64 vbo, u64 bytes, u32 *frame_size)
  	}
  
  	down_write(&ni->file.run_lock);
+	run_init(&run2);
+	run_truncate(run, 0);
+
  	/*
  	 * Enumerate all attribute segments and punch hole where necessary.
  	 */
@@ -2064,7 +2071,7 @@ int attr_punch_hole(struct ntfs_inode *ni, u64 vbo, u64 bytes, u32 *frame_size)
  	vcn = vbo >> sbi->cluster_bits;
  	len = bytes >> sbi->cluster_bits;
  	end = vcn + len;
-	dealloc = 0;
+	hole = 0;
  
  	svcn = le64_to_cpu(attr_b->nres.svcn);
  	evcn1 = le64_to_cpu(attr_b->nres.evcn) + 1;
@@ -2076,14 +2083,14 @@ int attr_punch_hole(struct ntfs_inode *ni, u64 vbo, u64 bytes, u32 *frame_size)
  		mi = mi_b;
  	} else if (!le_b) {
  		err = -EINVAL;
-		goto out;
+		goto bad_inode;
  	} else {
  		le = le_b;
  		attr = ni_find_attr(ni, attr_b, &le, ATTR_DATA, NULL, 0, &vcn,
  				    &mi);
  		if (!attr) {
  			err = -EINVAL;
-			goto out;
+			goto bad_inode;
  		}
  
  		svcn = le64_to_cpu(attr->nres.svcn);
@@ -2091,69 +2098,91 @@ int attr_punch_hole(struct ntfs_inode *ni, u64 vbo, u64 bytes, u32 *frame_size)
  	}
  
  	while (svcn < end) {
-		CLST vcn1, zero, dealloc2;
+		CLST vcn1, zero, hole2 = hole;
  
  		err = attr_load_runs(attr, ni, run, &svcn);
  		if (err)
-			goto out;
+			goto done;
  		vcn1 = max(vcn, svcn);
  		zero = min(end, evcn1) - vcn1;
  
-		dealloc2 = dealloc;
-		err = run_deallocate_ex(sbi, run, vcn1, zero, &dealloc, true);
+		/*
+		 * Check range [vcn1 + zero).
+		 * Calculate how many clusters there are.
+		 * Don't do any destructive actions.
+		 */
+		err = run_deallocate_ex(NULL, run, vcn1, zero, &hole2, false);
  		if (err)
-			goto out;
+			goto done;
  
-		if (dealloc2 == dealloc) {
-			/* Looks like the required range is already sparsed. */
-		} else {
-			if (!run_add_entry(run, vcn1, SPARSE_LCN, zero,
-					   false)) {
-				err = -ENOMEM;
-				goto out;
-			}
+		/* Check if required range is already hole. */
+		if (hole2 == hole)
+			goto next_attr;
  
-			err = mi_pack_runs(mi, attr, run, evcn1 - svcn);
+		/* Make a clone of run to undo. */
+		err = run_clone(run, &run2);
+		if (err)
+			goto done;
+
+		/* Make a hole range (sparse) [vcn1 + zero). */
+		if (!run_add_entry(run, vcn1, SPARSE_LCN, zero, false)) {
+			err = -ENOMEM;
+			goto done;
+		}
+
+		/* Update run in attribute segment. */
+		err = mi_pack_runs(mi, attr, run, evcn1 - svcn);
+		if (err)
+			goto done;
+		next_svcn = le64_to_cpu(attr->nres.evcn) + 1;
+		if (next_svcn < evcn1) {
+			/* Insert new attribute segment. */
+			err = ni_insert_nonresident(ni, ATTR_DATA, NULL, 0, run,
+						    next_svcn,
+						    evcn1 - next_svcn, a_flags,
+						    &attr, &mi, &le);
  			if (err)
-				goto out;
-			next_svcn = le64_to_cpu(attr->nres.evcn) + 1;
-			if (next_svcn < evcn1) {
-				err = ni_insert_nonresident(ni, ATTR_DATA, NULL,
-							    0, run, next_svcn,
-							    evcn1 - next_svcn,
-							    a_flags, &attr, &mi,
-							    &le);
-				if (err)
-					goto out;
-				/* Layout of records maybe changed. */
-				attr_b = NULL;
-			}
+				goto undo_punch;
+
+			/* Layout of records maybe changed. */
+			attr_b = NULL;
  		}
+
+		/* Real deallocate. Should not fail. */
+		run_deallocate_ex(sbi, &run2, vcn1, zero, &hole, true);
+
+next_attr:
  		/* Free all allocated memory. */
  		run_truncate(run, 0);
  
  		if (evcn1 >= alen)
  			break;
  
+		/* Get next attribute segment. */
  		attr = ni_enum_attr_ex(ni, attr, &le, &mi);
  		if (!attr) {
  			err = -EINVAL;
-			goto out;
+			goto bad_inode;
  		}
  
  		svcn = le64_to_cpu(attr->nres.svcn);
  		evcn1 = le64_to_cpu(attr->nres.evcn) + 1;
  	}
  
-	total_size -= (u64)dealloc << sbi->cluster_bits;
+done:
+	if (!hole)
+		goto out;
+
  	if (!attr_b) {
  		attr_b = ni_find_attr(ni, NULL, NULL, ATTR_DATA, NULL, 0, NULL,
  				      &mi_b);
  		if (!attr_b) {
  			err = -EINVAL;
-			goto out;
+			goto bad_inode;
  		}
  	}
+
+	total_size -= (u64)hole << sbi->cluster_bits;
  	attr_b->nres.total_size = cpu_to_le64(total_size);
  	mi_b->dirty = true;
  
@@ -2163,11 +2192,23 @@ int attr_punch_hole(struct ntfs_inode *ni, u64 vbo, u64 bytes, u32 *frame_size)
  	mark_inode_dirty(&ni->vfs_inode);
  
  out:
+	run_close(&run2);
  	up_write(&ni->file.run_lock);
-	if (err)
-		_ntfs_bad_inode(&ni->vfs_inode);
-
  	return err;
+
+bad_inode:
+	_ntfs_bad_inode(&ni->vfs_inode);
+	goto out;
+
+undo_punch:
+	/*
+	 * Restore packed runs.
+	 * 'mi_pack_runs' should not fail, cause we restore original.
+	 */
+	if (mi_pack_runs(mi, attr, &run2, evcn1 - svcn))
+		goto bad_inode;
+
+	goto done;
  }
  
  /*
diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h
index c3e17090effc..23f93c263091 100644
--- a/fs/ntfs3/ntfs_fs.h
+++ b/fs/ntfs3/ntfs_fs.h
@@ -798,6 +798,7 @@ int run_unpack_ex(struct runs_tree *run, struct ntfs_sb_info *sbi, CLST ino,
  #define run_unpack_ex run_unpack
  #endif
  int run_get_highest_vcn(CLST vcn, const u8 *run_buf, u64 *highest_vcn);
+int run_clone(const struct runs_tree *run, struct runs_tree *new_run);
  
  /* Globals from super.c */
  void *ntfs_set_shared(void *ptr, u32 bytes);
diff --git a/fs/ntfs3/run.c b/fs/ntfs3/run.c
index e4bd46b02531..ed09b7a6f6e5 100644
--- a/fs/ntfs3/run.c
+++ b/fs/ntfs3/run.c
@@ -1157,3 +1157,28 @@ int run_get_highest_vcn(CLST vcn, const u8 *run_buf, u64 *highest_vcn)
  	*highest_vcn = vcn64 - 1;
  	return 0;
  }
+
+/*
+ * run_clone
+ *
+ * Make a copy of run
+ */
+int run_clone(const struct runs_tree *run, struct runs_tree *new_run)
+{
+	size_t bytes = run->count * sizeof(struct ntfs_run);
+
+	if (bytes > new_run->allocated) {
+		struct ntfs_run *new_ptr = kvmalloc(bytes, GFP_KERNEL);
+
+		if (!new_ptr)
+			return -ENOMEM;
+
+		kvfree(new_run->runs);
+		new_run->runs = new_ptr;
+		new_run->allocated = bytes;
+	}
+
+	memcpy(new_run->runs, run->runs, bytes);
+	new_run->count = run->count;
+	return 0;
+}
-- 
2.37.0

Re: [PATCH 3/6] fs/ntfs3: Refactoring attr_punch_hole to restore after errors

[Joe Perches]

On Wed, 2022-07-13 at 19:46 +0300, Konstantin Komarov wrote:
> Added comments to code
> Added new function run_clone to make a copy of run
> Added done and undo labels for restoring after errors

trivia:

> diff --git a/fs/ntfs3/attrib.c b/fs/ntfs3/attrib.c
[]
> index 7bcae3094712..24d545041787 100644llocate_ex(struct ntfs_sb_info *sbi, struct runs_tree *run,
>   		}
>   
>   		if (lcn != SPARSE_LCN) {
> -			mark_as_free_ex(sbi, lcn, clen, trim);
> +			if (sbi) {
> +				/* mark bitmap range [lcn + clen) as free and trim clusters. */

presumably the brackets or parentheses [ ) should match

[]

> @@ -2091,69 +2098,91 @@ int attr_punch_hole(struct ntfs_inode *ni, u64 vbo, u64 bytes, u32 *frame_size)
[]
> +
> +		/* Make a hole range (sparse) [vcn1 + zero). */

here too

> diff --git a/fs/ntfs3/run.c b/fs/ntfs3/run.c
[]
> @@ -1157,3 +1157,28 @@ int run_get_highest_vcn(CLST vcn, const u8 *run_buf, u64 *highest_vcn)
>   	*highest_vcn = vcn64 - 1;
>   	return 0;
>   }
> +
> +/*
> + * run_clone
> + *
> + * Make a copy of run
> + */
> +int run_clone(const struct runs_tree *run, struct runs_tree *new_run)
> +{
> +	size_t bytes = run->count * sizeof(struct ntfs_run);
> +
> +	if (bytes > new_run->allocated) {
> +		struct ntfs_run *new_ptr = kvmalloc(bytes, GFP_KERNEL);

kvmalloc_array ?

> +
> +		if (!new_ptr)
> +			return -ENOMEM;
> +
> +		kvfree(new_run->runs);
> +		new_run->runs = new_ptr;
> +		new_run->allocated = bytes;
> +	}
> +
> +	memcpy(new_run->runs, run->runs, bytes);
> +	new_run->count = run->count;
> +	return 0;
> +}

[PATCH 4/6] fs/ntfs3: Refactoring attr_insert_range to restore after errors

[Konstantin Komarov]

Added done and undo labels for restoring after errors

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
  fs/ntfs3/attrib.c | 115 ++++++++++++++++++++++++++++++++++------------
  1 file changed, 86 insertions(+), 29 deletions(-)

diff --git a/fs/ntfs3/attrib.c b/fs/ntfs3/attrib.c
index 24d545041787..71f870d497ae 100644
--- a/fs/ntfs3/attrib.c
+++ b/fs/ntfs3/attrib.c
@@ -2275,28 +2275,29 @@ int attr_insert_range(struct ntfs_inode *ni, u64 vbo, u64 bytes)
  
  	if (!attr_b->non_res) {
  		err = attr_set_size(ni, ATTR_DATA, NULL, 0, run,
-				    data_size + bytes, NULL, false, &attr);
+				    data_size + bytes, NULL, false, NULL);
+
+		le_b = NULL;
+		attr_b = ni_find_attr(ni, NULL, &le_b, ATTR_DATA, NULL, 0, NULL,
+				      &mi_b);
+		if (!attr_b) {
+			err = -EINVAL;
+			goto bad_inode;
+		}
+
  		if (err)
  			goto out;
-		if (!attr->non_res) {
+
+		if (!attr_b->non_res) {
  			/* Still resident. */
-			char *data = Add2Ptr(attr, attr->res.data_off);
+			char *data = Add2Ptr(attr_b, attr_b->res.data_off);
  
  			memmove(data + bytes, data, bytes);
  			memset(data, 0, bytes);
-			err = 0;
-			goto out;
+			goto done;
  		}
+
  		/* Resident files becomes nonresident. */
-		le_b = NULL;
-		attr_b = ni_find_attr(ni, NULL, &le_b, ATTR_DATA, NULL, 0, NULL,
-				      &mi_b);
-		if (!attr_b)
-			return -ENOENT;
-		if (!attr_b->non_res) {
-			err = -EINVAL;
-			goto out;
-		}
  		data_size = le64_to_cpu(attr_b->nres.data_size);
  		alloc_size = le64_to_cpu(attr_b->nres.alloc_size);
  	}
@@ -2314,14 +2315,14 @@ int attr_insert_range(struct ntfs_inode *ni, u64 vbo, u64 bytes)
  		mi = mi_b;
  	} else if (!le_b) {
  		err = -EINVAL;
-		goto out;
+		goto bad_inode;
  	} else {
  		le = le_b;
  		attr = ni_find_attr(ni, attr_b, &le, ATTR_DATA, NULL, 0, &vcn,
  				    &mi);
  		if (!attr) {
  			err = -EINVAL;
-			goto out;
+			goto bad_inode;
  		}
  
  		svcn = le64_to_cpu(attr->nres.svcn);
@@ -2344,7 +2345,6 @@ int attr_insert_range(struct ntfs_inode *ni, u64 vbo, u64 bytes)
  		goto out;
  
  	next_svcn = le64_to_cpu(attr->nres.evcn) + 1;
-	run_truncate_head(run, next_svcn);
  
  	while ((attr = ni_enum_attr_ex(ni, attr, &le, &mi)) &&
  	       attr->type == ATTR_DATA && !attr->name_len) {
@@ -2357,9 +2357,27 @@ int attr_insert_range(struct ntfs_inode *ni, u64 vbo, u64 bytes)
  		mi->dirty = true;
  	}
  
+	if (next_svcn < evcn1 + len) {
+		err = ni_insert_nonresident(ni, ATTR_DATA, NULL, 0, run,
+					    next_svcn, evcn1 + len - next_svcn,
+					    a_flags, NULL, NULL, NULL);
+
+		le_b = NULL;
+		attr_b = ni_find_attr(ni, NULL, &le_b, ATTR_DATA, NULL, 0, NULL,
+				      &mi_b);
+		if (!attr_b) {
+			err = -EINVAL;
+			goto bad_inode;
+		}
+
+		if (err) {
+			/* ni_insert_nonresident failed. Try to undo. */
+			goto undo_insert_range;
+		}
+	}
+
  	/*
-	 * Update primary attribute segment in advance.
-	 * pointer attr_b may become invalid (layout of mft is changed)
+	 * Update primary attribute segment.
  	 */
  	if (vbo <= ni->i_valid)
  		ni->i_valid += bytes;
@@ -2374,14 +2392,7 @@ int attr_insert_range(struct ntfs_inode *ni, u64 vbo, u64 bytes)
  		attr_b->nres.valid_size = cpu_to_le64(ni->i_valid);
  	mi_b->dirty = true;
  
-	if (next_svcn < evcn1 + len) {
-		err = ni_insert_nonresident(ni, ATTR_DATA, NULL, 0, run,
-					    next_svcn, evcn1 + len - next_svcn,
-					    a_flags, NULL, NULL, NULL);
-		if (err)
-			goto out;
-	}
-
+done:
  	ni->vfs_inode.i_size += bytes;
  	ni->ni_flags |= NI_FLAG_UPDATE_PARENT;
  	mark_inode_dirty(&ni->vfs_inode);
@@ -2390,8 +2401,54 @@ int attr_insert_range(struct ntfs_inode *ni, u64 vbo, u64 bytes)
  	run_truncate(run, 0); /* clear cached values. */
  
  	up_write(&ni->file.run_lock);
-	if (err)
-		_ntfs_bad_inode(&ni->vfs_inode);
  
  	return err;
+
+bad_inode:
+	_ntfs_bad_inode(&ni->vfs_inode);
+	goto out;
+
+undo_insert_range:
+	svcn = le64_to_cpu(attr_b->nres.svcn);
+	evcn1 = le64_to_cpu(attr_b->nres.evcn) + 1;
+
+	if (svcn <= vcn && vcn < evcn1) {
+		attr = attr_b;
+		le = le_b;
+		mi = mi_b;
+	} else if (!le_b) {
+		goto bad_inode;
+	} else {
+		le = le_b;
+		attr = ni_find_attr(ni, attr_b, &le, ATTR_DATA, NULL, 0, &vcn,
+				    &mi);
+		if (!attr) {
+			goto bad_inode;
+		}
+
+		svcn = le64_to_cpu(attr->nres.svcn);
+		evcn1 = le64_to_cpu(attr->nres.evcn) + 1;
+	}
+
+	if (attr_load_runs(attr, ni, run, NULL))
+		goto bad_inode;
+
+	if (!run_collapse_range(run, vcn, len))
+		goto bad_inode;
+
+	if (mi_pack_runs(mi, attr, run, evcn1 + len - svcn))
+		goto bad_inode;
+
+	while ((attr = ni_enum_attr_ex(ni, attr, &le, &mi)) &&
+	       attr->type == ATTR_DATA && !attr->name_len) {
+		le64_sub_cpu(&attr->nres.svcn, len);
+		le64_sub_cpu(&attr->nres.evcn, len);
+		if (le) {
+			le->vcn = attr->nres.svcn;
+			ni->attr_list.dirty = true;
+		}
+		mi->dirty = true;
+	}
+
+	goto out;
  }
-- 
2.37.0

[PATCH 5/6] fs/ntfs3: Create MFT zone only if length is large enough

[Konstantin Komarov]

Also removed uninformative print

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
  fs/ntfs3/fsntfs.c | 11 +++--------
  1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/fs/ntfs3/fsntfs.c b/fs/ntfs3/fsntfs.c
index e6f5bf684da4..d09213ae9755 100644
--- a/fs/ntfs3/fsntfs.c
+++ b/fs/ntfs3/fsntfs.c
@@ -806,12 +806,6 @@ int ntfs_refresh_zone(struct ntfs_sb_info *sbi)
  
  	/* Try to allocate clusters after last MFT run. */
  	zlen = wnd_find(wnd, sbi->zone_max, lcn_s, 0, &lcn_s);
-	if (!zlen) {
-		ntfs_notice(sbi->sb, "MftZone: unavailable");
-		return 0;
-	}
-
-	/* Truncate too large zone. */
  	wnd_zone_set(wnd, lcn_s, zlen);
  
  	return 0;
@@ -2473,8 +2467,9 @@ void mark_as_free_ex(struct ntfs_sb_info *sbi, CLST lcn, CLST len, bool trim)
  	if (zlen == zone_len) {
  		/* MFT zone already has maximum size. */
  	} else if (!zone_len) {
-		/* Create MFT zone. */
-		wnd_zone_set(wnd, lcn, zlen);
+		/* Create MFT zone only if 'zlen' is large enough. */
+		if (zlen == sbi->zone_max)
+			wnd_zone_set(wnd, lcn, zlen);
  	} else {
  		CLST zone_lcn = wnd_zone_bit(wnd);
  
-- 
2.37.0

[PATCH 6/6] fs/ntfs3: Make ni_ins_new_attr return error

[Konstantin Komarov]

Function ni_ins_new_attr now returns ERR_PTR(err),
so we check it now in other functions like ni_expand_mft_list

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
  fs/ntfs3/frecord.c | 29 ++++++++++++++++++++++++++---
  1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
index bdc568053fae..381a38a06ec2 100644
--- a/fs/ntfs3/frecord.c
+++ b/fs/ntfs3/frecord.c
@@ -469,7 +469,7 @@ ni_ins_new_attr(struct ntfs_inode *ni, struct mft_inode *mi,
  				&ref, &le);
  		if (err) {
  			/* No memory or no space. */
-			return NULL;
+			return ERR_PTR(err);
  		}
  		le_added = true;
  
@@ -1011,6 +1011,8 @@ static int ni_ins_attr_ext(struct ntfs_inode *ni, struct ATTR_LIST_ENTRY *le,
  				       name_off, svcn, ins_le);
  		if (!attr)
  			continue;
+		if (IS_ERR(attr))
+			return PTR_ERR(attr);
  
  		if (ins_attr)
  			*ins_attr = attr;
@@ -1032,8 +1034,15 @@ static int ni_ins_attr_ext(struct ntfs_inode *ni, struct ATTR_LIST_ENTRY *le,
  
  	attr = ni_ins_new_attr(ni, mi, le, type, name, name_len, asize,
  			       name_off, svcn, ins_le);
-	if (!attr)
+	if (!attr) {
+		err = -EINVAL;
  		goto out2;
+	}
+
+	if (IS_ERR(attr)) {
+		err = PTR_ERR(attr);
+		goto out2;
+	}
  
  	if (ins_attr)
  		*ins_attr = attr;
@@ -1045,7 +1054,6 @@ static int ni_ins_attr_ext(struct ntfs_inode *ni, struct ATTR_LIST_ENTRY *le,
  out2:
  	ni_remove_mi(ni, mi);
  	mi_put(mi);
-	err = -EINVAL;
  
  out1:
  	ntfs_mark_rec_free(sbi, rno, is_mft);
@@ -1101,6 +1109,11 @@ static int ni_insert_attr(struct ntfs_inode *ni, enum ATTR_TYPE type,
  	if (asize <= free) {
  		attr = ni_ins_new_attr(ni, &ni->mi, NULL, type, name, name_len,
  				       asize, name_off, svcn, ins_le);
+		if (IS_ERR(attr)) {
+			err = PTR_ERR(attr);
+			goto out;
+		}
+
  		if (attr) {
  			if (ins_attr)
  				*ins_attr = attr;
@@ -1198,6 +1211,11 @@ static int ni_insert_attr(struct ntfs_inode *ni, enum ATTR_TYPE type,
  		goto out;
  	}
  
+	if (IS_ERR(attr)) {
+		err = PTR_ERR(attr);
+		goto out;
+	}
+
  	if (ins_attr)
  		*ins_attr = attr;
  	if (ins_mi)
@@ -1313,6 +1331,11 @@ static int ni_expand_mft_list(struct ntfs_inode *ni)
  		goto out;
  	}
  
+	if (IS_ERR(attr)) {
+		err = PTR_ERR(attr);
+		goto out;
+	}
+
  	attr->non_res = 1;
  	attr->name_off = SIZEOF_NONRESIDENT_LE;
  	attr->flags = 0;
-- 
2.37.0