[PATCH 0/2] fs/ntfs3: Refactoring and improving logic in run_pack

[PATCH 0/2] fs/ntfs3: Refactoring and improving logic in run_pack

[Konstantin Komarov]

2 patches:
- some comments and making function static;
- improving speed of run_pack by checking runs in advance

Konstantin Komarov (2):
   fs/ntfs3: Added comments to frecord functions
   fs/ntfs3: Check possible errors in run_pack in advance

  fs/ntfs3/bitmap.c  |  3 +--
  fs/ntfs3/frecord.c |  8 ++++----
  fs/ntfs3/ntfs_fs.h |  1 -
  fs/ntfs3/run.c     | 41 +++++++++++++++++++++++------------------
  4 files changed, 28 insertions(+), 25 deletions(-)

-- 
2.37.0

[PATCH 1/2] fs/ntfs3: Added comments to frecord functions

[Konstantin Komarov]

Added some comments in frecord.c for more context.
Also changed run_lookup to static because it's an internal function.

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
  fs/ntfs3/bitmap.c  | 3 +--
  fs/ntfs3/frecord.c | 8 ++++----
  fs/ntfs3/ntfs_fs.h | 1 -
  fs/ntfs3/run.c     | 2 +-
  4 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/fs/ntfs3/bitmap.c b/fs/ntfs3/bitmap.c
