[PATCHv2 0/3] reiser4: discard support: perform discard before all deallocations.

[PATCHv2 0/3] reiser4: discard support: perform discard before all deallocations.

[Ivan Shapovalov]

This applies on top of PATCHv6 of "initial implementation".
The first patch has been already sent some time ago, but I've included it
here for sake of clarity.

v2: fix disk space leak in error path, fix comments and commit messages, etc.

Ivan Shapovalov (3):
  reiser4: fix reiser4_post_{commit,write_back}_hook() and their invocations.
  reiser4: discard support: use reiser4_post_write_back_hook() for discarding and completing deferred deallocations.
  reiser4: discard support: perform discard before all deallocations.

[PATCHv2 1/3] reiser4: fix reiser4_post_{commit,write_back}_hook() and their invocations.

[Ivan Shapovalov]

- let all hooks be called from one place (reiser4_write_logs())
- don't call reiser4_post_write_back_hook() twice
- fix reiser4_post_write_back_hook(): call the correct method of space allocator

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

diff --git a/fs/reiser4/block_alloc.c b/fs/reiser4/block_alloc.c
index 81ed96f..3a4b667 100644
--- a/fs/reiser4/block_alloc.c
+++ b/fs/reiser4/block_alloc.c
@@ -1150,7 +1150,7 @@ void reiser4_post_write_back_hook(void)
 {
 	assert("zam-504", get_current_super_private() != NULL);
 
-	sa_post_commit_hook();
+	sa_post_write_back_hook();
 }
 
 /*
diff --git a/fs/reiser4/wander.c b/fs/reiser4/wander.c
index 0b518c3..4e29de8 100644
--- a/fs/reiser4/wander.c
+++ b/fs/reiser4/wander.c
@@ -1140,7 +1140,6 @@ static int write_tx_back(struct commit_handle * ch)
 	int ret;
 	int barrier;
 
-	reiser4_post_commit_hook();
 	fq = get_fq_for_current_atom();
 	if (IS_ERR(fq))
 		return  PTR_ERR(fq);
@@ -1165,7 +1164,6 @@ static int write_tx_back(struct commit_handle * ch)
 		if (ret)
 			return ret;
 	}
-	reiser4_post_write_back_hook();
 	return 0;
 }
 
@@ -1251,6 +1249,7 @@ int reiser4_write_logs(long *nr_submitted)
 	spin_lock_atom(atom);
 	reiser4_atom_set_stage(atom, ASTAGE_POST_COMMIT);
 	spin_unlock_atom(atom);
+	reiser4_post_commit_hook();
 
 	ret = write_tx_back(&ch);
 	reiser4_post_write_back_hook();
-- 
2.0.2

[PATCHv2 2/3] reiser4: discard support: use reiser4_post_write_back_hook() for discarding and completing deferred deallocations.

[Ivan Shapovalov]

This hook is now called after immediate deallocations in the wandered
logs code.

Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
---
 fs/reiser4/block_alloc.c | 24 +++++++++++++++++++-----
 fs/reiser4/txnmgr.c      | 11 -----------
 fs/reiser4/wander.c      |  3 ++-
 3 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/fs/reiser4/block_alloc.c b/fs/reiser4/block_alloc.c
index 7b5000b..4ce2a16 100644
--- a/fs/reiser4/block_alloc.c
+++ b/fs/reiser4/block_alloc.c
@@ -1136,15 +1136,13 @@ apply_dset(txn_atom * atom UNUSED_ARG, const reiser4_block_nr * a,
 
 void reiser4_post_commit_hook(void)
 {
+#ifdef REISER4_DEBUG
 	txn_atom *atom;
 
 	atom = get_current_atom_locked();
 	assert("zam-452", atom->stage == ASTAGE_POST_COMMIT);
 	spin_unlock_atom(atom);
-
-	/* do the block deallocation which was deferred
-	   until commit is done */
-	atom_dset_deferred_apply(atom, apply_dset, NULL, 0);
+#endif
 
 	assert("zam-504", get_current_super_private() != NULL);
 	sa_post_commit_hook();
