On Thu, Apr 14, 2022 at 03:54:42PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> Every time someone changes the transaction reservation sizes, they
> introduce potential compatibility problems if the changes affect the
> minimum log size that we validate at mount time. If the minimum log
> size gets larger (which should be avoided because doing so presents a
> serious risk of log livelock), filesystems created with old mkfs will
> not mount on a newer kernel; if the minimum size shrinks, filesystems
> created with newer mkfs will not mount on older kernels.
>
> Therefore, enable the creation of a shadow log reservation structure
> where we can "undo" the effects of tweaks when computing minimum log
> sizes. These shadow reservations should never be used in practice, but
> they insulate us from perturbations in minimum log size.
>
> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_log_rlimit.c | 17 +++++++++++++----
> fs/xfs/libxfs/xfs_trans_resv.c | 12 ++++++++++++
> fs/xfs/libxfs/xfs_trans_resv.h | 2 ++
> fs/xfs/xfs_trace.h | 12 ++++++++++--
> 4 files changed, 37 insertions(+), 6 deletions(-)
>
>
> diff --git a/fs/xfs/libxfs/xfs_log_rlimit.c b/fs/xfs/libxfs/xfs_log_rlimit.c
> index 67798ff5e14e..2bafc69cac15 100644
> --- a/fs/xfs/libxfs/xfs_log_rlimit.c
> +++ b/fs/xfs/libxfs/xfs_log_rlimit.c
> @@ -14,6 +14,7 @@
> #include "xfs_trans_space.h"
> #include "xfs_da_btree.h"
> #include "xfs_bmap_btree.h"
> +#include "xfs_trace.h"
>
> /*
> * Calculate the maximum length in bytes that would be required for a local
> @@ -47,18 +48,25 @@ xfs_log_get_max_trans_res(
> struct xfs_trans_res *max_resp)
> {
> struct xfs_trans_res *resp;
> + struct xfs_trans_res *start_resp;
> struct xfs_trans_res *end_resp;
> + struct xfs_trans_resv *resv;
> int log_space = 0;
> int attr_space;
>
> attr_space = xfs_log_calc_max_attrsetm_res(mp);
>
> - resp = (struct xfs_trans_res *)M_RES(mp);
> - end_resp = (struct xfs_trans_res *)(M_RES(mp) + 1);
> - for (; resp < end_resp; resp++) {
> + resv = kmem_zalloc(sizeof(struct xfs_trans_resv), 0);
> + xfs_trans_resv_calc_logsize(mp, resv);
> +
> + start_resp = (struct xfs_trans_res *)resv;
> + end_resp = (struct xfs_trans_res *)(resv + 1);
> + for (resp = start_resp; resp < end_resp; resp++) {
> int tmp = resp->tr_logcount > 1 ?
> resp->tr_logres * resp->tr_logcount :
> resp->tr_logres;
> +
> + trace_xfs_trans_resv_calc_logsize(mp, resp - start_resp, resp);
> if (log_space < tmp) {
> log_space = tmp;
> *max_resp = *resp; /* struct copy */
This took me a while to get my head around. The minimum logsize
calculation stuff is all a bit of a mess.
Essentially, we call xfs_log_get_max_trans_res() from two places.
One is to calculate the minimum log size, the other is the
transaction reservation tracing code done when M_RES(mp) is set up
via xfs_trans_trace_reservations(). We don't need the call from
xfs_trans_trace_reservations() - it's trivial to scan the list of
tracepoints emitted by this function at mount time to find the
maximum reservation.
Hence I think we should start by removing that call to this
function, and making this a static function called only from
xfs_log_calc_minimum_size().
At this point, we can use an on-stack struct xfs_trans_resv for the
calculated values - no need for memory allocation here as we will
never be short of stack space in this path.
The tracing in the loop also wants an integer index into the struct
xfs_trans_resv structure, so it should be changed to match what
xfs_trans_trace_reservations() does:
struct xfs_trans_resv resv;
struct xfs_trans_res *resp;
struct xfs_trans_res *end_resp;
....
xfs_trans_resv_calc(mp, &resv)
resp = (struct xfs_trans_res *)&resv;
end_resp = (struct xfs_trans_res *)(&resv + 1);
for (i = 0; resp < end_resp; resp++) {
.....
trace_xfs_trans_resv_calc_logsize(mp, i, resp);
....
}
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index 6f83d9b306ee..12d4e451e70e 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -933,3 +933,15 @@ xfs_trans_resv_calc(
> /* Put everything back the way it was. This goes at the end. */
> mp->m_rmap_maxlevels = rmap_maxlevels;
> }
> +
> +/*
> + * Compute an alternate set of log reservation sizes for use exclusively with
> + * minimum log size calculations.
> + */
> +void
> +xfs_trans_resv_calc_logsize(
> + struct xfs_mount *mp,
> + struct xfs_trans_resv *resp)
> +{
> + xfs_trans_resv_calc(mp, resp);
> +}
This function and it's name was waht confused me for a while - I
don't think it belongs in this patch, and I don't think it belongs
in this file when it's filled out in the next patch. It's basically
handling things specific to minimum log size calculations, so with
the above mods I think it should also end up being static to
libxfs/xfs_log_rlimit.c.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
