Reduce the number or threads used in the brick process

[amarts]

If I start just 1 fsd process, below is the summary of threads:

sh$ ps -T -p 23149 # 23419 is PID of gluster brick process.
23149 23149 ?        00:00:00 glusterfsd
23149 23150 ?        00:00:00 glfs_timer
23149 23151 ?        00:00:00 glfs_sigwait
23149 23152 ?        00:00:00 glfs_memsweep
23149 23153 ?        00:00:00 glfs_sproc0
23149 23154 ?        00:00:00 glfs_sproc1
23149 23155 ?        00:00:00 glfs_epoll000
23149 23156 ?        00:00:00 glfs_rpcrqhnd
23149 23157 ?        00:00:00 glfs_rpcrqhnd
23149 23158 ?        00:00:00 glfs_idxwrker
23149 23159 ?        00:00:00 glfs_iotwr000
23149 23160 ?        00:00:00 glfs_brssign
23149 23161 ?        00:00:00 glfs_brswrker
23149 23162 ?        00:00:00 glfs_clogecon
23149 23163 ?        00:00:00 glfs_clogd000
23149 23164 ?        00:00:00 glfs_clogd001
23149 23165 ?        00:00:00 glfs_clogd002
23149 23166 ?        00:00:00 glfs_posix_rese
23149 23167 ?        00:00:00 glfs_posixhc
23149 23168 ?        00:00:00 glfs_posixjan
23149 23169 ?        00:00:00 glfs_posixfsy

That is 20 different threads, where only 3 threads here would be required in I/O path. So there is a lot of benefit if we move other threads into a single pool (if it makes sense), where we make use of timer to trigger time based validations (like glfs_posixjanitor etc).

[amarts]

@mohit84 @xhernandez @KritikaDhananjay @poornimag @ShyamsundarR @raghavendrahg @csabahenk @mchangir

[mohit84]

I am also planning to resolve the same, we need to load xlator only while user enable otherwise we should not load it in graph.

[xhernandez]

I think we should apply the same point of view we are using for the unified caching framework and the transaction framework. We should provide a framework able to manage a thread pool and delegate all/most of the asynchronous tasks to it instead of creating a single thread (or even a small thread pool in some cases) for each specific requirement.

Moving to a centralized thread pool approach also has some benefits:

• Better resource usage (less threads, less stacks, less context switching, less probability of lock contention).
• Reduce some latencies (if used correctly, we can delegate work to other threads and continue polling fuse/network or doing other tasks sooner instead of waiting till the io-threads xlator).
• It would be easier to control state of each available thread, making it possible to use userspace-rcu library in its most optimum form.

[atinmu]

Threads per brick instance:

timer thread = 1
sigwait = 1
memsweep = 1
sproc = (depends on the load min is 2 max is 16)
rpcreqhnd (depends on server event threads)
poller (depends on server event thread configuration)
idxwrker (per brick 1 thread)
iotworker (depends on the load by default per brick 1 max is 16)
brssign (per brick 1)
brswrker (per brick 1)
clogecon (per brick 1)
clogd(per brick 3)
posix_re(per brick 1)
posixhc (per brick 1)
posixjan (per brick 1)
posixfsy (per brick 1)

Credits - @mohit84

[kotreshhr]

How is this data collected ? I think we should be collecting this data with all brick side options enabled which are used.
e.g. If geo-rep/glusterfind is setup, few changelogs threads would get added.

[mohit84]

These threads are created by a brick process with default volume option. Some of the threads are common like poller threads shared by all bricks instances.

[kotreshhr]

While the threads output did capture threads by changelog and bitrot when it's not enabled, it missed two more threads that get created when it's enabled.

glusterclogro ----> (1 per brick) --> changelog_rollover
glusterclogfsyn --->(1 per brick) --> changelog_fsync_thread

[poornimag]

The problems here are:

• Number of threads (some global, some per xl)
• Memory usage (inode table, per thread pool, etc?)
• Performance (because of number of threads, contention in the rpc, lock contention, contention in posix, etc.)

With thread pool, we can reduce the thread count. We really need the global thread pool, and submitting tasks approach for gluster, so that all the gluster processes benefit from it. But the memory footprint may be still higher than out expectations, for 100s of graphs. And graph activate and deactivate leaks memory and has so many corner conditions to be handled.