@@ -1152,8 +1150,24 @@ void reiser4_post_commit_hook(void)
 
 void reiser4_post_write_back_hook(void)
 {
-	assert("zam-504", get_current_super_private() != NULL);
+	txn_atom *atom;
+	int ret;
 
+	/* process and issue discard requests */
+	do {
+		atom = get_current_atom_locked();
+		ret = discard_atom(*atom);
+	} while (ret == -E_REPEAT);
+
+	if (ret) {
+		warning("intelfx-8", "discard atom failed (%ld)", ret);
+	}
+
+	/* do the block deallocation which was deferred
+	   until commit is done */
+	atom_dset_deferred_apply(atom, apply_dset, NULL, 0);
+
+	assert("zam-504", get_current_super_private() != NULL);
 	sa_post_write_back_hook();
 }
 
diff --git a/fs/reiser4/txnmgr.c b/fs/reiser4/txnmgr.c
index f27d1dc..317bc4f 100644
--- a/fs/reiser4/txnmgr.c
+++ b/fs/reiser4/txnmgr.c
@@ -1089,17 +1089,6 @@ static int commit_current_atom(long *nr_submitted, txn_atom ** atom)
 	if (ret < 0)
 		reiser4_panic("zam-597", "write log failed (%ld)\n", ret);
 
