Skip to content

Commit

Permalink
Add config param Cache_FDs and use it in the same way as in ibm2.5 code
Browse files Browse the repository at this point in the history
Similar to ibm2.5, have a config parameter Cache_FDs such that:

If "Cache_FDs" is set to false, the reaper thread aggressively
closes FDs , significantly reducing the number of open FDs.
This will help to maintain a minimal number of open FDs.

If "Cache_FDs" is set to true (default), FDs are cached, and the
LRU reaper thread closes FDs only when the current open FD count
reaches or exceeds the "fds_lowat" threshold.

Change-Id: Ica3a5e7ea1a0b81665b8d9ad701dccb7090209a8
Signed-off-by: Madhu Thorat <madhu.punjabi@in.ibm.com>
(cherry picked from commit 9bf5651)
Signed-off-by: Frank S. Filz <ffilzlnx@mindspring.com>
  • Loading branch information
ffilz committed Aug 3, 2023
1 parent 71bcc21 commit 7180f81
Show file tree
Hide file tree
Showing 7 changed files with 57 additions and 19 deletions.
9 changes: 9 additions & 0 deletions src/FSAL/Stackable_FSALs/FSAL_MDCACHE/mdcache_ext.h
Expand Up @@ -84,6 +84,15 @@ struct mdcache_parameter {
/** Base interval in seconds between runs of the LRU cleaner
thread. Defaults to 60, settable with LRU_Run_Interval. */
uint32_t lru_run_interval;
/** If "Cache_FDs" is set to false, the reaper thread aggressively
* closes FDs , significantly reducing the number of open FDs.
* This will help to maintain a minimal number of open FDs.
*
* If "Cache_FDs" is set to true (default), FDs are cached, and the
* LRU reaper thread closes FDs only when the current open FD count
* reaches or exceeds the "fds_lowat" threshold.
*/
bool Cache_FDs;
/** The percentage of the system-imposed maximum of file
descriptors at which Ganesha will deny requests.
Defaults to 99, settable with FD_Limit_Percent. */
Expand Down
1 change: 1 addition & 0 deletions src/FSAL/Stackable_FSALs/FSAL_MDCACHE/mdcache_lru.c
Expand Up @@ -1566,6 +1566,7 @@ void init_fds_limit(void)
struct fd_lru_parameter fd_lru_parameter;

fd_lru_parameter.lru_run_interval = mdcache_param.lru_run_interval;
fd_lru_parameter.Cache_FDs = mdcache_param.Cache_FDs;
fd_lru_parameter.fd_limit_percent = mdcache_param.fd_limit_percent;
fd_lru_parameter.fd_hwmark_percent = mdcache_param.fd_hwmark_percent;
fd_lru_parameter.fd_lwmark_percent = mdcache_param.fd_lwmark_percent;
Expand Down
2 changes: 2 additions & 0 deletions src/FSAL/Stackable_FSALs/FSAL_MDCACHE/mdcache_read_conf.c
Expand Up @@ -74,6 +74,8 @@ static struct config_item mdcache_params[] = {
mdcache_parameter, chunks_lwmark),
CONF_ITEM_UI32("LRU_Run_Interval", 1, 24 * 3600, 90,
mdcache_parameter, lru_run_interval),
CONF_ITEM_BOOL("Cache_FDs", true,
mdcache_parameter, Cache_FDs),
CONF_ITEM_UI32("FD_Limit_Percent", 0, 100, 99,
mdcache_parameter, fd_limit_percent),
CONF_ITEM_UI32("FD_HWMark_Percent", 0, 100, 90,
Expand Down
45 changes: 26 additions & 19 deletions src/FSAL/commonlib.c
Expand Up @@ -1362,6 +1362,7 @@ int32_t fsal_fd_global_counter;
uint32_t fsal_fd_state_counter;
uint32_t fsal_fd_temp_counter;
time_t lru_run_interval;
bool Cache_FDs;
static struct fridgethr *fd_lru_fridge;

uint32_t lru_try_one(void)
Expand Down Expand Up @@ -1417,7 +1418,6 @@ struct fd_lru_state fd_lru_state;
uint32_t futility_count;
uint32_t required_progress;
uint32_t reaper_work;
time_t lru_run_interval;

/**
* @brief Function that executes in the fd_lru thread
Expand Down Expand Up @@ -1498,24 +1498,38 @@ void fd_lru_run(struct fridgethr_context *ctx)
fd_lru_state.futility = 0;
}

/* Reap file descriptors. This is a preliminary example of the
L2 functionality rather than something we expect to be
permanent. (It will have to adapt heavily to the new FSAL
API, for example.) */
/* Check for fd_state transitions */

LogDebug(COMPONENT_FSAL,
"FD count fsal_fd_global_counter is %"PRIi32
" and low water mark is %"PRIi32
" and high water mark is %"PRIi32" %s",
currentopen, fd_lru_state.fds_lowat, fd_lru_state.fds_hiwat,
((currentopen >= fd_lru_state.fds_lowat)
|| (Cache_FDs == false))
? "(reaping)" : "(not reaping)");

if (currentopen < fd_lru_state.fds_lowat) {
LogDebug(COMPONENT_FSAL,
"FD count fsal_fd_global_counter is %"PRIi32
" and low water mark is %d: not reaping.",
atomic_fetch_int32_t(&fsal_fd_global_counter),
fd_lru_state.fds_lowat);
if (atomic_fetch_uint32_t(&fd_lru_state.fd_state) > FD_LOW) {
LogEvent(COMPONENT_FSAL,
"Return to normal fd reaping.");
atomic_store_uint32_t(&fd_lru_state.fd_state, FD_LOW);
}
} else {
} else if (currentopen < fd_lru_state.fds_hiwat &&
atomic_fetch_uint32_t(&fd_lru_state.fd_state) == FD_LIMIT) {
LogEvent(COMPONENT_FSAL,
"Count of fd is below high water mark.");
atomic_store_uint32_t(&fd_lru_state.fd_state,
FD_MIDDLE);
}

/* Reap file descriptors. This is a preliminary example of the
* L2 functionality rather than something we expect to be
* permanent. (It will have to adapt heavily to the new FSAL
* API, for example.)
*/

if ((currentopen >= fd_lru_state.fds_lowat) || (Cache_FDs == false)) {
/* The count of open file descriptors before this run
of the reaper. */
int32_t formeropen = currentopen;
Expand All @@ -1525,14 +1539,6 @@ void fd_lru_run(struct fridgethr_context *ctx)
uint32_t workpass = 0;
time_t curr_time = time(NULL);

if (currentopen < fd_lru_state.fds_hiwat &&
atomic_fetch_uint32_t(&fd_lru_state.fd_state) == FD_LIMIT) {
LogEvent(COMPONENT_FSAL,
"Count of fd is below high water mark.");
atomic_store_uint32_t(&fd_lru_state.fd_state,
FD_MIDDLE);
}

if ((curr_time >= fd_lru_state.prev_time) &&
(curr_time - fd_lru_state.prev_time <
fridgethr_getwait(ctx)))
Expand Down Expand Up @@ -1855,6 +1861,7 @@ fsal_status_t fd_lru_pkginit(struct fd_lru_parameter *params)
required_progress = params->required_progress;
reaper_work = params->reaper_work;
lru_run_interval = params->lru_run_interval;
Cache_FDs = params->Cache_FDs;

This comment has been minimized.

Copy link
@drieber

drieber Sep 29, 2023

Contributor

The actual 'params' passed to fd_lru_pkginit by mdcache_lru_pkginit never had a value assigned to Cache_FDs, so here we end up reading an uninitialized value. Look in mdcache_lru_pkginit, in mdcache_lru.c.


memset(&frp, 0, sizeof(struct fridgethr_params));
frp.thr_max = 1;
Expand Down
2 changes: 2 additions & 0 deletions src/config_samples/config.txt
Expand Up @@ -807,6 +807,8 @@ MDCACHE {}

LRU_Run_Interval(uint32, range 1 to 24 * 3600, default 90)

Cache_FDs(bool, true)

FD_Limit_Percent(uint32, range 0 to 100, default 99)

FD_HWMark_Percent(uint32, range 0 to 100, default 90)
Expand Down
9 changes: 9 additions & 0 deletions src/doc/man/ganesha-cache-config.rst
Expand Up @@ -71,6 +71,15 @@ Chunks_LWMark(uint32, range 1 to UINT32_MAX, default 1000)
LRU_Run_Interval(uint32, range 1 to 24 * 3600, default 90)
Base interval in seconds between runs of the LRU cleaner thread.

Cache_FDs(bool, true)
If "Cache_FDs" is set to false, the reaper thread aggressively
closes FDs , significantly reducing the number of open FDs.
This will help to maintain a minimal number of open FDs.

If "Cache_FDs" is set to true (default), FDs are cached, and the
LRU reaper thread closes FDs only when the current open FD count
reaches or exceeds the "fds_lowat" threshold.

FD_Limit_Percent(uint32, range 0 to 100, default 99)
The percentage of the system-imposed maximum of file descriptors at which
Ganesha will deny requests.
Expand Down
8 changes: 8 additions & 0 deletions src/include/fsal.h
Expand Up @@ -668,6 +668,14 @@ struct fd_lru_parameter {
/** Base interval in seconds between runs of the LRU cleaner
thread. Defaults to 60, settable with LRU_Run_Interval. */
uint32_t lru_run_interval;
/** If Cache_FDs is false then FDs will remained cached till the LRU
* reaper thread invokes and tries to close the FDs.
*
* If Cache_FDs is true (default) then FDs get cached and LRU reaper
* thread on invocation will try to close the FDs only when the
* currentopen >= fds_lowat (FD low watermark).
*/
bool Cache_FDs;
/** The percentage of the system-imposed maximum of file
descriptors at which Ganesha will deny requests.
Defaults to 99, settable with FD_Limit_Percent. */
Expand Down

4 comments on commit 7180f81

@drieber
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell this change introduced a read of uninitialized value:

fsal_status_t
mdcache_lru_pkginit(void)
{
...
struct fd_lru_parameter fd_lru_parameter;
.......... // sets many fields in fd_lru_parameter but does NOT set field Cache_FDs
status = fd_lru_pkginit(&fd_lru_parameter);
return status;
}

bool Cache_FDs; <==== This is a global variable

fsal_status_t fd_lru_pkginit(struct fd_lru_parameter *params)
{
.....
Cache_FDs = params->Cache_FDs; <=== This is reading uninitialized memory to set the global variable.
....
}

I found this problem with ubsan.

@ffilz
Copy link
Member Author

@ffilz ffilz commented on 7180f81 Sep 29, 2023

Choose a reason for hiding this comment

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

I annotated the flow of this parameter from the MDCACHE config through the FSAL parameters to the FSAL global variable. It looks like it's all there.

Can you annotate where you think something is missing?

@ffilz
Copy link
Member Author

@ffilz ffilz commented on 7180f81 Sep 29, 2023

Choose a reason for hiding this comment

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

Ah, OK, I see it now, it's needed at about line 1652 in FSAL/Stackable_FSALs/FSAL_MDCACHE/mdcache_lru.c. Could you open a github issue, and if you're willing to make the patch, please do so and submit to gerrithub, otherwise I'll probably get to it next week.

Thanks

Frank

@drieber
Copy link
Contributor

Choose a reason for hiding this comment

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

Please sign in to comment.