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

[bug:1193929] GlusterFS can be improved #1000

Closed
gluster-ant opened this issue Mar 12, 2020 · 1,017 comments · Fixed by #1765, #2980, #2992 or #3014
Closed

[bug:1193929] GlusterFS can be improved #1000

gluster-ant opened this issue Mar 12, 2020 · 1,017 comments · Fixed by #1765, #2980, #2992 or #3014
Labels
Migrated release 7 release 8 release 8 release-6 Use this label for the release 6 backports Type:Bug

Comments

@gluster-ant
Copy link
Collaborator

URL: https://bugzilla.redhat.com/1193929
Creator: jdarcy at redhat
Time: 20150218T15:06:47

I hope this bug is never fixed.

The upstream patch process requires that each patch have an associated bug ID before it can be merged. However, there is no requirement that the bug contain any information or receive any kind of signoff before the patch can proceed. As a result, many of our developers have the habit of creating such "placeholder" bugs every time they want to make a change, even if it's just a random cleanup/idea and not an actual bug fix or requested/tracked feature request. Also, any patch with the dreaded "rfc" bug ID (which would be appropriate for such changes) is unlikely to be reviewed.

This bug exists to satisfy our process requirement, without the additional negatives of clogging up our triage/tracking processes and making it appear that the code has more bugs (which would be bad) when in fact a developer had more ideas (which is generally good).

@gluster-ant
Copy link
Collaborator Author

Time: 20160727T09:30:29
vbellur at redhat commented:
REVIEW: http://review.gluster.org/14638 (tier: Suppress warning when tiering is disabled) posted (#2) for review on master by Prashanth Pai ([email protected])

@gluster-ant
Copy link
Collaborator Author

Time: 20160727T09:31:27
vbellur at redhat commented:
REVIEW: http://review.gluster.org/14638 (tier: Suppress warning when tiering is disabled) posted (#3) for review on master by Prashanth Pai ([email protected])

@gluster-ant
Copy link
Collaborator Author

Time: 20160727T10:40:53
vbellur at redhat commented:
REVIEW: http://review.gluster.org/11922 (README: improve readability and add a clickable link) posted (#5) for review on master by Niels de Vos ([email protected])

@gluster-ant
Copy link
Collaborator Author

Time: 20160727T11:12:09
vbellur at redhat commented:
COMMIT: http://review.gluster.org/14638 committed in master by Jeff Darcy ([email protected])

commit 9828029
Author: Prashanth Pai [email protected]
Date: Mon May 30 11:56:17 2016 +0530

tier: Suppress warning when tiering is disabled

Suppress -Wunused-function compile time warnings when tiering is
disabled with --disable-tiering.

BUG: 1193929
Change-Id: I396e03631606ce60a953ed5e124986ae2c803abd
Signed-off-by: Prashanth Pai <[email protected]>
Reviewed-on: http://review.gluster.org/14638
NetBSD-regression: NetBSD Build System <[email protected]>
CentOS-regression: Gluster Build System <[email protected]>
Smoke: Gluster Build System <[email protected]>
Reviewed-by: Jeff Darcy <[email protected]>
Tested-by: Gluster Build System <[email protected]>

@gluster-ant
Copy link
Collaborator Author

Time: 20160727T11:15:19
vbellur at redhat commented:
COMMIT: http://review.gluster.org/11922 committed in master by Jeff Darcy ([email protected])

commit dbc8dac
Author: Niels de Vos [email protected]
Date: Fri Aug 14 11:13:54 2015 +0200

README: improve readability and add a clickable link

It is easier to have a link that can be clicked.

Change-Id: Id0f75b3e68ca358c218e7f1f00769545dab0c058
BUG: 1193929
Signed-off-by: Niels de Vos <[email protected]>
Reviewed-on: http://review.gluster.org/11922
NetBSD-regression: NetBSD Build System <[email protected]>
CentOS-regression: Gluster Build System <[email protected]>
Smoke: Gluster Build System <[email protected]>
Reviewed-by: Jeff Darcy <[email protected]>

@gluster-ant
Copy link
Collaborator Author