-	/* process and issue discard requests */
-	do {
-		spin_lock_atom(*atom);
-		ret = discard_atom(*atom);
-	} while (ret == -E_REPEAT);
-
-	if (ret) {
-		warning("intelfx-8", "discard atom failed (%ld)", ret);
-		ret = 0; /* the discard is optional, don't fail the commit */
-	}
-
 	/* The atom->ovrwr_nodes list is processed under commit mutex held
 	   because of bitmap nodes which are captured by special way in
 	   reiser4_pre_commit_hook_bitmap(), that way does not include
diff --git a/fs/reiser4/wander.c b/fs/reiser4/wander.c
index 4e29de8..04ddec6 100644
--- a/fs/reiser4/wander.c
+++ b/fs/reiser4/wander.c
@@ -1252,7 +1252,6 @@ int reiser4_write_logs(long *nr_submitted)
 	reiser4_post_commit_hook();
 
 	ret = write_tx_back(&ch);
-	reiser4_post_write_back_hook();
 
       up_and_ret:
 	if (ret) {
@@ -1265,6 +1264,8 @@ int reiser4_write_logs(long *nr_submitted)
 	dealloc_tx_list(&ch);
 	dealloc_wmap(&ch);
 
+	reiser4_post_write_back_hook();
+
 	put_overwrite_set(&ch);
 
 	done_commit_handle(&ch);
-- 
2.0.2

[PATCHv2 3/3] reiser4: discard support: perform discard before all deallocations.

[Ivan Shapovalov]

When discard is enabled, immediate deallocations are made deferred,
and thus discard procedure takes place right before any deallocation
is made. This allows to avoid costly bitmap checks at discard time.

Note that this commit itself is incomplete and needs a modified
discard_extent() function which does not check discarded extents
in the bitmap.

Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
---
 fs/reiser4/block_alloc.c | 31 ++++++---------------
 fs/reiser4/discard.c     | 72 ++++++++++++++++++++++++++++++++++++------------
 fs/reiser4/discard.h     | 17 ++++++++++--
 fs/reiser4/txnmgr.c      | 24 ----------------
 fs/reiser4/txnmgr.h      | 13 ++-------
 5 files changed, 79 insertions(+), 78 deletions(-)

diff --git a/fs/reiser4/block_alloc.c b/fs/reiser4/block_alloc.c
index 4ce2a16..98080a1 100644
--- a/fs/reiser4/block_alloc.c
+++ b/fs/reiser4/block_alloc.c
@@ -1007,7 +1007,8 @@ reiser4_dealloc_blocks(const reiser4_block_nr * start,
 		spin_unlock_reiser4_super(sbinfo);
 	}
 
-	if (flags & BA_DEFER) {
+	if ((flags & BA_DEFER) ||
+	    reiser4_is_set(reiser4_get_current_sb(), REISER4_DISCARD)) {
 		/* store deleted block numbers in the atom's deferred delete set
 		   for further actual deletion */
 		do {
@@ -1028,25 +1029,6 @@ reiser4_dealloc_blocks(const reiser4_block_nr * start,
 		spin_unlock_atom(atom);
 
 	} else {
-		/* store deleted block numbers in the atom's immediate delete set
-		   for further processing */
-		do {
-			atom = get_current_atom_locked();
-			assert("intelfx-51", atom != NULL);
-
-			ret = atom_dset_immediate_add_extent(atom, &new_entry, start, len);
-
-			if (ret == -ENOMEM)
-				return ret;
-
-			/* This loop might spin at most two times */
-		} while (ret == -E_REPEAT);
-
-		assert("intelfx-52", ret == 0);
-		assert("intelfx-53", atom != NULL);
-
-		spin_unlock_atom(atom);
-
 		assert("zam-425", get_current_super_private() != NULL);
 		sa_dealloc_blocks(reiser4_get_space_allocator(ctx->super),
 				  *start, *len);
@@ -1150,22 +1132,27 @@ void reiser4_post_commit_hook(void)
 
 void reiser4_post_write_back_hook(void)
 {
+	struct list_head discarded_set;
 	txn_atom *atom;
 	int ret;
 
 	/* process and issue discard requests */
+	blocknr_list_init (&discarded_set);
 	do {
 		atom = get_current_atom_locked();
-		ret = discard_atom(*atom);
+		ret = discard_atom(atom, &discarded_set);
 	} while (ret == -E_REPEAT);
 
 	if (ret) {
 		warning("intelfx-8", "discard atom failed (%ld)", ret);
 	}
 
+	atom = get_current_atom_locked();
+	discard_atom_post(atom, &discarded_set);
+
 	/* do the block deallocation which was deferred
 	   until commit is done */
-	atom_dset_deferred_apply(atom, apply_dset, NULL, 0);
+	atom_dset_deferred_apply(atom, apply_dset, NULL, 1);
 
 	assert("zam-504", get_current_super_private() != NULL);
 	sa_post_write_back_hook();
diff --git a/fs/reiser4/discard.c b/fs/reiser4/discard.c
index 3c8ee89..8442619 100644
--- a/fs/reiser4/discard.c
+++ b/fs/reiser4/discard.c
@@ -33,24 +33,43 @@
  * MECHANISM:
  *
  * During the transaction deallocated extents are recorded in atom's delete
- * sets. There are two delete sets, because data in one of them (delete_set) is
- * also used by other parts of reiser4. The second delete set (aux_delete_set)
- * complements the first one and is maintained only when discard is enabled.
+ * set. In reiser4, there are two methods to deallocate a block:
+ * 1. deferred deallocation, enabled by BA_DEFER flag to reiser4_dealloc_block().
+ *    In this mode, blocks are stored to delete set instead of being marked free
+ *    immediately. After committing the transaction, the delete set is "applied"
+ *    by the block allocator and all these blocks are marked free in memory
+ *    (see reiser4_post_write_back_hook()).
+ *    Space management plugins also read the delete set to update on-disk
+ *    allocation records (see reiser4_pre_commit_hook()).
+ * 2. immediate deallocation (the opposite).
+ *    In this mode, blocks are marked free immediately. This is used by the
+ *    journal subsystem to manage space used by the journal records, so these
+ *    allocations are not visible to the space management plugins and never hit
+ *    the disk.
  *
- * Together these sets constitute "the discard set" -- blocks that have to be
- * considered for discarding. On atom commit we will generate a minimal
- * superset of the discard set, comprised of whole erase units.
+ * When discard is enabled, all immediate deallocations become deferred. This
+ * is OK because journal's allocations happen after reiser4_pre_commit_hook()
+ * where the on-disk space allocation records are updated. So, in this mode
+ * the atom's delete set becomes "the discard set" -- list of blocks that have
+ * to be considered for discarding.
+ *
+ * On atom commit we will generate a minimal superset of the discard set,
+ * comprised of whole erase units.
+ *
+ * Discarding is performed before completing deferred deallocations, hence all
+ * extents in the discard set are still marked as allocated and cannot contain
+ * any data. Thus we can avoid any checks for blocks directly present in the
+ * discard set.
+ *
+ * However, we pad each extent from both sides to erase unit boundaries, and
+ * these paddings still have to be checked if they fall outside of initial
+ * extent (may not happen if block size > erase unit size).
  *
  * So, at commit time the following actions take place:
  * - delete sets are merged to form the discard set;
  * - elements of the discard set are sorted;
  * - the discard set is iterated, joining any adjacent extents;
- * - each of resulting extents is "covered" by erase units:
- *   - its start is rounded down to the closest erase unit boundary;
- *   - starting from this block, extents of erase unit length are created
- *     until the original extent is fully covered;
- * - the calculated erase units are checked to be fully deallocated;
- * - remaining (valid) erase units are then passed to blkdev_issue_discard().
+ * - <TODO>
  */
 
 #include "discard.h"
@@ -167,7 +186,7 @@ static int discard_extent(txn_atom *atom UNUSED_ARG,
 	return 0;
 }
 
-int discard_atom(txn_atom *atom)
+int discard_atom(txn_atom *atom, struct list_head *processed_set)
 {
 	int ret;
 	struct list_head discard_set;
@@ -178,24 +197,27 @@ int discard_atom(txn_atom *atom)
 	}
 
 	assert("intelfx-28", atom != NULL);
+	assert("intelfx-59", processed_entries != NULL);
 
-	if (list_empty(&atom->discard.delete_set) &&
-	    list_empty(&atom->discard.aux_delete_set)) {
-		spin_unlock_atom(atom);
+	if (list_empty(&atom->discard.delete_set)) {
+		/* Nothing left to discard. */
 		return 0;
 	}
 
 	/* Take the delete sets from the atom in order to release atom spinlock. */
 	blocknr_list_init(&discard_set);
 	blocknr_list_merge(&atom->discard.delete_set, &discard_set);
-	blocknr_list_merge(&atom->discard.aux_delete_set, &discard_set);
 	spin_unlock_atom(atom);
 
 	/* Sort the discard list, joining adjacent and overlapping extents. */
 	blocknr_list_sort_and_join(&discard_set);
 
 	/* Perform actual dirty work. */
-	ret = blocknr_list_iterator(NULL, &discard_set, &discard_extent, NULL, 1);
+	ret = blocknr_list_iterator(NULL, &discard_set, &discard_extent, NULL, 0);
+
+	/* Add processed extents to the temporary list. */
+	blocknr_list_merge(&discard_set, processed_set);
+
 	if (ret != 0) {
 		return ret;
 	}
@@ -204,6 +226,20 @@ int discard_atom(txn_atom *atom)
 	return -E_REPEAT;
 }
 
+void discard_atom_post(txn_atom *atom, struct list_head *processed_set)
+{
+	assert("intelfx-60", atom != NULL);
+	assert("intelfx-61", processed_entries != NULL);
+
+	if (!reiser4_is_set(reiser4_get_current_sb(), REISER4_DISCARD)) {
+		spin_unlock_atom(atom);
+		return;
+	}
+
+	blocknr_list_merge(processed_set, &atom->discard.delete_set);
+	spin_unlock_atom(atom);
+}
+
 /* Make Linus happy.
    Local variables:
    c-indentation-style: "K&R"
diff --git a/fs/reiser4/discard.h b/fs/reiser4/discard.h
index ea46334..5f0d0d8 100644
--- a/fs/reiser4/discard.h
+++ b/fs/reiser4/discard.h
@@ -11,11 +11,22 @@
 
 /**
  * Issue discard requests for all block extents recorded in @atom's delete sets,
- * if discard is enabled. In this case the delete sets are cleared.
+ * if discard is enabled. The extents processed are removed from the @atom's
+ * delete sets and stored in @processed_set.
  *
- * @atom should be locked on entry and is unlocked on exit.
+ * @atom must be locked on entry and is unlocked on exit.
+ * @processed_set must be initialized with blocknr_list_init().
  */
-extern int discard_atom(txn_atom *atom);
+extern int discard_atom(txn_atom *atom, struct list_head *processed_set);
+
+/**
+ * Splices @processed_set back to @atom's delete set.
+ * Must be called after discard_atom() loop, using the same @processed_set.
+ *
+ * @atom must be locked on entry and is unlocked on exit.
+ * @processed_set must be the same as passed to discard_atom().
+ */
+extern void discard_atom_post(txn_atom *atom, struct list_head *processed_set);
 
 /* __FS_REISER4_DISCARD_H__ */
 #endif
diff --git a/fs/reiser4/txnmgr.c b/fs/reiser4/txnmgr.c
index 317bc4f..d73ecb9 100644
--- a/fs/reiser4/txnmgr.c
+++ b/fs/reiser4/txnmgr.c
@@ -3081,7 +3081,6 @@ void atom_dset_init(txn_atom *atom)
 {
 	if (reiser4_is_set(reiser4_get_current_sb(), REISER4_DISCARD)) {
 		blocknr_list_init(&atom->discard.delete_set);
-		blocknr_list_init(&atom->discard.aux_delete_set);
 	} else {
 		blocknr_set_init(&atom->nodiscard.delete_set);
 	}
@@ -3091,7 +3090,6 @@ void atom_dset_destroy(txn_atom *atom)
 {
 	if (reiser4_is_set(reiser4_get_current_sb(), REISER4_DISCARD)) {
 		blocknr_list_destroy(&atom->discard.delete_set);
-		blocknr_list_destroy(&atom->discard.aux_delete_set);
 	} else {
 		blocknr_set_destroy(&atom->nodiscard.delete_set);
 	}
@@ -3101,7 +3099,6 @@ void atom_dset_merge(txn_atom *from, txn_atom *to)
 {
 	if (reiser4_is_set(reiser4_get_current_sb(), REISER4_DISCARD)) {
 		blocknr_list_merge(&from->discard.delete_set, &to->discard.delete_set);
-		blocknr_list_merge(&from->discard.aux_delete_set, &to->discard.aux_delete_set);
 	} else {
 		blocknr_set_merge(&from->nodiscard.delete_set, &to->nodiscard.delete_set);
 	}
@@ -3155,27 +3152,6 @@ extern int atom_dset_deferred_add_extent(txn_atom *atom,
 	return ret;
 }
 
-extern int atom_dset_immediate_add_extent(txn_atom *atom,
-                                          void **new_entry,
-                                          const reiser4_block_nr *start,
-                                          const reiser4_block_nr *len)
-{
-	int ret;
-
-	if (reiser4_is_set(reiser4_get_current_sb(), REISER4_DISCARD)) {
-		ret = blocknr_list_add_extent(atom,
-		                             &atom->discard.aux_delete_set,
-		                             (blocknr_list_entry**)new_entry,
-		                             start,
-		                             len);
-	} else {
-		/* no-op */
-		ret = 0;
-	}
-
-	return ret;
-}
-
 /*
  * Local variables:
  * c-indentation-style: "K&R"
diff --git a/fs/reiser4/txnmgr.h b/fs/reiser4/txnmgr.h
index 02757a9..05990d8 100644
--- a/fs/reiser4/txnmgr.h
+++ b/fs/reiser4/txnmgr.h
@@ -259,10 +259,6 @@ struct txn_atom {
 			/* The atom's delete set. It collects block numbers which were
 			   deallocated with BA_DEFER, i. e. of ordinary nodes. */
 			struct list_head delete_set;
-
-			/* The atom's auxiliary delete set. It collects block numbers
-			   which were deallocated without BA_DEFER, i. e. immediately. */
-			struct list_head aux_delete_set;
 		} discard;
 	};
 
@@ -527,9 +523,8 @@ extern int blocknr_list_iterator(txn_atom *atom,
 
 /* These are wrappers for accessing and modifying atom's delete lists,
    depending on whether discard is enabled or not.
-   If it is enabled. both deferred and immediate delete lists are maintained,
-   and (less memory efficient) blocknr_lists are used for storage. Otherwise, only
-   deferred delete list is maintained and blocknr_set is used for its storage. */
+   If it is enabled, (less memory efficient) blocknr_list is used for delete
+   list storage. Otherwise, blocknr_set is used for this purpose. */
 extern void atom_dset_init(txn_atom *atom);
 extern void atom_dset_destroy(txn_atom *atom);
 extern void atom_dset_merge(txn_atom *from, txn_atom *to);
@@ -541,10 +536,6 @@ extern int atom_dset_deferred_add_extent(txn_atom *atom,
                                          void **new_entry,
                                          const reiser4_block_nr *start,
                                          const reiser4_block_nr *len);
-extern int atom_dset_immediate_add_extent(txn_atom *atom,
-                                          void **new_entry,
-                                          const reiser4_block_nr *start,
-                                          const reiser4_block_nr *len);
 
 /* flush code takes care about how to fuse flush queues */
 extern void flush_init_atom(txn_atom * atom);
-- 
2.0.2

Re: [PATCHv2 0/3] reiser4: discard support: perform discard before all deallocations.

[Edward Shishkin]

Thanks, looks OK (I haven't tested it yet though..)

I suggest to release a version without garbage collection for now
(just pass the sorted and merged extents to blkdev_issue_discard).

Thanks,
Edward.

On 07/21/2014 08:19 PM, Ivan Shapovalov wrote:
> This applies on top of PATCHv6 of "initial implementation".
> The first patch has been already sent some time ago, but I've included it
> here for sake of clarity.
>
> v2: fix disk space leak in error path, fix comments and commit messages, etc.
>
> Ivan Shapovalov (3):
>    reiser4: fix reiser4_post_{commit,write_back}_hook() and their invocations.
>    reiser4: discard support: use reiser4_post_write_back_hook() for discarding and completing deferred deallocations.
>    reiser4: discard support: perform discard before all deallocations.
>

Re: [PATCHv2 0/3] reiser4: discard support: perform discard before all deallocations.

[Ivan Shapovalov]

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

On Monday 28 July 2014 at 13:46:01, Edward Shishkin wrote:	
> Thanks, looks OK (I haven't tested it yet though..)
> 
> I suggest to release a version without garbage collection for now
> (just pass the sorted and merged extents to blkdev_issue_discard).

Yeah, seems reasonable.
I'll submit v7 of "initial implementation" shortly with simplified discard_extent(),
then we could apply two patchsets and that'll be it.

----
Regarding the "ideal" solution
----

It seems that reiser4_alloc_blocks() can't be told to "allocate this exact
count of blocks or fail". It allocates any number of blocks from 1 to needed,
and actual count is returned in *len.

Is it better to add an "exact" flag to reiser4_blocknr_hint or to add an
"allocate" flag to reiser4_check_blocks()?

-- 
Ivan Shapovalov / intelfx /

> 
> Thanks,
> Edward.
> 
> On 07/21/2014 08:19 PM, Ivan Shapovalov wrote:
> > This applies on top of PATCHv6 of "initial implementation".
> > The first patch has been already sent some time ago, but I've included it
> > here for sake of clarity.
> >
> > v2: fix disk space leak in error path, fix comments and commit messages, etc.
> >
> > Ivan Shapovalov (3):
> >    reiser4: fix reiser4_post_{commit,write_back}_hook() and their invocations.
> >    reiser4: discard support: use reiser4_post_write_back_hook() for discarding and completing deferred deallocations.
> >    reiser4: discard support: perform discard before all deallocations.
> >
> 

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

Re: [PATCHv2 0/3] reiser4: discard support: perform discard before all deallocations.

[Edward Shishkin]

On 07/29/2014 11:29 AM, Ivan Shapovalov wrote:
> On Monday 28 July 2014 at 13:46:01, Edward Shishkin wrote:	
>> Thanks, looks OK (I haven't tested it yet though..)
>>
>> I suggest to release a version without garbage collection for now
>> (just pass the sorted and merged extents to blkdev_issue_discard).
> Yeah, seems reasonable.
> I'll submit v7 of "initial implementation" shortly with simplified discard_extent(),
> then we could apply two patchsets and that'll be it.
>
> ----
> Regarding the "ideal" solution


BTW, do we need this "ideal solution" at all?
For example, my SSD Samsung 840 EVO shows erase_unit = 512 bytes,
and alignment = 0. It means that in my case everything will be discarded
in the best form with the simplified discard_extent().

I am not lazy, just want to make sure we do useful work :)
May be it makes sense to perform a small investigation of the SSD market
first?


> ----
>
> It seems that reiser4_alloc_blocks() can't be told to "allocate this exact
> count of blocks or fail". It allocates any number of blocks from 1 to needed,
> and actual count is returned in *len.
>
> Is it better to add an "exact" flag to reiser4_blocknr_hint or to add an
> "allocate" flag to reiser4_check_blocks()?


Let's decide what we want from check_blocks().

When (discard_offset % block_size != 0) we'll always need (even in the
case of empty headp (tailp) to check the left (right) neighbor block of
the extent. If it is busy, then we'll need to reduce alen (alen -= d_uni)
because of the "shift".

It means that we want check_blocks() to do the following:
. go leftward (rightward) and allocate not more than @count continuous
free blocks;
. return actual number of allocated blocks.

It seems that currently we don't have suitable primitives in our space
allocator. I prefer to not modify existing ones and write a new primitive.

Edward.

Re: [PATCHv2 0/3] reiser4: discard support: perform discard before all deallocations.

[Ivan Shapovalov]

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

On Thursday 31 July 2014 at 23:29:05, Edward Shishkin wrote:	
> 
> On 07/29/2014 11:29 AM, Ivan Shapovalov wrote:
> > On Monday 28 July 2014 at 13:46:01, Edward Shishkin wrote:	
> >> Thanks, looks OK (I haven't tested it yet though..)
> >>
> >> I suggest to release a version without garbage collection for now
> >> (just pass the sorted and merged extents to blkdev_issue_discard).
> > Yeah, seems reasonable.
> > I'll submit v7 of "initial implementation" shortly with simplified discard_extent(),
> > then we could apply two patchsets and that'll be it.
> >
> > ----
> > Regarding the "ideal" solution
> 
> 
> BTW, do we need this "ideal solution" at all?
> For example, my SSD Samsung 840 EVO shows erase_unit = 512 bytes,
> and alignment = 0. It means that in my case everything will be discarded
> in the best form with the simplified discard_extent().
> 
> I am not lazy, just want to make sure we do useful work :)
> May be it makes sense to perform a small investigation of the SSD market
> first?

Erase unit can't be 512 bytes. Well, it can, but this is _highly_ unlikely...
(byte < page < block; writes are per-page, erases are per-block, and
minimal common page size is 512 bytes)

Try to check with `hdparm -I /dev/sdX | grep TRIM`, or, even better, pick an
unused sector, write something to it and try to discard it using
`hdparm --trim-sector-ranges`, then check the sector's contents.

> 
> 
> > ----
> >
> > It seems that reiser4_alloc_blocks() can't be told to "allocate this exact
> > count of blocks or fail". It allocates any number of blocks from 1 to needed,
> > and actual count is returned in *len.
> >
> > Is it better to add an "exact" flag to reiser4_blocknr_hint or to add an
> > "allocate" flag to reiser4_check_blocks()?
> 
> 
> Let's decide what we want from check_blocks().
> 
> When (discard_offset % block_size != 0) we'll always need (even in the
> case of empty headp (tailp) to check the left (right) neighbor block of
> the extent. If it is busy, then we'll need to reduce alen (alen -= d_uni)
> because of the "shift".
> 
> It means that we want check_blocks() to do the following:
> . go leftward (rightward) and allocate not more than @count continuous
> free blocks;
> . return actual number of allocated blocks.
> 
> It seems that currently we don't have suitable primitives in our space
> allocator. I prefer to not modify existing ones and write a new primitive.

Yeah, after another glance through bitmap code this seems to be the cleanest
way. The alloc_blocks_{forward,backward}_bitmap() functions simply convert the
hint to an "internal representation" (search start/end, length min/max) and
call bitmap_alloc_{forward,backward}().

I'd write something like alloc_blocks_*_exact_bitmap(start, min_len, max_len)
eventually calling into the same bitmap_alloc_*() functions. They also can be
reused in the batch discard code (if we decide to implement it). OK?

> 
> Edward.


Thanks,
-- 
Ivan Shapovalov / intelfx /

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