index e3b5680fd516..bb9ebb160227 100644
--- a/fs/ntfs3/bitmap.c
+++ b/fs/ntfs3/bitmap.c
@@ -1393,9 +1393,8 @@ int wnd_extend(struct wnd_bitmap *wnd, size_t new_bits)
  
  void wnd_zone_set(struct wnd_bitmap *wnd, size_t lcn, size_t len)
  {
-	size_t zlen;
+	size_t zlen = wnd->zone_end - wnd->zone_bit;
  
-	zlen = wnd->zone_end - wnd->zone_bit;
  	if (zlen)
  		wnd_add_free_ext(wnd, wnd->zone_bit, zlen, false);
  
diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
index acd9f444bd64..bc48923693a9 100644
--- a/fs/ntfs3/frecord.c
+++ b/fs/ntfs3/frecord.c
@@ -1287,7 +1287,7 @@ static int ni_expand_mft_list(struct ntfs_inode *ni)
  	done = asize - run_size - SIZEOF_NONRESIDENT;
  	le32_sub_cpu(&ni->mi.mrec->used, done);
  
-	/* Estimate the size of second part: run_buf=NULL. */
+	/* Estimate packed size (run_buf=NULL). */
  	err = run_pack(run, svcn, evcn + 1 - svcn, NULL, sbi->record_size,
  		       &plen);
  	if (err < 0)
@@ -1317,6 +1317,7 @@ static int ni_expand_mft_list(struct ntfs_inode *ni)
  	attr->name_off = SIZEOF_NONRESIDENT_LE;
  	attr->flags = 0;
  
+	/* This function can't fail - cause already checked above. */
  	run_pack(run, svcn, evcn + 1 - svcn, Add2Ptr(attr, SIZEOF_NONRESIDENT),
  		 run_size, &plen);
  
@@ -1392,8 +1393,6 @@ int ni_expand_list(struct ntfs_inode *ni)
  
  	/* Split MFT data as much as possible. */
  	err = ni_expand_mft_list(ni);
-	if (err)
-		goto out;
  
  out:
  	return !err && !done ? -EOPNOTSUPP : err;
@@ -1419,6 +1418,7 @@ int ni_insert_nonresident(struct ntfs_inode *ni, enum ATTR_TYPE type,
  	u32 run_size, asize;
  	struct ntfs_sb_info *sbi = ni->mi.sbi;
  
+	/* Estimate packed size (run_buf=NULL). */
  	err = run_pack(run, svcn, len, NULL, sbi->max_bytes_per_attr - run_off,
  		       &plen);
  	if (err < 0)
@@ -1448,12 +1448,12 @@ int ni_insert_nonresident(struct ntfs_inode *ni, enum ATTR_TYPE type,
  	attr->name_off = cpu_to_le16(name_off);
  	attr->flags = flags;
  
+	/* This function can't fail - cause already checked above. */
  	run_pack(run, svcn, len, Add2Ptr(attr, run_off), run_size, &plen);
  
  	attr->nres.svcn = cpu_to_le64(svcn);
  	attr->nres.evcn = cpu_to_le64((u64)svcn + len - 1);
  
-	err = 0;
  	if (new_attr)
  		*new_attr = attr;
  
diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h
index b88721e48458..cf1fa69a0eb8 100644
--- a/fs/ntfs3/ntfs_fs.h
+++ b/fs/ntfs3/ntfs_fs.h
@@ -773,7 +773,6 @@ bool run_lookup_entry(const struct runs_tree *run, CLST vcn, CLST *lcn,
  void run_truncate(struct runs_tree *run, CLST vcn);
  void run_truncate_head(struct runs_tree *run, CLST vcn);
  void run_truncate_around(struct runs_tree *run, CLST vcn);
-bool run_lookup(const struct runs_tree *run, CLST vcn, size_t *index);
  bool run_add_entry(struct runs_tree *run, CLST vcn, CLST lcn, CLST len,
  		   bool is_mft);
  bool run_collapse_range(struct runs_tree *run, CLST vcn, CLST len);
diff --git a/fs/ntfs3/run.c b/fs/ntfs3/run.c
index 7609d45a2d72..95fb9d739706 100644
--- a/fs/ntfs3/run.c
+++ b/fs/ntfs3/run.c
@@ -31,7 +31,7 @@ struct ntfs_run {
   * Case of entry missing from list 'index' will be set to
   * point to insertion position for the entry question.
   */
-bool run_lookup(const struct runs_tree *run, CLST vcn, size_t *index)
+static bool run_lookup(const struct runs_tree *run, CLST vcn, size_t *index)
  {
  	size_t min_idx, max_idx, mid_idx;
  	struct ntfs_run *r;
-- 
2.37.0

[PATCH 2/2] fs/ntfs3: Check possible errors in run_pack in advance

[Konstantin Komarov]

Checking in advance speeds things up in some cases.

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
  fs/ntfs3/run.c | 39 ++++++++++++++++++++++-----------------
  1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/fs/ntfs3/run.c b/fs/ntfs3/run.c
index 95fb9d739706..e4bd46b02531 100644
--- a/fs/ntfs3/run.c
+++ b/fs/ntfs3/run.c
@@ -821,26 +821,36 @@ int run_pack(const struct runs_tree *run, CLST svcn, CLST len, u8 *run_buf,
  	CLST next_vcn, vcn, lcn;
  	CLST prev_lcn = 0;
  	CLST evcn1 = svcn + len;
+	const struct ntfs_run *r, *r_end;
  	int packed_size = 0;
  	size_t i;
-	bool ok;
  	s64 dlcn;
  	int offset_size, size_size, tmp;
  
-	next_vcn = vcn = svcn;
-
  	*packed_vcns = 0;
  
  	if (!len)
  		goto out;
  
-	ok = run_lookup_entry(run, vcn, &lcn, &len, &i);
+	/* Check all required entries [svcn, encv1) available. */
+	if (!run_lookup(run, svcn, &i))
+		return -ENOENT;
  
-	if (!ok)
-		goto error;
+	r_end = run->runs + run->count;
+	r = run->runs + i;
  
-	if (next_vcn != vcn)
-		goto error;
+	for (next_vcn = r->vcn + r->len; next_vcn < evcn1;
+	     next_vcn = r->vcn + r->len) {
+		if (++r >= r_end || r->vcn != next_vcn)
+			return -ENOENT;
+	}
+
+	/* Repeat cycle above and pack runs. Assume no errors. */
+	r = run->runs + i;
+	len = svcn - r->vcn;
+	vcn = svcn;
+	lcn = r->lcn == SPARSE_LCN ? SPARSE_LCN : (r->lcn + len);
+	len = r->len - len;
  
  	for (;;) {
  		next_vcn = vcn + len;
@@ -889,12 +899,10 @@ int run_pack(const struct runs_tree *run, CLST svcn, CLST len, u8 *run_buf,
  		if (packed_size + 1 >= run_buf_size || next_vcn >= evcn1)
  			goto out;
  
-		ok = run_get_entry(run, ++i, &vcn, &lcn, &len);
-		if (!ok)
-			goto error;
-
-		if (next_vcn != vcn)
-			goto error;
+		r += 1;
+		vcn = r->vcn;
+		lcn = r->lcn;
+		len = r->len;
  	}
  
  out:
@@ -903,9 +911,6 @@ int run_pack(const struct runs_tree *run, CLST svcn, CLST len, u8 *run_buf,
  		run_buf[0] = 0;
  
  	return packed_size + 1;
-
-error:
-	return -EOPNOTSUPP;
  }
  
  /*
-- 
2.37.0

Re: [PATCH 0/2] fs/ntfs3: Refactoring and improving logic in run_pack

[Christoph Hellwig]

Hi Konstantin,

now that you have time to actively work on the ntfs3 driver again, can
you consider looking into converting the I/O path to iomap, as already
request during the merge?  Getting drivers off the old buffer head based
I/O helpers is something we need to address in the coming years, so
any relatively simple and actually maintained file system would be a
good start.

On Wed, Jul 06, 2022 at 08:31:25PM +0300, Konstantin Komarov wrote:
> 2 patches:
> - some comments and making function static;
> - improving speed of run_pack by checking runs in advance
> 
> Konstantin Komarov (2):
>   fs/ntfs3: Added comments to frecord functions
>   fs/ntfs3: Check possible errors in run_pack in advance
> 
>  fs/ntfs3/bitmap.c  |  3 +--
>  fs/ntfs3/frecord.c |  8 ++++----
>  fs/ntfs3/ntfs_fs.h |  1 -
>  fs/ntfs3/run.c     | 41 +++++++++++++++++++++++------------------
>  4 files changed, 28 insertions(+), 25 deletions(-)
> 
> -- 
> 2.37.0
> 
---end quoted text---

Re: [PATCH 0/2] fs/ntfs3: Refactoring and improving logic in run_pack

[Konstantin Komarov]

On 7/7/22 07:42, Christoph Hellwig wrote:
> Hi Konstantin,
> 
> now that you have time to actively work on the ntfs3 driver again, can
> you consider looking into converting the I/O path to iomap, as already
> request during the merge?  Getting drivers off the old buffer head based
> I/O helpers is something we need to address in the coming years, so
> any relatively simple and actually maintained file system would be a
> good start.
> 
> On Wed, Jul 06, 2022 at 08:31:25PM +0300, Konstantin Komarov wrote:
>> 2 patches:
>> - some comments and making function static;
>> - improving speed of run_pack by checking runs in advance
>>
>> Konstantin Komarov (2):
>>    fs/ntfs3: Added comments to frecord functions
>>    fs/ntfs3: Check possible errors in run_pack in advance
>>
>>   fs/ntfs3/bitmap.c  |  3 +--
>>   fs/ntfs3/frecord.c |  8 ++++----
>>   fs/ntfs3/ntfs_fs.h |  1 -
>>   fs/ntfs3/run.c     | 41 +++++++++++++++++++++++------------------
>>   4 files changed, 28 insertions(+), 25 deletions(-)
>>
>> -- 
>> 2.37.0
>>
> ---end quoted text---

Hi Christoph

I will look into converting the I/O path to iomap.
Thank you for reminding about it.

Re: [PATCH 0/2] fs/ntfs3: Refactoring and improving logic in run_pack

[Christoph Hellwig]

On Wed, Jul 13, 2022 at 07:41:58PM +0300, Konstantin Komarov wrote:
> I will look into converting the I/O path to iomap.
> Thank you for reminding about it.

If you have any questions or need help feel free to contact me.