On Fri, Mar 25, 2022 at 07:48:08AM -1000, Eric Sandeen wrote:
> Darrick, it bugged me a bit that the way this patch worked was to go through
> one set of calculations to sort out a log size, then throw in a new heuristic
> or two to change that result. And TBH the 7/8 bit seems very ad hoc when
> we already had the prior 95% heuristic.
>
> While I know that most of this goes away by the last patch, I'm considering
> deferring patches 4 & 5 for the next release because of the impact on
> fstests, so would like to land somewhere clean by patch 3.
>
> What do you think of this patch as a replacement for this patch 3/5? It's a
> bit clearer to me, and results in (almost) the same net results as your patch,
> with minor differences around 512MB filesystems due to the removal of the
> 7/8 limit.
>
> Depending on what you think, I can tweak your commit log as needed, leave
> your authorship, and add a:
>
> [sandeen: consolidate heuristics into something a bit more clear]
>
> or something like that... or, claim authorship (and all ensuing bugs) ;)
Man, my email is slow today!
This looks ok to me, though at this point you've basically reauthored
the whole thing. You might as well own it and add:
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--D
>
> Thanks,
> -Eric
>
> diff --git a/include/xfs_multidisk.h b/include/xfs_multidisk.h
> index a16a9fe2..ef4443b0 100644
> --- a/include/xfs_multidisk.h
> +++ b/include/xfs_multidisk.h
> @@ -17,8 +17,6 @@
> #define XFS_MIN_INODE_PERBLOCK 2 /* min inodes per block */
> #define XFS_DFL_IMAXIMUM_PCT 25 /* max % of space for inodes */
> #define XFS_MIN_REC_DIRSIZE 12 /* 4096 byte dirblocks (V2) */
> -#define XFS_DFL_LOG_FACTOR 5 /* default log size, factor */
> - /* with max trans reservation */
> #define XFS_MAX_INODE_SIG_BITS 32 /* most significant bits in an
> * inode number that we'll
> * accept w/o warnings
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index ad776492..cf1d64a2 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -18,6 +18,14 @@
> #define GIGABYTES(count, blog) ((uint64_t)(count) << (30 - (blog)))
> #define MEGABYTES(count, blog) ((uint64_t)(count) << (20 - (blog)))
>
> +/*
> + * Realistically, the log should never be smaller than 64MB. Studies by the
> + * kernel maintainer in early 2022 have shown a dramatic reduction in long tail
> + * latency of the xlog grant head waitqueue when running a heavy metadata
> + * update workload when the log size is at least 64MB.
> + */
> +#define XFS_MIN_REALISTIC_LOG_BLOCKS(blog) (MEGABYTES(64, (blog)))
> +
> /*
> * Use this macro before we have superblock and mount structure to
> * convert from basic blocks to filesystem blocks.
> @@ -3297,7 +3305,7 @@ calculate_log_size(
> struct xfs_mount *mp)
> {
> struct xfs_sb *sbp = &mp->m_sb;
> - int min_logblocks;
> + int min_logblocks; /* absolute minimum */
> struct xfs_mount mount;
>
> /* we need a temporary mount to calculate the minimum log size. */
> @@ -3339,29 +3347,19 @@ _("external log device size %lld blocks too small, must be at least %lld blocks\
>
> /* internal log - if no size specified, calculate automatically */
> if (!cfg->logblocks) {
> - if (cfg->dblocks < GIGABYTES(1, cfg->blocklog)) {
> - /* tiny filesystems get minimum sized logs. */
> - cfg->logblocks = min_logblocks;
> - } else if (cfg->dblocks < GIGABYTES(16, cfg->blocklog)) {
> + /* Use a 2048:1 fs:log ratio for most filesystems */
> + cfg->logblocks = (cfg->dblocks << cfg->blocklog) / 2048;
> + cfg->logblocks = cfg->logblocks >> cfg->blocklog;
>
> - /*
> - * For small filesystems, we want to use the
> - * XFS_MIN_LOG_BYTES for filesystems smaller than 16G if
> - * at all possible, ramping up to 128MB at 256GB.
> - */
> - cfg->logblocks = min(XFS_MIN_LOG_BYTES >> cfg->blocklog,
> - min_logblocks * XFS_DFL_LOG_FACTOR);
> - } else {
> - /*
> - * With a 2GB max log size, default to maximum size
> - * at 4TB. This keeps the same ratio from the older
> - * max log size of 128M at 256GB fs size. IOWs,
> - * the ratio of fs size to log size is 2048:1.
> - */
> - cfg->logblocks = (cfg->dblocks << cfg->blocklog) / 2048;
> - cfg->logblocks = cfg->logblocks >> cfg->blocklog;
> - }
> + /* But don't go below a reasonable size */
> + cfg->logblocks = max(cfg->logblocks,
> + XFS_MIN_REALISTIC_LOG_BLOCKS(cfg->blocklog));
> +
> + /* And for a tiny filesystem, use the absolute minimum size */
> + if (cfg->dblocks < MEGABYTES(512, cfg->blocklog))
> + cfg->logblocks = min_logblocks;
>
> + /* Enforce min/max limits */
> clamp_internal_log_size(cfg, mp, min_logblocks);
>
> validate_log_size(cfg->logblocks, cfg->blocklog, min_logblocks);
>
>
