Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup macros to set and retrieve blkptr birth times #15962

Merged
merged 1 commit into from
Mar 25, 2024

Conversation

grwilson
Copy link
Member

@grwilson grwilson commented Mar 4, 2024

There exist a couple of macros that are used to update the blkptr birth times but they can often be confusing. For example, the BP_PHYSICAL_BIRTH() macro will provide either the physical birth time if it is set or else return back the logical birth time. The complement to this macro is BP_SET_BIRTH() which will set the logical birth time and set the physical birth time if they are not the same. Consumers may get confused when they are trying to get the physical birth time and use the BP_PHYSICAL_BIRTH() macro only to find out that the logical birth time is what is actually returned.

This PR cleans up these macros and makes them symmetrical. The same functionally is preserved but the name is changed. Consumers that used to call BP_PHYSICAL_BIRTH() will now call BP_GET_BIRTH(). In additional to cleaning up this naming conventions, two new sets of macros are introduced -- BP_[SET|GET]LOGICAL_BIRTH() and BP[SET|GET]_PHYSICAL_BIRTH. These new macros allow the consumer to get and set the specific birth time.

@grwilson grwilson force-pushed the blkptr_birth branch 2 times, most recently from a1b75ae to 2cecdbb Compare March 11, 2024 15:37
@amotin
Copy link
Member

amotin commented Mar 11, 2024

This is a backwards compatible change which does not require a feature flag since those upper bits are all zero.

It is compatible only until the upper bits are reused for something later. After that only systems having this code will be able to read the pool. Depending on the nature of these later changes I wonder if it could make sense to introduce feature flag here to avoid it or at least reduce transition effects later. Otherwise I am not sure what is the befit of freeing space now, if we'll need pretty strict feature flag for it later any way. We could just make the later feature(s) to cut the space from the birth time on as needed basis instead of reserving abstract 2x16bit now.

@grwilson
Copy link
Member Author

We can leave the upper 16-bits in place and just provide the macros so that we have a consistent way of manipulating the blkptr birth times. This would also be helpful to my future work.

As you mentioned, once the new feature is ready, a determination of the feature flag would be necessary. I'm not sure if others features are looking at re-purposing the upper bits of the birth time and that's why I wanted to start this discussion.

@grwilson grwilson marked this pull request as ready for review March 11, 2024 18:20
module/zfs/brt.c Outdated Show resolved Hide resolved
@amotin
Copy link
Member

amotin commented Mar 11, 2024

We can leave the upper 16-bits in place and just provide the macros so that we have a consistent way of manipulating the blkptr birth times. This would also be helpful to my future work.

Sounds good to me.

@grwilson grwilson force-pushed the blkptr_birth branch 2 times, most recently from 7c3cae3 to 6774446 Compare March 13, 2024 12:50
@grwilson grwilson changed the title Reduce blkptr logical and physical birth to 48 bits Cleanup macros to set and retrieve blkptr birth times Mar 13, 2024
@grwilson
Copy link
Member Author

I have updated the PR to only provide the macro changes. The changes to reduce the birth time fields in the blkptr pointer have been removed.

Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good, but I found few places to pick on, or at least to think. :)

module/zfs/bpobj.c Show resolved Hide resolved
include/sys/spa.h Outdated Show resolved Hide resolved
module/zfs/vdev_raidz.c Outdated Show resolved Hide resolved
module/zfs/zio.c Outdated Show resolved Hide resolved
module/zfs/zio.c Outdated Show resolved Hide resolved
module/zfs/metaslab.c Outdated Show resolved Hide resolved
include/sys/spa.h Outdated Show resolved Hide resolved
Copy link
Contributor

@mmaybee mmaybee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. You might mention in the description that, along the way, you are removing the unused GRID macros and changing the comments to note that these bits in the bp are currently unused (pad).

@grwilson grwilson force-pushed the blkptr_birth branch 2 times, most recently from 6a32feb to 89a4843 Compare March 21, 2024 03:18
@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Mar 21, 2024
@grwilson grwilson force-pushed the blkptr_birth branch 2 times, most recently from fc05612 to d3dbf17 Compare March 25, 2024 13:13
There exist a couple of macros that are used to update the blkptr birth
times but they can often be confusing. For example, the
BP_PHYSICAL_BIRTH() macro will provide either the physical birth time
if it is set or else return back the logical birth time. The
complement to this macro is BP_SET_BIRTH() which will set the logical
birth time and set the physical birth time if they are not the same.
Consumers may get confused when they are trying to get the physical
birth time and use the BP_PHYSICAL_BIRTH() macro only to find out that
the logical birth time is what is actually returned.

This change cleans up these macros and makes them symmetrical. The same
functionally is preserved but the name is changed. Instead of calling
BP_PHYSICAL_BIRTH(), consumer can now call BP_GET_BIRTH(). In
additional to cleaning up this naming conventions, two new sets of
macros are introduced -- BP_[SET|GET]_LOGICAL_BIRTH() and
BP_[SET|GET]_PHYSICAL_BIRTH.  These new macros allow the consumer to
get and set the specific birth time.

As part of the cleanup, the unused GRID macros have been removed and
that portion of the blkptr are currently unused.

Signed-off-by: George Wilson <[email protected]>
@behlendorf behlendorf merged commit 493fcce into openzfs:master Mar 25, 2024
19 checks passed
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 4, 2024
There exist a couple of macros that are used to update the blkptr birth
times but they can often be confusing. For example, the
BP_PHYSICAL_BIRTH() macro will provide either the physical birth time
if it is set or else return back the logical birth time. The
complement to this macro is BP_SET_BIRTH() which will set the logical
birth time and set the physical birth time if they are not the same.
Consumers may get confused when they are trying to get the physical
birth time and use the BP_PHYSICAL_BIRTH() macro only to find out that
the logical birth time is what is actually returned.

This change cleans up these macros and makes them symmetrical. The same
functionally is preserved but the name is changed. Instead of calling
BP_PHYSICAL_BIRTH(), consumer can now call BP_GET_BIRTH(). In
additional to cleaning up this naming conventions, two new sets of
macros are introduced -- BP_[SET|GET]_LOGICAL_BIRTH() and
BP_[SET|GET]_PHYSICAL_BIRTH.  These new macros allow the consumer to
get and set the specific birth time.

As part of the cleanup, the unused GRID macros have been removed and
that portion of the blkptr are currently unused.

Reviewed-by: Matthew Ahrens <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: Mark Maybee <[email protected]>
Signed-off-by: George Wilson <[email protected]>
Closes openzfs#15962
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants