Rebuild related changes based on metadata #28

mynktl · 2018-03-26T16:09:10Z

- API to get modified data from metadata
- Handling of rebuild IO from other replica according to metadata
- Removing old rebuilding code (txg base rebuilding)

- Handling of rebuild IO from other replica according to metadata - Removing code based on txg number Signed-off-by: mayank <mayank.patel@cloudbyte.com>

satbirchhikara

Hi Mayank,

I have not looked in to changes yet. Just want to check if we missing data API will work on given range ?

Idea is that dowgrade replica may pass <io_seq, start offset of scan, len/end offset of scan> to healthy replica,
asking healthy replica to just scan data between and <len/end offset of scan>.

This is required where we want to rebuild a downgraded replica from multiple healthy replicas for different ranges in LUN.

One usecase where it can make sense is assume, a new replica has been added to cluster in that case, instead of rebuilding from one healthy replica, we can partition LUN in smaller ranges/chunks and can rebuild from different healthy replicas.

I am not sure if but if fresh replica added, we may need an optimized way of copying data from healthy replicas to this newly added replica by avoiding some of the checks.

satbirchhikara · 2018-03-27T10:45:43Z

lib/libzpool/uzfs_io.c

-			    len, (void **)&chunk_io);
-			if (!count)
-				goto exit_with_error;
+	if ((zv->zv_status & ZVOL_REBUILDING_IN_PROGRESS) &&


zv_status is a variable to maintain state not flags. For improving readability better to use "=="/"logical equal" to operator.

mynktl · 2018-03-27T12:27:25Z

Hi @satbirchhikara
Current implementation of API doesn't support reading missing data from specified range of volume. I will add this support in API to get missing data from specified range only.

In case of fresh replica, if we allows write IO's in fresh replica then we need to check for overlaps in uzfs_write_data. This may consumes more times since we are allowing writes.
But if we don't allow write IO's in fresh replica then we can use snapshot from healthy replica and then using rebuild, we can get fresh replica to healthy state.

Snapshot created with provided metadata will be removed if (offset + length) == (end of metadata) - Replaced dsl_pool_hold with dmu_object_own to hold snapshot in uzfs_get_io_diff Since, it was causing deadlock - Functionality test for partial read from uzfs_get_io_diff Signed-off-by: mayank <mayank.patel@cloudbyte.com>

gila

Looks good as usual

jkryl · 2018-03-29T10:52:17Z

include/sys/uzfs_zvol.h

 	uint64_t rebuild_bytes;
-	avl_tree_t *incoming_io_tree;   /* incoming io tree */
 } zvol_rebuild_data_t;


should this structure be removed? (there is just one number inside and stale comment)

Thanks for pointing out this.
zvol_rebuild_data_t was meant to hold all rebuild related variables. So better to keep as it is.

As of now, there are two variables we are using for rebuild purpose rebuild_bytes and zv_rebuild_status. I will move zv_rebuild_status to zvol_rebuild_data_t structure.

I will also remove stale comment.

vishnuitta · 2018-03-31T18:08:10Z

lib/libzpool/uzfs_rebuilding.c

