On Thu, Oct 29, 2020 at 03:43:46PM +0530, Chandan Babu R wrote:
> This commit moves over the code which computes stripe alignment and
> extent size hint alignment into a separate function. Apart from
> xfs_bmap_btalloc(), the new function will be used by another function
> introduced in a future commit.
>
> Reviewed-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> Signed-off-by: Chandan Babu R <chandanrlinux@xxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 88 +++++++++++++++++++++++-----------------
> 1 file changed, 51 insertions(+), 37 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 64c4d0e384a5..935f2d506748 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3463,13 +3463,58 @@ xfs_bmap_btalloc_accounting(
> args->len);
> }
>
> +static void
Why not return stripe_align instead of passing pointers?
> +xfs_bmap_compute_alignments(
> + struct xfs_bmalloca *ap,
> + struct xfs_alloc_arg *args,
> + int *stripe_align)
> +{
> + struct xfs_mount *mp = args->mp;
> + xfs_extlen_t align = 0; /* minimum allocation alignment */
> + int error;
> +
> + /* stripe alignment for allocation is determined by mount parameters */
> + *stripe_align = 0;
> + if (mp->m_swidth && (mp->m_flags & XFS_MOUNT_SWALLOC))
> + *stripe_align = mp->m_swidth;
> + else if (mp->m_dalign)
> + *stripe_align = mp->m_dalign;
> +
> + if (ap->flags & XFS_BMAPI_COWFORK)
> + align = xfs_get_cowextsz_hint(ap->ip);
> + else if (ap->datatype & XFS_ALLOC_USERDATA)
> + align = xfs_get_extsz_hint(ap->ip);
> + if (align) {
> + error = xfs_bmap_extsize_align(mp, &ap->got, &ap->prev,
> + align, 0, ap->eof, 0, ap->conv,
> + &ap->offset, &ap->length);
> + ASSERT(!error);
> + ASSERT(ap->length);
> + }
> +
> + /* apply extent size hints if obtained earlier */
> + if (align) {
> + args->prod = align;
> + div_u64_rem(ap->offset, args->prod, &args->mod);
> + if (args->mod)
> + args->mod = args->prod - args->mod;
> + } else if (mp->m_sb.sb_blocksize >= PAGE_SIZE) {
> + args->prod = 1;
> + args->mod = 0;
> + } else {
> + args->prod = PAGE_SIZE >> mp->m_sb.sb_blocklog;
> + div_u64_rem(ap->offset, args->prod, &args->mod);
> + if (args->mod)
> + args->mod = args->prod - args->mod;
> + }
> +}
> +
> STATIC int
> xfs_bmap_btalloc(
> struct xfs_bmalloca *ap) /* bmap alloc argument struct */
> {
> xfs_mount_t *mp; /* mount point structure */
> xfs_alloctype_t atype = 0; /* type for allocation routines */
> - xfs_extlen_t align = 0; /* minimum allocation alignment */
> xfs_agnumber_t fb_agno; /* ag number of ap->firstblock */
> xfs_agnumber_t ag;
> xfs_alloc_arg_t args;
> @@ -3489,25 +3534,11 @@ xfs_bmap_btalloc(
>
> mp = ap->ip->i_mount;
>
> - /* stripe alignment for allocation is determined by mount parameters */
> - stripe_align = 0;
> - if (mp->m_swidth && (mp->m_flags & XFS_MOUNT_SWALLOC))
> - stripe_align = mp->m_swidth;
> - else if (mp->m_dalign)
> - stripe_align = mp->m_dalign;
> -
> - if (ap->flags & XFS_BMAPI_COWFORK)
> - align = xfs_get_cowextsz_hint(ap->ip);
> - else if (ap->datatype & XFS_ALLOC_USERDATA)
> - align = xfs_get_extsz_hint(ap->ip);
> - if (align) {
> - error = xfs_bmap_extsize_align(mp, &ap->got, &ap->prev,
> - align, 0, ap->eof, 0, ap->conv,
> - &ap->offset, &ap->length);
> - ASSERT(!error);
> - ASSERT(ap->length);
> - }
> + memset(&args, 0, sizeof(args));
> + args.tp = ap->tp;
> + args.mp = mp;
FWIW you might as well clean up the variable declarations while you're
moving this stuff around:
STATIC int
xfs_bmap_btalloc(
struct xfs_bmalloca *ap)
{
struct xfs_mount *mp = ap->ip->i_mount;
struct xfs_alloc_arg args = { .tp = ap->tp, .mp = mp };
And then you can get rid of the memset call.
AFAICT there aren't any data dependencies between the parts where we
initialize args.fsbno and where we set args.prod and args.mod, so I
guess this is a reasonable hoist.
Other than those cleanups, this looks ok to me.
--D
>
> + xfs_bmap_compute_alignments(ap, &args, &stripe_align);
>
> nullfb = ap->tp->t_firstblock == NULLFSBLOCK;
> fb_agno = nullfb ? NULLAGNUMBER : XFS_FSB_TO_AGNO(mp,
> @@ -3538,9 +3569,6 @@ xfs_bmap_btalloc(
> * Normal allocation, done through xfs_alloc_vextent.
> */
> tryagain = isaligned = 0;
> - memset(&args, 0, sizeof(args));
> - args.tp = ap->tp;
> - args.mp = mp;
> args.fsbno = ap->blkno;
> args.oinfo = XFS_RMAP_OINFO_SKIP_UPDATE;
>
> @@ -3571,21 +3599,7 @@ xfs_bmap_btalloc(
> args.total = ap->total;
> args.minlen = ap->minlen;
> }
> - /* apply extent size hints if obtained earlier */
> - if (align) {
> - args.prod = align;
> - div_u64_rem(ap->offset, args.prod, &args.mod);
> - if (args.mod)
> - args.mod = args.prod - args.mod;
> - } else if (mp->m_sb.sb_blocksize >= PAGE_SIZE) {
> - args.prod = 1;
> - args.mod = 0;
> - } else {
> - args.prod = PAGE_SIZE >> mp->m_sb.sb_blocklog;
> - div_u64_rem(ap->offset, args.prod, &args.mod);
> - if (args.mod)
> - args.mod = args.prod - args.mod;
> - }
> +
> /*
> * If we are not low on available data blocks, and the underlying
> * logical volume manager is a stripe, and the file offset is zero then
> --
> 2.28.0
>
