[RFC] [PATCHv3 0/9] reiser4: batch discard support (FITRIM ioctl): initial implementation.

[RFC] [PATCHv3 0/9] reiser4: batch discard support (FITRIM ioctl): initial implementation.

[Ivan Shapovalov]

This patch series implements FITRIM ioctl support in reiser4.

The FITRIM ioctl is supposed to be applied to any directory inside the target
filesystem. At least, this can be deduced from `fstrim` utility behavior on
mainstream filesystems (e. g. ext2/3/4).

The idea of implementation is per Edward's advices: FITRIM handler iteratively
grabs a portion of disk space, sequentially allocates extents within grabbed
space and commits the resulting atom. This way we avoid starving concurrent
processes of the disk space. This process is repeated until the partition has
been fully scanned and processed.

Points I'm really uncertain of:
- grabbing fixed amount of space (new BA_SOME_SPACE flag, grabs 25% of disk space)
- creation of empty atoms (reiser4_create_atom())
- handling of empty atoms in commit_current_atom()

v2: - fix PATCH 5/7 "reiser4: txnmgr: call reiser4_post_write_back_hook() for empty atoms."
      * fix a typo
      * release atom spinlock before calling reiser4_post_write_back_hook()

v3: - use bitwise shift instead of integer division by 4 to calculate 25%
    - honor "minlen" of ioctl parameters (minimal extent length to consider for discarding)
    - fix a stupidity in reiser4_trim_fs() (two local variables holding the same superblock pointer)

Ivan Shapovalov (9):
  reiser4: block_alloc: add BA_SOME_SPACE flag for grabbing a fixed amount of space.
  reiser4: block_alloc: add a "forward" parameter to reiser4_blocknr_hint to allocate blocks only in forward direction.
  reiser4: txnmgr: free allocated but unneeded atom in atom_begin_and_assign_to_txnh().
  reiser4: txnmgr: add reiser4_create_atom() which creates an empty atom without capturing any nodes.
  reiser4: txnmgr: call reiser4_post_write_back_hook() for empty atoms.
  reiser4: batch discard support: add a dummy FITRIM ioctl handler for directories.
  reiser4: batch discard support: actually implement the FITRIM ioctl handler.
  reiser4: block_alloc: add a "min_len" parameter to reiser4_blocknr_hint to limit allocated extent length from below.
  reiser4: batch discard support: honor minimal extent length passed from the userspace.

 fs/reiser4/block_alloc.c         |   5 ++
 fs/reiser4/block_alloc.h         |  12 +++-
 fs/reiser4/plugin/dir/dir.h      |   2 +
 fs/reiser4/plugin/file_ops.c     |  62 +++++++++++++++++++
 fs/reiser4/plugin/object.c       |   6 +-
 fs/reiser4/plugin/space/bitmap.c |  21 +++++--
 fs/reiser4/super_ops.c           | 126 +++++++++++++++++++++++++++++++++++++++
 fs/reiser4/txnmgr.c              |  57 ++++++++++++------
 fs/reiser4/txnmgr.h              |   2 +
 9 files changed, 265 insertions(+), 28 deletions(-)

-- 
2.0.4

[RFC] [PATCHv3 1/9] reiser4: block_alloc: add BA_SOME_SPACE flag for grabbing a fixed amount of space.

[Ivan Shapovalov]

This is used for FITRIM ioctl which will iteratively grab, allocate and trim disk space
bit by bit to avoid starving the rest of system.

Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
---
 fs/reiser4/block_alloc.c | 5 +++++
 fs/reiser4/block_alloc.h | 5 ++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/reiser4/block_alloc.c b/fs/reiser4/block_alloc.c
index d3d730c..9eed7fc 100644
--- a/fs/reiser4/block_alloc.c
+++ b/fs/reiser4/block_alloc.c
@@ -276,6 +276,11 @@ reiser4_grab(reiser4_context * ctx, __u64 count, reiser4_ba_flags_t flags)
 
 	free_blocks = sbinfo->blocks_free;
 
