From: Chandan Babu R <chandanrlinux@gmail.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org, david@fromorbit.com,
hch@infradead.org, allison.henderson@oracle.com
Subject: Re: [PATCH V8 12/14] xfs: Compute bmap extent alignments in a separate function
Date: Fri, 30 Oct 2020 21:56:57 +0530 [thread overview]
Message-ID: <802040417.dp7cjt2klD@garuda> (raw)
In-Reply-To: <20201030155212.GQ1061252@magnolia>
On Friday 30 October 2020 9:22:12 PM IST Darrick J. Wong wrote:
> On Fri, Oct 30, 2020 at 01:46:18PM +0530, Chandan Babu R wrote:
> > On Friday 30 October 2020 3:51:30 AM IST Darrick J. Wong wrote:
> > > 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@oracle.com>
> > > > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> > > > ---
> > > > 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_exact_minlen_extent_alloc() introduced in the last patch would invoke
> > this function passing NULL value as the third argument i.e. it does not need
> > "stripe alignment" to be computed. Hence xfs_bmap_exact_minlen_extent_alloc()
> > would ignore the return value of xfs_bmap_compute_alignments(). This was the
> > reason for deciding on passing a pointer to the stripe_align variable as an
> > argument.
>
> I would have thought that an out parameter that isn't used by all
> callers would be the perfect use of a return, especially since passing
> by pointer means that the compiler has to spill stripe_align to a memory
> location both here and in the callers and cannot keep the value in
> registers.
You are right. I will implement the change pointed out by you. Thanks once
again for your suggestions.
>
> --D
>
> > >
> > > > +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.
> >
> > Sure, I will make the changes that have been suggested.
> >
> > >
> > > 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
> > >
> >
> >
>
--
chandan
next prev parent reply other threads:[~2020-10-30 16:27 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-29 10:13 [PATCH V8 00/14] Bail out if transaction can cause extent count to overflow Chandan Babu R
2020-10-29 10:13 ` [PATCH V8 01/14] xfs: Add helper for checking per-inode extent count overflow Chandan Babu R
2020-10-29 10:13 ` [PATCH V8 02/14] xfs: Check for extent overflow when trivally adding a new extent Chandan Babu R
2020-10-29 10:13 ` [PATCH V8 03/14] xfs: Check for extent overflow when punching a hole Chandan Babu R
2020-10-29 10:13 ` [PATCH V8 04/14] xfs: Check for extent overflow when adding/removing xattrs Chandan Babu R
2020-10-29 10:13 ` [PATCH V8 05/14] xfs: Check for extent overflow when adding/removing dir entries Chandan Babu R
2020-10-29 10:13 ` [PATCH V8 06/14] xfs: Check for extent overflow when writing to unwritten extent Chandan Babu R
2020-10-29 10:13 ` [PATCH V8 07/14] xfs: Check for extent overflow when moving extent from cow to data fork Chandan Babu R
2020-10-29 10:13 ` [PATCH V8 08/14] xfs: Check for extent overflow when remapping an extent Chandan Babu R
2020-10-29 10:13 ` [PATCH V8 09/14] xfs: Check for extent overflow when swapping extents Chandan Babu R
2020-10-29 10:13 ` [PATCH V8 10/14] xfs: Introduce error injection to reduce maximum inode fork extent count Chandan Babu R
2020-10-29 10:13 ` [PATCH V8 11/14] xfs: Remove duplicate assert statement in xfs_bmap_btalloc() Chandan Babu R
2020-10-29 10:13 ` [PATCH V8 12/14] xfs: Compute bmap extent alignments in a separate function Chandan Babu R
2020-10-29 22:21 ` Darrick J. Wong
2020-10-30 8:16 ` Chandan Babu R
2020-10-30 15:52 ` Darrick J. Wong
2020-10-30 16:26 ` Chandan Babu R [this message]
2020-10-29 10:13 ` [PATCH V8 13/14] xfs: Process allocated extent " Chandan Babu R
2020-10-29 22:22 ` Darrick J. Wong
2020-10-29 10:13 ` [PATCH V8 14/14] xfs: Introduce error injection to allocate only minlen size extents for files Chandan Babu R
2020-10-29 22:33 ` Darrick J. Wong
2020-10-30 8:17 ` Chandan Babu R
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=802040417.dp7cjt2klD@garuda \
--to=chandanrlinux@gmail.com \
--cc=allison.henderson@oracle.com \
--cc=darrick.wong@oracle.com \
--cc=david@fromorbit.com \
--cc=hch@infradead.org \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.