rbd: disable reflink while creating XFS filesystems #1257
Conversation
Current versions of the mkfs.xfs binary enable reflink support by
default. This causes problems on systems where the kernel does not
support this feature. When the kernel the feature does not support, but
the filesystem has it enabled, the following error is logged in `dmesg`:
XFS: Superblock has unknown read-only compatible features (0x4) enabled
Introduce a check to see if mkfs.xfs supports the `-m reflink=` option.
In case it does, pass `-m reflink=0` while creating the filesystem.
The check is executed once during the first XFS filesystem creation. The
result of the check is cached until the nodeserver restarts.
Fixes: ceph#966
Signed-off-by: Niels de Vos <ndevos@redhat.com>
|
@humblec PTAL |
| if xfsHasReflink != xfsReflinkUnset { | ||
| return xfsHasReflink == xfsReflinkSupport | ||
| } | ||
|
|
There was a problem hiding this comment.
@nixpanic do we need this whitespace here?
There was a problem hiding this comment.
not required, but it splits two parts of unrelated code:
- check the cached value
- no cache, so test for it
| return true | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
@nixpanic do we need this whitespace here?
There was a problem hiding this comment.
Just clean coding, I think? This is not an else statement that would follow the if
| @@ -84,6 +92,10 @@ var ( | |||
| Backport: true, | |||
| }, // RHEL 8.2 | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
@nixpanic do we need this whitespace here?
There was a problem hiding this comment.
not sure if it is required, but splitting a multi-line variable with an empty line between the next multi-line comment looks clean to me
There was a problem hiding this comment.
so in many places we dont leave a newline before multiline comment and in some places like this we do. Thats what I am trying to understand.
|
@nixpanic my small concern here is, the pattern we follow for especially for whitespace or lines in between . Some reviewers say remove it , we should have some guidelines . Otherwise its going to be messy to read through the code in different sources..etc. @Madhu-1 @ShyamsundarR thoughts? |
Sure , please open an issue to add/update guidelines doc. |
My preference strongly goes to having an empty line between sections of code; basically the place where a comment could be placed for sections that might be non-trivial to understand. Dense code is much more exhausting to read. Splitting code sections by empty lines also makes it a little easier to identify if a section could be split into its own function, making the work for maintainers a little easier. |
I agree with @nixpanic , sections of code should be separated by a line space, allows for better redability of code. |
Describe what this PR does
Current versions of the mkfs.xfs binary enable reflink support by
default. This causes problems on systems where the kernel does not
support this feature. When the kernel the feature does not support, but
the filesystem has it enabled, the following error is logged in
dmesg:Introduce a check to see if mkfs.xfs supports the
-m reflink=option.In case it does, pass
-m reflink=0while creating the filesystem.The check is executed once during the first XFS filesystem creation. The
result of the check is cached until the nodeserver restarts.
Is there anything that requires special attention
By disabling reflink, PVCs with RBD+XFS will be usable on systems that have a kernel that does not support reflink (like CentOS-7). Mixing systems with new XFS (CentOS-8) and old kernels will not be a problem.
Related issues
Fixes: #966
Future concerns
Reflink is a feature that could be useful for some workloads. There is a plan to enable the feature through #1256.