Re: [Tarantool-patches] [PATCH v2 03/10] wal: don't touch box.cfg.wal_dir more than once

[Serge Petrenko <sergepetrenko@tarantool.org>]

04.09.2020 01:51, Vladislav Shpilevoy пишет:
> Relay.cc and box.cc obtained box.cfg.wal_dir value using
> cfg_gets() call. To initialize WAL and create struct recovery
> objects.
>
> That is not only a bit dangerous (cfg_gets() uses Lua API and can
> throw a Lua error) and slow, but also not necessary - wal_dir
> parameter is constant, it can't be changed after instance start.
>
> It means, the value can be stored somewhere one time and then used
> without Lua.
>
> Main motivation is that the WAL directory path will be needed
> inside relay threads to restart their recovery iterators in the
> Raft patch. They can't use cfg_gets(), because Lua lives in TX
> thread. But can access a constant global variable, introduced in
> this patch (it existed before, but now has a method to get it).
>
> Needed for #1146


LGTM

> ---
>   src/box/box.cc   | 9 ++++-----
>   src/box/relay.cc | 7 ++-----
>   src/box/wal.c    | 6 ++++++
>   src/box/wal.h    | 7 +++++++
>   4 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 0813603c0..eeb00d5e2 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -2395,8 +2395,7 @@ local_recovery(const struct tt_uuid *instance_uuid,
>   	wal_stream_create(&wal_stream);
>   
>   	struct recovery *recovery;
> -	recovery = recovery_new(cfg_gets("wal_dir"),
> -				cfg_geti("force_recovery"),
> +	recovery = recovery_new(wal_dir(), cfg_geti("force_recovery"),
>   				checkpoint_vclock);
>   
>   	/*
> @@ -2469,7 +2468,7 @@ local_recovery(const struct tt_uuid *instance_uuid,
>   		recovery_follow_local(recovery, &wal_stream.base, "hot_standby",
>   				      cfg_getd("wal_dir_rescan_delay"));
>   		while (true) {
> -			if (path_lock(cfg_gets("wal_dir"), &wal_dir_lock))
> +			if (path_lock(wal_dir(), &wal_dir_lock))
>   				diag_raise();
>   			if (wal_dir_lock >= 0)
>   				break;
> @@ -2616,7 +2615,7 @@ box_cfg_xc(void)
>   	 * Lock the write ahead log directory to avoid multiple
>   	 * instances running in the same dir.
>   	 */
> -	if (path_lock(cfg_gets("wal_dir"), &wal_dir_lock) < 0)
> +	if (path_lock(wal_dir(), &wal_dir_lock) < 0)
>   		diag_raise();
>   	if (wal_dir_lock < 0) {
>   		/**
> @@ -2625,7 +2624,7 @@ box_cfg_xc(void)
>   		 * WAL dir must contain at least one xlog.
>   		 */
>   		if (!cfg_geti("hot_standby") || checkpoint == NULL)
> -			tnt_raise(ClientError, ER_ALREADY_RUNNING, cfg_gets("wal_dir"));
> +			tnt_raise(ClientError, ER_ALREADY_RUNNING, wal_dir());
>   	}
>   
>   	struct journal bootstrap_journal;
> diff --git a/src/box/relay.cc b/src/box/relay.cc
> index a7843a8c2..124b0f52f 100644
> --- a/src/box/relay.cc
> +++ b/src/box/relay.cc
> @@ -34,7 +34,6 @@
>   #include "tt_static.h"
>   #include "scoped_guard.h"
>   #include "cbus.h"
> -#include "cfg.h"
>   #include "errinj.h"
>   #include "fiber.h"
>   #include "say.h"
> @@ -369,8 +368,7 @@ relay_final_join(int fd, uint64_t sync, struct vclock *start_vclock,
>   		relay_delete(relay);
>   	});
>   
> -	relay->r = recovery_new(cfg_gets("wal_dir"), false,
> -			       start_vclock);
> +	relay->r = recovery_new(wal_dir(), false, start_vclock);
>   	vclock_copy(&relay->stop_vclock, stop_vclock);
>   
>   	int rc = cord_costart(&relay->cord, "final_join",
> @@ -731,8 +729,7 @@ relay_subscribe(struct replica *replica, int fd, uint64_t sync,
>   	});
>   
>   	vclock_copy(&relay->local_vclock_at_subscribe, &replicaset.vclock);
> -	relay->r = recovery_new(cfg_gets("wal_dir"), false,
> -			        replica_clock);
> +	relay->r = recovery_new(wal_dir(), false, replica_clock);
>   	vclock_copy(&relay->tx.vclock, replica_clock);
>   	relay->version_id = replica_version_id;
>   
> diff --git a/src/box/wal.c b/src/box/wal.c
> index 045006b60..e181e58d9 100644
> --- a/src/box/wal.c
> +++ b/src/box/wal.c
> @@ -201,6 +201,12 @@ wal_mode(void)
>   	return wal_writer_singleton.wal_mode;
>   }
>   
> +const char *
> +wal_dir(void)
> +{
> +	return wal_writer_singleton.wal_dir.dirname;
> +}
> +
>   static void
>   wal_write_to_disk(struct cmsg *msg);
>   
> diff --git a/src/box/wal.h b/src/box/wal.h
> index 9d0cada46..581306fe9 100644
> --- a/src/box/wal.h
> +++ b/src/box/wal.h
> @@ -98,6 +98,13 @@ wal_enable(void);
>   void
>   wal_free(void);
>   
> +/**
> + * Get WAL directory path. The value never changes after box is
> + * configured first time. Safe to use from multiple threads.
> + */
> +const char *
> +wal_dir(void);
> +
>   struct wal_watcher_msg {
>   	struct cmsg cmsg;
>   	struct wal_watcher *watcher;

-- 
Serge Petrenko