Time: 20170410T10:22:26
bugzilla-bot at gluster.org commented:
REVIEW: https://review.gluster.org/16200 (glusterd: Propagate EADDRINUSE correctly to parent process) posted (#6) for review on master by Prashanth Pai ([email protected])

@gluster-ant
Copy link
Collaborator Author

Time: 20170413T03:49:06
bugzilla-bot at gluster.org commented:
COMMIT: https://review.gluster.org/16200 committed in master by Atin Mukherjee ([email protected])

commit 94afe2c
Author: Prashanth Pai [email protected]
Date: Mon Dec 19 16:28:06 2016 +0530

glusterd: Propagate EADDRINUSE correctly to parent process

exit()/_exit():
Only the least significant 8 bits i.e (err & 255) shall be available
to the waiting parent process on calling _exit() or exit() with an
integer exit status. If this number is negative, the parent process
doesn't readily get what it's really looking forward to handle.

For example: EADDRINUSE is 98 and if exit status code is set to -98,
the waiting parent process shall get 158 (= -98 & 255) as exit status.

BUG: 1193929

Change-Id: Idc6b0f40c2332e087e584b4b40cbf0d29168c9cd
Signed-off-by: Prashanth Pai <[email protected]>
Reviewed-on: https://review.gluster.org/16200
NetBSD-regression: NetBSD Build System <[email protected]>
Reviewed-by: Amar Tumballi <[email protected]>
CentOS-regression: Gluster Build System <[email protected]>
Smoke: Gluster Build System <[email protected]>
Reviewed-by: Atin Mukherjee <[email protected]>

@gluster-ant
Copy link
Collaborator Author

Time: 20170427T13:05:43
bugzilla-bot at gluster.org commented:
REVIEW: https://review.gluster.org/17129 (glusterd: Fix removing pmap entry on rpc disconnect) posted (#1) for review on master by Prashanth Pai ([email protected])

@gluster-ant
Copy link
Collaborator Author

Time: 20170427T13:07:44
bugzilla-bot at gluster.org commented:
REVIEW: https://review.gluster.org/17129 (glusterd: Fix removing pmap entry on rpc disconnect) posted (#2) for review on master by Prashanth Pai ([email protected])

@gluster-ant
Copy link
Collaborator Author

Time: 20170428T17:15:33
bugzilla-bot at gluster.org commented:
COMMIT: https://review.gluster.org/17129 committed in master by Jeff Darcy ([email protected])

commit 081f9fe
Author: Prashanth Pai [email protected]
Date: Thu Apr 27 18:26:02 2017 +0530

glusterd: Fix removing pmap entry on rpc disconnect

Problem:
The following line of code intended to remove pmap entry for the
connection during disconnects:

    pmap_registry_remove (this, 0, NULL, GF_PMAP_PORT_NONE, xprt);

However, no pmap entry will have it's type set to GF_PMAP_PORT_NONE
at any point in time. So a call to pmap_registry_search_by_xprt() in
pmap_registry_remove() will always fail to find a match.

Fix:
Optionally ignore pmap entry's type in pmap_registry_search_by_xprt().

BUG: 1193929
Change-Id: I705f101739ab1647ff52a92820d478354407264a
Signed-off-by: Prashanth Pai <[email protected]>
Reviewed-on: https://review.gluster.org/17129
Smoke: Gluster Build System <[email protected]>
NetBSD-regression: NetBSD Build System <[email protected]>
CentOS-regression: Gluster Build System <[email protected]>
Reviewed-by: Jeff Darcy <[email protected]>

@gluster-ant
Copy link
Collaborator Author

Time: 20170630T13:20:14
bugzilla-bot at gluster.org commented:
REVIEW: https://review.gluster.org/17659 (Link against missed libraries to resolve symbols) posted (#1) for review on master by Prashanth Pai ([email protected])

@gluster-ant
Copy link
Collaborator Author

Time: 20170630T17:59:53
bugzilla-bot at gluster.org commented:
REVIEW: https://review.gluster.org/17659 (Link against missed libraries to resolve symbols) posted (#2) for review on master by Prashanth Pai ([email protected])

@gluster-ant
Copy link
Collaborator Author

Time: 20170703T10:58:18
bugzilla-bot at gluster.org commented:
COMMIT: https://review.gluster.org/17659 committed in master by Jeff Darcy ([email protected])