Hence, proposing that for brick multiplexing use case alone, we work with single graph and modify posix to handle multiple bricks. This can work as we attach bricks of similar graph in brick multiplexing. The cons of this approach:

• Single inode table, will have to address the lock contention
• Code changed in posix, rpc to handle multiple bricks in same graph, the subvolume(brick path) has to be passed from rpc to posix(the fops originating below rpc needs to be handled).
• QOS not possible (may be not?)

Pros:

• Graph attach and detach will be simpler
• Threads count will be reduced
• Memory footprint will be reduced.

Thought?

[prashanthpai]

Hence, proposing that for brick multiplexing use case alone, we work with single graph and modify posix to handle multiple bricks. This can work as we attach bricks of similar graph in brick multiplexing.

That's a good approach - practical and takes a lot less effort 👍

[amarts]

Hence, proposing that for brick multiplexing use case alone, we work with single graph and modify posix to handle multiple bricks. This can work as we attach bricks of similar graph in brick multiplexing.

Thinking about it, this needs changes in 'management' layer, but we can make sure all these volumes have their export directory mounted inside an exported path in glusterfs, and treat each different volume as a subdir mount. So, clients only get access to what they need, and there won't be any self-heald service problems, and for matrix also, it is directory level (at least df -h works per directory).

A good thought to think towards.

@raghavendra-talur @JohnStrunk @ShyamsundarR this is similar to how we have subdir mount, but to provide snapshots, we have LVs exported as subdirs in a larger gluster export ?

A good topic to discuss in new repo called GCS, IMO. This discussion is taking this out of Gluster's scope altogether, and making it a provisioning and CSI goal.

[ShyamsundarR]

Hence, proposing that for brick multiplexing use case alone, we work with single graph and modify posix to handle multiple bricks. This can work as we attach bricks of similar graph in brick multiplexing.

Thinking about it, this needs changes in 'management' layer, but we can make sure all these volumes have their export directory mounted inside an exported path in glusterfs, and treat each different volume as a subdir mount. So, clients only get access to what they need, and there won't be any self-heald service problems, and for matrix also, it is directory level (at least df -h works per directory).

A good thought to think towards.

@raghavendra-talur @JohnStrunk @ShyamsundarR this is similar to how we have subdir mount, but to provide snapshots, we have LVs exported as subdirs in a larger gluster export ?

Tagging @jdarcy as well on this, for insights.

Commentary provided is not to dissuade the thought, but to voice possible issues that may need handling.

Some of the concerns/issues to address is GFID name space collusion across the various bricks and dealing with the root inode for each (which will collide). Others are features that are at a volume level and how these can adapt (geo-rep (changelog), quota, index) to such a scheme.

• GFID collusion
  How can we treat separate GFID namespaces within the same inode table?

• Security
  Going by the sub-dir approach, and a common inode table across volumes, how do we prevent access to GFIDs that do not belong to a volume from another client? (resolution failures?)

• Sub-dirs mounted on a single directory method
  Which local FS mount holds the .glusterfs namespace across all other namespaces? (we cannot hardlink across mounts)

I ask the above as initial proposal deals with handling this at POSIX, and later we are stating we will mount each volume export inside an exported path.

• Can we segregate volumes that need certain data services from others?
  Say, a volume needs geo-rep and others not, will this be possible with changelog seeing it as a single namespace across volumes? (and other xlators in this problem space)

Generic thoughts on existing model:
Brick-mux, was to also fold io-threads to a single instance, thus enabling scaling of actual io workers, than each volume having it's own pool (patch: https://review.gluster.org/#/c/15643/ )

Having a thread pool and a means of submitting jobs to it, can help in thread counts and better framework for future expansion as well. (as discussed above)

Adding sub-brick graphs, takes away the constraints of xlator development to address possibility of multiple bricks by the same instance of the xlator.

Brick attach/detach and all problems thereof are IMO only postponed when folding this into POSIX. For example, when a brick is detached, when are its inodes in the brick process evicted? Currently the entire inode table is evicted, and all inodes released/forgotten when a brick is detached, thus providing an easier cleanup experience.