+				last_lun_offset = lun_offset;
+			}
+			diff_count++;
+		} else if (!ret) {


this should never happen in our code path

This is not required if the single write has single io number.
I will merge this case with on_disk_io_number > rebuild io number.

vishnuitta · 2018-03-31T18:14:49Z

lib/libzpool/uzfs_rebuilding.c

+		    metadatasize;
+		ret = compare_blk_metadata(rd_metadata, metadata);
+		if (ret == -1) {
+			// old io number < new io number


on_disk io number < incoming IO number

Ok. that makes sense. I will update comment.

vishnuitta · 2018-03-31T18:16:12Z

lib/libzpool/uzfs_rebuilding.c

+		ret = compare_blk_metadata(rd_metadata, metadata);
+		if (ret == -1) {
+			// old io number < new io number
+			if (diff_count == 0) {


no need of curly braces..

vishnuitta · 2018-03-31T18:19:59Z

include/uzfs_rebuilding.h

+ * than w_metadata then that part of IO's will be discarded else it will be
+ * added to list.
+ */
+int uzfs_search_nonoverlapping_io(zvol_state_t *zv, uint64_t offset,


uzfs_get_condensed_old_ondisk_blks..?

vishnuitta · 2018-03-31T18:23:06Z

lib/libzpool/uzfs_io.c

@@ -310,3 +308,52 @@ uzfs_zvol_get_rebuild_status(zvol_state_t *zv)
 {
 	return (zv->zv_rebuild_status);
 }
+
+/*
+ * This assumes snapshot, so, no need of range lock


this doesn't seems to be the case all the time

yes. Actually, range lock for metadata is not needed since all the operation on metadata will go through uzfs_read_data and uzfs_write_data.
In uzfs_get_io_diff, we are calling uzfs_read_metadata but that is on snapshot so no need of range lock.
I will remove range lock for metadata.

vishnuitta · 2018-03-31T18:26:33Z

lib/libzpool/uzfs_io.c

+	uint64_t blocksize = zv->zv_volmetablocksize;
+	uint64_t len_in_first_aligned_block, bytes, read = 0;
+	uint64_t end = offset + len;
+	uint64_t metaobjectsize = P2ALIGN(zv->zv_volsize,


this doesn't seems to be correct.. we need to care of metadatasize also..

yes. I will update this.

vishnuitta · 2018-03-31T18:35:04Z

lib/libzpool/uzfs_io.c

+			bytes = metaobjectsize - offset;
+
+		ret = dmu_read(zv->zv_objset, ZVOL_META_OBJ, offset, bytes,
+		    buf + read, 0);


should we also have a check that we are not writing more than allocated space?

I think - not required.. this should be good

vishnuitta · 2018-03-31T18:43:42Z

lib/libzpool/uzfs_rebuilding.c

+
+int
+uzfs_get_io_diff(zvol_state_t *zv, blk_metadata_t *low,
+    uzfs_get_io_diff_cb_t *func, off_t zvol_offset, size_t zvol_len,


make zvol_offset, zvol_len as lun_offset, lun_len

vishnuitta · 2018-03-31T19:29:38Z

lib/libzpool/uzfs_rebuilding.c

+		}
+
+		strfree(snap_name);
+		ret = dmu_objset_own(dataset, DMU_OST_ANY, B_TRUE, snap_zv,


how about using uzfs_open_dataset_init, which creates and initializes zv..?

yes. we can use it. I will try that and update changes.

vishnuitta · 2018-03-31T19:31:02Z

lib/libzpool/uzfs_rebuilding.c

+	if (end > metaobjectsize)
+		end = metaobjectsize;
+
+	ret = get_metadata_snapshot_info(zv, low, &snap_zv);


get_snapshot_zv

ok. I will make this function generic as get_snapshot_zv(zv, snap_name, snap_zv)

vishnuitta · 2018-03-31T20:11:42Z

lib/libzpool/uzfs_rebuilding.c

+					last_index = i;
+				}
+				diff_count++;
+				if (last_md != NULL &&


check on last_md is not required. add some comments for this logic.

vishnuitta · 2018-03-31T20:21:10Z

include/uzfs_io.h

+/*
+ * API to read metadata
+ */
+extern int uzfs_read_metadata(void *zv, char *buf, uint64_t offset,


make zv as zvol_state_t

as these APIs are being used in production also, having strict check on arguments will be good

vishnuitta · 2018-03-31T20:26:13Z

include/sys/uzfs_zvol.h

-typedef int (uzfs_txg_diff_traverse_cb_t)(off_t offset, size_t len,
-    uint64_t blkid, void *arg);
+typedef int (uzfs_get_io_diff_cb_t)(off_t offset, size_t len,
+    blk_metadata_t *metadata, objset_t *obj, void *arg);


how about passing zvol_state_t and using uzfs_read_data rather than dmu_read in callback implementation?

vishnuitta

overall, looks good.
metaobjectsize issue in uzfs_read_metadata should have caught in unit testing code. probably, higher active data size may be required.

- code changes according to review comments Signed-off-by: mayank <mayank.patel@cloudbyte.com>

vishnuitta · 2018-04-02T13:50:20Z

lib/libzpool/uzfs_io.c

@@ -48,14 +49,9 @@

 #define	WRITE_METADATA(zv, metablk, mdata, tx)			\
 	do {							\
-		rl_t *mrl;					\


this will be good if offset and len are multiple of zv_metavolblocksize. And, uzfs also recevies IOs in same way from tgt.
So, please add VERIFY in read and write APIs for offset & len to be multiple of zv_metavolblocksize

vishnuitta · 2018-04-02T13:52:36Z

lib/libzpool/uzfs_rebuilding.c

+	dataset = kmem_asprintf("%s@%s", strchr(zv->zv_name, '/') + 1,
+	    snap_name);
+
+	ret = uzfs_open_dataset(zv->zv_spa, dataset, snap_zv);


uzfs_open_dataset calls uzfs_open_dataset_init.. please change uzfs_open_dataset_init as uzfs_own_dataset

vishnuitta · 2018-04-02T14:16:30Z

lib/libzpool/uzfs_rebuilding.c

+				// same as last one.
+				diff_count++;
+
+				if (compare_blk_metadata((blk_metadata_t *)


diff_count++ should go into 'else' case, and, last_lun_offset/last_md/last_index need to be set in 'if' after EXECUTE_... macro

vishnuitta · 2018-04-02T14:20:30Z

lib/libzpool/uzfs_rebuilding.c

+			lun_offset += zv->zv_metavolblocksize;
+		}
+
+		if (diff_count) {


it is not needed here

- changes according to review comments - uzfs_open_dataset_init changed to uzfs_own_dataset for better readability Signed-off-by: mayank <mayank.patel@cloudbyte.com>

- setting metavolblocksize to 512 bytes when creating volume through CLI Signed-off-by: mayank <mayank.patel@cloudbyte.com>

vishnuitta

looks good

vishnuitta · 2018-04-05T07:32:11Z

Due to conflicts on this PR, same changes raised in PR31. Hence, closing this.

vishnuitta · 2018-04-05T07:32:33Z

Due to conflicts on this PR, same changes raised in PR31. Hence, closing this.

mynktl requested review from jkryl, gila, vishnuitta and satbirchhikara March 26, 2018 16:09

- API to get modified data from metadata

9b3f6b5

- Handling of rebuild IO from other replica according to metadata - Removing code based on txg number Signed-off-by: mayank <mayank.patel@cloudbyte.com>

mynktl force-pushed the cstor64 branch from c88c527 to 9b3f6b5 Compare March 27, 2018 08:34

satbirchhikara reviewed Mar 27, 2018

View reviewed changes

gila approved these changes Mar 28, 2018

View reviewed changes

jkryl reviewed Mar 29, 2018

View reviewed changes

vishnuitta reviewed Mar 31, 2018

View reviewed changes

mayank and others added 2 commits April 2, 2018 18:27

- resolving conflicts with zfs-0.7-release

f23fb72

- code changes according to review comments Signed-off-by: mayank <mayank.patel@cloudbyte.com>

Merge branch 'zfs-0.7-release' into cstor64

81124e0

vishnuitta reviewed Apr 2, 2018

View reviewed changes

jkryl approved these changes Apr 2, 2018

View reviewed changes

mayank added 3 commits April 3, 2018 12:48

- fix in uzfs_get_io_diff API

4d15b29

- changes according to review comments - uzfs_open_dataset_init changed to uzfs_own_dataset for better readability Signed-off-by: mayank <mayank.patel@cloudbyte.com>

Merge branch 'cstor64' of github.com:mynktl/zfs into cstor64

955f331

- reducing number of tests for rebuilding

38f3c28

- setting metavolblocksize to 512 bytes when creating volume through CLI Signed-off-by: mayank <mayank.patel@cloudbyte.com>

vishnuitta approved these changes Apr 3, 2018

View reviewed changes

vishnuitta closed this Apr 5, 2018

vishnuitta reopened this Apr 5, 2018

vishnuitta closed this Apr 5, 2018

Rebuild related changes based on metadata #28

Rebuild related changes based on metadata #28

Conversation

mynktl commented Mar 26, 2018

satbirchhikara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mynktl commented Mar 27, 2018

gila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishnuitta Mar 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishnuitta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishnuitta Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishnuitta Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishnuitta left a comment

Choose a reason for hiding this comment

vishnuitta commented Apr 5, 2018

vishnuitta commented Apr 5, 2018

vishnuitta Mar 31, 2018 •

edited

Loading

vishnuitta Apr 2, 2018 •

edited

Loading

vishnuitta Apr 2, 2018 •

edited

Loading