commit 97a0869
Author: Prashanth Pai [email protected]
Date: Fri Jun 30 15:52:53 2017 +0530

Link against missed libraries to resolve symbols

When external programs perform a dlopen("..so", RTLD_LAZY|RTLD_LOCAL)
on some shared objects like xlators, it can fail with dlerror set to
error string "undefined symbol <some-type>".

This was observed for the following shared objects: fuse.so, quota.so,
quotad.so, server.so, libgfrpc.so and socket.so

P.S: This was found while running a go program which fetches the list
of xlator options (volume_option_t) from xlator's shared object.

BUG: 1193929
Change-Id: I7b958409cf11fb67c2be32a3f85a96fb1260236b
Signed-off-by: Prashanth Pai <[email protected]>
Reviewed-on: https://review.gluster.org/17659
Smoke: Gluster Build System <[email protected]>
Reviewed-by: Amar Tumballi <[email protected]>
CentOS-regression: Gluster Build System <[email protected]>
NetBSD-regression: NetBSD Build System <[email protected]>
Reviewed-by: Jeff Darcy <[email protected]>

@gluster-ant
Copy link
Collaborator Author

Time: 20170712T07:38:51
bugzilla-bot at gluster.org commented:
REVIEW: https://review.gluster.org/17753 (socket: avoid calling GF_CALLOC() in init_openssl_mt()) posted (#1) for review on master by Prashanth Pai ([email protected])

@gluster-ant
Copy link
Collaborator Author

Time: 20170713T05:58:26
bugzilla-bot at gluster.org commented:
REVIEW: https://review.gluster.org/17753 (socket: avoid calling GF_CALLOC() in init_openssl_mt()) posted (#2) for review on master by Prashanth Pai ([email protected])

@gluster-ant
Copy link
Collaborator Author

Time: 20170713T13:50:11
bugzilla-bot at gluster.org commented:
REVIEW: https://review.gluster.org/17753 (socket: call init_openssl_mt() in init() and add cleanup) posted (#3) for review on master by Prashanth Pai ([email protected])

@gluster-ant
Copy link
Collaborator Author

Time: 20170714T12:38:34
bugzilla-bot at gluster.org commented:
REVIEW: https://review.gluster.org/17753 (socket: call init_openssl_mt() in init() and add cleanup) posted (#4) for review on master by Prashanth Pai ([email protected])

@gluster-ant
Copy link
Collaborator Author

Time: 20170717T17:09:09
bugzilla-bot at gluster.org commented:
COMMIT: https://review.gluster.org/17753 committed in master by Jeff Darcy ([email protected])

commit 58a15ae
Author: Prashanth Pai [email protected]
Date: Wed Jul 12 12:59:35 2017 +0530

socket: call init_openssl_mt() in init() and add cleanup

init_openssl_mt() wasn't explicitly invoked and was run implicitly before
dlopen() returned as it was tagged as __attribute__ ((constructor)). This
function used to call GF_CALLOC() which wasn't available or initialized
when socket.so is dlopen()ed by an external program. The program used to
crash with SIGSEGV as follows:

0x00007ffff5efe1ad in __gf_calloc (nmemb=41, size=40, type=158, typestr=0x7ffff63eb3d6 "gf_sock_mt_lock_array")
at mem-pool.c:109
0x00007ffff63e6acf in init_openssl_mt () at socket.c:4016
0x00007ffff7de90da in call_init.part () from /lib64/ld-linux-x86-64.so.2
0x00007ffff7de91eb in _dl_init () from /lib64/ld-linux-x86-64.so.2
0x00007ffff7dedde1 in dl_open_worker () from /lib64/ld-linux-x86-64.so.2
0x00007ffff7de8f84 in _dl_catch_error () from /lib64/ld-linux-x86-64.so.2
0x00007ffff7ded339 in _dl_open () from /lib64/ld-linux-x86-64.so.2

This change moves call to init_openssl_mt() from being a constructor function
to the init() function and introduces fini_openssl_mt() which cleans up
resources (called in destructor).