I think we need to detail the current approach out, and in the meantime address existing concerns as well.

[gluster-ant]

A patch https://review.gluster.org/20761 has been posted that references this issue.

Posix: Instead of using one thread for janitor, use synctask

With brick mux, the number of threads increases as the number of
bricks increases. As an initiative to reduce the number of
threads in brick mux scenario, replcing janitor thread to use
synctask infra.

Updates #475
Change-Id: I0c4aaf728125ab7264442fde59f3d08542785f73
Signed-off-by: Poornima G pgurusid@redhat.com

[gluster-ant]

A patch https://review.gluster.org/20859 has been posted that references this issue.

io-threads: Associate io-threads with glusterfs_ctx_t

Updates #475
Change-Id: I131b9f7f8d95a58b1dc3667bf973a6e1afd9ce50
Signed-off-by: Poornima G pgurusid@redhat.com

[nbalacha]

I think the following suggestion brings up major issues as highlighted by Shyam and is not a good approach.
"Single inode table, will have to address the lock contention"
Root inodes will clash, as will any other hidden folders with fixed gfids.
I vote for cleaning up the existing approach instead.

[gluster-ant]

A patch https://review.gluster.org/21116 has been posted that references this issue.

Posix: Instead of using a thread for fsyncer, use synctask

With brick mux, the number of threads increases as the number of
bricks increases. As an initiative to reduce the number of
threads in brick mux scenario, replcing fsyncer thread to use
synctask infra.

afr_changelog_fsync is the only code that currently uses batch-fsync.
At the posix_fsync, the fsync fop is handed over to the fsyncer thread
if batch-fsync-mode is set(default set) and xdata contains batch-fsync.
Even when the batch_fsync_delay_usec is 0, the fsync is handed to the
fsyncer thread, so the main thread is not blocked on fsync. With this
patch this behaviour is changed. If the batch_fsync_delay_usec is 0,
there is no need to hand over fsync to separate thread, as fsync is
executed in io-threads. If batch_fsync_delay_usec > 0 then the batch
fsync happens on a timely basis in synctask thread.

Updates #475
Change-Id: I560ab1f3d690f353130cf46cbefec55fa8dfd3b4
Signed-off-by: Poornima G pgurusid@redhat.com

[amarts]

Should this be a feature at all? Or a bug to be precise?

If we need to continue here, @ShyamsundarR @xhernandez please confirm if commit messages are sufficient for providing SpecApproved ?

I see that a user wouldn't even need to know about it, or see any changes in documentation in this issue, and hence providing DocApproved!

[ShyamsundarR]

Should this be a feature at all? Or a bug to be precise?

If we need to continue here, @ShyamsundarR @xhernandez please confirm if commit messages are sufficient for providing SpecApproved ?

I see these as major enhancements, that are better documented than left in commit messages. In this case I would prefer that we add some documentation in tree (i.e under developer-guide/features) that explain out the various changes and the current model of reducing the thread counts.

[gluster-ant]

A patch https://review.gluster.org/21688 has been posted that references this issue.

doc: Add brick mux thread reduction design doc in developer guide

Change-Id: I5309efb5826d4385eadea29c1cf973f504eef9e5
Updates: #475
Signed-off-by: Poornima G pgurusid@redhat.com

[poornimag]

@ShyamsundarR , @amarts have submitted design doc @ https://review.gluster.org/#/c/glusterfs/+/21688
Can SpecApproved flag be provided? Thanks.

[gluster-ant]

A patch https://review.gluster.org/21688 has been posted that references this issue.

doc: Add brick mux thread reduction design doc in developer guide

Change-Id: I5309efb5826d4385eadea29c1cf973f504eef9e5
Updates: #475
Signed-off-by: Poornima G pgurusid@redhat.com

[gluster-ant]

A patch https://review.gluster.org/21896 has been posted that references this issue.

feature/changelog: Avoid thread creation if xlator is not enabled

Problem: Avoid thread creation for changelog
for a volume if feature is not enabled

Solution: Before thread creation check the flag if feature
is enabled

Change-Id: Ifd00286e0966049e8eb9f21567fe407cf11bb02a
Updates: #475
Signed-off-b: Mohit Agrawal moagrawal@redhat.com

