update major=2 layout w/o flog

[guoanwu]

This change is 

[lplewa]

Please squash your commits (https://pmem.io/blog/2014/09/git-workflow/)
Also please provide a description of your change, so we can understand your patch better.

[codecov]

Codecov Report

Merging #5444 (9e0aeb2) into master (0fb5df0) will decrease coverage by 0.14%.
The diff coverage is 41.44%.

@@            Coverage Diff             @@
##           master    #5444      +/-   ##
==========================================
- Coverage   72.16%   72.01%   -0.15%     
==========================================
  Files         193      193              
  Lines       30327    30454     +127     
==========================================
+ Hits        21885    21932      +47     
- Misses       8442     8522      +80     

[guoanwu]

Great, I will follow and update. Thanks!

[guoanwu]

still keep the default major as 1 and add some test for use the pmemspoil to change the version and check the DEBUG log output and see if it has switch to the major version 2.

[lplewa]

Please revert DML/DSA changes from this PR.
It will be much easier to review and merge your layout changes, and then discuss DSA enablement in libpmemblk

[guoanwu]

got it, i already revert back. Thank you.

[lplewa]

Please fix commit author field (for some commits author is "root")

Reviewable status: 0 of 5 files reviewed, 8 unresolved discussions (waiting on @guoanwu and @lplewa)

---

src/libpmemblk/btt.c line 208 at r7 (raw file):

		/* os_spinlock_t list_lock; */
		os_mutex_t list_lock;

Mutex or spinlock?

Code quote:

		/* os_spinlock_t list_lock; */
		os_mutex_t list_lock;

---

src/libpmemblk/btt.c line 801 at r7 (raw file):

}

static int btt_map_read(struct btt *bttp, struct arena *arena,

Please add a function description

---

src/libpmemblk/btt.c line 822 at r7 (raw file):

}

static int btt_freelist_init(struct btt *bttp, struct arena *arena)

Please add a function description

---