BUG: 1193929
Change-Id: Iab690897ec34e24c33f6b43f8d8d9f8fd75ac607
Signed-off-by: Prashanth Pai <[email protected]>
Reviewed-on: https://review.gluster.org/17753
Smoke: Gluster Build System <[email protected]>
CentOS-regression: Gluster Build System <[email protected]>
Reviewed-by: Amar Tumballi <[email protected]>
Reviewed-by: Jeff Darcy <[email protected]>

@gluster-ant
Copy link
Collaborator Author

Time: 20170721T10:21:47
bugzilla-bot at gluster.org commented:
REVIEW: https://review.gluster.org/17848 (rpc: prevent logging null client [Invalid argument]) posted (#1) for review on master by Prashanth Pai ([email protected])

@gluster-ant
Copy link
Collaborator Author

Time: 20170721T10:52:37
bugzilla-bot at gluster.org commented:
REVIEW: https://review.gluster.org/17848 (rpc: prevent logging null client [Invalid argument]) posted (#2) for review on master by Prashanth Pai ([email protected])

@gluster-ant
Copy link
Collaborator Author

Time: 20170725T06:28:59
bugzilla-bot at gluster.org commented:
REVIEW: https://review.gluster.org/17848 (rpc: prevent logging null client [Invalid argument]) posted (#3) for review on master by Prashanth Pai ([email protected])

@gluster-ant
Copy link
Collaborator Author

Time: 20170725T12:47:11
bugzilla-bot at gluster.org commented:
COMMIT: https://review.gluster.org/17848 committed in master by Jeff Darcy ([email protected])

commit 7140858
Author: Prashanth Pai [email protected]
Date: Fri Jul 21 15:48:23 2017 +0530

rpc: prevent logging null client [Invalid argument]

The following log entry is observed during volume operations such
as volume create:

[2017-07-20 05:13:43.213797] E [client_t.c:321:gf_client_ref]
(-->/usr/local/lib/libgfrpc.so.0(rpcsvc_request_create+0x1a4) [0x7f987f66cd20]
-->/usr/local/lib/libgfrpc.so.0(rpcsvc_request_init+0xd0) [0x7f987f66ca23]
-->/usr/local/lib/libglusterfs.so.0(gf_client_ref+0x56) [0x7f987f91cbd5] )
0-client_t: null client [Invalid argument]

Change-Id: I49ba753e8d1a828bb275b0ccb1a181706774f387
BUG: 1193929
Signed-off-by: Prashanth Pai <[email protected]>
Reviewed-on: https://review.gluster.org/17848
Reviewed-by: Raghavendra G <[email protected]>
Reviewed-by: Amar Tumballi <[email protected]>
Tested-by: Amar Tumballi <[email protected]>
Smoke: Gluster Build System <[email protected]>
CentOS-regression: Gluster Build System <[email protected]>

@gluster-ant
Copy link
Collaborator Author

Time: 20171017T05:56:22
bugzilla-bot at gluster.org commented:
REVIEW: https://review.gluster.org/18535 (worm/read-only: Add sentinel NULL key to options) posted (#1) for review on master by Prashanth Pai ([email protected])

@gluster-ant
Copy link
Collaborator Author

Time: 20171017T11:09:59
bugzilla-bot at gluster.org commented:
COMMIT: https://review.gluster.org/18535 committed in master by Prashanth Pai ([email protected])

commit 4f3c680
Author: Prashanth Pai [email protected]
Date: Tue Oct 17 10:57:13 2017 +0530

worm/read-only: Add sentinel NULL key to options

BUG: 1193929
Change-Id: Ibfee382362826556e2e760f9b946c83445d6a08e
Signed-off-by: Prashanth Pai <[email protected]>

@gluster-ant
Copy link
Collaborator Author

Time: 20171227T07:07:44
bugzilla-bot at gluster.org commented:
REVIEW: https://review.gluster.org/19056 (Use RTLD_LOCAL for symbol resolution) posted (#3) for review on master by Prashanth Pai

@gluster-ant
Copy link
Collaborator Author

Time: 20171227T15:50:20
bugzilla-bot at gluster.org commented:
COMMIT: https://review.gluster.org/19056 committed in master by "Prashanth Pai" [email protected] with a commit message- Use RTLD_LOCAL for symbol resolution