[gluster-ant]

A patch https://review.gluster.org/21941 has been posted that references this issue.

mpx-restart-crash.t: add details on memory and threads consumed

Change-Id: Ie051ed246493ef290be1d781e4f326bc5384d4e5
updates: #475

[gluster-ant]

A patch https://review.gluster.org/21943 has been posted that references this issue.

feature/bitrot: Avoid thread creation if xlator is not enabled

Problem: Avoid thread creation for bitrot-stub
for a volume if feature is not enabled

Solution: Before thread creation check the flag if feature
is enabled

Updates: #475
Change-Id: I2c6cc35bba142d4b418cc986ada588e558512c8e
Signed-off-by: Mohit Agrawal moagrawal@redhat.com
Signed-off-by: Kotresh HR khiremat@redhat.com

[gluster-ant]

A patch https://review.gluster.org/21944 has been posted that references this issue.

Posix: Instead of using a thread for fsyncer, use synctask

With brick mux, the number of threads increases as the number of
bricks increases. As an initiative to reduce the number of
threads in brick mux scenario, replcing fsyncer thread to use
synctask infra.

afr_changelog_fsync is the only code that currently uses batch-fsync.
At the posix_fsync, the fsync fop is handed over to the fsyncer thread
if batch-fsync-mode is set(default set) and xdata contains batch-fsync.
Even when the batch_fsync_delay_usec is 0, the fsync is handed to the
fsyncer thread, so the main thread is not blocked on fsync. With this
patch this behaviour is changed. If the batch_fsync_delay_usec is 0,
there is no need to hand over fsync to separate thread, as fsync is
executed in io-threads. If batch_fsync_delay_usec > 0 then the batch
fsync happens on a timely basis in synctask thread.

Updates #475
Change-Id: I560ab1f3d690f353130cf46cbefec55fa8dfd3b4
Signed-off-by: Poornima G pgurusid@redhat.com

[gluster-ant]

A patch https://review.gluster.org/21945 has been posted that references this issue.

io-threads: Associate io-threads with glusterfs_ctx_t

io-threads are currently associated with xlator, every brick
graph has one instance of io-threads. Everytime a brick graph
is attached in brick mux use case, new set of io-threads are
spawned. At scale, this becomes overhead. It is good to associate
resources like io-threads with glusterfs_ctx which is global to
the process.

Note that, this patch reduces the performance of brick mux use
case, due to lock contention in io-thread implementation.
Will have subsequent patches by Mohit Agarwal solving the lock
contention.

Updates: #475
Change-Id: I131b9f7f8d95a58b1dc3667bf973a6e1afd9ce50
Signed-off-by: Poornima G pgurusid@redhat.com
Signed-off-by: Harpreet kaur hlalwani@redhat.com

[gluster-ant]

A patch https://review.gluster.org/21942 has been posted that references this issue.

feature/changelog: Avoid thread creation if xlator is not enabled

Problem: Avoid thread creation for changelog
for a volume if feature is not enabled

Solution: Before thread creation check the flag if feature
is enabled

Change-Id: Ifd00286e0966049e8eb9f21567fe407cf11bb02a
Updates: #475
Signed-off-b: Mohit Agrawal moagrawal@redhat.com

[gluster-ant]

A patch https://review.gluster.org/21958 has been posted that references this issue.

bitrot|changelog: Avoid thread creation if xlator is not enabled

Problem: Avoid thread creation for bitrot-stub and changelog
for a volume if feature is not enabled

Solution: Before thread creation check the flag if feature
is enabled

Change-Id: I944465079fcd038736eb021ad45a323cca5f8a64
Updates: #475
Signed-off-by: Mohit Agrawal moagrawal@redhat.com

[ShyamsundarR]

What is the completion status of this issue in release-6? The issue is still open, hence what is pending? Based on which we can decide what we need to get into the release notes for the release.

[stale]

Thank you for your contributions.
Noticed that this issue is not having any activity in last ~6 months! We are marking this issue as stale because it has not had recent activity.
It will be closed in 2 weeks if no one responds with a comment here.

[stale]

Closing this issue as there was no update since my last update on issue. If this is an issue which is still valid, feel free to open it.