Re: [PATCH v4 3/3] xfs: Add realtime fallback if data device full

[Dave Chinner <david@fromorbit.com>]

On Mon, Sep 18, 2017 at 08:52:38PM -0700, Richard Wareing wrote:
> - Adds tunable option to fallback to realtime device if configured
>   when data device is full.
> - Useful for realtime device users to help prevent ENOSPC errors when
>   selectively storing some files (e.g. small files) on data device, while
>   others are stored on realtime block device.
> - Set via the "rt_fallback_pct" sysfs value which is available if
>   the kernel is compiled with CONFIG_XFS_RT.
> 
> Signed-off-by: Richard Wareing <rwareing@fb.com>
> ---
> Changes since v3:
> * None, new patch to patch set
> 
>  fs/xfs/xfs_bmap_util.c |  4 +++-
>  fs/xfs/xfs_fsops.c     |  4 ++++
>  fs/xfs/xfs_iomap.c     |  8 ++++++--
>  fs/xfs/xfs_mount.c     | 27 ++++++++++++++++++++++++++-
>  fs/xfs/xfs_mount.h     |  7 ++++++-
>  fs/xfs/xfs_rtalloc.c   | 14 ++++++++++++++
>  fs/xfs/xfs_rtalloc.h   |  3 ++-
>  fs/xfs/xfs_sysfs.c     | 39 +++++++++++++++++++++++++++++++++++++++
>  8 files changed, 100 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 2d253fb..9797c69 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1026,8 +1026,10 @@ xfs_alloc_file_space(
>  	if (len <= 0)
>  		return -EINVAL;
>  
> -	if (XFS_IS_REALTIME_MOUNT(mp))
> +	if (XFS_IS_REALTIME_MOUNT(mp)) {
>  		xfs_rt_alloc_min(ip, len);
> +		xfs_rt_fallback(ip, mp);
> +	}

This should really go inside xfs_inode_select_target() as space
availability is just another selection criteria....

>  	rt = XFS_IS_REALTIME_INODE(ip);
>  	extsz = xfs_get_extsz_hint(ip);
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index 6ccaae9..c15e906 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -610,6 +610,10 @@ xfs_growfs_data_private(
>  	xfs_set_low_space_thresholds(mp);
>  	mp->m_alloc_set_aside = xfs_alloc_set_aside(mp);
>  
> +	if (XFS_IS_REALTIME_MOUNT(mp)) {
> +		xfs_set_rt_min_fdblocks(mp);
> +	}

Normal way to do this is something like:

	mp->m_rt_min_fdblocks = xfs_calc_min_free_rtblocks(mp);

and check XFS_IS_REALTIME_MOUNT() inside that function.

And now, reading on, I find I've completely misunderstood what
those variable and function names mean, so they need renaming
to be clearer.

> +
> +/*
> + * precalculate minimum of data blocks required, if we fall
> + * below this value, we will fallback to the real-time device.
> + *
> + * m_rt_fallback_pct can only be non-zero if a real-time device
> + * is configured.
> + */
> +void
> +xfs_set_rt_min_fdblocks(
> +	struct xfs_mount	*mp)
> +{
> +	if (mp->m_rt_fallback_pct) {
> +		xfs_sb_t		*sbp = &mp->m_sb;
> +		xfs_extlen_t 	lsize;
> +		__uint64_t 		min_blocks;

Stray whitespace. If you use vim, add this macro to your
.vimrc so that it highlights stray whitespace for you:

" highlight whitespace damage
highlight RedundantSpaces ctermbg=red guibg=red
match RedundantSpaces /\s\+$\| \+\ze\t/

> +
> +		lsize = sbp->sb_logstart ? sbp->sb_logblocks : 0;
> +		min_blocks = (mp->m_sb.sb_dblocks - lsize) * mp->m_rt_fallback_pct;
> +		do_div(min_blocks, 100);

Why bother with the log size?

> +		/* Pre-compute minimum data blocks required before
> +		 * falling back to RT device for allocations
> +		 */

Comment format.

> +		mp->m_rt_min_fdblocks = min_blocks;

Hmmm - I wonder if it would be better to tie this into the existing
data device low space threshold code?

> +	}

Also, indenting.

	if (!XFS_IS_REALTIME_MOUNT(mp))
		return 0;
	if (!mp->m_rt_fallback_pct)
		return 0;
	....



> +}
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index 067be3b..36676c4 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -197,7 +197,11 @@ typedef struct xfs_mount {
>  	__uint32_t		m_generation;
>  
>  	bool			m_fail_unmount;
> -        uint                    m_rt_alloc_min; /* Min RT allocation */
> +	uint			m_rt_alloc_min; /* Min RT allocation */
> +	__uint8_t		m_rt_fallback_pct; /* Fall back to realtime device if

uint32_t is fine. We're moving away from the __[u]int types, so
we shouldn't really add any new ones.

> +void xfs_rt_fallback(
> +    struct xfs_inode    *ip,
> +    struct xfs_mount    *mp)

Mount first, then inode.

> +{
> +    if (!XFS_IS_REALTIME_INODE(ip)) {
> +        __uint64_t      free;
> +        free = percpu_counter_sum(&mp->m_fdblocks) -
> +            mp->m_alloc_set_aside;
> +        if (free < mp->m_rt_min_fdblocks) {
> +            ip->i_d.di_flags |= XFS_DIFLAG_REALTIME;
> +        }
> +    }
> +}

Indenting, but should really move to the target selection function.

> +STATIC ssize_t
> +rt_fallback_pct_store(
> +	struct kobject			*kobject,
> +	const char				*buf,
> +	size_t					count)
> +{
> +	struct xfs_mount		*mp = to_mp(kobject);
> +	int						ret;
> +	int						val;
> +
> +	ret = kstrtoint(buf, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* Only valid if using a real-time device */
> +	if (XFS_IS_REALTIME_MOUNT(mp) && ((val > 0) && (val <=100))) {
> +		mp->m_rt_fallback_pct = val;
> +		xfs_set_rt_min_fdblocks(mp);
> +	} else if (val <= 0) {
> +		mp->m_rt_fallback_pct = 0;
> +		mp->m_rt_min_fdblocks = 0;
> +	} else
> +		return -EINVAL;

Same issue as last patch.

Also, just set then threshold percentage, and if there is no error,
then call xfs_set_rt_min_fdblocks() rather than zeroing it directly.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com