RTLD_LOCAL is the default value for symbol visibility flag of dlopen()
in Linux and NetBSD. Using it avoids conflicts during symbol resolution.

This also allows us to detect xlators that have not been explicitly
linked with libraries that they use. This used to go unnoticed
when RTLD_GLOBAL was being used.

BUG: 1193929
Change-Id: I50db6ea14ffdee96596060c4d6bf71cd3c432f7b
Signed-off-by: Prashanth Pai [email protected]

@gluster-ant
Copy link
Collaborator Author

Time: 20180129T08:58:41
bugzilla-bot at gluster.org commented:
REVIEW: https://review.gluster.org/19339 (libglusterfs: cleanup useless need_lookup) posted (#3) for review on master by Niels de Vos

@gluster-ant
Copy link
Collaborator Author

Time: 20180207T05:10:09
bugzilla-bot at gluster.org commented:
REVIEW: https://review.gluster.org/19517 (log: Change some frequent log entries to DEBUG) posted (#1) for review on master by Prashanth Pai

Shwetha-Acharya pushed a commit that referenced this issue Mar 27, 2023
On some systems, the 'char' type is interpreted as an unsigned char.
This may cause some issues as Gluster code assumes that 'char' is
signed.

This patch adds the '-fsigned-char' option during compilation to make
sure it works as expected.

Updates: #1000

Signed-off-by: Xavi Hernandez <[email protected]>
mykaul added a commit to mykaul/glusterfs that referenced this issue Apr 29, 2024
- posix_readdirp_fill() does not need to copy or zero a gfid on every iteration:
use an existing one or a pre-zero'ed one.
- Make some functions static.
- A bit of code restructure.

Updates: gluster#1000
Signed-off-by: Yaniv Kaul <[email protected]>
mykaul added a commit to mykaul/glusterfs that referenced this issue May 24, 2024
In some cases, we can return early - when iatt structure is NULL.
Specifically, this happens when we call those functions in part of
MAKE_INODE_HANDLE() macro - which in some cases is not using that structure.

This saves us few calls. Specifically, we do not need to fetch mdata xattr, which may save
us from some lgetxattr() and such.

Updates: gluster#1000
Signed-off-by: Yaniv Kaul <[email protected]>
mykaul added a commit to mykaul/glusterfs that referenced this issue May 24, 2024
…art with zero's