+	if (flags & BA_SOME_SPACE) {
+		/* Reserve 25% of all free space. */
+		count = free_blocks >> 2;
+	}
+
 	if ((use_reserved && free_blocks < count) ||
 	    (!use_reserved && free_blocks < count + sbinfo->blocks_reserved)) {
 		ret = RETERR(-ENOSPC);
diff --git a/fs/reiser4/block_alloc.h b/fs/reiser4/block_alloc.h
index a4e98af..bfc6be9 100644
--- a/fs/reiser4/block_alloc.h
+++ b/fs/reiser4/block_alloc.h
@@ -79,7 +79,10 @@ enum reiser4_ba_flags {
 	BA_FORCE = (1 << 5),
 
 	/* use default start value for free blocks search. */
-	BA_USE_DEFAULT_SEARCH_START = (1 << 6)
+	BA_USE_DEFAULT_SEARCH_START = (1 << 6),
+
+	/* reserve some fixed amount of space */
+	BA_SOME_SPACE = (1 << 7),
 };
 
 typedef enum reiser4_ba_flags reiser4_ba_flags_t;
-- 
2.0.4

[RFC] [PATCHv3 2/9] reiser4: block_alloc: add a "forward" parameter to reiser4_blocknr_hint to allocate blocks only in forward direction.

[Ivan Shapovalov]

Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
---
 fs/reiser4/block_alloc.h         | 5 +++--
 fs/reiser4/plugin/space/bitmap.c | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/reiser4/block_alloc.h b/fs/reiser4/block_alloc.h
index bfc6be9..08b3941 100644
--- a/fs/reiser4/block_alloc.h
+++ b/fs/reiser4/block_alloc.h
@@ -51,9 +51,10 @@ struct reiser4_blocknr_hint {
 	/* block allocator assumes that blocks, which will be mapped to disk,
 	   are in this specified block_stage */
 	block_stage_t block_stage;
-	/* If direction = 1 allocate blocks in backward direction from the end
-	 * of disk to the beginning of disk.  */
+	/* Allocate blocks only in backward direction starting from blk. */
 	unsigned int backward:1;
+	/* Allocate blocks only in forward direction starting from blk. */
+	unsigned int forward:1;
 
 };
 
diff --git a/fs/reiser4/plugin/space/bitmap.c b/fs/reiser4/plugin/space/bitmap.c
index 3da3f6b..9beaf66 100644
--- a/fs/reiser4/plugin/space/bitmap.c
+++ b/fs/reiser4/plugin/space/bitmap.c
@@ -1127,7 +1127,8 @@ static int alloc_blocks_forward(reiser4_blocknr_hint *hint, int needed,
 	/* There is only one bitmap search if max_dist was specified or first
 	   pass was from the beginning of the bitmap. We also do one pass for
 	   scanning bitmap in backward direction. */
-	if (!(actual_len != 0 || hint->max_dist != 0 || search_start == 0)) {
+	if (actual_len == 0 && search_start != 0 &&
+	    hint->max_dist == 0 && hint->forward == 0) {
 		/* next step is a scanning from 0 to search_start */
 		search_end = search_start;
 		search_start = 0;
-- 
2.0.4

[RFC] [PATCHv3 3/9] reiser4: txnmgr: free allocated but unneeded atom in atom_begin_and_assign_to_txnh().

[Ivan Shapovalov]

It is unclear why it hasn't been done before, as there was a
corresponding notice, and such change apparently does not carry any side
effects.

Do this to avoid duplicating code in the next commit.

Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
---
 fs/reiser4/txnmgr.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/fs/reiser4/txnmgr.c b/fs/reiser4/txnmgr.c
index d73ecb9..68070a9 100644
--- a/fs/reiser4/txnmgr.c
+++ b/fs/reiser4/txnmgr.c
@@ -709,6 +709,9 @@ static int atom_begin_and_assign_to_txnh(txn_atom ** atom_alloc, txn_handle * tx
 			return RETERR(-ENOMEM);
 	}
 
+	atom = *atom_alloc;
+	*atom_alloc = NULL;
+
 	/* and, also, txnmgr spin lock should be taken before jnode and txnh
 	   locks. */
 	mgr = &get_super_private(reiser4_get_current_sb())->tmgr;
@@ -717,18 +720,14 @@ static int atom_begin_and_assign_to_txnh(txn_atom ** atom_alloc, txn_handle * tx
 
 	/* Check whether new atom still needed */
 	if (txnh->atom != NULL) {
-		/* NOTE-NIKITA probably it is rather better to free
-		 * atom_alloc here than thread it up to reiser4_try_capture() */
-
 		spin_unlock_txnh(txnh);
 		spin_unlock_txnmgr(mgr);
 
+		kmem_cache_free(_atom_slab, atom);
+
 		return -E_REPEAT;
 	}
 
-	atom = *atom_alloc;
-	*atom_alloc = NULL;
-
 	atom_init(atom);
 
 	assert("jmacd-17", atom_isclean(atom));
@@ -2031,15 +2030,6 @@ int reiser4_try_capture(jnode *node, znode_lock_mode lock_mode,
 		goto repeat;
 	}
 
-	/* free extra atom object that was possibly allocated by
-	   try_capture_block().
-
-	   Do this before acquiring jnode spin lock to
-	   minimize time spent under lock. --nikita */
-	if (atom_alloc != NULL) {
-		kmem_cache_free(_atom_slab, atom_alloc);
-	}
-
 	if (ret != 0) {
 		if (ret == -E_BLOCK) {
 			assert("nikita-3360",
-- 
2.0.4

[RFC] [PATCHv3 4/9] reiser4: txnmgr: add reiser4_create_atom() which creates an empty atom without capturing any nodes.

[Ivan Shapovalov]

Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
---
 fs/reiser4/txnmgr.c | 24 ++++++++++++++++++++++++
 fs/reiser4/txnmgr.h |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/fs/reiser4/txnmgr.c b/fs/reiser4/txnmgr.c
index 68070a9..2862940 100644
--- a/fs/reiser4/txnmgr.c
+++ b/fs/reiser4/txnmgr.c
@@ -2050,6 +2050,30 @@ int reiser4_try_capture(jnode *node, znode_lock_mode lock_mode,
 	return ret;
 }
 
+/* This function ensures that the current transcrash has an atom without
+ * capturing anything. If needed, an empty atom is created and assigned.
+ */
+int reiser4_create_atom(void)
+{
+	txn_atom *atom_alloc = NULL;
+	txn_handle *txnh = get_current_context()->trans;
+	int ret;
+
+	do {
+		spin_lock_txnh(txnh);
+		if (txnh->atom == NULL) {
+			spin_unlock_txnh(txnh);
+			/* assign empty atom to the txnh and repeat */
+			ret = atom_begin_and_assign_to_txnh(&atom_alloc, txnh);
+		} else {
+			spin_unlock_txnh(txnh);
+			ret = 0;
+		}
+	} while (ret == -E_REPEAT);
+
+	return ret;
+}
+
 static void release_two_atoms(txn_atom *one, txn_atom *two)
 {
 	spin_unlock_atom(one);
diff --git a/fs/reiser4/txnmgr.h b/fs/reiser4/txnmgr.h
index 72b84a2..50a1f54 100644
--- a/fs/reiser4/txnmgr.h
+++ b/fs/reiser4/txnmgr.h
@@ -437,6 +437,8 @@ extern void reiser4_uncapture_jnode(jnode *);
 extern int reiser4_capture_inode(struct inode *);
 extern int reiser4_uncapture_inode(struct inode *);
 
+extern int reiser4_create_atom(void);
+
 extern txn_atom *get_current_atom_locked_nocheck(void);
 
 #if REISER4_DEBUG
-- 
2.0.4

[RFC] [PATCHv3 5/9] reiser4: txnmgr: call reiser4_post_write_back_hook() also for empty atoms.

[Ivan Shapovalov]

The atoms generated by reiser4_trim_fs(), despite being empty (zero
capture_count), still have non-empty delete sets which have to be
processed.

Also the empty atom check has been moved after changing atom stage
to ASTAGE_PRE_COMMIT, because reiser4_post_write_back_hook() requires
atom to be unlocked.

Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
---
 fs/reiser4/txnmgr.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/reiser4/txnmgr.c b/fs/reiser4/txnmgr.c
index 2862940..2efdf85 100644
--- a/fs/reiser4/txnmgr.c
+++ b/fs/reiser4/txnmgr.c
@@ -1064,9 +1064,6 @@ static int commit_current_atom(long *nr_submitted, txn_atom ** atom)
 		return RETERR(-E_REPEAT);
 	}
 
-	if ((*atom)->capture_count == 0)
-		goto done;
-
 	/* Up to this point we have been flushing and after flush is called we
 	   return -E_REPEAT.  Now we can commit.  We cannot return -E_REPEAT
 	   at this point, commit should be successful. */
@@ -1074,6 +1071,14 @@ static int commit_current_atom(long *nr_submitted, txn_atom ** atom)
 	ON_DEBUG(((*atom)->committer = current));
 	spin_unlock_atom(*atom);
 
+	if ((*atom)->capture_count == 0) {
+		/* Process the atom's delete set.
+		 * Even if the atom has no captured nodes, the delete set may
+		 * still be non-empty (see e. g. reiser4_trim_fs()). */
+		reiser4_post_write_back_hook();
+		goto done;
+	}
+
 	ret = current_atom_complete_writes();
 	if (ret)
 		return ret;
@@ -1100,8 +1105,8 @@ static int commit_current_atom(long *nr_submitted, txn_atom ** atom)
 	reiser4_invalidate_list(ATOM_WB_LIST(*atom));
 	assert("zam-927", list_empty(&(*atom)->inodes));
 
+done:
 	spin_lock_atom(*atom);
- done:
 	reiser4_atom_set_stage(*atom, ASTAGE_DONE);
 	ON_DEBUG((*atom)->committer = NULL);
 
-- 
2.0.4

[RFC] [PATCHv3 6/9] reiser4: batch discard support: add a dummy FITRIM ioctl handler for directories.

[Ivan Shapovalov]

Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
---
 fs/reiser4/plugin/dir/dir.h  |  2 ++
 fs/reiser4/plugin/file_ops.c | 27 +++++++++++++++++++++++++++
 fs/reiser4/plugin/object.c   |  6 +++++-
 3 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/fs/reiser4/plugin/dir/dir.h b/fs/reiser4/plugin/dir/dir.h
index 4a91ebe..5eb7bda 100644
--- a/fs/reiser4/plugin/dir/dir.h
+++ b/fs/reiser4/plugin/dir/dir.h
@@ -10,6 +10,8 @@
 
 #include <linux/fs.h>*/
 
+long reiser4_ioctl_dir_common(struct file *file, unsigned int cmd, unsigned long arg);
+
 /* declarations of functions implementing HASHED_DIR_PLUGIN_ID dir plugin */
 
 /* "hashed" directory methods of dir plugin */
diff --git a/fs/reiser4/plugin/file_ops.c b/fs/reiser4/plugin/file_ops.c
index 466da64..65e2b02 100644
--- a/fs/reiser4/plugin/file_ops.c
+++ b/fs/reiser4/plugin/file_ops.c
@@ -107,6 +107,33 @@ int reiser4_sync_file_common(struct file *file, loff_t start, loff_t end, int da
 	return 0;
 }
 
+/** reiser4_ioctl_dir_common - ioctl of struct file_operations for typical directory
+ */
+long reiser4_ioctl_dir_common(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct inode *inode = file_inode(file);
+	struct super_block *super = inode->i_sb;
+	reiser4_context *ctx;
+	int ret;
+
+	ctx = reiser4_init_context(super);
+	if (IS_ERR(ctx))
+		return PTR_ERR(ctx);
+
+	switch (cmd) {
+	case FITRIM:
+		warning("intelfx-62", "FITRIM ioctl not implemented");
+		/* fall-through to -ENOSYS */
+
+	default:
+		ret = RETERR(-ENOSYS);
+		break;
+	}
+
+	reiser4_exit_context(ctx);
+	return ret;
+}
+
 /*
  * Local variables:
  * c-indentation-style: "K&R"
diff --git a/fs/reiser4/plugin/object.c b/fs/reiser4/plugin/object.c
index 553f1e2..ce09a8a 100644
--- a/fs/reiser4/plugin/object.c
+++ b/fs/reiser4/plugin/object.c
@@ -251,7 +251,11 @@ static struct file_operations directory_f_ops = {
 	.read = generic_read_dir,
 	.iterate = reiser4_iterate_common,
 	.release = reiser4_release_dir_common,
-	.fsync = reiser4_sync_common
+	.fsync = reiser4_sync_common,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = reiser4_ioctl_dir_common,
+#endif
+	.unlocked_ioctl = reiser4_ioctl_dir_common
 };
 static struct address_space_operations directory_a_ops = {
 	.writepage = writepage_bugop,
-- 
2.0.4

[RFC] [PATCHv3 7/9] reiser4: batch discard support: actually implement the FITRIM ioctl handler.

[Ivan Shapovalov]

It works in an iterative way, grabbing some fixed amount of space and allocating
blocks until grabbed space is exhausted or partition's end is reached.

Blocks are scheduled for discard by allocating them and immediately adding them
to the atom's delete set, which is read and processed at atom commit time.

After reaching the allocation stop condition, the atom is force-committed and,
if the allocation has been stopped due to grabbed space exhaustion (not due to
reaching end of partition), another iteration happens.

Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
---
 fs/reiser4/plugin/file_ops.c |  41 +++++++++++++-
 fs/reiser4/super_ops.c       | 127 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 165 insertions(+), 3 deletions(-)

diff --git a/fs/reiser4/plugin/file_ops.c b/fs/reiser4/plugin/file_ops.c
index 65e2b02..7686406 100644
--- a/fs/reiser4/plugin/file_ops.c
+++ b/fs/reiser4/plugin/file_ops.c
@@ -8,6 +8,9 @@
 #include "../inode.h"
 #include "object.h"
 
+#include <linux/fs.h>
+#include <linux/blkdev.h>
+
 /* file operations */
 
 /* implementation of vfs's llseek method of struct file_operations for
@@ -20,6 +23,9 @@ loff_t reiser4_llseek_dir_common(struct file *, loff_t, int origin);
 */
 int reiser4_iterate_common(struct file *, struct dir_context *);
 
+/* this function is implemented in super_ops.c */
+int reiser4_trim_fs(struct super_block *super, struct fstrim_range* range);
+
 /**
  * reiser4_release_dir_common - release of struct file_operations
  * @inode: inode of released file
@@ -121,9 +127,38 @@ long reiser4_ioctl_dir_common(struct file *file, unsigned int cmd, unsigned long
 		return PTR_ERR(ctx);
 
 	switch (cmd) {
-	case FITRIM:
-		warning("intelfx-62", "FITRIM ioctl not implemented");
-		/* fall-through to -ENOSYS */
+	case FITRIM: {
+		struct request_queue *q = bdev_get_queue(super->s_bdev);
+		struct fstrim_range range;
+
+		if (!capable(CAP_SYS_ADMIN)) {
+			ret = RETERR(-EPERM);
+			break;
+		}
+
+		if (!blk_queue_discard(q)) {
+			ret = RETERR(-EOPNOTSUPP);
+			break;
+		}
+
+		if (copy_from_user(&range, (struct fstrim_range __user *)arg,
+		    sizeof(range))) {
+			ret = RETERR(-EFAULT);
+			break;
+		}
+
+		range.minlen = max((unsigned int)range.minlen,
+				   q->limits.discard_granularity);
+
+		ret = reiser4_trim_fs(super, &range);
+
+		if (copy_to_user((struct fstrim_range __user *)arg, &range,
+		    sizeof(range))) {
+			ret = RETERR(-EFAULT);
+		}
+
+		break;
+	}
 
 	default:
 		ret = RETERR(-ENOSYS);
diff --git a/fs/reiser4/super_ops.c b/fs/reiser4/super_ops.c
index bcd7fd6..d232f30 100644
--- a/fs/reiser4/super_ops.c
+++ b/fs/reiser4/super_ops.c
@@ -471,6 +471,133 @@ static int reiser4_show_options(struct seq_file *m, struct dentry *dentry)
 	return 0;
 }
 
+/**
+ * reiser4_trim_fs - discards all free space in a filesystem
+ * @super: the superblock of filesystem to discard
+ * @range: parameters for discarding
+ *
+ * Called from @reiser4_ioctl_dir_common().
+ */
+int reiser4_trim_fs(struct super_block *super, struct fstrim_range* range)
+{
+	reiser4_blocknr_hint hint;
+	reiser4_block_nr start, end, len, minlen, discarded_count = 0;
+	reiser4_context *ctx;
+	txn_atom *atom;
+	int ret, finished = 0;
+
+	reiser4_blocknr_hint_init(&hint);
+	ctx = get_current_context();
+
+	/*
+	 * Configure the hint for block allocator.
+	 * We will allocate in forward direction only.
+	 */
+	hint.blk = range->start >> super->s_blocksize_bits;
+	hint.max_dist = range->len >> super->s_blocksize_bits;
+	hint.block_stage = BLOCK_GRABBED;
+	hint.forward = 1;
+
+	end = hint.blk + hint.max_dist;
+	minlen = range->minlen >> super->s_blocksize_bits;
+
+	/*
+	 * We will perform the process in iterations in order not to starve
+	 * the rest of the system of disk space.
+	 * Each iteration we will grab some space (using the BA_SOME_SPACE
+	 * flag, which currently grabs 25% of free disk space), create an empty
+	 * atom and sequentially allocate extents of disk space while we can
+	 * afford it (i. e. while we haven't reached the end of partition AND
+	 * while we haven't exhausted the grabbed space).
+	 */
+	do {
+		/*
+		 * Grab some sane amount of space.
+		 * We will allocate blocks until end of the partition or until
+		 * the grabbed space is exhausted.
+		 */
+		ret = reiser4_grab_reserved(super, 0, BA_CAN_COMMIT | BA_SOME_SPACE);
+		if (ret != 0)
+			goto out;
+		if (ctx->grabbed_blocks == 0)
+			goto out;
+
+		/*
+		 * We will not capture anything, so we need an empty atom.
+		 */
+		ret = reiser4_create_atom();
+		if (ret != 0)
+			goto out;
+
+		while (ctx->grabbed_blocks != 0) {
+			/*
+			 * Allocate no more than is grabbed.
+			 * FIXME: use minlen.
+			 */
+			len = ctx->grabbed_blocks;
+			ret = reiser4_alloc_blocks(&hint, &start, &len, 0 /* flags */);
+			if (ret == -ENOSPC)
+				break;
+			if (ret != 0)
+				goto out;
+
+			/*
+			 * Update the hint in order for the next scan to start
+			 * right after the newly allocated extent.
+			 */
+			hint.blk = start + len;
+			hint.max_dist = end - hint.blk;
+
+			/*
+			 * Mark the newly allocated extent for deallocation.
+			 * Discard happens on deallocation.
+			 */
+			ret = reiser4_dealloc_blocks(&start, &len, BLOCK_NOT_COUNTED, BA_DEFER);
+			if (ret != 0)
+				goto out;
+
+			/*
+			 * FIXME: get the actual discarded block count,
+			 * accounting for speculative discard of extent heads and tails.
+			 *
+			 * PRIORITY: LOW because we have already allocated all
+			 * possible space, so allocations of heads/tails will
+			 * fail unless there is a concurrent process reclaiming
+			 * space.
+			 */
+			discarded_count += len;
+		}
+
+		assert("intelfx-64", (ret == 0) || (ret == -ENOSPC));
+
+		if (ret == -ENOSPC) {
+			ret = 0;
+			finished = 1;
+		}
+
+		/*
+		 * Extents marked for deallocation are discarded here, as part
+		 * of committing current atom.
+		 */
+		atom = get_current_atom_locked();
+		spin_lock_txnh(ctx->trans);
+		force_commit_atom(ctx->trans);
+		all_grabbed2free();
+		grab_space_enable();
+	} while (!finished);
+
+out:
+	reiser4_release_reserved(super);
+	reiser4_blocknr_hint_done(&hint);
+
+	/*
+	 * Update the statistics.
+	 */
+	range->len = discarded_count << super->s_blocksize_bits;
+
+	return ret;
+}
+
 struct super_operations reiser4_super_operations = {
 	.alloc_inode = reiser4_alloc_inode,
 	.destroy_inode = reiser4_destroy_inode,
-- 
2.0.4

[RFC] [PATCHv3 8/9] reiser4: block_alloc: add a "min_len" parameter to reiser4_blocknr_hint to limit allocated extent length from below.

[Ivan Shapovalov]

The default has been 1, that is, the first encountered free extent with any length
from 1 to requested is allocated and returned.

So, allow changing minimal extent length to allocate.

Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
---
 fs/reiser4/block_alloc.h         |  2 ++
 fs/reiser4/plugin/space/bitmap.c | 18 ++++++++++++++----
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/fs/reiser4/block_alloc.h b/fs/reiser4/block_alloc.h
index 08b3941..16f9810 100644
--- a/fs/reiser4/block_alloc.h
+++ b/fs/reiser4/block_alloc.h
@@ -45,6 +45,8 @@ struct reiser4_blocknr_hint {
 	reiser4_block_nr blk;
 	/* if not zero, it is a region size we search for free blocks in */
 	reiser4_block_nr max_dist;
+	/* if not zero, minimal length of an extent to allocate */
+	reiser4_block_nr min_len;
 	/* level for allocation, may be useful have branch-level and higher
 	   write-optimized. */
 	tree_level level;
diff --git a/fs/reiser4/plugin/space/bitmap.c b/fs/reiser4/plugin/space/bitmap.c
index 9beaf66..8309cf9 100644
--- a/fs/reiser4/plugin/space/bitmap.c
+++ b/fs/reiser4/plugin/space/bitmap.c
@@ -1101,7 +1101,7 @@ static int alloc_blocks_forward(reiser4_blocknr_hint *hint, int needed,
 				reiser4_block_nr *start, reiser4_block_nr *len)
 {
 	struct super_block *super = get_current_context()->super;
-	int actual_len;
+	int min_len, actual_len;
 
 	reiser4_block_nr search_start;
 	reiser4_block_nr search_end;
@@ -1117,12 +1117,17 @@ static int alloc_blocks_forward(reiser4_blocknr_hint *hint, int needed,
 		    LIMIT(hint->blk + hint->max_dist,
 			  reiser4_block_count(super));
 
+	if (hint->min_len == 0)
+		min_len = 1;
+	else
+		min_len = (int)hint->min_len;
+
 	/* We use @hint -> blk as a search start and search from it to the end
 	   of the disk or in given region if @hint -> max_dist is not zero */
 	search_start = hint->blk;
 
 	actual_len =
-	    bitmap_alloc_forward(&search_start, &search_end, 1, needed);
+	    bitmap_alloc_forward(&search_start, &search_end, min_len, needed);
 
 	/* There is only one bitmap search if max_dist was specified or first
 	   pass was from the beginning of the bitmap. We also do one pass for
@@ -1150,7 +1155,7 @@ static int alloc_blocks_backward(reiser4_blocknr_hint * hint, int needed,
 {
 	reiser4_block_nr search_start;
 	reiser4_block_nr search_end;
-	int actual_len;
+	int min_len, actual_len;
 
 	ON_DEBUG(struct super_block *super = reiser4_get_current_sb());
 
@@ -1164,8 +1169,13 @@ static int alloc_blocks_backward(reiser4_blocknr_hint * hint, int needed,
 	else
 		search_end = search_start - hint->max_dist;
 
+	if (hint->min_len == 0)
+		min_len = 1;
+	else
+		min_len = (int)hint->min_len;
+
 	actual_len =
-	    bitmap_alloc_backward(&search_start, &search_end, 1, needed);
+	    bitmap_alloc_backward(&search_start, &search_end, min_len, needed);
 	if (actual_len == 0)
 		return RETERR(-ENOSPC);
 	if (actual_len < 0)
-- 
2.0.4

[RFC] [PATCHv3 9/9] reiser4: batch discard support: honor minimal extent length passed from the userspace.

[Ivan Shapovalov]

---
 fs/reiser4/super_ops.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/reiser4/super_ops.c b/fs/reiser4/super_ops.c
index d232f30..f4cf0ff 100644
--- a/fs/reiser4/super_ops.c
+++ b/fs/reiser4/super_ops.c
@@ -481,7 +481,7 @@ static int reiser4_show_options(struct seq_file *m, struct dentry *dentry)
 int reiser4_trim_fs(struct super_block *super, struct fstrim_range* range)
 {
 	reiser4_blocknr_hint hint;
-	reiser4_block_nr start, end, len, minlen, discarded_count = 0;
+	reiser4_block_nr start, end, len, discarded_count = 0;
 	reiser4_context *ctx;
 	txn_atom *atom;
 	int ret, finished = 0;
@@ -495,11 +495,11 @@ int reiser4_trim_fs(struct super_block *super, struct fstrim_range* range)
 	 */
 	hint.blk = range->start >> super->s_blocksize_bits;
 	hint.max_dist = range->len >> super->s_blocksize_bits;
+	hint.min_len = range->minlen >> super->s_blocksize_bits;
 	hint.block_stage = BLOCK_GRABBED;
 	hint.forward = 1;
 
 	end = hint.blk + hint.max_dist;
-	minlen = range->minlen >> super->s_blocksize_bits;
 
 	/*
 	 * We will perform the process in iterations in order not to starve
@@ -532,7 +532,6 @@ int reiser4_trim_fs(struct super_block *super, struct fstrim_range* range)
 		while (ctx->grabbed_blocks != 0) {
 			/*
 			 * Allocate no more than is grabbed.
-			 * FIXME: use minlen.
 			 */
 			len = ctx->grabbed_blocks;
 			ret = reiser4_alloc_blocks(&hint, &start, &len, 0 /* flags */);
-- 
2.0.4

Re: [RFC] [PATCHv3 0/9] reiser4: batch discard support (FITRIM ioctl): initial implementation.

[Ivan Shapovalov]

[-- Attachment #1: Type: text/plain, Size: 6175 bytes --]

On Monday 18 August 2014 at 01:52:50, Ivan Shapovalov wrote:	
> [...]
> 
> Ivan Shapovalov (9):
>   reiser4: block_alloc: add BA_SOME_SPACE flag for grabbing a fixed amount of space.
>   reiser4: block_alloc: add a "forward" parameter to reiser4_blocknr_hint to allocate blocks only in forward direction.
>   reiser4: txnmgr: free allocated but unneeded atom in atom_begin_and_assign_to_txnh().
>   reiser4: txnmgr: add reiser4_create_atom() which creates an empty atom without capturing any nodes.
>   reiser4: txnmgr: call reiser4_post_write_back_hook() for empty atoms.
>   reiser4: batch discard support: add a dummy FITRIM ioctl handler for directories.
>   reiser4: batch discard support: actually implement the FITRIM ioctl handler.
>   reiser4: block_alloc: add a "min_len" parameter to reiser4_blocknr_hint to limit allocated extent length from below.
>   reiser4: batch discard support: honor minimal extent length passed from the userspace.
> 
>  [...]

This is a diff between v2 and v3, for ease of reviewing.

diff --git a/fs/reiser4/block_alloc.c b/fs/reiser4/block_alloc.c
index 8797b43..9eed7fc 100644
--- a/fs/reiser4/block_alloc.c
+++ b/fs/reiser4/block_alloc.c
@@ -278,8 +278,7 @@ reiser4_grab(reiser4_context * ctx, __u64 count, reiser4_ba_flags_t flags)
 
 	if (flags & BA_SOME_SPACE) {
 		/* Reserve 25% of all free space. */
-		count = free_blocks;
-		do_div(count, 4);
+		count = free_blocks >> 2;
 	}
 
 	if ((use_reserved && free_blocks < count) ||
diff --git a/fs/reiser4/block_alloc.h b/fs/reiser4/block_alloc.h
index 08b3941..16f9810 100644
--- a/fs/reiser4/block_alloc.h
+++ b/fs/reiser4/block_alloc.h
@@ -45,6 +45,8 @@ struct reiser4_blocknr_hint {
 	reiser4_block_nr blk;
 	/* if not zero, it is a region size we search for free blocks in */
 	reiser4_block_nr max_dist;
+	/* if not zero, minimal length of an extent to allocate */
+	reiser4_block_nr min_len;
 	/* level for allocation, may be useful have branch-level and higher
 	   write-optimized. */
 	tree_level level;
diff --git a/fs/reiser4/plugin/space/bitmap.c b/fs/reiser4/plugin/space/bitmap.c
index 9beaf66..8309cf9 100644
--- a/fs/reiser4/plugin/space/bitmap.c
+++ b/fs/reiser4/plugin/space/bitmap.c
@@ -1101,7 +1101,7 @@ static int alloc_blocks_forward(reiser4_blocknr_hint *hint, int needed,
 				reiser4_block_nr *start, reiser4_block_nr *len)
 {
 	struct super_block *super = get_current_context()->super;
-	int actual_len;
+	int min_len, actual_len;
 
 	reiser4_block_nr search_start;
 	reiser4_block_nr search_end;
@@ -1117,12 +1117,17 @@ static int alloc_blocks_forward(reiser4_blocknr_hint *hint, int needed,
 		    LIMIT(hint->blk + hint->max_dist,
 			  reiser4_block_count(super));
 
+	if (hint->min_len == 0)
+		min_len = 1;
+	else
+		min_len = (int)hint->min_len;
+
 	/* We use @hint -> blk as a search start and search from it to the end
 	   of the disk or in given region if @hint -> max_dist is not zero */
 	search_start = hint->blk;
 
 	actual_len =
-	    bitmap_alloc_forward(&search_start, &search_end, 1, needed);
+	    bitmap_alloc_forward(&search_start, &search_end, min_len, needed);
 
 	/* There is only one bitmap search if max_dist was specified or first
 	   pass was from the beginning of the bitmap. We also do one pass for
@@ -1150,7 +1155,7 @@ static int alloc_blocks_backward(reiser4_blocknr_hint * hint, int needed,
 {
 	reiser4_block_nr search_start;
 	reiser4_block_nr search_end;
-	int actual_len;
+	int min_len, actual_len;
 
 	ON_DEBUG(struct super_block *super = reiser4_get_current_sb());
 
@@ -1164,8 +1169,13 @@ static int alloc_blocks_backward(reiser4_blocknr_hint * hint, int needed,
 	else
 		search_end = search_start - hint->max_dist;
 
+	if (hint->min_len == 0)
+		min_len = 1;
+	else
+		min_len = (int)hint->min_len;
+
 	actual_len =
-	    bitmap_alloc_backward(&search_start, &search_end, 1, needed);
+	    bitmap_alloc_backward(&search_start, &search_end, min_len, needed);
 	if (actual_len == 0)
 		return RETERR(-ENOSPC);
 	if (actual_len < 0)
diff --git a/fs/reiser4/super_ops.c b/fs/reiser4/super_ops.c
index 0048e83..f4cf0ff 100644
--- a/fs/reiser4/super_ops.c
+++ b/fs/reiser4/super_ops.c
@@ -14,7 +14,6 @@
 #include <linux/debugfs.h>
 #include <linux/backing-dev.h>
 #include <linux/module.h>
-#include <linux/delay.h>
 
 /* slab cache for inodes */
 static struct kmem_cache *inode_cache;
@@ -482,28 +481,25 @@ static int reiser4_show_options(struct seq_file *m, struct dentry *dentry)
 int reiser4_trim_fs(struct super_block *super, struct fstrim_range* range)
 {
 	reiser4_blocknr_hint hint;
-	reiser4_block_nr start, end, len, minlen, discarded_count = 0;
+	reiser4_block_nr start, end, len, discarded_count = 0;
 	reiser4_context *ctx;
-	struct super_block *sb;
 	txn_atom *atom;
 	int ret, finished = 0;
 
 	reiser4_blocknr_hint_init(&hint);
 	ctx = get_current_context();
-	sb = ctx->super;
 
 	/*
 	 * Configure the hint for block allocator.
 	 * We will allocate in forward direction only.
 	 */
-
-	hint.blk = range->start >> sb->s_blocksize_bits;
-	hint.max_dist = range->len >> sb->s_blocksize_bits;
+	hint.blk = range->start >> super->s_blocksize_bits;
+	hint.max_dist = range->len >> super->s_blocksize_bits;
+	hint.min_len = range->minlen >> super->s_blocksize_bits;
 	hint.block_stage = BLOCK_GRABBED;
 	hint.forward = 1;
 
 	end = hint.blk + hint.max_dist;
-	minlen = range->minlen >> sb->s_blocksize_bits;
 
 	/*
 	 * We will perform the process in iterations in order not to starve
@@ -536,7 +532,6 @@ int reiser4_trim_fs(struct super_block *super, struct fstrim_range* range)
 		while (ctx->grabbed_blocks != 0) {
 			/*
 			 * Allocate no more than is grabbed.
-			 * FIXME: use minlen.
 			 */
 			len = ctx->grabbed_blocks;
 			ret = reiser4_alloc_blocks(&hint, &start, &len, 0 /* flags */);
@@ -597,7 +592,7 @@ out:
 	/*
 	 * Update the statistics.
 	 */
-	range->len = discarded_count << sb->s_blocksize_bits;
+	range->len = discarded_count << super->s_blocksize_bits;
 
 	return ret;
 }

Thanks,
-- 
Ivan Shapovalov / intelfx /

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [RFC] [PATCHv3 1/9] reiser4: block_alloc: add BA_SOME_SPACE flag for grabbing a fixed amount of space.

[Edward Shishkin]

On 08/17/2014 11:52 PM, Ivan Shapovalov wrote:
> This is used for FITRIM ioctl which will iteratively grab, allocate and trim disk space
> bit by bit to avoid starving the rest of system.
>
> Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
> ---
>   fs/reiser4/block_alloc.c | 5 +++++
>   fs/reiser4/block_alloc.h | 5 ++++-
>   2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/fs/reiser4/block_alloc.c b/fs/reiser4/block_alloc.c
> index d3d730c..9eed7fc 100644
> --- a/fs/reiser4/block_alloc.c
> +++ b/fs/reiser4/block_alloc.c
> @@ -276,6 +276,11 @@ reiser4_grab(reiser4_context * ctx, __u64 count, reiser4_ba_flags_t flags)
>   
>   	free_blocks = sbinfo->blocks_free;
>   
> +	if (flags & BA_SOME_SPACE) {
> +		/* Reserve 25% of all free space. */

if (free_blocks == 0) {
         /* nothing to trim */
         ret= RETERR(-ENOSPC);
         goto unlock_and_ret;
}

> +		count = free_blocks >> 2;

if (count == 0)
         /* there are 1, 2, or 3 free blocks  */
         count = free_blocks;

> +	}
> +
>   	if ((use_reserved && free_blocks < count) ||
>   	    (!use_reserved && free_blocks < count + sbinfo->blocks_reserved)) {
>   		ret = RETERR(-ENOSPC);
> diff --git a/fs/reiser4/block_alloc.h b/fs/reiser4/block_alloc.h
> index a4e98af..bfc6be9 100644
> --- a/fs/reiser4/block_alloc.h
> +++ b/fs/reiser4/block_alloc.h
> @@ -79,7 +79,10 @@ enum reiser4_ba_flags {
>   	BA_FORCE = (1 << 5),
>   
>   	/* use default start value for free blocks search. */
> -	BA_USE_DEFAULT_SEARCH_START = (1 << 6)
> +	BA_USE_DEFAULT_SEARCH_START = (1 << 6),
> +
> +	/* reserve some fixed amount of space */
> +	BA_SOME_SPACE = (1 << 7),
>   };
>   
>   typedef enum reiser4_ba_flags reiser4_ba_flags_t;

Re: [RFC] [PATCHv3 1/9] reiser4: block_alloc: add BA_SOME_SPACE flag for grabbing a fixed amount of space.

[Ivan Shapovalov]

[-- Attachment #1: Type: text/plain, Size: 1390 bytes --]

On Monday 20 October 2014 at 00:04:39, Edward Shishkin wrote:	
> 
> On 08/17/2014 11:52 PM, Ivan Shapovalov wrote:
> > This is used for FITRIM ioctl which will iteratively grab, allocate and trim disk space
> > bit by bit to avoid starving the rest of system.
> >
> > Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
> > ---
> >   fs/reiser4/block_alloc.c | 5 +++++
> >   fs/reiser4/block_alloc.h | 5 ++++-
> >   2 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/reiser4/block_alloc.c b/fs/reiser4/block_alloc.c
> > index d3d730c..9eed7fc 100644
> > --- a/fs/reiser4/block_alloc.c
> > +++ b/fs/reiser4/block_alloc.c
> > @@ -276,6 +276,11 @@ reiser4_grab(reiser4_context * ctx, __u64 count, reiser4_ba_flags_t flags)
> >   
> >   	free_blocks = sbinfo->blocks_free;
> >   
> > +	if (flags & BA_SOME_SPACE) {
> > +		/* Reserve 25% of all free space. */
> 
> if (free_blocks == 0) {
>          /* nothing to trim */
>          ret= RETERR(-ENOSPC);
>          goto unlock_and_ret;
> }

Makes sense, but IMO without returning error. TRIM'ing a completely full
filesystem is a no-op (== all blocks busy, nothing to trim), not a failure.

> 
> > +		count = free_blocks >> 2;
> 
> if (count == 0)
>          /* there are 1, 2, or 3 free blocks  */
>          count = free_blocks;

Done.

Thanks,
-- 
Ivan Shapovalov / intelfx /

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [RFC] [PATCHv3 7/9] reiser4: batch discard support: actually implement the FITRIM ioctl handler.

[Edward Shishkin]

On 08/17/2014 11:52 PM, Ivan Shapovalov wrote:
> It works in an iterative way, grabbing some fixed amount of space and allocating
> blocks until grabbed space is exhausted or partition's end is reached.
>
> Blocks are scheduled for discard by allocating them and immediately adding them
> to the atom's delete set, which is read and processed at atom commit time.
>
> After reaching the allocation stop condition, the atom is force-committed and,
> if the allocation has been stopped due to grabbed space exhaustion (not due to
> reaching end of partition), another iteration happens.
>
> Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
> ---
>   fs/reiser4/plugin/file_ops.c |  41 +++++++++++++-
>   fs/reiser4/super_ops.c       | 127 +++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 165 insertions(+), 3 deletions(-)
>
> diff --git a/fs/reiser4/plugin/file_ops.c b/fs/reiser4/plugin/file_ops.c
> index 65e2b02..7686406 100644
> --- a/fs/reiser4/plugin/file_ops.c
> +++ b/fs/reiser4/plugin/file_ops.c
> @@ -8,6 +8,9 @@
>   #include "../inode.h"
>   #include "object.h"
>   
> +#include <linux/fs.h>
> +#include <linux/blkdev.h>
> +
>   /* file operations */
>   
>   /* implementation of vfs's llseek method of struct file_operations for
> @@ -20,6 +23,9 @@ loff_t reiser4_llseek_dir_common(struct file *, loff_t, int origin);
>   */
>   int reiser4_iterate_common(struct file *, struct dir_context *);
>   
> +/* this function is implemented in super_ops.c */
> +int reiser4_trim_fs(struct super_block *super, struct fstrim_range* range);
> +
>   /**
>    * reiser4_release_dir_common - release of struct file_operations
>    * @inode: inode of released file
> @@ -121,9 +127,38 @@ long reiser4_ioctl_dir_common(struct file *file, unsigned int cmd, unsigned long
>   		return PTR_ERR(ctx);
>   
>   	switch (cmd) {
> -	case FITRIM:
> -		warning("intelfx-62", "FITRIM ioctl not implemented");
> -		/* fall-through to -ENOSYS */
> +	case FITRIM: {
> +		struct request_queue *q = bdev_get_queue(super->s_bdev);
> +		struct fstrim_range range;
> +
> +		if (!capable(CAP_SYS_ADMIN)) {
> +			ret = RETERR(-EPERM);
> +			break;
> +		}
> +
> +		if (!blk_queue_discard(q)) {
> +			ret = RETERR(-EOPNOTSUPP);
> +			break;
> +		}
> +
> +		if (copy_from_user(&range, (struct fstrim_range __user *)arg,
> +		    sizeof(range))) {
> +			ret = RETERR(-EFAULT);
> +			break;
> +		}
> +
> +		range.minlen = max((unsigned int)range.minlen,
> +				   q->limits.discard_granularity);
> +
> +		ret = reiser4_trim_fs(super, &range);
> +
> +		if (copy_to_user((struct fstrim_range __user *)arg, &range,
> +		    sizeof(range))) {
> +			ret = RETERR(-EFAULT);
> +		}
> +
> +		break;
> +	}
>   
>   	default:
>   		ret = RETERR(-ENOSYS);
> diff --git a/fs/reiser4/super_ops.c b/fs/reiser4/super_ops.c
> index bcd7fd6..d232f30 100644
> --- a/fs/reiser4/super_ops.c
> +++ b/fs/reiser4/super_ops.c
> @@ -471,6 +471,133 @@ static int reiser4_show_options(struct seq_file *m, struct dentry *dentry)
>   	return 0;
>   }
>   
> +/**
> + * reiser4_trim_fs - discards all free space in a filesystem
> + * @super: the superblock of filesystem to discard
> + * @range: parameters for discarding
> + *
> + * Called from @reiser4_ioctl_dir_common().
> + */
> +int reiser4_trim_fs(struct super_block *super, struct fstrim_range* range)
> +{
> +	reiser4_blocknr_hint hint;
> +	reiser4_block_nr start, end, len, minlen, discarded_count = 0;
> +	reiser4_context *ctx;
> +	txn_atom *atom;
> +	int ret, finished = 0;
> +
> +	reiser4_blocknr_hint_init(&hint);
> +	ctx = get_current_context();
> +
> +	/*
> +	 * Configure the hint for block allocator.
> +	 * We will allocate in forward direction only.
> +	 */
> +	hint.blk = range->start >> super->s_blocksize_bits;
> +	hint.max_dist = range->len >> super->s_blocksize_bits;
> +	hint.block_stage = BLOCK_GRABBED;
> +	hint.forward = 1;
> +
> +	end = hint.blk + hint.max_dist;
> +	minlen = range->minlen >> super->s_blocksize_bits;
> +
> +	/*
> +	 * We will perform the process in iterations in order not to starve
> +	 * the rest of the system of disk space.
> +	 * Each iteration we will grab some space (using the BA_SOME_SPACE
> +	 * flag, which currently grabs 25% of free disk space), create an empty
> +	 * atom and sequentially allocate extents of disk space while we can
> +	 * afford it (i. e. while we haven't reached the end of partition AND
> +	 * while we haven't exhausted the grabbed space).
> +	 */
> +	do {
> +		/*
> +		 * Grab some sane amount of space.
> +		 * We will allocate blocks until end of the partition or until
> +		 * the grabbed space is exhausted.
> +		 */
> +		ret = reiser4_grab_reserved(super, 0, BA_CAN_COMMIT | BA_SOME_SPACE);


reiser4_grab_reserved() grabs space from the reserved area (5%).
This is needed to make sure that unlink(), truncate(), etc. won't
fail, if there is no free space on disk. I don't think that FITRIM
ioctl needs this reserved area.

If BA_SOME_SPACE is set, then you lock a superblock, and grab 25% of
the current free space. It means that your grab will always succeed,
so that BA_CAN_COMMIT flag is also not needed.


> +		if (ret != 0)
> +			goto out;
> +		if (ctx->grabbed_blocks == 0)
> +			goto out;
> +
> +		/*
> +		 * We will not capture anything, so we need an empty atom.
> +		 */
> +		ret = reiser4_create_atom();
> +		if (ret != 0)
> +			goto out;
> +
> +		while (ctx->grabbed_blocks != 0) {
> +			/*
> +			 * Allocate no more than is grabbed.
> +			 * FIXME: use minlen.
> +			 */
> +			len = ctx->grabbed_blocks;
> +			ret = reiser4_alloc_blocks(&hint, &start, &len, 0 /* flags */);
> +			if (ret == -ENOSPC)
> +				break;
> +			if (ret != 0)
> +				goto out;
> +
> +			/*
> +			 * Update the hint in order for the next scan to start
> +			 * right after the newly allocated extent.
> +			 */
> +			hint.blk = start + len;
> +			hint.max_dist = end - hint.blk;
> +
> +			/*
> +			 * Mark the newly allocated extent for deallocation.
> +			 * Discard happens on deallocation.
> +			 */
> +			ret = reiser4_dealloc_blocks(&start, &len, BLOCK_NOT_COUNTED, BA_DEFER);
> +			if (ret != 0)
> +				goto out;
> +
> +			/*
> +			 * FIXME: get the actual discarded block count,
> +			 * accounting for speculative discard of extent heads and tails.
> +			 *
> +			 * PRIORITY: LOW because we have already allocated all
> +			 * possible space, so allocations of heads/tails will
> +			 * fail unless there is a concurrent process reclaiming
> +			 * space.
> +			 */
> +			discarded_count += len;
> +		}
> +
> +		assert("intelfx-64", (ret == 0) || (ret == -ENOSPC));
> +
> +		if (ret == -ENOSPC) {
> +			ret = 0;
> +			finished = 1;
> +		}
> +
> +		/*
> +		 * Extents marked for deallocation are discarded here, as part
> +		 * of committing current atom.
> +		 */
> +		atom = get_current_atom_locked();
> +		spin_lock_txnh(ctx->trans);
> +		force_commit_atom(ctx->trans);
> +		all_grabbed2free();
> +		grab_space_enable();
> +	} while (!finished);
> +
> +out:
> +	reiser4_release_reserved(super);
> +	reiser4_blocknr_hint_done(&hint);
> +
> +	/*
> +	 * Update the statistics.
> +	 */
> +	range->len = discarded_count << super->s_blocksize_bits;
> +
> +	return ret;
> +}
> +
>   struct super_operations reiser4_super_operations = {
>   	.alloc_inode = reiser4_alloc_inode,
>   	.destroy_inode = reiser4_destroy_inode,

Re: [RFC] [PATCHv3 7/9] reiser4: batch discard support: actually implement the FITRIM ioctl handler.

[Ivan Shapovalov]

[-- Attachment #1: Type: text/plain, Size: 1609 bytes --]

On Monday 20 October 2014 at 12:54:13, Edward Shishkin wrote:	
> On 08/17/2014 11:52 PM, Ivan Shapovalov wrote:
> > [...]
> > +		/*
> > +		 * Grab some sane amount of space.
> > +		 * We will allocate blocks until end of the partition or until
> > +		 * the grabbed space is exhausted.
> > +		 */
> > +		ret = reiser4_grab_reserved(super, 0, BA_CAN_COMMIT | BA_SOME_SPACE);
> 
> 
> reiser4_grab_reserved() grabs space from the reserved area (5%).
> This is needed to make sure that unlink(), truncate(), etc. won't
> fail, if there is no free space on disk. I don't think that FITRIM
> ioctl needs this reserved area.

Well, IIUC, it doesn't hurt: any "legitimate" user of the reserved space
will block on the mutex and eventually proceed. At the same time, given
a filesystem with (5% + eps) free space left, not using the reserved space
will result in trimming of (eps) blocks at a time.

Anyway, I should also fix calculation of space to grab in 1/9: if BA_RESERVED
is not set, then the amount to grab should be calculated as 25% of non-reserved
free space, not as 25% of all free space...

> If BA_SOME_SPACE is set, then you lock a superblock, and grab 25% of
> the current free space. It means that your grab will always succeed,
> so that BA_CAN_COMMIT flag is also not needed.

If we decide on dropping BA_RESERVED, then you're right here;
reiser4_grab(...BA_SOME_SPACE...) won't return ENOSPC and so this is pointless.

If we decide on keeping BA_RESERVED, then this should also stay in order not to
trigger the assertion.

Thanks,
-- 
Ivan Shapovalov / intelfx /

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [RFC] [PATCHv3 7/9] reiser4: batch discard support: actually implement the FITRIM ioctl handler.

[Edward Shishkin]

On 10/21/2014 12:39 AM, Ivan Shapovalov wrote:
> On Monday 20 October 2014 at 12:54:13, Edward Shishkin wrote:	
>> On 08/17/2014 11:52 PM, Ivan Shapovalov wrote:
>>> [...]
>>> +		/*
>>> +		 * Grab some sane amount of space.
>>> +		 * We will allocate blocks until end of the partition or until
>>> +		 * the grabbed space is exhausted.
>>> +		 */
>>> +		ret = reiser4_grab_reserved(super, 0, BA_CAN_COMMIT | BA_SOME_SPACE);
>>
>> reiser4_grab_reserved() grabs space from the reserved area (5%).
>> This is needed to make sure that unlink(), truncate(), etc. won't
>> fail, if there is no free space on disk. I don't think that FITRIM
>> ioctl needs this reserved area.
> Well, IIUC, it doesn't hurt:


"doesn't hurt" is not enough. If you want to take a resource,
you should be going to explain, why do you need this.


>   any "legitimate" user of the reserved space
> will block on the mutex and eventually proceed. At the same time, given
> a filesystem with (5% + eps) free space left, not using the reserved space
> will result in trimming of (eps) blocks at a time.
>
> Anyway, I should also fix calculation of space to grab in 1/9: if BA_RESERVED
> is not set, then the amount to grab should be calculated as 25% of non-reserved
> free space, not as 25% of all free space...
>
>> If BA_SOME_SPACE is set, then you lock a superblock, and grab 25% of
>> the current free space. It means that your grab will always succeed,
>> so that BA_CAN_COMMIT flag is also not needed.
> If we decide on dropping BA_RESERVED, then you're right here;
> reiser4_grab(...BA_SOME_SPACE...) won't return ENOSPC and so this is pointless.
>
> If we decide on keeping BA_RESERVED, then this should also stay in order not to
> trigger the assertion.
>
> Thanks,

Re: [RFC] [PATCHv3 1/9] reiser4: block_alloc: add BA_SOME_SPACE flag for grabbing a fixed amount of space.

[Edward Shishkin]

On 10/20/2014 12:36 PM, Ivan Shapovalov wrote:
> On Monday 20 October 2014 at 00:04:39, Edward Shishkin wrote:	
>> On 08/17/2014 11:52 PM, Ivan Shapovalov wrote:
>>> This is used for FITRIM ioctl which will iteratively grab, allocate and trim disk space
>>> bit by bit to avoid starving the rest of system.
>>>
>>> Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
>>> ---
>>>    fs/reiser4/block_alloc.c | 5 +++++
>>>    fs/reiser4/block_alloc.h | 5 ++++-
>>>    2 files changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/reiser4/block_alloc.c b/fs/reiser4/block_alloc.c
>>> index d3d730c..9eed7fc 100644
>>> --- a/fs/reiser4/block_alloc.c
>>> +++ b/fs/reiser4/block_alloc.c
>>> @@ -276,6 +276,11 @@ reiser4_grab(reiser4_context * ctx, __u64 count, reiser4_ba_flags_t flags)
>>>    
>>>    	free_blocks = sbinfo->blocks_free;
>>>    
>>> +	if (flags & BA_SOME_SPACE) {
>>> +		/* Reserve 25% of all free space. */
>> if (free_blocks == 0) {
>>           /* nothing to trim */
>>           ret= RETERR(-ENOSPC);
>>           goto unlock_and_ret;
>> }
> Makes sense, but IMO without returning error. TRIM'ing a completely full
> filesystem is a no-op (== all blocks busy, nothing to trim), not a failure.


(sbinfo->blocks_free) is a rather bad estimation of free space amount,
but we have nothing better. I suggest to return -ENOSPC here, and use
reiser4_grab_space() with BA_CAN_COMMIT to make sure that FITRIM
ioctl gets the "second soft ENOSPC".

Thanks,
Edward.

Re: [RFC] [PATCHv3 2/9] reiser4: block_alloc: add a "forward" parameter to reiser4_blocknr_hint to allocate blocks only in forward direction.

[Edward Shishkin]

On 08/17/2014 11:52 PM, Ivan Shapovalov wrote:
> Signed-off-by: Ivan Shapovalov<intelfx100@gmail.com>
> ---
>   fs/reiser4/block_alloc.h         | 5 +++--
>   fs/reiser4/plugin/space/bitmap.c | 3 ++-
>   2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/fs/reiser4/block_alloc.h b/fs/reiser4/block_alloc.h
> index bfc6be9..08b3941 100644
> --- a/fs/reiser4/block_alloc.h
> +++ b/fs/reiser4/block_alloc.h
> @@ -51,9 +51,10 @@ struct reiser4_blocknr_hint {
>   	/* block allocator assumes that blocks, which will be mapped to disk,
>   	   are in this specified block_stage */
>   	block_stage_t block_stage;
> -	/* If direction = 1 allocate blocks in backward direction from the end
> -	 * of disk to the beginning of disk.  */
> +	/* Allocate blocks only in backward direction starting from blk. */
>   	unsigned int backward:1;
> +	/* Allocate blocks only in forward direction starting from blk. */
> +	unsigned int forward:1;


I suggest to call this bitfield "monotonic_forward"


>   
>   };
>   
> diff --git a/fs/reiser4/plugin/space/bitmap.c b/fs/reiser4/plugin/space/bitmap.c
> index 3da3f6b..9beaf66 100644
> --- a/fs/reiser4/plugin/space/bitmap.c
> +++ b/fs/reiser4/plugin/space/bitmap.c
> @@ -1127,7 +1127,8 @@ static int alloc_blocks_forward(reiser4_blocknr_hint *hint, int needed,
>   	/* There is only one bitmap search if max_dist was specified or first
>   	   pass was from the beginning of the bitmap. We also do one pass for
>   	   scanning bitmap in backward direction. */
> -	if (!(actual_len != 0 || hint->max_dist != 0 || search_start == 0)) {
> +	if (actual_len == 0 && search_start != 0 &&
> +	    hint->max_dist == 0 && hint->forward == 0) {
>   		/* next step is a scanning from 0 to search_start */
>   		search_end = search_start;
>   		search_start = 0;


OK with this patch,
I'll continue to review this patch series at leisure.

Thanks,
Edward.

Re: [RFC] [PATCHv3 1/9] reiser4: block_alloc: add BA_SOME_SPACE flag for grabbing a fixed amount of space.

[Ivan Shapovalov]

[-- Attachment #1: Type: text/plain, Size: 1686 bytes --]

On Tuesday 21 October 2014 at 12:32:24, Edward Shishkin wrote:	
> 
> On 10/20/2014 12:36 PM, Ivan Shapovalov wrote:
> > On Monday 20 October 2014 at 00:04:39, Edward Shishkin wrote:	
> >> On 08/17/2014 11:52 PM, Ivan Shapovalov wrote:
> >>> This is used for FITRIM ioctl which will iteratively grab, allocate and trim disk space
> >>> bit by bit to avoid starving the rest of system.
> >>>
> >>> Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
> >>> ---
> >>>    fs/reiser4/block_alloc.c | 5 +++++
> >>>    fs/reiser4/block_alloc.h | 5 ++++-
> >>>    2 files changed, 9 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/fs/reiser4/block_alloc.c b/fs/reiser4/block_alloc.c
> >>> index d3d730c..9eed7fc 100644
> >>> --- a/fs/reiser4/block_alloc.c
> >>> +++ b/fs/reiser4/block_alloc.c
> >>> @@ -276,6 +276,11 @@ reiser4_grab(reiser4_context * ctx, __u64 count, reiser4_ba_flags_t flags)
> >>>    
> >>>    	free_blocks = sbinfo->blocks_free;
> >>>    
> >>> +	if (flags & BA_SOME_SPACE) {
> >>> +		/* Reserve 25% of all free space. */
> >> if (free_blocks == 0) {
> >>           /* nothing to trim */
> >>           ret= RETERR(-ENOSPC);
> >>           goto unlock_and_ret;
> >> }
> > Makes sense, but IMO without returning error. TRIM'ing a completely full
> > filesystem is a no-op (== all blocks busy, nothing to trim), not a failure.
> 
> 
> (sbinfo->blocks_free) is a rather bad estimation of free space amount,
> but we have nothing better. I suggest to return -ENOSPC here, and use
> reiser4_grab_space() with BA_CAN_COMMIT to make sure that FITRIM
> ioctl gets the "second soft ENOSPC".

Yeah, OK.

-- 
Ivan Shapovalov / intelfx /

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [RFC] [PATCHv3 7/9] reiser4: batch discard support: actually implement the FITRIM ioctl handler.

[Ivan Shapovalov]

[-- Attachment #1: Type: text/plain, Size: 1322 bytes --]

On Tuesday 21 October 2014 at 12:14:02, Edward Shishkin wrote:	
> 
> On 10/21/2014 12:39 AM, Ivan Shapovalov wrote:
> > On Monday 20 October 2014 at 12:54:13, Edward Shishkin wrote:	
> >> On 08/17/2014 11:52 PM, Ivan Shapovalov wrote:
> >>> [...]
> >>> +		/*
> >>> +		 * Grab some sane amount of space.
> >>> +		 * We will allocate blocks until end of the partition or until
> >>> +		 * the grabbed space is exhausted.
> >>> +		 */
> >>> +		ret = reiser4_grab_reserved(super, 0, BA_CAN_COMMIT | BA_SOME_SPACE);
> >>
> >> reiser4_grab_reserved() grabs space from the reserved area (5%).
> >> This is needed to make sure that unlink(), truncate(), etc. won't
> >> fail, if there is no free space on disk. I don't think that FITRIM
> >> ioctl needs this reserved area.
> > Well, IIUC, it doesn't hurt:
> 
> 
> "doesn't hurt" is not enough. If you want to take a resource,
> you should be going to explain, why do you need this.

I've explained the reason below... isn't it sufficient?

-- 
Ivan Shapovalov / intelfx /

> >   any "legitimate" user of the reserved space
> > will block on the mutex and eventually proceed. At the same time, given
> > a filesystem with (5% + eps) free space left, not using the reserved space
> > will result in trimming of (eps) blocks at a time.
> >
> > [...]

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [RFC] [PATCHv3 7/9] reiser4: batch discard support: actually implement the FITRIM ioctl handler.

[Edward Shishkin]

On 10/21/2014 06:18 PM, Ivan Shapovalov wrote:
> On Tuesday 21 October 2014 at 12:14:02, Edward Shishkin wrote:	
>> On 10/21/2014 12:39 AM, Ivan Shapovalov wrote:
>>> On Monday 20 October 2014 at 12:54:13, Edward Shishkin wrote:	
>>>> On 08/17/2014 11:52 PM, Ivan Shapovalov wrote:
>>>>> [...]
>>>>> +		/*
>>>>> +		 * Grab some sane amount of space.
>>>>> +		 * We will allocate blocks until end of the partition or until
>>>>> +		 * the grabbed space is exhausted.
>>>>> +		 */
>>>>> +		ret = reiser4_grab_reserved(super, 0, BA_CAN_COMMIT | BA_SOME_SPACE);
>>>>   any "legitimate" user of the reserved space
>>>> will block on the mutex and eventually proceed. At the same time, given
>>>> a filesystem with (5% + eps) free space left, not using the reserved space
>>>> will result in trimming of (eps) blocks at a time.
>>>>
>>>> reiser4_grab_reserved() grabs space from the reserved area (5%).
>>>> This is needed to make sure that unlink(), truncate(), etc. won't
>>>> fail, if there is no free space on disk. I don't think that FITRIM
>>>> ioctl needs this reserved area.
>>> Well, IIUC, it doesn't hurt:
>>
>> "doesn't hurt" is not enough. If you want to take a resource,
>> you should be going to explain, why do you need this.
> I've explained the reason below... isn't it sufficient?


Sorry, but I don't see any explanation.

Thanks,
Edward.

Re: [RFC] [PATCHv3 7/9] reiser4: batch discard support: actually implement the FITRIM ioctl handler.

[Ivan Shapovalov]

[-- Attachment #1: Type: text/plain, Size: 1659 bytes --]

On Tuesday 21 October 2014 at 18:21:56, Edward Shishkin wrote:	
> On 10/21/2014 06:18 PM, Ivan Shapovalov wrote:
> > On Tuesday 21 October 2014 at 12:14:02, Edward Shishkin wrote:	
> >> On 10/21/2014 12:39 AM, Ivan Shapovalov wrote:
> >>> On Monday 20 October 2014 at 12:54:13, Edward Shishkin wrote:	
> >>>> On 08/17/2014 11:52 PM, Ivan Shapovalov wrote:
> >>>>> [...]
> >>>>> +		/*
> >>>>> +		 * Grab some sane amount of space.
> >>>>> +		 * We will allocate blocks until end of the partition or until
> >>>>> +		 * the grabbed space is exhausted.
> >>>>> +		 */
> >>>>> +		ret = reiser4_grab_reserved(super, 0, BA_CAN_COMMIT | BA_SOME_SPACE);
> >>>>   any "legitimate" user of the reserved space
> >>>> will block on the mutex and eventually proceed. At the same time, given
> >>>> a filesystem with (5% + eps) free space left, not using the reserved space
> >>>> will result in trimming of (eps) blocks at a time.
> >>>>
> >>>> reiser4_grab_reserved() grabs space from the reserved area (5%).
> >>>> This is needed to make sure that unlink(), truncate(), etc. won't
> >>>> fail, if there is no free space on disk. I don't think that FITRIM
> >>>> ioctl needs this reserved area.
> >>> Well, IIUC, it doesn't hurt:
> >>
> >> "doesn't hurt" is not enough. If you want to take a resource,
> >> you should be going to explain, why do you need this.
> > I've explained the reason below... isn't it sufficient?
> 
> 
> Sorry, but I don't see any explanation.

"given a filesystem with (5% + eps) free space left, not using the reserved
space will result in trimming of (eps) blocks at a time."

-- 
Ivan Shapovalov / intelfx /

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [RFC] [PATCHv3 7/9] reiser4: batch discard support: actually implement the FITRIM ioctl handler.

[Edward Shishkin]

On 10/21/2014 06:23 PM, Ivan Shapovalov wrote:
> On Tuesday 21 October 2014 at 18:21:56, Edward Shishkin wrote:	
>> On 10/21/2014 06:18 PM, Ivan Shapovalov wrote:
>>> On Tuesday 21 October 2014 at 12:14:02, Edward Shishkin wrote:	
>>>> On 10/21/2014 12:39 AM, Ivan Shapovalov wrote:
>>>>> On Monday 20 October 2014 at 12:54:13, Edward Shishkin wrote:	
>>>>>> On 08/17/2014 11:52 PM, Ivan Shapovalov wrote:
>>>>>>> [...]
>>>>>>> +		/*
>>>>>>> +		 * Grab some sane amount of space.
>>>>>>> +		 * We will allocate blocks until end of the partition or until
>>>>>>> +		 * the grabbed space is exhausted.
>>>>>>> +		 */
>>>>>>> +		ret = reiser4_grab_reserved(super, 0, BA_CAN_COMMIT | BA_SOME_SPACE);
>>>>>>    any "legitimate" user of the reserved space
>>>>>> will block on the mutex and eventually proceed. At the same time, given
>>>>>> a filesystem with (5% + eps) free space left, not using the reserved space
>>>>>> will result in trimming of (eps) blocks at a time.
>>>>>>
>>>>>> reiser4_grab_reserved() grabs space from the reserved area (5%).
>>>>>> This is needed to make sure that unlink(), truncate(), etc. won't
>>>>>> fail, if there is no free space on disk. I don't think that FITRIM
>>>>>> ioctl needs this reserved area.
>>>>> Well, IIUC, it doesn't hurt:
>>>> "doesn't hurt" is not enough. If you want to take a resource,
>>>> you should be going to explain, why do you need this.
>>> I've explained the reason below... isn't it sufficient?
>>
>> Sorry, but I don't see any explanation.
> "given a filesystem with (5% + eps) free space left, not using the reserved
> space will result in trimming of (eps) blocks at a time."
>


This is something that I am not able to parse :)

Ok, it is clear, why we can not fail with -ENOSPC when trying to delete
a file from a full partition, yes?
I want to see explanation, why FITRIM ioclt can not finish the work when
there is no free space on disk.

Thanks,
Edward.

Re: [RFC] [PATCHv3 7/9] reiser4: batch discard support: actually implement the FITRIM ioctl handler.

[Ivan Shapovalov]

[-- Attachment #1: Type: text/plain, Size: 1342 bytes --]

On Tuesday 21 October 2014 at 18:33:11, Edward Shishkin wrote:	
> On 10/21/2014 06:23 PM, Ivan Shapovalov wrote:
> > On Tuesday 21 October 2014 at 18:21:56, Edward Shishkin wrote:	
> >> [...]
> >>
> >> Sorry, but I don't see any explanation.
> > "given a filesystem with (5% + eps) free space left, not using the reserved
> > space will result in trimming of (eps) blocks at a time."
> >
> 
> 
> This is something that I am not able to parse :)
> 
> Ok, it is clear, why we can not fail with -ENOSPC when trying to delete
> a file from a full partition, yes?

Sure. Please note that making reiser4_trim_fs's allocations BA_RESERVED will
not hinder this: delete_mutex is there for a reason.

> I want to see explanation, why FITRIM ioclt can not finish the work when
> there is no free space on disk.

Suppose we do not use reserved space for reiser4_trim_fs's allocations.
Let's analyze those two cases:

1. There is <= 5% free space on disk.
Initial grabbing fails, nothing can be trimmed.
This is wrong.

2. There is 5% + X (where X is some small number) free space on disk.
We can grab only X blocks at a time, so a total of ((SIZE * 5% / X) + 1)
transactions will be created. BTW, if X < erase unit, nothing can be trimmed.
This is ineffective.

Hope this makes sense.

-- 
Ivan Shapovalov / intelfx /

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [RFC] [PATCHv3 7/9] reiser4: batch discard support: actually implement the FITRIM ioctl handler.

[Edward Shishkin]

On 10/21/2014 06:42 PM, Ivan Shapovalov wrote:
> On Tuesday 21 October 2014 at 18:33:11, Edward Shishkin wrote:	
>> On 10/21/2014 06:23 PM, Ivan Shapovalov wrote:
>>> On Tuesday 21 October 2014 at 18:21:56, Edward Shishkin wrote:	
>>>> [...]
>>>>
>>>> Sorry, but I don't see any explanation.
>>> "given a filesystem with (5% + eps) free space left, not using the reserved
>>> space will result in trimming of (eps) blocks at a time."
>>>
>>
>> This is something that I am not able to parse :)
>>
>> Ok, it is clear, why we can not fail with -ENOSPC when trying to delete
>> a file from a full partition, yes?
> Sure. Please note that making reiser4_trim_fs's allocations BA_RESERVED will
> not hinder this: delete_mutex is there for a reason.


Sure, and this is the reason, because when I hear "let's take a mutex",
I start to feel bad :)


>
>> I want to see explanation, why FITRIM ioclt can not finish the work when
>> there is no free space on disk.
> Suppose we do not use reserved space for reiser4_trim_fs's allocations.
> Let's analyze those two cases:
>
> 1. There is <= 5% free space on disk.
> Initial grabbing fails, nothing can be trimmed.
> This is wrong.
>
> 2. There is 5% + X (where X is some small number) free space on disk.
> We can grab only X blocks at a time, so a total of ((SIZE * 5% / X) + 1)
> transactions will be created. BTW, if X < erase unit, nothing can be trimmed.
> This is ineffective.
>
> Hope this makes sense.


This is already much better, thanks!
Then I should say that the whole idea to reserve % of free space is 
incorrect.
Try to reserve 1% of total partition size with BA_CAN_COMMIT for an 
iteration.
If it fails, then try to reserve 1 block with BA_CAN_COMMIT.

Thanks,
Edward.

Re: [RFC] [PATCHv3 7/9] reiser4: batch discard support: actually implement the FITRIM ioctl handler.

[Ivan Shapovalov]

[-- Attachment #1: Type: text/plain, Size: 2304 bytes --]

On Tuesday 21 October 2014 at 20:01:55, Edward Shishkin wrote:	
> On 10/21/2014 06:42 PM, Ivan Shapovalov wrote:
> > On Tuesday 21 October 2014 at 18:33:11, Edward Shishkin wrote:	
> >> On 10/21/2014 06:23 PM, Ivan Shapovalov wrote:
> >>> On Tuesday 21 October 2014 at 18:21:56, Edward Shishkin wrote:	
> >>>> [...]
> >>>>
> >>>> Sorry, but I don't see any explanation.
> >>> "given a filesystem with (5% + eps) free space left, not using the reserved
> >>> space will result in trimming of (eps) blocks at a time."
> >>>
> >>
> >> This is something that I am not able to parse :)
> >>
> >> Ok, it is clear, why we can not fail with -ENOSPC when trying to delete
> >> a file from a full partition, yes?
> > Sure. Please note that making reiser4_trim_fs's allocations BA_RESERVED will
> > not hinder this: delete_mutex is there for a reason.
> 
> 
> Sure, and this is the reason, because when I hear "let's take a mutex",
> I start to feel bad :)
> 
> 
> >
> >> I want to see explanation, why FITRIM ioclt can not finish the work when
> >> there is no free space on disk.
> > Suppose we do not use reserved space for reiser4_trim_fs's allocations.
> > Let's analyze those two cases:
> >
> > 1. There is <= 5% free space on disk.
> > Initial grabbing fails, nothing can be trimmed.
> > This is wrong.
> >
> > 2. There is 5% + X (where X is some small number) free space on disk.
> > We can grab only X blocks at a time, so a total of ((SIZE * 5% / X) + 1)
> > transactions will be created. BTW, if X < erase unit, nothing can be trimmed.
> > This is ineffective.
> >
> > Hope this makes sense.
> 
> 
> This is already much better, thanks!
> Then I should say that the whole idea to reserve % of free space is 
> incorrect.
> Try to reserve 1% of total partition size with BA_CAN_COMMIT for an 
> iteration.
> If it fails, then try to reserve 1 block with BA_CAN_COMMIT.

Hm? Both described cases are still broken.

In #1 (free space <= 5%) nothing changes; reiser4_trim_fs will never be able
to do its task without using reserved space.

In updated #2 (5% < free space < 6%), a transaction per every block (!)
will be created, and none of them will trim anything (1 block < erase unit).

Have I misunderstood you?

-- 
Ivan Shapovalov / intelfx /

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [RFC] [PATCHv3 7/9] reiser4: batch discard support: actually implement the FITRIM ioctl handler.

[Edward Shishkin]

On 10/21/2014 08:11 PM, Ivan Shapovalov wrote:
> On Tuesday 21 October 2014 at 20:01:55, Edward Shishkin wrote:	
>> On 10/21/2014 06:42 PM, Ivan Shapovalov wrote:
>>> On Tuesday 21 October 2014 at 18:33:11, Edward Shishkin wrote:	
>>>> On 10/21/2014 06:23 PM, Ivan Shapovalov wrote:
>>>>> On Tuesday 21 October 2014 at 18:21:56, Edward Shishkin wrote:	
>>>>>> [...]
>>>>>>
>>>>>> Sorry, but I don't see any explanation.
>>>>> "given a filesystem with (5% + eps) free space left, not using the reserved
>>>>> space will result in trimming of (eps) blocks at a time."
>>>>>
>>>> This is something that I am not able to parse :)
>>>>
>>>> Ok, it is clear, why we can not fail with -ENOSPC when trying to delete
>>>> a file from a full partition, yes?
>>> Sure. Please note that making reiser4_trim_fs's allocations BA_RESERVED will
>>> not hinder this: delete_mutex is there for a reason.
>>
>> Sure, and this is the reason, because when I hear "let's take a mutex",
>> I start to feel bad :)
>>
>>
>>>> I want to see explanation, why FITRIM ioclt can not finish the work when
>>>> there is no free space on disk.
>>> Suppose we do not use reserved space for reiser4_trim_fs's allocations.
>>> Let's analyze those two cases:
>>>
>>> 1. There is <= 5% free space on disk.
>>> Initial grabbing fails, nothing can be trimmed.
>>> This is wrong.
>>>
>>> 2. There is 5% + X (where X is some small number) free space on disk.
>>> We can grab only X blocks at a time, so a total of ((SIZE * 5% / X) + 1)
>>> transactions will be created. BTW, if X < erase unit, nothing can be trimmed.
>>> This is ineffective.
>>>
>>> Hope this makes sense.
>>
>> This is already much better, thanks!
>> Then I should say that the whole idea to reserve % of free space is
>> incorrect.
>> Try to reserve 1% of total partition size with BA_CAN_COMMIT for an
>> iteration.
>> If it fails, then try to reserve 1 block with BA_CAN_COMMIT.
> Hm? Both described cases are still broken.
>
> In #1 (free space <= 5%) nothing changes; reiser4_trim_fs will never be able
> to do its task without using reserved space.
>
> In updated #2 (5% < free space < 6%), a transaction per every block (!)
> will be created, and none of them will trim anything (1 block < erase unit).


If (5% < free_space < 5% + erase_unit), then fail with ENOSPC.
Why not? This is not the unlink case...

Re: [RFC] [PATCHv3 7/9] reiser4: batch discard support: actually implement the FITRIM ioctl handler.

[Ivan Shapovalov]

[-- Attachment #1: Type: text/plain, Size: 2667 bytes --]

On Tuesday 21 October 2014 at 20:48:59, Edward Shishkin wrote:	
> On 10/21/2014 08:11 PM, Ivan Shapovalov wrote:
> > On Tuesday 21 October 2014 at 20:01:55, Edward Shishkin wrote:	
> >> On 10/21/2014 06:42 PM, Ivan Shapovalov wrote:
> >>> On Tuesday 21 October 2014 at 18:33:11, Edward Shishkin wrote:	
> >>>> [...]
> >>>>
> >>>> Ok, it is clear, why we can not fail with -ENOSPC when trying to delete
> >>>> a file from a full partition, yes?
> >>> Sure. Please note that making reiser4_trim_fs's allocations BA_RESERVED will
> >>> not hinder this: delete_mutex is there for a reason.
> >>
> >> Sure, and this is the reason, because when I hear "let's take a mutex",
> >> I start to feel bad :)

Does this (ab)use of delete_mutex violates any ordering?
If not, it should be OK.

> >>>> I want to see explanation, why FITRIM ioclt can not finish the work when
> >>>> there is no free space on disk.
> >>> Suppose we do not use reserved space for reiser4_trim_fs's allocations.
> >>> Let's analyze those two cases:
> >>>
> >>> 1. There is <= 5% free space on disk.
> >>> Initial grabbing fails, nothing can be trimmed.
> >>> This is wrong.
> >>>
> >>> 2. There is 5% + X (where X is some small number) free space on disk.
> >>> We can grab only X blocks at a time, so a total of ((SIZE * 5% / X) + 1)
> >>> transactions will be created. BTW, if X < erase unit, nothing can be trimmed.
> >>> This is ineffective.
> >>>
> >>> Hope this makes sense.
> >>
> >> This is already much better, thanks!
> >> Then I should say that the whole idea to reserve % of free space is
> >> incorrect.
> >> Try to reserve 1% of total partition size with BA_CAN_COMMIT for an
> >> iteration.
> >> If it fails, then try to reserve 1 block with BA_CAN_COMMIT.
> > Hm? Both described cases are still broken.
> >
> > In #1 (free space <= 5%) nothing changes; reiser4_trim_fs will never be able
> > to do its task without using reserved space.
> >
> > In updated #2 (5% < free space < 6%), a transaction per every block (!)
> > will be created, and none of them will trim anything (1 block < erase unit).
> 
> 
> If (5% < free_space < 5% + erase_unit), then fail with ENOSPC.

...and s/1 block/1 erase unit/ in your proposal?
Still this will create too many transactions.

> Why not? This is not the unlink case...

It looks like fighting an artificially created problem. I fail to see why
can't we use BA_RESERVED. This will solve both cases at no harm.

Note -- I'm not saying "let's take this resource, it does not harm", but
"let's take this resource, it solves two cases AND does not harm" :)

-- 
Ivan Shapovalov / intelfx /

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [RFC] [PATCHv3 7/9] reiser4: batch discard support: actually implement the FITRIM ioctl handler.

[Edward Shishkin]

On 10/21/2014 09:00 PM, Ivan Shapovalov wrote:
> On Tuesday 21 October 2014 at 20:48:59, Edward Shishkin wrote:	
>> On 10/21/2014 08:11 PM, Ivan Shapovalov wrote:
>>> On Tuesday 21 October 2014 at 20:01:55, Edward Shishkin wrote:	
>>>> On 10/21/2014 06:42 PM, Ivan Shapovalov wrote:
>>>>> On Tuesday 21 October 2014 at 18:33:11, Edward Shishkin wrote:	
>>>>>> [...]
>>>>>>
>>>>>> Ok, it is clear, why we can not fail with -ENOSPC when trying to delete
>>>>>> a file from a full partition, yes?
>>>>> Sure. Please note that making reiser4_trim_fs's allocations BA_RESERVED will
>>>>> not hinder this: delete_mutex is there for a reason.
>>>> Sure, and this is the reason, because when I hear "let's take a mutex",
>>>> I start to feel bad :)
> Does this (ab)use of delete_mutex violates any ordering?
> If not, it should be OK.
>
>>>>>> I want to see explanation, why FITRIM ioclt can not finish the work when
>>>>>> there is no free space on disk.
>>>>> Suppose we do not use reserved space for reiser4_trim_fs's allocations.
>>>>> Let's analyze those two cases:
>>>>>
>>>>> 1. There is <= 5% free space on disk.
>>>>> Initial grabbing fails, nothing can be trimmed.
>>>>> This is wrong.
>>>>>
>>>>> 2. There is 5% + X (where X is some small number) free space on disk.
>>>>> We can grab only X blocks at a time, so a total of ((SIZE * 5% / X) + 1)
>>>>> transactions will be created. BTW, if X < erase unit, nothing can be trimmed.
>>>>> This is ineffective.
>>>>>
>>>>> Hope this makes sense.
>>>> This is already much better, thanks!
>>>> Then I should say that the whole idea to reserve % of free space is
>>>> incorrect.
>>>> Try to reserve 1% of total partition size with BA_CAN_COMMIT for an
>>>> iteration.
>>>> If it fails, then try to reserve 1 block with BA_CAN_COMMIT.
>>> Hm? Both described cases are still broken.
>>>
>>> In #1 (free space <= 5%) nothing changes; reiser4_trim_fs will never be able
>>> to do its task without using reserved space.
>>>
>>> In updated #2 (5% < free space < 6%), a transaction per every block (!)
>>> will be created, and none of them will trim anything (1 block < erase unit).
>>
>> If (5% < free_space < 5% + erase_unit), then fail with ENOSPC.
> ...and s/1 block/1 erase unit/ in your proposal?
> Still this will create too many transactions.
>
>> Why not? This is not the unlink case...
> It looks like fighting an artificially created problem. I fail to see why
> can't we use BA_RESERVED.


Let's assign you a pair of tickets with deadlock issues
in reiser4? I think you will change your opinion quickly :)


>   This will solve both cases at no harm.
>
> Note -- I'm not saying "let's take this resource, it does not harm", but
> "let's take this resource, it solves two cases AND does not harm" :)


OK, you won,
do it with grab_reserved :)

Edward.

Re: [RFC] [PATCHv3 2/9] reiser4: block_alloc: add a "forward" parameter to reiser4_blocknr_hint to allocate blocks only in forward direction.

[Ivan Shapovalov]

[-- Attachment #1: Type: text/plain, Size: 2387 bytes --]

On Tuesday 21 October 2014 at 17:50:23, Edward Shishkin wrote:	
> On 08/17/2014 11:52 PM, Ivan Shapovalov wrote:
> > Signed-off-by: Ivan Shapovalov<intelfx100@gmail.com>
> > ---
> >   fs/reiser4/block_alloc.h         | 5 +++--
> >   fs/reiser4/plugin/space/bitmap.c | 3 ++-
> >   2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/reiser4/block_alloc.h b/fs/reiser4/block_alloc.h
> > index bfc6be9..08b3941 100644
> > --- a/fs/reiser4/block_alloc.h
> > +++ b/fs/reiser4/block_alloc.h
> > @@ -51,9 +51,10 @@ struct reiser4_blocknr_hint {
> >   	/* block allocator assumes that blocks, which will be mapped to disk,
> >   	   are in this specified block_stage */
> >   	block_stage_t block_stage;
> > -	/* If direction = 1 allocate blocks in backward direction from the end
> > -	 * of disk to the beginning of disk.  */
> > +	/* Allocate blocks only in backward direction starting from blk. */
> >   	unsigned int backward:1;
> > +	/* Allocate blocks only in forward direction starting from blk. */
> > +	unsigned int forward:1;
> 
> 
> I suggest to call this bitfield "monotonic_forward"

Isn't it an opposite of "backward"? Should we rename both?

BTW, in plugin/space/bitmap.c:1141 (around that line)
in function alloc_blocks_forward()
shouldn't the second scan be done with bitmap_alloc_backward(), as per the
comment?

Thanks,
-- 
Ivan Shapovalov / intelfx /

> >   
> >   };
> >   
> > diff --git a/fs/reiser4/plugin/space/bitmap.c b/fs/reiser4/plugin/space/bitmap.c
> > index 3da3f6b..9beaf66 100644
> > --- a/fs/reiser4/plugin/space/bitmap.c
> > +++ b/fs/reiser4/plugin/space/bitmap.c
> > @@ -1127,7 +1127,8 @@ static int alloc_blocks_forward(reiser4_blocknr_hint *hint, int needed,
> >   	/* There is only one bitmap search if max_dist was specified or first
> >   	   pass was from the beginning of the bitmap. We also do one pass for
> >   	   scanning bitmap in backward direction. */
> > -	if (!(actual_len != 0 || hint->max_dist != 0 || search_start == 0)) {
> > +	if (actual_len == 0 && search_start != 0 &&
> > +	    hint->max_dist == 0 && hint->forward == 0) {
> >   		/* next step is a scanning from 0 to search_start */
> >   		search_end = search_start;
> >   		search_start = 0;
> 
> 
> OK with this patch,
> I'll continue to review this patch series at leisure.
> 
> Thanks,
> Edward.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [RFC] [PATCHv3 2/9] reiser4: block_alloc: add a "forward" parameter to reiser4_blocknr_hint to allocate blocks only in forward direction.

[Edward Shishkin]

On 10/23/2014 09:24 AM, Ivan Shapovalov wrote:
> On Tuesday 21 October 2014 at 17:50:23, Edward Shishkin wrote:	
>> On 08/17/2014 11:52 PM, Ivan Shapovalov wrote:
>>> Signed-off-by: Ivan Shapovalov<intelfx100@gmail.com>
>>> ---
>>>    fs/reiser4/block_alloc.h         | 5 +++--
>>>    fs/reiser4/plugin/space/bitmap.c | 3 ++-
>>>    2 files changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/reiser4/block_alloc.h b/fs/reiser4/block_alloc.h
>>> index bfc6be9..08b3941 100644
>>> --- a/fs/reiser4/block_alloc.h
>>> +++ b/fs/reiser4/block_alloc.h
>>> @@ -51,9 +51,10 @@ struct reiser4_blocknr_hint {
>>>    	/* block allocator assumes that blocks, which will be mapped to disk,
>>>    	   are in this specified block_stage */
>>>    	block_stage_t block_stage;
>>> -	/* If direction = 1 allocate blocks in backward direction from the end
>>> -	 * of disk to the beginning of disk.  */
>>> +	/* Allocate blocks only in backward direction starting from blk. */
>>>    	unsigned int backward:1;
>>> +	/* Allocate blocks only in forward direction starting from blk. */
>>> +	unsigned int forward:1;
>>
>> I suggest to call this bitfield "monotonic_forward"
> Isn't it an opposite of "backward"? Should we rename both?


No, it isn't. And this is the reason of the rename.
monotonic_forward is to skip the second pass in alloc_blocks_forward()


>
> BTW, in plugin/space/bitmap.c:1141 (around that line)
> in function alloc_blocks_forward()
> shouldn't the second scan be done with bitmap_alloc_backward(), as per the
> comment?


I think that those comment means a jump in backward direction
(to the start) and one more pass with bitmap_alloc_forward().
Such a "compound, non-monotonic" forward...

Edward.

Re: [RFC] [PATCHv3 2/9] reiser4: block_alloc: add a "forward" parameter to reiser4_blocknr_hint to allocate blocks only in forward direction.

[Ivan Shapovalov]

[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]

On Thursday 23 October 2014 at 16:37:00, Edward Shishkin wrote:	
> On 10/23/2014 09:24 AM, Ivan Shapovalov wrote:
> > On Tuesday 21 October 2014 at 17:50:23, Edward Shishkin wrote:	
> >> [...]
> >>
> >> I suggest to call this bitfield "monotonic_forward"
> > Isn't it an opposite of "backward"? Should we rename both?
> 
> 
> No, it isn't. And this is the reason of the rename.
> monotonic_forward is to skip the second pass in alloc_blocks_forward()

OK, will rename.

> > BTW, in plugin/space/bitmap.c:1141 (around that line)
> > in function alloc_blocks_forward()
> > shouldn't the second scan be done with bitmap_alloc_backward(), as per the
> > comment?
> 
> 
> I think that those comment means a jump in backward direction
> (to the start) and one more pass with bitmap_alloc_forward().
> Such a "compound, non-monotonic" forward...

I don't intend to argue here, but doesn't that defeat the purpose of hint->blk?
The first pass begins from the *closest* blocks. And if it fails, the second
pass begins from the *most distant* blocks...

("closest" and "most distant" terms are used here with respect to hint->blk)

Thanks,
-- 
Ivan Shapovalov / intelfx /

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [RFC] [PATCHv3 2/9] reiser4: block_alloc: add a "forward" parameter to reiser4_blocknr_hint to allocate blocks only in forward direction.

[Edward Shishkin]

On 10/23/2014 10:58 PM, Ivan Shapovalov wrote:
> On Thursday 23 October 2014 at 16:37:00, Edward Shishkin wrote:	
>> On 10/23/2014 09:24 AM, Ivan Shapovalov wrote:
>>> On Tuesday 21 October 2014 at 17:50:23, Edward Shishkin wrote:	
>>>> [...]
>>>>
>>>> I suggest to call this bitfield "monotonic_forward"
>>> Isn't it an opposite of "backward"? Should we rename both?
>>
>> No, it isn't. And this is the reason of the rename.
>> monotonic_forward is to skip the second pass in alloc_blocks_forward()
> OK, will rename.
>
>>> BTW, in plugin/space/bitmap.c:1141 (around that line)
>>> in function alloc_blocks_forward()
>>> shouldn't the second scan be done with bitmap_alloc_backward(), as per the
>>> comment?
>>
>> I think that those comment means a jump in backward direction
>> (to the start) and one more pass with bitmap_alloc_forward().
>> Such a "compound, non-monotonic" forward...
> I don't intend to argue here, but doesn't that defeat the purpose of hint->blk?
> The first pass begins from the *closest* blocks. And if it fails, the second
> pass begins from the *most distant* blocks...
>
> ("closest" and "most distant" terms are used here with respect to hint->blk)


You don't like the block allocator?
Actually it was a subject of long investigations in ReiserFS (v3).
You can find a number of block allocators in ./fs/reiserfs/bitmap.c
I don't know which one went to reiser4..

Edward.

Re: [RFC] [PATCHv3 2/9] reiser4: block_alloc: add a "forward" parameter to reiser4_blocknr_hint to allocate blocks only in forward direction.

[Ivan Shapovalov]

[-- Attachment #1: Type: text/plain, Size: 1282 bytes --]

On Sunday 26 October 2014 at 17:45:33, Edward Shishkin wrote:	
> [...]
> >
> >>> BTW, in plugin/space/bitmap.c:1141 (around that line)
> >>> in function alloc_blocks_forward()
> >>> shouldn't the second scan be done with bitmap_alloc_backward(), as per the
> >>> comment?
> >>
> >> I think that those comment means a jump in backward direction
> >> (to the start) and one more pass with bitmap_alloc_forward().
> >> Such a "compound, non-monotonic" forward...
> > I don't intend to argue here, but doesn't that defeat the purpose of hint->blk?
> > The first pass begins from the *closest* blocks. And if it fails, the second
> > pass begins from the *most distant* blocks...
> >
> > ("closest" and "most distant" terms are used here with respect to hint->blk)
> 
> 
> You don't like the block allocator?
> Actually it was a subject of long investigations in ReiserFS (v3).
> You can find a number of block allocators in ./fs/reiserfs/bitmap.c
> I don't know which one went to reiser4..

I've specifically stated that I don't intend to argue or claim that the
allocator is wrong in any way :)

That thing has just seemed strange to me, so I decided to ask whether is this
intentional and, if yes, what was the intention.

-- 
Ivan Shapovalov / intelfx /

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 213 bytes --]