src/libpmemblk/btt.c line 838 at r7 (raw file):

	uint32_t aba_map_size = (arena->internal_nlba>>3) + 1;
	aba_map = Zalloc(aba_map_size);
	if ((!aba_map)) {

Double ()

---

src/libpmemblk/btt.c line 1709 at r7 (raw file):

	/*
	 * Read the current map entry to get the post-map LBA for the data
	 * block read.

As you copied this logic to the btt_map_read() please use this function here.

Code quote:

	/*
	 * Read the current map entry to get the post-map LBA for the data
	 * block read.
	 */
	uint32_t entry;

	/* convert pre-map LBA into an offset into the map */
	map_entry_off = arenap->mapoff + BTT_MAP_ENTRY_SIZE * premap_lba;

	if ((*bttp->ns_cbp->nsread)(bttp->ns, lane, &entry,
				sizeof(entry), map_entry_off) < 0)
		return -1;

	entry = le32toh(entry);

---

src/libpmemblk/btt.c line 1934 at r7 (raw file):

		LOG(3, "free_entry %u (before mask %u)", free_entry,
			arenap->flogs[lane].flog.old_map);
		} else {

coding style

---

src/libpmemblk/btt.c line 1950 at r7 (raw file):

	uint64_t data_block_off = arenap->dataoff +
		(uint64_t)(free_entry & BTT_MAP_ENTRY_LBA_MASK) *
			arenap->internal_lbasize;

please do not include random whitespace changes.

---

src/libpmemblk/btt.c line 2146 at r7 (raw file):

		/* this is an uninitialized map entry, set the default value */
		if (map_entry_is_initial(entry)) {
			if (arenap->major == 1) entry = i;

coding style

[guoanwu]

uint64_t data_block_off = arenap->dataoff +

updated and using the btt_map_read APIs to be more clear and update the coding style issues.

[guoanwu]

Please fix commit author field (for some commits author is "root")

Reviewable status: 0 of 5 files reviewed, 8 unresolved discussions (waiting on @guoanwu and @lplewa)

src/libpmemblk/btt.c line 208 at r7 (raw file):

		/* os_spinlock_t list_lock; */
		os_mutex_t list_lock;

Mutex or spinlock?

Code quote:

		/* os_spinlock_t list_lock; */
		os_mutex_t list_lock;

src/libpmemblk/btt.c line 801 at r7 (raw file):

}

static int btt_map_read(struct btt *bttp, struct arena *arena,

Please add a function description

src/libpmemblk/btt.c line 822 at r7 (raw file):

}

static int btt_freelist_init(struct btt *bttp, struct arena *arena)

Please add a function description

src/libpmemblk/btt.c line 838 at r7 (raw file):

	uint32_t aba_map_size = (arena->internal_nlba>>3) + 1;
	aba_map = Zalloc(aba_map_size);
	if ((!aba_map)) {

Double ()

src/libpmemblk/btt.c line 1709 at r7 (raw file):

	/*
	 * Read the current map entry to get the post-map LBA for the data
	 * block read.

As you copied this logic to the btt_map_read() please use this function here.

Code quote:

	/*
	 * Read the current map entry to get the post-map LBA for the data
	 * block read.
	 */
	uint32_t entry;

	/* convert pre-map LBA into an offset into the map */
	map_entry_off = arenap->mapoff + BTT_MAP_ENTRY_SIZE * premap_lba;

	if ((*bttp->ns_cbp->nsread)(bttp->ns, lane, &entry,
				sizeof(entry), map_entry_off) < 0)
		return -1;

	entry = le32toh(entry);

src/libpmemblk/btt.c line 1934 at r7 (raw file):

		LOG(3, "free_entry %u (before mask %u)", free_entry,
			arenap->flogs[lane].flog.old_map);
		} else {

coding style

src/libpmemblk/btt.c line 1950 at r7 (raw file):

	uint64_t data_block_off = arenap->dataoff +
		(uint64_t)(free_entry & BTT_MAP_ENTRY_LBA_MASK) *
			arenap->internal_lbasize;

please do not include random whitespace changes.

src/libpmemblk/btt.c line 2146 at r7 (raw file):

		/* this is an uninitialized map entry, set the default value */
		if (map_entry_is_initial(entry)) {
			if (arenap->major == 1) entry = i;

coding style

done

[guoanwu]

@lplewa , the review change already finished, please review.

[kswiecicki]

Reviewed 1 of 3 files at r3, 3 of 4 files at r4, 5 of 6 files at r9, all commit messages.
Reviewable status: 9 of 10 files reviewed, 8 unresolved discussions (waiting on @guoanwu, @kswiecicki, and @lplewa)

---

src/libpmemblk/btt.h line 2 at r11 (raw file):

/* SPDX-License-Identifier: BSD-3-Clause */
/* Copyright 2014-2022, Intel Corporation */

The copyright date shouldn't be changed when there's no other change to the file.

Code quote:

/* Copyright 2014-2022, Intel Corporation */

---

src/libpmemblk/btt.c line 816 at r11 (raw file):

	 * no operation on the freelist
	 * free the freelist.free_array
	 */

I think this would sound better:
free_num = 0 means that the whole data block has been used up. In that case, free the freelist.free_array.

Code quote:

	/*
	 * if free_num = 0, means all data block been written
	 * no operation on the freelist
	 * free the freelist.free_array
	 */

---

src/libpmemblk/btt.c line 844 at r11 (raw file):

#endif

	uint32_t aba_map_size = (arena->internal_nlba>>3) + 1;

Could you add a comment explaining why is arena->internal_nlba being shifted by 3 particuralrly?

Code quote:

(arena->internal_nlba>>3) + 1;

---

src/libpmemblk/btt.c line 854 at r11 (raw file):

	 * prepare the aba_map, each aba will be in a bit.
	 * occupied bit=1, free bit=0.
	 * the scan will take times, once execution during initilization.

Describe what the scan is looking for.

Code quote:

the scan will take times, once execution during initilization.

---

src/libpmemblk/btt.c line 875 at r11 (raw file):

	/*
	 * Scan the aba_bitmap , use the static array, that will take 1% memory.

One space too many.

Code quote:

Scan the aba_bitmap , use

---

src/libpmemblk/btt_layout.h line 64 at r11 (raw file):

 */
/*
 * #define BTTINFO_MAJOR_VERSION 2

The major version is still defined as 1.

Code quote:

/*
 * #define BTTINFO_MAJOR_VERSION 2
 */
#define BTTINFO_MAJOR_VERSION 1
#define BTTINFO_MINOR_VERSION 1

[guoanwu]

Reviewed 1 of 3 files at r3, 3 of 4 files at r4, 5 of 6 files at r9, all commit messages.
Reviewable status: 9 of 10 files reviewed, 8 unresolved discussions (waiting on @guoanwu, @kswiecicki, and @lplewa)

src/libpmemblk/btt.h line 2 at r11 (raw file):

/* SPDX-License-Identifier: BSD-3-Clause */
/* Copyright 2014-2022, Intel Corporation */

The copyright date shouldn't be changed when there's no other change to the file.

Code quote:

/* Copyright 2014-2022, Intel Corporation */

src/libpmemblk/btt.c line 816 at r11 (raw file):

	 * no operation on the freelist
	 * free the freelist.free_array
	 */

I think this would sound better: free_num = 0 means that the whole data block has been used up. In that case, free the freelist.free_array.

Code quote:

	/*
	 * if free_num = 0, means all data block been written
	 * no operation on the freelist
	 * free the freelist.free_array
	 */

src/libpmemblk/btt.c line 844 at r11 (raw file):

#endif

	uint32_t aba_map_size = (arena->internal_nlba>>3) + 1;

Could you add a comment explaining why is arena->internal_nlba being shifted by 3 particuralrly?

Code quote:

(arena->internal_nlba>>3) + 1;

src/libpmemblk/btt.c line 854 at r11 (raw file):

	 * prepare the aba_map, each aba will be in a bit.
	 * occupied bit=1, free bit=0.
	 * the scan will take times, once execution during initilization.

Describe what the scan is looking for.

Code quote:

the scan will take times, once execution during initilization.

src/libpmemblk/btt.c line 875 at r11 (raw file):

	/*
	 * Scan the aba_bitmap , use the static array, that will take 1% memory.

One space too many.

Code quote:

Scan the aba_bitmap , use

src/libpmemblk/btt_layout.h line 64 at r11 (raw file):

 */
/*
 * #define BTTINFO_MAJOR_VERSION 2

The major version is still defined as 1.

Code quote:

/*
 * #define BTTINFO_MAJOR_VERSION 2
 */
#define BTTINFO_MAJOR_VERSION 1
#define BTTINFO_MINOR_VERSION 1

Since the layout has changed. So far some unit test to check the layout and will failure. you can just change the major version here to enable the new algorithm. And we see a pmemspoil tool i think that can change the major version directly from the super block, that can switch to the new algorithm seamless.

[guoanwu]

about the copy right, if not change, cstyle can't pass.
@lplewa , can you help on this.

[guoanwu]

please review and check the PR to move the PR forward.