uuid_compare (https://github.com/cloudbase/libuuid/blob/master/compare.c ) can be skipped,
if the UUID doesn't start with all zero's.

Updates: gluster#1000
Signed-off-by: Yaniv Kaul <[email protected]>
mykaul added a commit to mykaul/glusterfs that referenced this issue May 24, 2024
mykaul added a commit to mykaul/glusterfs that referenced this issue May 24, 2024
Make it more readable
Remove unused parameters
Reduce redunandat work
Don't query errno if not needed

Updates: gluster#1000
Signed-off-by: Yaniv Kaul <[email protected]>
mykaul added a commit to mykaul/glusterfs that referenced this issue May 24, 2024
In some functions (posix_writev, fallocate, ...) it is used to be called unconditionally.
However, it doesn't need to be - only when we need ctx to do locking.

Updates: gluster#1000
Signed-off-by: Yaniv Kaul <[email protected]>
mykaul added a commit to mykaul/glusterfs that referenced this issue May 24, 2024
…e path

We fetch it once (in posix_writev() or posix_writev_fill_rsp_dict() in io_uring code)
and then again in _fill_writev_xdata().

Signed-off-by: Yaniv Kaul <[email protected]>
Updates: gluster#1000
mykaul added a commit to mykaul/glusterfs that referenced this issue May 24, 2024
…e path

We fetch it once (in posix_writev() or posix_writev_fill_rsp_dict() in io_uring code)
and then again in _fill_writev_xdata().

Signed-off-by: Yaniv Kaul <[email protected]>
Updates: gluster#1000
mykaul added a commit to mykaul/glusterfs that referenced this issue May 24, 2024
The key can be a pointer, pointing to the xattr, instead of copying it.

Updates: gluster#1000
Signed-off-by: Yaniv Kaul <[email protected]>
mykaul added a commit to mykaul/glusterfs that referenced this issue May 31, 2024
fuse-bridge.c:433:13: warning: the comparison will always evaluate as 'true' for the address of 'name' will never be NULL [-Waddress]
  433 |         if (dentry->name) {
      |             ^~~~~~
In file included from ../../../../libglusterfs/src/glusterfs/statedump.h:15,
                 from fuse-bridge.h:23,
                 from fuse-bridge.c:14:
../../../../libglusterfs/src/glusterfs/inode.h:76:10: note: 'name' declared here
   76 |     char name[];                  /* name of the directory entry */
      |          ^~~~

Updates: gluster#1000
Signed-off-by: Yaniv Kaul <[email protected]>
mykaul added a commit to mykaul/glusterfs that referenced this issue Jun 4, 2024
- Reduced an allocation for the name, made it part of the item allocated.
- Kept the length of the name - for faster searches
- Made some functions static, code cleanups

Updates: gluster#1000
Signed-off-by: Yaniv Kaul <[email protected]>
mykaul added a commit to mykaul/glusterfs that referenced this issue Jun 4, 2024
- Reduced an allocation for the name, made it part of the item allocated.
- Kept the length of the name - for faster searches
- Made some functions static, code cleanups

Updates: gluster#1000
Signed-off-by: Yaniv Kaul <[email protected]>
amarts pushed a commit that referenced this issue Oct 18, 2024
fuse-bridge.c:433:13: warning: the comparison will always evaluate as 'true' for the address of 'name' will never be NULL [-Waddress]
  433 |         if (dentry->name) {
      |             ^~~~~~
In file included from ../../../../libglusterfs/src/glusterfs/statedump.h:15,
                 from fuse-bridge.h:23,
                 from fuse-bridge.c:14:
../../../../libglusterfs/src/glusterfs/inode.h:76:10: note: 'name' declared here
   76 |     char name[];                  /* name of the directory entry */
      |          ^~~~

Updates: #1000

Signed-off-by: Yaniv Kaul <[email protected]>
amarts pushed a commit that referenced this issue Oct 18, 2024
- Reduced an allocation for the name, made it part of the item allocated.
- Kept the length of the name - for faster searches
- Made some functions static, code cleanups

Updates: #1000

Signed-off-by: Yaniv Kaul <[email protected]>
amarts pushed a commit that referenced this issue Oct 18, 2024
…e path (#4362)

We fetch it once (in posix_writev() or posix_writev_fill_rsp_dict() in io_uring code)
and then again in _fill_writev_xdata().


Updates: #1000

Signed-off-by: Yaniv Kaul <[email protected]>
amarts pushed a commit that referenced this issue Nov 5, 2024
In some cases, we can return early - when iatt structure is NULL.
Specifically, this happens when we call those functions in part of
MAKE_INODE_HANDLE() macro - which in some cases is not using that structure.

This saves us few calls. Specifically, we do not need to fetch mdata xattr, which may save
us from some lgetxattr() and such.

Updates: #1000

Signed-off-by: Yaniv Kaul <[email protected]>
amarts pushed a commit that referenced this issue Nov 5, 2024
- posix_readdirp_fill() does not need to copy or zero a gfid on every iteration:
use an existing one or a pre-zero'ed one.
- Make some functions static.
- A bit of code restructure.

Updates: #1000

Signed-off-by: Yaniv Kaul <[email protected]>
amarts pushed a commit that referenced this issue Nov 5, 2024
* inode.c: do not call uuid_compare() to test for root if it doesn't start with zero's

uuid_compare (https://github.com/cloudbase/libuuid/blob/master/compare.c ) can be skipped,
if the UUID doesn't start with all zero's.

Updates: #1000
Signed-off-by: Yaniv Kaul <[email protected]>

* posix_handle_gfid_path(): call uuid_utoa() only if needed.

Updates: #1000
Signed-off-by: Yaniv Kaul <[email protected]>

---------

Signed-off-by: Yaniv Kaul <[email protected]>
amarts pushed a commit that referenced this issue Nov 5, 2024
Make it more readable
Remove unused parameters
Reduce redunandat work
Don't query errno if not needed

Updates: #1000

Signed-off-by: Yaniv Kaul <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment